diff --git a/bin/src/modules/rapifier.rs b/bin/src/modules/rapifier.rs index 8978eb43..93091dfa 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 f193a6d2..9f3fa1ee 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 61d54e4b..bccb2e03 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 017007cc..c603b901 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 00000000..7bdf3bf2 --- /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 da67e899..1ba158c5 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 00000000..18eb2449 --- /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 bd7e4a0c..edbb4f3b 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 3f9e9b7f..2d3c2655 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); + } +}