sqf: prevent changing reserved variables (#804)

libs/sqf/src/analyze/lints/s18_in_vehicle_check.rs

@@ -150,12 +150,12 @@ impl Code for CodeS18InVehicleCheck {
 
 impl CodeS18InVehicleCheck {
     #[must_use]
-    pub fn new(span: Range<usize>, processed: &Processed, severity: Severity, var: String, negated: bool) -> Self {
+    pub fn new(span: Range<usize>, processed: &Processed, severity: Severity, ident: String, negated: bool) -> Self {
         Self {
             span,
             severity,
             diagnostic: None,
-            ident: var,
+            ident,
             negated,
         }
         .generate_processed(processed)

libs/sqf/src/analyze/lints/s22_this_call.rs

@@ -8,9 +8,9 @@ use hemtt_workspace::{
 
 use crate::{analyze::SqfLintData, BinaryCommand, Expression};
 
-crate::lint!(LintS22InVehicleCheck);
+crate::lint!(LintS22ThisCall);
 
-impl Lint<SqfLintData> for LintS22InVehicleCheck {
+impl Lint<SqfLintData> for LintS22ThisCall {
     fn ident(&self) -> &str {
         "this_call"
     }

libs/sqf/src/analyze/lints/s23_reassign_reserved_variable.rs

@@ -0,0 +1,299 @@
+use std::{collections::HashMap, ops::Range, sync::Arc};
+
+use hemtt_common::config::LintConfig;
+use hemtt_workspace::{
+    lint::{AnyLintRunner, Lint, LintRunner},
+    reporting::{Code, Codes, Diagnostic, Label, Processed, Severity}, WorkspacePath,
+};
+
+use crate::{analyze::SqfLintData, BinaryCommand, Expression, Statement, UnaryCommand};
+
+crate::lint!(LintS23ReassignReservedVariable);
+
+impl Lint<SqfLintData> for LintS23ReassignReservedVariable {
+    fn ident(&self) -> &str {
+        "reasign_reserved_variable"
+    }
+
+    fn sort(&self) -> u32 {
+        230
+    }
+
+    fn description(&self) -> &str {
+        "Prevents reassigning reserved variables"
+    }
+
+    fn documentation(&self) -> &str {
+        r"### Example
+
+**Incorrect**
+```sqf
+call {
+    _this = 1;
+};
+```
+
+```sqf
+{
+    private _forEachIndex = random 5;
+} forEach allUnits;
+
+### Explanation
+
+Reassigning reserved variables can lead to unintentional behavior.
+"
+    }
+
+    fn default_config(&self) -> LintConfig {
+        LintConfig::error()
+    }
+
+    fn runners(&self) -> Vec<Box<dyn AnyLintRunner<SqfLintData>>> {
+        vec![Box::new(StatementsRunner {}), Box::new(ExpressionRunner {})]
+    }
+}
+
+static RESERVED: [&str; 8] = [
+    "this","_this","_forEachIndex","_exception","_thisScript","_thisFSM","thisList","thisTrigger",
+];
+
+struct StatementsRunner {}
+impl LintRunner<SqfLintData> for StatementsRunner {
+    type Target = crate::Statements;
+
+    #[allow(clippy::significant_drop_tightening)]
+    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 mut just_saved: Option<(String, String, Range<usize>)> = None;
+        let mut codes: Codes = Vec::new();
+        let mut need_to_restore: HashMap<String, (String, Range<usize>, Range<usize>)> = HashMap::new();
+
+        for statement in target.content() {
+            let (Statement::AssignGlobal(var, exp, span) | Statement::AssignLocal(var, exp, span)) = statement else {
+                just_saved.take();
+                continue
+            };
+
+            if let Some((saved, original, saved_span)) = just_saved.as_ref() {
+                if saved == var {
+                    need_to_restore.insert(original.to_string(), (saved.to_string(), span.clone(), saved_span.clone()));
+                    just_saved.take();
+                    continue
+                }
+            }
+
+            if let Some((saved, original_save, saved_span)) = need_to_restore.get(var) {
+                codes.push(Arc::new(CodeS23ReassignReservedVariable::new(Variant::SavedWhileSaved(var.to_string(), span.clone(), original_save.clone(), saved.clone(), saved_span.clone()), processed, config.severity())));
+            }
+
+            if let Expression::Variable(restoring, _) = exp {
+                if need_to_restore.remove(restoring).is_some() {
+                    continue
+                }
+            }
+
+            if RESERVED.contains(&var.as_str()) {
+                codes.push(Arc::new(CodeS23ReassignReservedVariable::new(Variant::Overwrite(var.to_string(), span.clone()), processed, config.severity())));
+            } else if let Expression::Variable(saved, new_saved_span) = exp {
+                if RESERVED.contains(&saved.as_str()) {
+                    just_saved.replace((saved.to_string(), var.to_string(), new_saved_span.clone()));
+                    continue
+                }
+            }
+        }
+
+        for (saved, (original, span, saved_span)) in need_to_restore {
+            codes.push(Arc::new(CodeS23ReassignReservedVariable::new(Variant::NeverRestored((saved, span.clone()), (original, saved_span.clone())), processed, config.severity())));
+        }
+
+        codes
+    }
+}
+
+struct ExpressionRunner {}
+impl LintRunner<SqfLintData> for ExpressionRunner {
+    type Target = crate::Expression;
+
+    #[allow(clippy::significant_drop_tightening)]
+    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 exp = match target {
+            Expression::UnaryCommand(UnaryCommand::Named(cmd), exp, _) |
+            Expression::BinaryCommand(BinaryCommand::Named(cmd), _, exp, _) if cmd.to_lowercase() == "params" => exp,
+            _ => return Vec::new(),
+        };
+
+        if let Expression::Array(values, _) | Expression::ConsumeableArray(values, _) = &**exp {
+            for param in values {
+                let (name, span) = match &param {
+                    Expression::String(name, span, _) => (name, span),
+                    Expression::Array(values, _) => {
+                        if let Some(Expression::String(name, span, _)) = values.first() {
+                            (name, span)
+                        } else {
+                            continue
+                        }
+                    }
+                    _ => continue,
+                };
+                if RESERVED.contains(&&**name) {
+                    return vec![Arc::new(CodeS23ReassignReservedVariable::new(Variant::Overwrite(name.to_string(), span.clone()), processed, config.severity()))];
+                }
+            }
+        }
+
+        Vec::new()
+    }
+}
+
+pub enum Variant {
+    Overwrite(String, Range<usize>),
+    NeverRestored((String, Range<usize>),(String, Range<usize>)),
+    SavedWhileSaved(String, Range<usize>, Range<usize>, String, Range<usize>),
+}
+
+impl Variant {
+    #[must_use]
+    pub fn span(&self) -> Range<usize> {
+        match self {
+            Self::Overwrite(_, span) |
+            Self::NeverRestored((_, span), _) |
+            Self::SavedWhileSaved(_, span, _, _, _) => span.clone(),
+        }
+    }
+}
+
+#[allow(clippy::module_name_repetitions)]
+pub struct CodeS23ReassignReservedVariable {
+    variant: Variant,
+    severity: Severity,
+    diagnostic: Option<Diagnostic>,
+}
+
+impl Code for CodeS23ReassignReservedVariable {
+    fn ident(&self) -> &'static str {
+        "L-S23"
+    }
+
+    fn link(&self) -> Option<&str> {
+        Some("/analysis/sqf.html#reasign_reserved_variable")
+    }
+
+    fn severity(&self) -> Severity {
+        self.severity
+    }
+
+    fn message(&self) -> String {
+        match &self.variant {
+            Variant::Overwrite(var, _) => format!("Reassigning reserved variable `{var}`"),
+            Variant::NeverRestored((saved, _), (original, _)) => format!("Reserved variable `{original}` was never restored after being saved to `{saved}`"),
+            Variant::SavedWhileSaved(saved, _, _, original, _) => format!("Holder variable `{saved}` is overwritten before restoring `{original}`"),
+        }
+    }
+
+    fn label_message(&self) -> String {
+        String::new()
+    }
+
+    fn diagnostic(&self) -> Option<Diagnostic> {
+        self.diagnostic.clone()
+    }
+}
+
+impl CodeS23ReassignReservedVariable {
+    #[must_use]
+    pub fn new(variant: Variant, processed: &Processed, severity: Severity) -> Self {
+        Self {
+            variant,
+            severity,
+            diagnostic: None,
+        }
+        .generate_processed(processed)
+    }
+
+    fn generate_processed(mut self, processed: &Processed) -> Self {
+        let Some(mut diag) = Diagnostic::new_for_processed(&self, self.variant.span(), processed) else {
+            return self
+        };
+        diag = diag.clear_labels();
+        match &self.variant {
+            Variant::Overwrite(var, span) => {
+                let Some(info) = get_span_info(span.clone(), processed) else {
+                    return self;
+                };
+                diag = diag.with_label(Label::primary(
+                    info.0,
+                    info.1,
+                ).with_message(format!("`{var}` is reserved")));
+            }
+            Variant::NeverRestored((saved, saved_span), (original, original_span)) => {
+                let Some(saved_info) = get_span_info(saved_span.clone(), processed) else {
+                    return self;
+                };
+                let Some(original_info) = get_span_info(original_span.clone(), processed) else {
+                    return self;
+                };
+                diag = diag.with_label(Label::secondary(
+                    saved_info.0,
+                    saved_info.1,
+                ).with_message(format!("`{original}` is modified here"))).with_label(Label::primary(
+                    original_info.0,
+                    original_info.1,
+                ).with_message(format!("`{saved}` is never restored to `{original}`")));
+            }
+            Variant::SavedWhileSaved(saved, saved_span, changed_span, original, original_span) => {
+                let Some(saved_info) = get_span_info(saved_span.clone(), processed) else {
+                    return self;
+                };
+                let Some(changed_span) = get_span_info(changed_span.clone(), processed) else {
+                    return self;
+                };
+                let Some(original_info) = get_span_info(original_span.clone(), processed) else {
+                    return self;
+                };
+                diag = diag.with_label(Label::primary(
+                    saved_info.0,
+                    saved_info.1,
+                ).with_message(format!("`{saved}` is overwritten before restoring `{original}`"))).with_label(Label::secondary(
+                    changed_span.0,
+                    changed_span.1,
+                ).with_message(format!("`{original}` is changed"))).with_label(Label::secondary(
+                    original_info.0,
+                    original_info.1,
+                ).with_message(format!("`{saved}` saves the state of {original}")));
+            }
+        }
+        self.diagnostic = Some(diag);
+        self
+    }
+}
+
+fn get_span_info(span: Range<usize>, processed: &Processed) -> Option<(WorkspacePath, Range<usize>)> {
+    let map_start = processed.mapping(span.start)?;
+    let map_end = processed.mapping(span.end)?;
+    let map_file = processed.source(map_start.source())?;
+    Some((
+        map_file.0.clone(),
+        map_start.original_start()..map_end.original_start(),
+    ))
+}

libs/sqf/tests/lints.rs

@@ -87,3 +87,4 @@ analyze!(s18_in_vehicle_check);
 analyze!(s20_bool_static_comparison);
 analyze!(s21_invalid_comparisons);
 analyze!(s22_this_call);
+analyze!(s23_reassign_reserved_variable);

libs/sqf/tests/lints/s23_reassign_reserved_variable/source.sqf

@@ -0,0 +1,33 @@
+// error for overwriting
+call {
+    _this = 123;
+};
+
+// error for overwriting
+call {
+    params ["_this"];
+};
+
+// error for not restoring
+call {
+    private _savedThis = _this;
+    _this = 123;
+    call _my_fnc;
+};
+
+// error for saving while still saved
+call {
+    private _savedThis = _this;
+    _this = 123;
+    private _savedThis = 1234;
+    call _my_fnc;
+    _this = _savedThis;
+};
+
+// no error
+call {
+    private _savedThis = _this;
+    _this = 123;
+    call _my_fnc;
+    _this = _savedThis;
+};

libs/sqf/tests/lints/s23_reassign_reserved_variable/stdout.ansi

@@ -0,0 +1,33 @@
+error[L-S23]: Reassigning reserved variable `_this`
+  ┌─ source.sqf:3:5
+  │
+3 │     _this = 123;
+  │     ^^^^^^^^^^^ `_this` is reserved
+
+
+error[L-S23]: Reassigning reserved variable `_this`
+  ┌─ source.sqf:8:13
+  │
+8 │     params ["_this"];
+  │             ^^^^^^^ `_this` is reserved
+
+
+error[L-S23]: Reserved variable `_this` was never restored after being saved to `_savedThis`
+   ┌─ source.sqf:13:26
+   │
+13 │     private _savedThis = _this;
+   │                          ^^^^^ `_savedThis` is never restored to `_this`
+14 │     _this = 123;
+   │     ----------- `_this` is modified here
+
+
+error[L-S23]: Holder variable `_savedThis` is overwritten before restoring `_this`
+   ┌─ source.sqf:22:5
+   │
+20 │     private _savedThis = _this;
+   │                          ----- `_savedThis` saves the state of _this
+21 │     _this = 123;
+   │     ----------- `_this` is changed
+22 │     private _savedThis = 1234;
+   │     ^^^^^^^^^^^^^^^^^^^^^^^^^ `_savedThis` is overwritten before restoring `_this`
+