Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(lsp): correctly infer file source from un-named files #1413

Merged
merged 2 commits into from
Jan 3, 2024
Merged

fix(lsp): correctly infer file source from un-named files #1413

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a2195b7
fix(lsp): correctly infer file source from un-named files
ematipico Jan 3, 2024
18a27ae
add test
ematipico Jan 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -279,6 +279,7 @@ Biome now scores 97% compatibility with Prettier and features more than 180 lint
#### Bug fixes

- Fix [#933](https://github.com/biomejs/biome/issues/933). Some files are properly ignored in the LSP too. E.g. `package.json`, `tsconfig.json`, etc.
- Fix [#1394](https://github.com/biomejs/biome/issues/1394), by inferring the language extension from the internal saved files. Now newly created files JavaScript correctly show diagnostics.

### Formatter

Expand Down
97 changes: 97 additions & 0 deletions crates/biome_lsp/tests/server.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,21 @@ impl Server {
.await
}

async fn open_untitled_document(&mut self, text: impl Display) -> Result<()> {
self.notify(
"textDocument/didOpen",
DidOpenTextDocumentParams {
text_document: TextDocumentItem {
uri: url!("untitled-1"),
language_id: String::from("javascript"),
version: 0,
text: text.to_string(),
},
},
)
.await
}

/// Opens a document with given contents and given name. The name must contain the extension too
async fn open_named_document(
&mut self,
Expand Down Expand Up @@ -624,6 +639,88 @@ async fn pull_diagnostics() -> Result<()> {
Ok(())
}

#[tokio::test]
async fn pull_diagnostics_from_new_file() -> Result<()> {
let factory = ServerFactory::default();
let (service, client) = factory.create(None).into_inner();
let (stream, sink) = client.split();
let mut server = Server::new(service);

let (sender, mut receiver) = channel(CHANNEL_BUFFER_SIZE);
let reader = tokio::spawn(client_handler(stream, sink, sender));

server.initialize().await?;
server.initialized().await?;

server.open_untitled_document("if(a == b) {}").await?;

let notification = tokio::select! {
msg = receiver.next() => msg,
_ = sleep(Duration::from_secs(1)) => {
panic!("timed out waiting for the server to send diagnostics")
}
};

assert_eq!(
notification,
Some(ServerNotification::PublishDiagnostics(
PublishDiagnosticsParams {
uri: url!("untitled-1"),
version: Some(0),
diagnostics: vec![lsp::Diagnostic {
range: lsp::Range {
start: lsp::Position {
line: 0,
character: 5,
},
end: lsp::Position {
line: 0,
character: 7,
},
},
severity: Some(lsp::DiagnosticSeverity::ERROR),
code: Some(lsp::NumberOrString::String(String::from(
"lint/suspicious/noDoubleEquals",
))),
code_description: Some(CodeDescription {
href: Url::parse("https://biomejs.dev/linter/rules/no-double-equals")
.unwrap()
}),
source: Some(String::from("biome")),
message: String::from(
"Use === instead of ==.\n== is only allowed when comparing against `null`",
),
related_information: Some(vec![lsp::DiagnosticRelatedInformation {
location: lsp::Location {
uri: url!("untitled-1"),
range: lsp::Range {
start: lsp::Position {
line: 0,
character: 5,
},
end: lsp::Position {
line: 0,
character: 7,
},
},
},
message: String::new(),
}]),
tags: None,
data: None,
}],
}
))
);

server.close_document().await?;

server.shutdown().await?;
reader.abort();

Ok(())
}

fn fixable_diagnostic(line: u32) -> Result<lsp::Diagnostic> {
Ok(lsp::Diagnostic {
range: lsp::Range {
Expand Down
168 changes: 90 additions & 78 deletions crates/biome_service/src/file_handlers/javascript.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ use biome_rowan::{AstNode, BatchMutationExt, Direction, FileSource, NodeCache};
use std::borrow::Cow;
use std::fmt::Debug;
use std::path::PathBuf;
use tracing::{debug, error, info, trace};
use tracing::{debug, debug_span, error, info, trace};

#[derive(Debug, Clone, Default, serde::Serialize, serde::Deserialize)]
#[cfg_attr(feature = "schema", derive(schemars::JsonSchema))]
Expand Down Expand Up @@ -267,89 +267,101 @@ fn debug_formatter_ir(
}

fn lint(params: LintParams) -> LintResults {
let Ok(file_source) = params.parse.file_source(params.path) else {
return LintResults {
errors: 0,
diagnostics: vec![],
skipped_diagnostics: 0,
};
};
let tree = params.parse.tree();
let mut diagnostics = params.parse.into_diagnostics();
debug_span!("Linting JavaScript file", path =? params.path, language =? params.language)
.in_scope(move || {
let file_source = match params.parse.file_source(params.path) {
Ok(file_source) => file_source,
Err(_) => {
if let Some(file_source) = params.language.as_js_file_source() {
file_source
} else {
return LintResults {
errors: 0,
diagnostics: vec![],
skipped_diagnostics: 0,
};
}
}
};
Comment on lines +272 to +285
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the new change and the bulk of the logic.

let tree = params.parse.tree();
let mut diagnostics = params.parse.into_diagnostics();

let analyzer_options =
compute_analyzer_options(&params.settings, PathBuf::from(params.path.as_path()));
let analyzer_options =
compute_analyzer_options(&params.settings, PathBuf::from(params.path.as_path()));

let mut diagnostic_count = diagnostics.len() as u64;
let mut errors = diagnostics
.iter()
.filter(|diag| diag.severity() <= Severity::Error)
.count();
let mut diagnostic_count = diagnostics.len() as u64;
let mut errors = diagnostics
.iter()
.filter(|diag| diag.severity() <= Severity::Error)
.count();

let has_lint = params.filter.categories.contains(RuleCategories::LINT);

info!("Analyze file {}", params.path.display());
let (_, analyze_diagnostics) = analyze(
&tree,
params.filter,
&analyzer_options,
file_source,
|signal| {
if let Some(mut diagnostic) = signal.diagnostic() {
// Do not report unused suppression comment diagnostics if this is a syntax-only analyzer pass
if !has_lint
&& diagnostic.category() == Some(category!("suppressions/unused"))
{
return ControlFlow::<Never>::Continue(());
}

let has_lint = params.filter.categories.contains(RuleCategories::LINT);
diagnostic_count += 1;

// We do now check if the severity of the diagnostics should be changed.
// The configuration allows to change the severity of the diagnostics emitted by rules.
let severity = diagnostic
.category()
.filter(|category| category.name().starts_with("lint/"))
.map(|category| {
params
.rules
.and_then(|rules| rules.get_severity_from_code(category))
.unwrap_or(Severity::Warning)
})
.unwrap_or_else(|| diagnostic.severity());

if severity >= Severity::Error {
errors += 1;
}

info!("Analyze file {}", params.path.display());
let (_, analyze_diagnostics) = analyze(
&tree,
params.filter,
&analyzer_options,
file_source,
|signal| {
if let Some(mut diagnostic) = signal.diagnostic() {
// Do not report unused suppression comment diagnostics if this is a syntax-only analyzer pass
if !has_lint && diagnostic.category() == Some(category!("suppressions/unused")) {
return ControlFlow::<Never>::Continue(());
}
if diagnostic_count <= params.max_diagnostics {
for action in signal.actions() {
if !action.is_suppression() {
diagnostic = diagnostic.add_code_suggestion(action.into());
}
}

diagnostic_count += 1;

// We do now check if the severity of the diagnostics should be changed.
// The configuration allows to change the severity of the diagnostics emitted by rules.
let severity = diagnostic
.category()
.filter(|category| category.name().starts_with("lint/"))
.map(|category| {
params
.rules
.and_then(|rules| rules.get_severity_from_code(category))
.unwrap_or(Severity::Warning)
})
.unwrap_or_else(|| diagnostic.severity());

if severity >= Severity::Error {
errors += 1;
}
let error = diagnostic.with_severity(severity);

if diagnostic_count <= params.max_diagnostics {
for action in signal.actions() {
if !action.is_suppression() {
diagnostic = diagnostic.add_code_suggestion(action.into());
diagnostics.push(biome_diagnostics::serde::Diagnostic::new(error));
}
}

let error = diagnostic.with_severity(severity);

diagnostics.push(biome_diagnostics::serde::Diagnostic::new(error));
}
ControlFlow::<Never>::Continue(())
},
);

diagnostics.extend(
analyze_diagnostics
.into_iter()
.map(biome_diagnostics::serde::Diagnostic::new)
.collect::<Vec<_>>(),
);
let skipped_diagnostics = diagnostic_count.saturating_sub(diagnostics.len() as u64);

LintResults {
diagnostics,
errors,
skipped_diagnostics,
}

ControlFlow::<Never>::Continue(())
},
);

diagnostics.extend(
analyze_diagnostics
.into_iter()
.map(biome_diagnostics::serde::Diagnostic::new)
.collect::<Vec<_>>(),
);
let skipped_diagnostics = diagnostic_count.saturating_sub(diagnostics.len() as u64);

LintResults {
diagnostics,
errors,
skipped_diagnostics,
}
})
}

struct ActionsVisitor<'a> {
Expand Down Expand Up @@ -380,7 +392,7 @@ impl RegistryVisitor<JsLanguage> for ActionsVisitor<'_> {
}
}

#[tracing::instrument(level = "trace", skip(parse))]
#[tracing::instrument(level = "debug", skip(parse, settings))]
fn code_actions(
parse: AnyParse,
range: TextRange,
Expand Down Expand Up @@ -555,7 +567,7 @@ fn fix_all(params: FixAllParams) -> Result<FixFileResult, WorkspaceError> {
}
}

#[tracing::instrument(level = "trace", skip(parse))]
#[tracing::instrument(level = "trace", skip(parse, settings))]
fn format(
rome_path: &RomePath,
parse: AnyParse,
Expand All @@ -576,7 +588,7 @@ fn format(
}
}
}
#[tracing::instrument(level = "trace", skip(parse))]
#[tracing::instrument(level = "trace", skip(parse, settings))]
fn format_range(
rome_path: &RomePath,
parse: AnyParse,
Expand All @@ -590,7 +602,7 @@ fn format_range(
Ok(printed)
}

#[tracing::instrument(level = "trace", skip(parse))]
#[tracing::instrument(level = "trace", skip(parse, settings))]
fn format_on_type(
rome_path: &RomePath,
parse: AnyParse,
Expand Down
Loading