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.

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

perf: Segregate syntax and semantic diagnostics #17775

Merged
merged 1 commit into from
Aug 5, 2024
Merged
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
@@ -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),
@@ -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}")
}
@@ -125,6 +129,7 @@ impl DiagnosticCode {
| DiagnosticCode::RustcLint(r)
| DiagnosticCode::Clippy(r)
| DiagnosticCode::Ra(r, _) => r,
DiagnosticCode::SyntaxError => "syntax-error",
}
}
}
@@ -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
@@ -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;

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.
@@ -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()),
}

@@ -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),
)
@@ -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>>> =
146 changes: 77 additions & 69 deletions crates/ide-diagnostics/src/tests.rs
Original file line number Diff line number Diff line change
@@ -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];
@@ -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");
}

@@ -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,
@@ -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();
@@ -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,
27 changes: 23 additions & 4 deletions crates/ide/src/lib.rs
Original file line number Diff line number Diff line change
@@ -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
@@ -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())
2 changes: 1 addition & 1 deletion crates/rust-analyzer/src/cli/analysis_stats.rs
Original file line number Diff line number Diff line change
@@ -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,
Loading