-
-
Notifications
You must be signed in to change notification settings - Fork 52
Add S52 lint rule for detecting duplicate cases in switch statements #1322
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
Merged
BrettMayson
merged 4 commits into
main
from
copilot/suggestion-lint-detect-duplicated-cases
Aug 31, 2026
Merged
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,267 @@ | ||
| use std::{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::LintData, BinaryCommand, Expression, Statement, UnaryCommand}; | ||
|
|
||
| crate::analyze::lint!(LintS52DuplicateCase); | ||
|
|
||
| impl Lint<LintData> for LintS52DuplicateCase { | ||
| fn ident(&self) -> &'static str { | ||
| "duplicate_case" | ||
| } | ||
|
|
||
| fn sort(&self) -> u32 { | ||
| 520 | ||
| } | ||
|
|
||
| fn description(&self) -> &'static str { | ||
| "Checks for duplicate cases in switch statements" | ||
| } | ||
|
|
||
| fn documentation(&self) -> &'static str { | ||
| r#"### Example | ||
|
|
||
| **Incorrect** | ||
| ```sqf | ||
| switch (_value) do { | ||
| case 1: { "one" }; | ||
| case 2: { "two" }; | ||
| case 1: { "one again" }; | ||
| }; | ||
| ``` | ||
|
|
||
| **Correct** | ||
| ```sqf | ||
| switch (_value) do { | ||
| case 1: { "one" }; | ||
| case 2: { "two" }; | ||
| case 3: { "three" }; | ||
| }; | ||
| ``` | ||
|
|
||
| ### Explanation | ||
|
|
||
| Having duplicate case labels in a switch statement is likely a mistake. Only the first case will be executed, and the duplicate case will never be reached. | ||
| "# | ||
| } | ||
|
|
||
| fn default_config(&self) -> LintConfig { | ||
| LintConfig::help() | ||
| } | ||
|
|
||
| fn runners(&self) -> Vec<Box<dyn AnyLintRunner<LintData>>> { | ||
| vec![Box::new(Runner)] | ||
| } | ||
| } | ||
|
|
||
| struct Runner; | ||
| impl LintRunner<LintData> for Runner { | ||
| type Target = crate::Expression; | ||
|
|
||
| fn run( | ||
| &self, | ||
| _project: Option<&hemtt_common::config::ProjectConfig>, | ||
| config: &LintConfig, | ||
| processed: Option<&hemtt_workspace::reporting::Processed>, | ||
| _runtime: &hemtt_common::config::RuntimeArguments, | ||
| target: &Self::Target, | ||
| _data: &LintData, | ||
| ) -> Codes { | ||
| let Some(processed) = processed else { | ||
| return Vec::new(); | ||
| }; | ||
|
|
||
| // Look for switch (expr) do { ... } | ||
| let Expression::BinaryCommand(BinaryCommand::Named(cmd), lhs, rhs, _) = target else { | ||
| return Vec::new(); | ||
| }; | ||
|
|
||
| if !cmd.eq_ignore_ascii_case("do") { | ||
| return Vec::new(); | ||
| } | ||
|
|
||
| // Check if this is a switch statement | ||
| let Expression::UnaryCommand(UnaryCommand::Named(unary), _, _) = lhs.as_ref() else { | ||
| return Vec::new(); | ||
| }; | ||
|
|
||
| if !unary.eq_ignore_ascii_case("switch") { | ||
| return Vec::new(); | ||
| } | ||
|
|
||
| // The right side should be a Code block containing case statements | ||
| let Expression::Code(body) = rhs.as_ref() else { | ||
| return Vec::new(); | ||
| }; | ||
|
|
||
| // Check if this is a switch statement by looking for case expressions | ||
| let mut case_values: Vec<(String, Range<usize>)> = Vec::new(); | ||
| let mut codes: Codes = Vec::new(); | ||
|
|
||
| for body_statement in body.content() { | ||
| let Statement::Expression(case_expr, _) = body_statement else { | ||
| continue; | ||
| }; | ||
|
|
||
| // Try to find case VALUE: EXPR pattern | ||
| // This is structured as: case VALUE : EXPR | ||
| // Where "case" is typically a unary command or binary command | ||
| // and ":" is an Associate operator | ||
|
|
||
| // Pattern 1: case VALUE : EXPR (where case is unary) | ||
| if let Expression::BinaryCommand(BinaryCommand::Associate, left, _right, _) = case_expr { | ||
| // Left side should be (case VALUE) - could be unary or binary | ||
| if let Expression::UnaryCommand(UnaryCommand::Named(name), value_expr, _) = left.as_ref() { | ||
| if name.eq_ignore_ascii_case("case") { | ||
| let case_source = value_expr.source(false); | ||
| let case_span = value_expr.span(); | ||
|
|
||
| // Check if we've seen this value before | ||
| for (existing_value, existing_span) in &case_values { | ||
| if existing_value == &case_source { | ||
| // Found a duplicate! | ||
| codes.push(Arc::new(CodeS52DuplicateCase::new( | ||
| case_span.clone(), | ||
| case_source.clone(), | ||
| existing_span.clone(), | ||
| processed, | ||
| config.severity(), | ||
| ))); | ||
| } | ||
| } | ||
|
|
||
| case_values.push((case_source, case_span)); | ||
| continue; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Pattern 2: case VALUE do EXPR (where case is binary) | ||
|
BrettMayson marked this conversation as resolved.
Outdated
|
||
| if let Expression::BinaryCommand(BinaryCommand::Named(cmd_name), inner_left, _inner_right, _) = | ||
| case_expr | ||
| { | ||
| if cmd_name.eq_ignore_ascii_case("do") { | ||
| if let Expression::BinaryCommand(BinaryCommand::Named(case_name), value_expr, _, _) = | ||
| inner_left.as_ref() | ||
| { | ||
| if case_name.eq_ignore_ascii_case("case") { | ||
| let case_source = value_expr.source(false); | ||
| let case_span = value_expr.span(); | ||
|
|
||
| // Check if we've seen this value before | ||
| for (existing_value, existing_span) in &case_values { | ||
| if existing_value == &case_source { | ||
| // Found a duplicate! | ||
| codes.push(Arc::new(CodeS52DuplicateCase::new( | ||
| case_span.clone(), | ||
| case_source.clone(), | ||
| existing_span.clone(), | ||
| processed, | ||
| config.severity(), | ||
| ))); | ||
| } | ||
| } | ||
|
|
||
| case_values.push((case_source, case_span)); | ||
| continue; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| codes | ||
| } | ||
| } | ||
|
|
||
| #[allow(clippy::module_name_repetitions)] | ||
| pub struct CodeS52DuplicateCase { | ||
| span: Range<usize>, | ||
| value: String, | ||
| first_span: Range<usize>, | ||
| severity: Severity, | ||
| diagnostic: Option<Diagnostic>, | ||
| } | ||
|
|
||
| impl Code for CodeS52DuplicateCase { | ||
| fn ident(&self) -> &'static str { | ||
| "L-S52" | ||
| } | ||
|
|
||
| fn include(&self) -> bool { | ||
| true | ||
|
BrettMayson marked this conversation as resolved.
Outdated
|
||
| } | ||
|
|
||
| fn link(&self) -> Option<&str> { | ||
| Some("/lints/sqf.html#duplicate_case") | ||
| } | ||
|
|
||
| fn severity(&self) -> Severity { | ||
| self.severity | ||
| } | ||
|
|
||
| fn message(&self) -> String { | ||
| format!("Duplicate case `{}` in switch statement", self.value) | ||
| } | ||
|
|
||
| fn label_message(&self) -> String { | ||
| "duplicate case".to_string() | ||
| } | ||
|
|
||
| fn diagnostic(&self) -> Option<Diagnostic> { | ||
| self.diagnostic.clone() | ||
| } | ||
| } | ||
|
|
||
| impl CodeS52DuplicateCase { | ||
| #[must_use] | ||
| pub fn new( | ||
| span: Range<usize>, | ||
| value: String, | ||
| first_span: Range<usize>, | ||
| processed: &Processed, | ||
| severity: Severity, | ||
| ) -> Self { | ||
| Self { | ||
| span, | ||
| value, | ||
| first_span, | ||
| severity, | ||
| diagnostic: None, | ||
| } | ||
| .generate_processed(processed) | ||
| } | ||
|
|
||
| fn generate_processed(mut self, processed: &Processed) -> Self { | ||
| let Some(mut diag) = Diagnostic::from_code_processed(&self, self.span.clone(), processed) | ||
| else { | ||
| return self; | ||
| }; | ||
|
|
||
| // Try to get info about the first span | ||
| if let Some((path, span)) = get_span_info(self.first_span.clone(), processed) { | ||
| diag = diag.with_label( | ||
| Label::secondary(path, span) | ||
| .with_message("first case here"), | ||
| ); | ||
| } | ||
| 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_end(), | ||
| )) | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| // Test duplicate case detection | ||
|
|
||
| // Simple duplicate | ||
| switch (_value) do { | ||
| case 1: { "one" }; | ||
| case 2: { "two" }; | ||
| case 1: { "one again" }; | ||
| }; | ||
|
|
||
| // Multiple duplicates | ||
| switch (_x) do { | ||
| case "a": { 1 }; | ||
| case "b": { 2 }; | ||
| case "a": { 3 }; | ||
| case "b": { 4 }; | ||
| }; | ||
|
|
||
| // Variable case (should be detected) | ||
| switch (_var) do { | ||
| case _distBottom: { [_x, 0] }; | ||
| case _distRight: { [_worldSize, _y] }; | ||
| case _distBottom: { [_x, _worldSize] }; | ||
| }; | ||
|
|
||
| // Default cases are OK (not considered cases for duplication) | ||
| switch (_value) do { | ||
| case 1: { "one" }; | ||
| case 2: { "two" }; | ||
| case default { "other" }; | ||
|
BrettMayson marked this conversation as resolved.
Outdated
|
||
| }; | ||
|
|
||
| // Single case is OK | ||
| switch (_value) do { | ||
| case 1: { "one" }; | ||
| }; | ||
|
|
||
42 changes: 42 additions & 0 deletions
42
libs/sqf/tests/snapshots/lints__simple_s52_duplicate_case.snap
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| --- | ||
| source: libs/sqf/tests/lints.rs | ||
| expression: lint(stringify! (s52_duplicate_case), true).0 | ||
| --- | ||
| [0m[1m[38;5;14mhelp[L-S52][0m[1m: Duplicate case `"a"` in switch statement[0m | ||
| [0m[36m┌─[0m s52_duplicate_case.sqf:14:10 | ||
| [0m[36m│[0m | ||
| [0m[36m12[0m [0m[36m│[0m case "a": { 1 }; | ||
| [0m[36m│[0m [0m[36m----[0m [0m[36mfirst case here[0m | ||
| [0m[36m13[0m [0m[36m│[0m case "b": { 2 }; | ||
| [0m[36m14[0m [0m[36m│[0m case [0m[36m"a"[0m: { 3 }; | ||
| [0m[36m│[0m [0m[36m^^^[0m [0m[36mduplicate case[0m | ||
|
|
||
|
|
||
| [0m[1m[38;5;14mhelp[L-S52][0m[1m: Duplicate case `"b"` in switch statement[0m | ||
| [0m[36m┌─[0m s52_duplicate_case.sqf:15:10 | ||
| [0m[36m│[0m | ||
| [0m[36m13[0m [0m[36m│[0m case "b": { 2 }; | ||
| [0m[36m│[0m [0m[36m----[0m [0m[36mfirst case here[0m | ||
| [0m[36m14[0m [0m[36m│[0m case "a": { 3 }; | ||
| [0m[36m15[0m [0m[36m│[0m case [0m[36m"b"[0m: { 4 }; | ||
| [0m[36m│[0m [0m[36m^^^[0m [0m[36mduplicate case[0m | ||
|
|
||
|
|
||
| [0m[1m[38;5;14mhelp[L-S52][0m[1m: Duplicate case `1` in switch statement[0m | ||
| [0m[36m┌─[0m s52_duplicate_case.sqf:7:10 | ||
| [0m[36m│[0m | ||
| [0m[36m5[0m [0m[36m│[0m case 1: { "one" }; | ||
| [0m[36m│[0m [0m[36m--[0m [0m[36mfirst case here[0m | ||
| [0m[36m6[0m [0m[36m│[0m case 2: { "two" }; | ||
| [0m[36m7[0m [0m[36m│[0m case [0m[36m1[0m: { "one again" }; | ||
| [0m[36m│[0m [0m[36m^[0m [0m[36mduplicate case[0m | ||
|
|
||
|
|
||
| [0m[1m[38;5;14mhelp[L-S52][0m[1m: Duplicate case `_distBottom` in switch statement[0m | ||
| [0m[36m┌─[0m s52_duplicate_case.sqf:22:10 | ||
| [0m[36m│[0m | ||
| [0m[36m20[0m [0m[36m│[0m case _distBottom: { [_x, 0] }; | ||
| [0m[36m│[0m [0m[36m------------[0m [0m[36mfirst case here[0m | ||
| [0m[36m21[0m [0m[36m│[0m case _distRight: { [_worldSize, _y] }; | ||
| [0m[36m22[0m [0m[36m│[0m case [0m[36m_distBottom[0m: { [_x, _worldSize] }; | ||
| [0m[36m│[0m [0m[36m^^^^^^^^^^^[0m [0m[36mduplicate case[0m |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.