Skip to content

perf: Segregate syntax and semantic diagnostics #17775

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

Merged
merged 1 commit into from
Aug 5, 2024
Merged

perf: Segregate syntax and semantic diagnostics #17775

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
78 changes: 61 additions & 17 deletions crates/ide-diagnostics/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ use syntax::{
#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)]
pub enum DiagnosticCode {
RustcHardError(&'static str),
SyntaxError,
RustcLint(&'static str),
Clippy(&'static str),
Ra(&'static str, Severity),
Expand All @@ -107,6 +108,9 @@ impl DiagnosticCode {
DiagnosticCode::RustcHardError(e) => {
format!("https://doc.rust-lang.org/stable/error_codes/{e}.html")
}
DiagnosticCode::SyntaxError => {
String::from("https://doc.rust-lang.org/stable/reference/")
}
DiagnosticCode::RustcLint(e) => {
format!("https://doc.rust-lang.org/rustc/?search={e}")
}
Expand All @@ -125,6 +129,7 @@ impl DiagnosticCode {
| DiagnosticCode::RustcLint(r)
| DiagnosticCode::Clippy(r)
| DiagnosticCode::Ra(r, _) => r,
DiagnosticCode::SyntaxError => "syntax-error",
}
}
}
Expand Down Expand Up @@ -154,7 +159,7 @@ impl Diagnostic {
message,
range: range.into(),
severity: match code {
DiagnosticCode::RustcHardError(_) => Severity::Error,
DiagnosticCode::RustcHardError(_) | DiagnosticCode::SyntaxError => Severity::Error,
// FIXME: Rustc lints are not always warning, but the ones that are currently implemented are all warnings.
DiagnosticCode::RustcLint(_) => Severity::Warning,
// FIXME: We can make this configurable, and if the user uses `cargo clippy` on flycheck, we can
Expand Down Expand Up @@ -297,31 +302,54 @@ impl DiagnosticsContext<'_> {
}
}

/// Request diagnostics for the given [`FileId`]. The produced diagnostics may point to other files
/// Request parser level diagnostics for the given [`FileId`].
pub fn syntax_diagnostics(
db: &RootDatabase,
config: &DiagnosticsConfig,
file_id: FileId,
) -> Vec<Diagnostic> {
let _p = tracing::info_span!("syntax_diagnostics").entered();

if config.disabled.contains("syntax-error") {
return Vec::new();
}

let sema = Semantics::new(db);
let file_id = sema
.attach_first_edition(file_id)
.unwrap_or_else(|| EditionedFileId::current_edition(file_id));

// [#3434] Only take first 128 errors to prevent slowing down editor/ide, the number 128 is chosen arbitrarily.
Copy link
Member Author

Choose a reason for hiding this comment

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

Previous comment was pointing the wrong issue no.34344

db.parse_errors(file_id)
.as_deref()
.into_iter()
.flatten()
.take(128)
.map(|err| {
Diagnostic::new(
DiagnosticCode::SyntaxError,
format!("Syntax Error: {err}"),
FileRange { file_id: file_id.into(), range: err.range() },
)
})
.collect()
}

/// Request semantic diagnostics for the given [`FileId`]. The produced diagnostics may point to other files
/// due to macros.
pub fn diagnostics(
pub fn semantic_diagnostics(
Copy link
Member Author

Choose a reason for hiding this comment

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

Calling db.parses once in fn syntax_diagnotics(..) and again in this function that previously only called once in fn diagnotics(..) is a bit reluctant, but I think that it won't harm perf much due to lru cache here;

rust-analyzer/crates/base-db/src/lib.rs

Lines 60 to 63 in aa00ddc

pub trait SourceDatabase: FileLoader + std::fmt::Debug {
/// Parses the file into the syntax tree.
#[salsa::lru]
fn parse(&self, file_id: EditionedFileId) -> Parse<ast::SourceFile>;

db: &RootDatabase,
config: &DiagnosticsConfig,
resolve: &AssistResolveStrategy,
file_id: FileId,
) -> Vec<Diagnostic> {
let _p = tracing::info_span!("diagnostics").entered();
let _p = tracing::info_span!("semantic_diagnostics").entered();
let sema = Semantics::new(db);
let file_id = sema
.attach_first_edition(file_id)
.unwrap_or_else(|| EditionedFileId::current_edition(file_id));
let mut res = Vec::new();

// [#34344] Only take first 128 errors to prevent slowing down editor/ide, the number 128 is chosen arbitrarily.
res.extend(db.parse_errors(file_id).as_deref().into_iter().flatten().take(128).map(|err| {
Diagnostic::new(
DiagnosticCode::RustcHardError("syntax-error"),
format!("Syntax Error: {err}"),
FileRange { file_id: file_id.into(), range: err.range() },
)
}));
let parse_errors = res.len();

let parse = sema.parse(file_id);

// FIXME: This iterates the entire file which is a rather expensive operation.
Expand All @@ -341,8 +369,11 @@ pub fn diagnostics(
match module {
// A bunch of parse errors in a file indicate some bigger structural parse changes in the
// file, so we skip semantic diagnostics so we can show these faster.
Some(m) if parse_errors < 16 => m.diagnostics(db, &mut diags, config.style_lints),
Some(_) => (),
Some(m) => {
if !db.parse_errors(file_id).as_deref().is_some_and(|es| es.len() >= 16) {
m.diagnostics(db, &mut diags, config.style_lints);
}
}
None => handlers::unlinked_file::unlinked_file(&ctx, &mut res, file_id.file_id()),
}

Expand All @@ -363,7 +394,7 @@ pub fn diagnostics(
res.extend(d.errors.iter().take(16).map(|err| {
{
Diagnostic::new(
DiagnosticCode::RustcHardError("syntax-error"),
DiagnosticCode::SyntaxError,
format!("Syntax Error in Expansion: {err}"),
ctx.resolve_precise_location(&d.node.clone(), d.precise_location),
)
Expand Down Expand Up @@ -464,6 +495,19 @@ pub fn diagnostics(
res
}

/// Request both syntax and semantic diagnostics for the given [`FileId`].
pub fn full_diagnostics(
db: &RootDatabase,
config: &DiagnosticsConfig,
resolve: &AssistResolveStrategy,
file_id: FileId,
) -> Vec<Diagnostic> {
let mut res = syntax_diagnostics(db, config, file_id);
let sema = semantic_diagnostics(db, config, resolve, file_id);
res.extend(sema);
res
}

// `__RA_EVERY_LINT` is a fake lint group to allow every lint in proc macros

static RUSTC_LINT_GROUPS_DICT: Lazy<FxHashMap<&str, Vec<&str>>> =
Expand Down
146 changes: 77 additions & 69 deletions crates/ide-diagnostics/src/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,10 +59,14 @@ fn check_nth_fix_with_config(
let after = trim_indent(ra_fixture_after);

let (db, file_position) = RootDatabase::with_position(ra_fixture_before);
let diagnostic =
super::diagnostics(&db, &config, &AssistResolveStrategy::All, file_position.file_id.into())
.pop()
.expect("no diagnostics");
let diagnostic = super::full_diagnostics(
&db,
&config,
&AssistResolveStrategy::All,
file_position.file_id.into(),
)
.pop()
.expect("no diagnostics");
let fix = &diagnostic
.fixes
.unwrap_or_else(|| panic!("{:?} diagnostic misses fixes", diagnostic.code))[nth];
Expand Down Expand Up @@ -102,37 +106,39 @@ pub(crate) fn check_has_fix(ra_fixture_before: &str, ra_fixture_after: &str) {
let (db, file_position) = RootDatabase::with_position(ra_fixture_before);
let mut conf = DiagnosticsConfig::test_sample();
conf.expr_fill_default = ExprFillDefaultMode::Default;
let fix =
super::diagnostics(&db, &conf, &AssistResolveStrategy::All, file_position.file_id.into())
.into_iter()
.find(|d| {
d.fixes
.as_ref()
.and_then(|fixes| {
fixes.iter().find(|fix| {
if !fix.target.contains_inclusive(file_position.offset) {
return false;
}
let actual = {
let source_change = fix.source_change.as_ref().unwrap();
let file_id =
*source_change.source_file_edits.keys().next().unwrap();
let mut actual = db.file_text(file_id).to_string();
let fix = super::full_diagnostics(
&db,
&conf,
&AssistResolveStrategy::All,
file_position.file_id.into(),
)
.into_iter()
.find(|d| {
d.fixes
.as_ref()
.and_then(|fixes| {
fixes.iter().find(|fix| {
if !fix.target.contains_inclusive(file_position.offset) {
return false;
}
let actual = {
let source_change = fix.source_change.as_ref().unwrap();
let file_id = *source_change.source_file_edits.keys().next().unwrap();
let mut actual = db.file_text(file_id).to_string();

for (edit, snippet_edit) in source_change.source_file_edits.values()
{
edit.apply(&mut actual);
if let Some(snippet_edit) = snippet_edit {
snippet_edit.apply(&mut actual);
}
}
actual
};
after == actual
})
})
.is_some()
});
for (edit, snippet_edit) in source_change.source_file_edits.values() {
edit.apply(&mut actual);
if let Some(snippet_edit) = snippet_edit {
snippet_edit.apply(&mut actual);
}
}
actual
};
after == actual
})
})
.is_some()
});
assert!(fix.is_some(), "no diagnostic with desired fix");
}

Expand All @@ -144,46 +150,48 @@ pub(crate) fn check_has_single_fix(ra_fixture_before: &str, ra_fixture_after: &s
let mut conf = DiagnosticsConfig::test_sample();
conf.expr_fill_default = ExprFillDefaultMode::Default;
let mut n_fixes = 0;
let fix =
super::diagnostics(&db, &conf, &AssistResolveStrategy::All, file_position.file_id.into())
.into_iter()
.find(|d| {
d.fixes
.as_ref()
.and_then(|fixes| {
n_fixes += fixes.len();
fixes.iter().find(|fix| {
if !fix.target.contains_inclusive(file_position.offset) {
return false;
}
let actual = {
let source_change = fix.source_change.as_ref().unwrap();
let file_id =
*source_change.source_file_edits.keys().next().unwrap();
let mut actual = db.file_text(file_id).to_string();
let fix = super::full_diagnostics(
&db,
&conf,
&AssistResolveStrategy::All,
file_position.file_id.into(),
)
.into_iter()
.find(|d| {
d.fixes
.as_ref()
.and_then(|fixes| {
n_fixes += fixes.len();
fixes.iter().find(|fix| {
if !fix.target.contains_inclusive(file_position.offset) {
return false;
}
let actual = {
let source_change = fix.source_change.as_ref().unwrap();
let file_id = *source_change.source_file_edits.keys().next().unwrap();
let mut actual = db.file_text(file_id).to_string();

for (edit, snippet_edit) in source_change.source_file_edits.values()
{
edit.apply(&mut actual);
if let Some(snippet_edit) = snippet_edit {
snippet_edit.apply(&mut actual);
}
}
actual
};
after == actual
})
})
.is_some()
});
for (edit, snippet_edit) in source_change.source_file_edits.values() {
edit.apply(&mut actual);
if let Some(snippet_edit) = snippet_edit {
snippet_edit.apply(&mut actual);
}
}
actual
};
after == actual
})
})
.is_some()
});
assert!(fix.is_some(), "no diagnostic with desired fix");
assert!(n_fixes == 1, "Too many fixes suggested");
}

/// Checks that there's a diagnostic *without* fix at `$0`.
pub(crate) fn check_no_fix(ra_fixture: &str) {
let (db, file_position) = RootDatabase::with_position(ra_fixture);
let diagnostic = super::diagnostics(
let diagnostic = super::full_diagnostics(
&db,
&DiagnosticsConfig::test_sample(),
&AssistResolveStrategy::All,
Expand Down Expand Up @@ -215,7 +223,7 @@ pub(crate) fn check_diagnostics_with_config(config: DiagnosticsConfig, ra_fixtur
.iter()
.copied()
.flat_map(|file_id| {
super::diagnostics(&db, &config, &AssistResolveStrategy::All, file_id.into())
super::full_diagnostics(&db, &config, &AssistResolveStrategy::All, file_id.into())
.into_iter()
.map(|d| {
let mut annotation = String::new();
Expand Down Expand Up @@ -277,10 +285,10 @@ fn test_disabled_diagnostics() {
let (db, file_id) = RootDatabase::with_single_file(r#"mod foo;"#);
let file_id = file_id.into();

let diagnostics = super::diagnostics(&db, &config, &AssistResolveStrategy::All, file_id);
let diagnostics = super::full_diagnostics(&db, &config, &AssistResolveStrategy::All, file_id);
assert!(diagnostics.is_empty());

let diagnostics = super::diagnostics(
let diagnostics = super::full_diagnostics(
&db,
&DiagnosticsConfig::test_sample(),
&AssistResolveStrategy::All,
Expand Down
27 changes: 23 additions & 4 deletions crates/ide/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -672,14 +672,33 @@ impl Analysis {
.unwrap_or_default())
}

/// Computes the set of diagnostics for the given file.
pub fn diagnostics(
/// Computes the set of parser level diagnostics for the given file.
pub fn syntax_diagnostics(
&self,
config: &DiagnosticsConfig,
file_id: FileId,
) -> Cancellable<Vec<Diagnostic>> {
self.with_db(|db| ide_diagnostics::syntax_diagnostics(db, config, file_id))
}

/// Computes the set of semantic diagnostics for the given file.
pub fn semantic_diagnostics(
&self,
config: &DiagnosticsConfig,
resolve: AssistResolveStrategy,
file_id: FileId,
) -> Cancellable<Vec<Diagnostic>> {
self.with_db(|db| ide_diagnostics::semantic_diagnostics(db, config, &resolve, file_id))
}

/// Computes the set of both syntax and semantic diagnostics for the given file.
pub fn full_diagnostics(
&self,
config: &DiagnosticsConfig,
resolve: AssistResolveStrategy,
file_id: FileId,
) -> Cancellable<Vec<Diagnostic>> {
self.with_db(|db| ide_diagnostics::diagnostics(db, config, &resolve, file_id))
self.with_db(|db| ide_diagnostics::full_diagnostics(db, config, &resolve, file_id))
}

/// Convenience function to return assists + quick fixes for diagnostics
Expand All @@ -697,7 +716,7 @@ impl Analysis {

self.with_db(|db| {
let diagnostic_assists = if diagnostics_config.enabled && include_fixes {
ide_diagnostics::diagnostics(db, diagnostics_config, &resolve, frange.file_id)
ide_diagnostics::full_diagnostics(db, diagnostics_config, &resolve, frange.file_id)
.into_iter()
.flat_map(|it| it.fixes.unwrap_or_default())
.filter(|it| it.target.intersect(frange.range).is_some())
Expand Down
2 changes: 1 addition & 1 deletion crates/rust-analyzer/src/cli/analysis_stats.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -977,7 +977,7 @@ impl flags::AnalysisStats {
let mut sw = self.stop_watch();

for &file_id in &file_ids {
_ = analysis.diagnostics(
_ = analysis.full_diagnostics(
&DiagnosticsConfig {
enabled: true,
proc_macros_enabled: true,
Expand Down
Loading