From c823e30747b96e6285e14d9396e99c5a813fe791 Mon Sep 17 00:00:00 2001 From: PabstMirror Date: Tue, 15 Oct 2024 03:05:25 -0500 Subject: [PATCH] sqf: Add lint for vars that are ALL_CAPS and not a macro (#793) --- libs/preprocessor/src/processor/directives.rs | 4 + libs/preprocessor/src/processor/mod.rs | 6 +- .../sqf/src/analyze/lints/s17_var_all_caps.rs | 189 ++++++++++++++++++ libs/sqf/tests/lints.rs | 1 + libs/sqf/tests/lints/project_tests.toml | 6 + .../tests/lints/s17_var_all_caps/source.sqf | 9 + .../tests/lints/s17_var_all_caps/stdout.ansi | 30 +++ libs/workspace/src/reporting/processed.rs | 17 +- 8 files changed, 251 insertions(+), 11 deletions(-) create mode 100644 libs/sqf/src/analyze/lints/s17_var_all_caps.rs create mode 100644 libs/sqf/tests/lints/s17_var_all_caps/source.sqf create mode 100644 libs/sqf/tests/lints/s17_var_all_caps/stdout.ansi diff --git a/libs/preprocessor/src/processor/directives.rs b/libs/preprocessor/src/processor/directives.rs index b911c4c7..29d3ce9a 100644 --- a/libs/preprocessor/src/processor/directives.rs +++ b/libs/preprocessor/src/processor/directives.rs @@ -289,6 +289,10 @@ impl Processor { }; #[cfg(feature = "lsp")] self.usage.insert(ident.position().clone(), Vec::new()); + self.macros + .entry(ident_string.clone()) + .or_default() + .push(ident.position().clone()); self.defines.insert( &ident_string, ( diff --git a/libs/preprocessor/src/processor/mod.rs b/libs/preprocessor/src/processor/mod.rs index d54c6b78..3071d2af 100644 --- a/libs/preprocessor/src/processor/mod.rs +++ b/libs/preprocessor/src/processor/mod.rs @@ -1,4 +1,3 @@ -#[cfg(feature = "lsp")] use std::collections::HashMap; use std::rc::Rc; use std::sync::Arc; @@ -36,6 +35,8 @@ pub struct Processor { pub(crate) token_count: usize, + macros: HashMap>, + #[cfg(feature = "lsp")] /// Map of token usage to definition /// (token, definition) @@ -94,10 +95,9 @@ impl Processor { Processed::new( buffer, + processor.macros, #[cfg(feature = "lsp")] processor.usage, - #[cfg(feature = "lsp")] - processor.declarations, processor.warnings, processor.no_rapify, ) diff --git a/libs/sqf/src/analyze/lints/s17_var_all_caps.rs b/libs/sqf/src/analyze/lints/s17_var_all_caps.rs new file mode 100644 index 00000000..3f75a5b4 --- /dev/null +++ b/libs/sqf/src/analyze/lints/s17_var_all_caps.rs @@ -0,0 +1,189 @@ +use std::{ops::Range, sync::Arc}; + +use hemtt_common::{config::LintConfig, similar_values}; +use hemtt_workspace::{ + lint::{AnyLintRunner, Lint, LintRunner}, + reporting::{Code, Codes, Diagnostic, Label, Processed, Severity, Symbol}, +}; + +use crate::{analyze::SqfLintData, Expression}; + +crate::lint!(LintS17VarAllCaps); + +impl Lint for LintS17VarAllCaps { + fn ident(&self) -> &str { + "var_all_caps" + } + + fn sort(&self) -> u32 { + 170 + } + + fn description(&self) -> &str { + "Checks for global variables that are ALL_CAPS and may actually be a undefined macro" + } + + fn documentation(&self) -> &str { + r#"### Configuration + +- **ignore**: An array of vars to ignore + +```toml +[lints.sqf.var_all_caps] +options.ignore = [ + "XMOD_TEST", "MYMOD_*", +] +``` + +### Example + +**Incorrect** +```sqf +private _z = _y + DO_NOT_EXIST; +``` + +### Explanation + +Variables that are all caps are usually reserved for macros. This should should help prevent any accidental typos or uses before definitions when using macros. +."# + } + fn default_config(&self) -> LintConfig { + LintConfig::help() + } + fn runners(&self) -> Vec>> { + vec![Box::new(Runner)] + } +} + +struct Runner; +impl LintRunner for Runner { + type Target = crate::Expression; + + fn run( + &self, + _project: Option<&hemtt_common::config::ProjectConfig>, + config: &LintConfig, + processed: Option<&hemtt_workspace::reporting::Processed>, + target: &Self::Target, + _data: &SqfLintData, + ) -> Codes { + let Some(processed) = processed else { + return Vec::new(); + }; + let Expression::Variable(var, span) = target else { + return Vec::new(); + }; + if var.starts_with('_') || &var.to_ascii_uppercase() != var || var == "SLX_XEH_COMPILE_NEW" + { + return Vec::new(); + } + if let Some(toml::Value::Array(ignore)) = config.option("ignore") { + if ignore.iter().any(|i| { + let s = i.as_str().unwrap_or_default(); + s == var || (s.ends_with('*') && var.starts_with(&s[0..s.len() - 1])) + }) { + return Vec::new(); + } + } + vec![Arc::new(CodeS17VarAllCaps::new( + span.clone(), + var.clone(), + processed, + config.severity(), + ))] + } +} + +#[allow(clippy::module_name_repetitions)] +pub struct CodeS17VarAllCaps { + span: Range, + ident: String, + similar: Vec, + severity: Severity, + diagnostic: Option, +} + +impl Code for CodeS17VarAllCaps { + fn ident(&self) -> &'static str { + "L-S17" + } + + fn link(&self) -> Option<&str> { + Some("/analysis/sqf.html#var_all_caps") + } + + fn severity(&self) -> Severity { + self.severity + } + + fn message(&self) -> String { + format!("Variable should not be all caps: {}", self.ident) + } + + fn note(&self) -> Option { + Some("All caps variables are usually reserved for macros".to_string()) + } + + fn label_message(&self) -> String { + "All caps variable".to_string() + } + + fn help(&self) -> Option { + if self.similar.is_empty() { + None + } else { + Some(format!("did you mean `{}`?", self.similar.join("`, `"))) + } + } + + fn diagnostic(&self) -> Option { + self.diagnostic.clone() + } +} + +impl CodeS17VarAllCaps { + #[must_use] + pub fn new(span: Range, ident: String, processed: &Processed, severity: Severity) -> Self { + Self { + similar: similar_values( + &ident, + &processed.macros().keys().map(std::string::String::as_str).collect::>(), + ) + .iter() + .map(std::string::ToString::to_string) + .collect(), + span, + ident, + severity, + diagnostic: None, + } + .generate_processed(processed) + } + + fn generate_processed(mut self, processed: &Processed) -> Self { + let Some(mut diagnostic) = Diagnostic::new_for_processed(&self, self.span.clone(), processed) else { + return self; + }; + self.diagnostic = Some(diagnostic.clone()); + let mut mappings = processed.mappings(self.span.start); + mappings.pop(); + let symbol = Symbol::Word(self.ident.clone()); + let Some(mapping) = mappings + .iter() + .find(|m| { + m.token().symbol() == &symbol + }) else { + return self; + }; + if let Some(l) = diagnostic.labels.get_mut(0) { *l = l.clone().with_message("Used in macro here"); } + diagnostic.labels.push( + Label::primary( + mapping.original().path().clone(), + mapping.original().span(), + ) + .with_message("All caps variable"), + ); + self.diagnostic = Some(diagnostic); + self + } +} diff --git a/libs/sqf/tests/lints.rs b/libs/sqf/tests/lints.rs index adc15251..0ad88968 100644 --- a/libs/sqf/tests/lints.rs +++ b/libs/sqf/tests/lints.rs @@ -82,4 +82,5 @@ analyze!(s07_select_parse_number); analyze!(s08_format_args); analyze!(s09_banned_command); analyze!(s11_if_not_else); +analyze!(s17_var_all_caps); analyze!(s18_in_vehicle_check); diff --git a/libs/sqf/tests/lints/project_tests.toml b/libs/sqf/tests/lints/project_tests.toml index bf02cf4a..1f9b267b 100644 --- a/libs/sqf/tests/lints/project_tests.toml +++ b/libs/sqf/tests/lints/project_tests.toml @@ -8,3 +8,9 @@ options.ignore = [ [lints.sqf] if_not_else = true + +[lints.sqf.var_all_caps] +enabled = true +options.ignore = [ + "AM_IGNORED", +] diff --git a/libs/sqf/tests/lints/s17_var_all_caps/source.sqf b/libs/sqf/tests/lints/s17_var_all_caps/source.sqf new file mode 100644 index 00000000..7b3a5b96 --- /dev/null +++ b/libs/sqf/tests/lints/s17_var_all_caps/source.sqf @@ -0,0 +1,9 @@ +#define EXIST 1 +#define TYPO 2 +#define NESTED systemChat UNDEFINED + +private _x = 1 + EXIST; +private _y = _x + AM_IGNORED; +private _z = _y + DO_NOT_EXIST; +private _w = _z + TPYO; +NESTED; diff --git a/libs/sqf/tests/lints/s17_var_all_caps/stdout.ansi b/libs/sqf/tests/lints/s17_var_all_caps/stdout.ansi new file mode 100644 index 00000000..1751eeb5 --- /dev/null +++ b/libs/sqf/tests/lints/s17_var_all_caps/stdout.ansi @@ -0,0 +1,30 @@ +help[L-S17]: Variable should not be all caps: DO_NOT_EXIST + ┌─ source.sqf:7:19 + │ +7 │ private _z = _y + DO_NOT_EXIST; + │ ^^^^^^^^^^^^ All caps variable + │ + = note: All caps variables are usually reserved for macros + + +help[L-S17]: Variable should not be all caps: TPYO + ┌─ source.sqf:8:19 + │ +8 │ private _w = _z + TPYO; + │ ^^^^ All caps variable + │ + = note: All caps variables are usually reserved for macros + = help: did you mean `TYPO`? + + +help[L-S17]: Variable should not be all caps: UNDEFINED + ┌─ source.sqf:3:27 + │ +3 │ #define NESTED systemChat UNDEFINED + │ ^^^^^^^^^ All caps variable + · +9 │ NESTED; + │ ^^^^^^ Used in macro here + │ + = note: All caps variables are usually reserved for macros + diff --git a/libs/workspace/src/reporting/processed.rs b/libs/workspace/src/reporting/processed.rs index 621e24a7..4cac5f0e 100644 --- a/libs/workspace/src/reporting/processed.rs +++ b/libs/workspace/src/reporting/processed.rs @@ -22,11 +22,7 @@ pub struct Processed { /// string offset(start, stop), source, source position mappings: Vec, - #[allow(dead_code)] - #[cfg(feature = "lsp")] - /// Map of token usage to definition - /// (token, definition) - declarations: HashMap, + macros: HashMap>, #[allow(dead_code)] #[cfg(feature = "lsp")] @@ -179,14 +175,13 @@ impl Processed { /// [`Error::Workspace`] if a workspace path could not be read pub fn new( output: Vec, + macros: HashMap>, #[cfg(feature = "lsp")] usage: HashMap>, - #[cfg(feature = "lsp")] declarations: HashMap, warnings: Codes, no_rapify: bool, ) -> Result { let mut processed = Self { - #[cfg(feature = "lsp")] - declarations, + macros, #[cfg(feature = "lsp")] usage, warnings, @@ -258,6 +253,12 @@ impl Processed { self.mappings(offset).last().copied() } + #[must_use] + /// Get the macros defined + pub const fn macros(&self) -> &HashMap> { + &self.macros + } + #[must_use] #[allow(clippy::cast_possible_truncation)] /// Get offset as number of raw bytes into the output string