From 571a662630ee90ec5b51daa0b62000b65c7422fa Mon Sep 17 00:00:00 2001 From: LinkIsGrim <69561145+LinkIsGrim@users.noreply.github.com> Date: Wed, 26 Aug 2026 01:53:37 -0300 Subject: [PATCH] hls: report the same diagnostics as the CLI for a file Both sides preprocess, parse and analyze a file the same way, then each collected the resulting codes separately - and the two collections had drifted apart. Preprocessor warnings reached `hemtt check` and never the editor: `pw1_redefine`, `pw2_invalid_config_case`, `pw3_padded_arg`, `pw4_include_case`, `pw5_undef_not_defined`. So did every config note and help, because the editor took only `warnings()` and `errors()` from the report while the CLI also pushed `notes_and_helps()`. `hemtt_sqf::check` and `hemtt_config::check` now own that middle. They fold `processed.warnings()` in with the lint codes, apply the CBA settings skip, and hand back the codes plus whatever the caller still needs. The ends stay where they were: the CLI compiles, rapifies and pushes to the addon, the language server renders LSP diagnostics and maintains its caches. This also fixes `Diagnostic::to_lsp`, which emitted one diagnostic per label using the parent code's message and severity. A secondary label therefore became a second diagnostic repeating the parent's message somewhere it did not apply: `PW1` marked the previous definition "redefining macro" as though it were the redefinition, and discarded the label's own "previous definition here". It now emits one diagnostic per primary label, falling back to all of them if a code has no primary. That applies to every multi-label diagnostic, not only `PW1`. Verified against a project built to trigger each case. `PW1`, `PW5` and `help[L-C12]` were reported by `hemtt check` and never published by the editor; they now appear in both, at the same locations. --- bin/src/modules/rapifier.rs | 29 +--- bin/src/modules/sqf.rs | 62 +++----- hls/src/config/lints.rs | 75 ++++----- hls/src/sqf/lints.rs | 86 ++++------ libs/config/src/check.rs | 111 +++++++++++++ libs/config/src/lib.rs | 1 + libs/sqf/src/check.rs | 148 ++++++++++++++++++ libs/sqf/src/lib.rs | 2 + .../workspace/src/reporting/diagnostic/mod.rs | 70 +++++++++ 9 files changed, 419 insertions(+), 165 deletions(-) create mode 100644 libs/config/src/check.rs create mode 100644 libs/sqf/src/check.rs diff --git a/bin/src/modules/rapifier.rs b/bin/src/modules/rapifier.rs index 8978eb439..93091dfae 100644 --- a/bin/src/modules/rapifier.rs +++ b/bin/src/modules/rapifier.rs @@ -3,12 +3,12 @@ use std::{collections::HashMap, path::PathBuf, sync::RwLock}; use hemtt_config::{ Config, analyze::{lint_all, lint_check}, - parse, rapify::Rapify, }; use hemtt_workspace::{ WorkspacePath, addons::{Addon, Location}, + reporting::CodesExt, }; use rayon::prelude::{IntoParallelRefIterator, ParallelIterator}; use vfs::VfsFileType; @@ -124,29 +124,16 @@ pub fn rapify(addon: &Addon, path: &WorkspacePath, ctx: &Context) -> Result configreport, - Err(errors) => { - for e in &errors { - report.push(e.clone()); - } - return Ok(report); - } + let Some(configreport) = checked.config else { + return Ok(report); }; configreport.push_to_addon(addon); - configreport.notes_and_helps().into_iter().for_each(|e| { - report.push(e.clone()); - }); - configreport.warnings().into_iter().for_each(|e| { - report.push(e.clone()); - }); - configreport.errors().into_iter().for_each(|e| { - report.push(e.clone()); - }); - if !configreport.errors().is_empty() { + if had_errors { return Ok(report); } let out = if std::path::Path::new(&path.filename()) diff --git a/bin/src/modules/sqf.rs b/bin/src/modules/sqf.rs index f193a6d27..9f3fa1ee6 100644 --- a/bin/src/modules/sqf.rs +++ b/bin/src/modules/sqf.rs @@ -2,8 +2,8 @@ use std::sync::Arc; use hemtt_common::version::Version; use hemtt_sqf::{ - analyze::{analyze, lint_all, lint_check}, - parser::{ParserError, database::Database}, + analyze::{lint_all, lint_check}, + parser::database::Database, }; use hemtt_workspace::reporting::{Code, CodesExt, Diagnostic, Severity}; use rayon::prelude::{IntoParallelRefIterator, ParallelIterator}; @@ -90,48 +90,26 @@ impl Module for SQFCompiler { return Err(e.into()); } }; - for warning in processed.warnings() { - report.push(warning.clone()); + let checked = hemtt_sqf::check::check( + &processed, + Some(ctx.config()), + addon, + database.clone(), + ); + if let Some(sqf_report) = checked.report { + sqf_report.push_to_addon(addon); } - match hemtt_sqf::parser::run(&database, &processed) { - Ok(sqf) => { - let (codes, sqf_report) = analyze( - &sqf, - Some(ctx.config()), - &processed, - addon.clone(), - database.clone(), - ); - if let Some(sqf_report) = sqf_report { - sqf_report.push_to_addon(addon); - } - if !codes.failed() { - let mut out = entry.with_extension("sqfc")?.create_file()?; - sqf.optimize().compile_to_writer(&processed, &mut out)?; - progress.inc(1); - } - for code in codes { - report.push(code); - } - Ok(report) - } - Err(ParserError::ParsingError(e)) => { - if hemtt_sqf::is_cba_settings(processed.as_str()) { - debug!("skipping apparent CBA settings file: {}", entry); - } else { - for error in e { - report.push(error); - } - } - Ok(report) - } - Err(ParserError::LexingError(e)) => { - for error in e { - report.push(error); - } - Ok(report) - } + if let Some(sqf) = checked.statements + && !checked.codes.failed() + { + let mut out = entry.with_extension("sqfc")?.create_file()?; + sqf.optimize().compile_to_writer(&processed, &mut out)?; + progress.inc(1); + } + for code in checked.codes { + report.push(code); } + Ok(report) }) .collect::, Error>>()?; for new_report in reports { diff --git a/hls/src/config/lints.rs b/hls/src/config/lints.rs index 61d54e4b6..bccb2e03c 100644 --- a/hls/src/config/lints.rs +++ b/hls/src/config/lints.rs @@ -79,53 +79,40 @@ async fn check_addon(source: WorkspacePath, workspace: EditorWorkspace) { Ok(processed) => { { let workspace_files = WorkspaceFiles::new(); - match hemtt_config::parse(workspace.config().as_ref(), &processed) { - Ok(report) => { - for code in report.warnings().iter().chain(report.errors().iter()) { - warn!("code: {:?}", code); - let Some(diag) = code.diagnostic() else { - continue; - }; - if diag.labels.iter().all(|l| l.file().is_include()) { - continue; - } - let lsp_diag = diag.to_lsp(&workspace_files); - for (file, diag) in lsp_diag { - lsp_diags.entry(file).or_insert_with(Vec::new).push(diag); - } - } - let config_analyzer = ConfigAnalyzer::get(); - config_analyzer.functions_defined.insert( - { - // `/folder/addon/blah` => addon - let parts: Vec<&str> = source.as_str().split('/').collect(); - if parts.len() < 3 { - warn!("Invalid config path: {}", source.as_str()); - if parts.len() == 2 { - parts[1].to_string() - } else { - source.as_str().to_string() - } + let checked = hemtt_config::check::check(&processed, workspace.config().as_ref()); + for code in &checked.codes { + let Some(diag) = code.diagnostic() else { + continue; + }; + // a diagnostic inside a vendored include is not actionable + // from the project, so it is not shown + if diag.labels.iter().all(|l| l.file().is_include()) { + continue; + } + let lsp_diag = diag.to_lsp(&workspace_files); + for (file, diag) in lsp_diag { + lsp_diags.entry(file).or_insert_with(Vec::new).push(diag); + } + } + if let Some(report) = checked.config { + let config_analyzer = ConfigAnalyzer::get(); + config_analyzer.functions_defined.insert( + { + // `/folder/addon/blah` => addon + let parts: Vec<&str> = source.as_str().split('/').collect(); + if parts.len() < 3 { + warn!("Invalid config path: {}", source.as_str()); + if parts.len() == 2 { + parts[1].to_string() } else { - parts[2].to_string() + source.as_str().to_string() } - }, - report.functions_defined().clone(), - ); - } - Err(err) => { - warn!("failed to process config: {:?}", err); - for error in err { - warn!("error: {:?}", error); - let Some(diag) = error.diagnostic() else { - continue; - }; - let lsp_diag = diag.to_lsp(&workspace_files); - for (file, diag) in lsp_diag { - lsp_diags.entry(file).or_insert_with(Vec::new).push(diag); + } else { + parts[2].to_string() } - } - } + }, + report.functions_defined().clone(), + ); } } let sources = processed.included_files().to_owned(); diff --git a/hls/src/sqf/lints.rs b/hls/src/sqf/lints.rs index 017007cc1..c603b9014 100644 --- a/hls/src/sqf/lints.rs +++ b/hls/src/sqf/lints.rs @@ -124,65 +124,35 @@ async fn check_sqf( Ok(processed) => { { let workspace_files = WorkspaceFiles::new(); - match hemtt_sqf::parser::run(&database, &processed) { - Ok(sqf) => { - let (codes, report) = hemtt_sqf::analyze::analyze( - &sqf, - workspace.config().as_ref(), - &processed, - addon.clone(), - database, - ); - if let Some(report) = report { - let cache = SqfAnalyzer::get(); - let mut functions_defined = cache - .functions_defined - .entry(addon.name().to_string()) - .or_insert_with(HashMap::new); - functions_defined.insert( - source.as_str().to_string(), - report.functions_defined().clone(), - ); - } - for code in codes { - let Some(diag) = code.diagnostic() else { - warn!("failed to get diagnostic"); - continue; - }; - if diag.labels.iter().all(|l| l.file().is_include()) { - continue; - } - let lsp_diag = diag.to_lsp(&workspace_files); - for (file, diag) in lsp_diag { - lsp_diags.entry(file).or_insert_with(Vec::new).push(diag); - } - } - } - Err(hemtt_sqf::parser::ParserError::ParsingError(e)) => { - if hemtt_sqf::is_cba_settings(processed.as_str()) { - debug!("skipping apparent CBA settings file: {}", source); - } else { - for error in e { - let Some(diag) = error.diagnostic() else { - continue; - }; - let diag = diag.to_lsp(&workspace_files); - for (file, diag) in diag { - lsp_diags.entry(file).or_insert_with(Vec::new).push(diag); - } - } - } + let checked = hemtt_sqf::check::check( + &processed, + workspace.config().as_ref(), + &addon, + database, + ); + if let Some(report) = checked.report { + let cache = SqfAnalyzer::get(); + let mut functions_defined = cache + .functions_defined + .entry(addon.name().to_string()) + .or_insert_with(HashMap::new); + functions_defined.insert( + source.as_str().to_string(), + report.functions_defined().clone(), + ); + } + for code in checked.codes { + let Some(diag) = code.diagnostic() else { + continue; + }; + // a diagnostic inside a vendored include is not actionable + // from the project, so it is not shown + if diag.labels.iter().all(|l| l.file().is_include()) { + continue; } - Err(e) => { - for error in e.codes() { - let Some(diag) = error.diagnostic() else { - continue; - }; - let diag = diag.to_lsp(&workspace_files); - for (file, diag) in diag { - lsp_diags.entry(file).or_insert_with(Vec::new).push(diag); - } - } + let lsp_diag = diag.to_lsp(&workspace_files); + for (file, diag) in lsp_diag { + lsp_diags.entry(file).or_insert_with(Vec::new).push(diag); } } } diff --git a/libs/config/src/check.rs b/libs/config/src/check.rs new file mode 100644 index 000000000..7bdf3bf2e --- /dev/null +++ b/libs/config/src/check.rs @@ -0,0 +1,111 @@ +//! Checking one preprocessed config file. +//! +//! Shared so the CLI and the language server cannot disagree about what a +//! file's diagnostics are. They have: preprocessor warnings and every note or +//! help were reported by the CLI and never shown in the editor, because each +//! side collected the codes it wanted separately. + +use hemtt_common::config::ProjectConfig; +use hemtt_workspace::reporting::{Codes, Processed}; + +use crate::{ConfigReport, parse}; + +/// The result of checking one preprocessed config file. +pub struct Checked { + /// Every diagnostic for the file: preprocessor warnings, then whatever + /// parsing produced, at every severity. + pub codes: Codes, + /// The parsed report, `None` if the file did not parse. + pub config: Option, +} + +#[must_use] +/// Check one preprocessed config file. +/// +/// Callers are left with what genuinely differs between them - the CLI +/// rapifies and pushes the report to the addon, the language server turns the +/// codes into LSP diagnostics. +pub fn check(processed: &Processed, project: Option<&ProjectConfig>) -> Checked { + // Preprocessor warnings belong to the file as much as lint codes do + let mut codes: Codes = processed.warnings().to_vec(); + match parse(project, processed) { + Ok(report) => { + // every severity, so a note or help cannot be dropped by one caller + codes.extend(report.codes().iter().cloned()); + Checked { + codes, + config: Some(report), + } + } + Err(errors) => { + codes.extend(errors); + Checked { + codes, + config: None, + } + } + } +} + +#[cfg(test)] +mod tests { + use hemtt_workspace::{Workspace, reporting::Codes, reporting::Processed}; + + use super::check; + + fn processed(contents: &str) -> Processed { + use std::io::Write; + let workspace = Workspace::builder() + .memory() + .finish(None, false, &hemtt_common::config::PDriveOption::Disallow) + .expect("workspace"); + let path = workspace.join("config.cpp").expect("join"); + let mut handle = path.create_file().expect("create"); + handle.write_all(contents.as_bytes()).expect("write"); + drop(handle); + hemtt_preprocessor::Processor::run( + &path, + &hemtt_common::config::PreprocessorOptions::default(), + ) + .expect("preprocesses") + } + + fn idents(codes: &Codes) -> Vec<&'static str> { + codes.iter().map(|code| code.ident()).collect() + } + + /// The CLI reported these and the editor did not, because each side + /// collected the codes it wanted separately. + #[test] + fn preprocessor_warnings_are_included() { + let processed = processed("#define A 1\n#define A 2\nclass x {};\n"); + let checked = check(&processed, None); + let codes = idents(&checked.codes); + assert!(codes.contains(&"PW1"), "{codes:?}"); + assert!(checked.config.is_some()); + } + + /// The editor took only `warnings()` and `errors()`, so a note or help + /// never reached it. + #[test] + fn every_severity_is_included() { + let processed = processed("class x {\n irDotSize = \"0.1/4\";\n};\n"); + let checked = check(&processed, None); + let codes = idents(&checked.codes); + assert!(codes.contains(&"L-C12"), "{codes:?}"); + } + + /// The parser recovers where it can, so a malformed file usually still + /// yields a report - what matters is that the errors come back either way. + #[test] + fn a_malformed_file_still_reports() { + let checked = check(&processed("class x {\n"), None); + assert!(!checked.codes.is_empty()); + assert!( + checked + .codes + .iter() + .any(|code| { code.severity() == hemtt_workspace::reporting::Severity::Error }) + ); + } +} diff --git a/libs/config/src/lib.rs b/libs/config/src/lib.rs index da67e899f..1ba158c58 100644 --- a/libs/config/src/lib.rs +++ b/libs/config/src/lib.rs @@ -10,6 +10,7 @@ use std::{ }; pub mod analyze; +pub mod check; pub mod display; mod model; pub mod parse; diff --git a/libs/sqf/src/check.rs b/libs/sqf/src/check.rs new file mode 100644 index 000000000..18eb24499 --- /dev/null +++ b/libs/sqf/src/check.rs @@ -0,0 +1,148 @@ +//! Checking one preprocessed SQF file. +//! +//! Shared so the CLI and the language server cannot disagree about what a +//! file's diagnostics are. They repeatedly have - see #1308 and #1309, both +//! of which were a rule applied in one copy of this pipeline and not the +//! other. + +use std::sync::Arc; + +use hemtt_common::config::ProjectConfig; +use hemtt_workspace::{ + addons::Addon, + reporting::{Codes, Processed}, +}; + +use crate::{ + Statements, + analyze::{SqfReport, analyze}, + parser::{ParserError, database::Database}, +}; + +/// The result of checking one preprocessed SQF file. +pub struct Checked { + /// Every diagnostic for the file: preprocessor warnings, then whatever + /// parsing or analysis produced. + pub codes: Codes, + /// The parsed statements, `None` if the file did not parse. + pub statements: Option, + /// The analysis report, `None` unless analysis ran. + pub report: Option, +} + +#[must_use] +/// Check one preprocessed SQF file. +/// +/// Callers are left with what genuinely differs between them - the CLI +/// compiles the statements and pushes the report to the addon, the language +/// server turns the codes into LSP diagnostics. +pub fn check( + processed: &Processed, + project: Option<&ProjectConfig>, + addon: &Arc, + database: Arc, +) -> Checked { + // Preprocessor warnings belong to the file as much as lint codes do + let mut codes: Codes = processed.warnings().to_vec(); + match crate::parser::run(&database, processed) { + Ok(statements) => { + let (lints, report) = analyze(&statements, project, processed, addon.clone(), database); + codes.extend(lints); + Checked { + codes, + statements: Some(statements), + report, + } + } + Err(ParserError::ParsingError(errors)) => { + // CBA settings files use `force` as a statement prefix and never + // parse; they are not meant to + if !crate::is_cba_settings(processed.as_str()) { + codes.extend(errors); + } + Checked { + codes, + statements: None, + report: None, + } + } + Err(ParserError::LexingError(errors)) => { + codes.extend(errors); + Checked { + codes, + statements: None, + report: None, + } + } + } +} + +#[cfg(test)] +mod tests { + use std::sync::Arc; + + use hemtt_workspace::{ + Workspace, + addons::Addon, + reporting::{Codes, Processed}, + }; + + use super::check; + use crate::parser::database::Database; + + fn processed(contents: &str) -> Processed { + use std::io::Write; + let workspace = Workspace::builder() + .memory() + .finish(None, false, &hemtt_common::config::PDriveOption::Disallow) + .expect("workspace"); + let path = workspace.join("test.sqf").expect("join"); + let mut handle = path.create_file().expect("create"); + handle.write_all(contents.as_bytes()).expect("write"); + drop(handle); + hemtt_preprocessor::Processor::run( + &path, + &hemtt_common::config::PreprocessorOptions::default(), + ) + .expect("preprocesses") + } + + fn checked(contents: &str) -> super::Checked { + check( + &processed(contents), + None, + &Arc::new(Addon::test_addon()), + Arc::new(Database::a3(false)), + ) + } + + fn idents(codes: &Codes) -> Vec<&'static str> { + codes.iter().map(|code| code.ident()).collect() + } + + /// The CLI reported these and the editor did not, because each side + /// collected the codes it wanted separately. + #[test] + fn preprocessor_warnings_are_included() { + let checked = checked("#define A 1\n#define A 2\nprivate _x = 1;\n"); + let codes = idents(&checked.codes); + assert!(codes.contains(&"PW1"), "{codes:?}"); + assert!(checked.statements.is_some()); + } + + /// CBA settings files use `force` as a statement prefix and never parse. + /// They are not meant to, so nothing is reported. + #[test] + fn cba_settings_are_skipped() { + let checked = checked("force ace_medical_level = 2;\n"); + assert!(checked.codes.is_empty(), "{:?}", idents(&checked.codes)); + assert!(checked.statements.is_none()); + } + + #[test] + fn a_file_that_does_not_parse_has_no_statements() { + let checked = checked("private _x = ;\n"); + assert!(checked.statements.is_none()); + assert!(!checked.codes.is_empty()); + } +} diff --git a/libs/sqf/src/lib.rs b/libs/sqf/src/lib.rs index bd7e4a0c0..edbb4f3b9 100644 --- a/libs/sqf/src/lib.rs +++ b/libs/sqf/src/lib.rs @@ -1,3 +1,5 @@ +#[cfg(feature = "parser")] +pub mod check; #[cfg(feature = "compiler")] pub mod compiler; #[cfg(feature = "parser")] diff --git a/libs/workspace/src/reporting/diagnostic/mod.rs b/libs/workspace/src/reporting/diagnostic/mod.rs index 3f9e9b7f3..2d3c2655a 100644 --- a/libs/workspace/src/reporting/diagnostic/mod.rs +++ b/libs/workspace/src/reporting/diagnostic/mod.rs @@ -318,7 +318,20 @@ impl Diagnostic { use tower_lsp::lsp_types::Url; let mut diags = Vec::new(); + // One diagnostic per primary label. A secondary label is context for + // the primary - "previous definition here" and the like - and carries + // its own message, so emitting it as its own diagnostic would repeat + // the parent's message somewhere it does not apply. If a code somehow + // has no primary label, fall back to all of them rather than dropping + // the diagnostic entirely. + let has_primary = self + .labels + .iter() + .any(|label| label.style == LabelStyle::Primary); for label in &self.labels { + if has_primary && label.style != LabelStyle::Primary { + continue; + } let start = label.span.start; let end = label.span.end; let start_line_index = files.line_index(&label.file, start).unwrap_or(0); @@ -373,3 +386,60 @@ const fn severity_to_lsp(severity: Severity) -> tower_lsp::lsp_types::Diagnostic Severity::Help | Severity::Note => tower_lsp::lsp_types::DiagnosticSeverity::INFORMATION, } } + +#[cfg(all(test, feature = "lsp"))] +mod tests { + use super::{Diagnostic, Label}; + use crate::{ + Workspace, WorkspacePath, + reporting::{Severity, files::WorkspaceFiles}, + }; + + fn file(contents: &str) -> WorkspacePath { + use std::io::Write; + let workspace = Workspace::builder() + .memory() + .finish(None, false, &hemtt_common::config::PDriveOption::Disallow) + .expect("workspace"); + let path = workspace.join("test.hpp").expect("join"); + let mut handle = path.create_file().expect("create"); + handle.write_all(contents.as_bytes()).expect("write"); + drop(handle); + path + } + + /// A secondary label is context for the primary, not a diagnostic of its + /// own - emitting it as one repeats the parent message somewhere it does + /// not apply. `PW1` marked the previous definition "redefining macro". + #[test] + fn secondary_labels_are_not_their_own_diagnostic() { + let path = file("line one\nline two\nline three\n"); + let diag = Diagnostic::new("PW1", "redefining macro") + .set_severity(Severity::Warning) + .with_label(Label::primary(path.clone(), 9..17)) + .with_label(Label::secondary(path, 0..8).with_message("previous definition here")); + + let lsp = diag.to_lsp(&WorkspaceFiles::new()); + assert_eq!(lsp.len(), 1, "{lsp:?}"); + assert_eq!(lsp[0].1.range.start.line, 1, "should be the primary label"); + assert_eq!(lsp[0].1.message, "redefining macro"); + } + + /// Two primaries are two separate places the same problem occurs. + #[test] + fn every_primary_label_is_a_diagnostic() { + let path = file("line one\nline two\nline three\n"); + let diag = Diagnostic::new("X", "problem") + .with_label(Label::primary(path.clone(), 0..8)) + .with_label(Label::primary(path, 9..17)); + assert_eq!(diag.to_lsp(&WorkspaceFiles::new()).len(), 2); + } + + /// Without a primary the diagnostic would otherwise vanish. + #[test] + fn secondary_only_still_reports() { + let path = file("line one\n"); + let diag = Diagnostic::new("X", "problem").with_label(Label::secondary(path, 0..8)); + assert_eq!(diag.to_lsp(&WorkspaceFiles::new()).len(), 1); + } +}