diff --git a/csharp/ql/integration-tests/posix/query-suite/csharp-code-quality.qls.expected b/csharp/ql/integration-tests/posix/query-suite/csharp-code-quality.qls.expected index d1b40bd013e7..14934899e0d8 100644 --- a/csharp/ql/integration-tests/posix/query-suite/csharp-code-quality.qls.expected +++ b/csharp/ql/integration-tests/posix/query-suite/csharp-code-quality.qls.expected @@ -3,6 +3,7 @@ ql/csharp/ql/src/API Abuse/FormatInvalid.ql ql/csharp/ql/src/API Abuse/NoDisposeCallOnLocalIDisposable.ql ql/csharp/ql/src/Bad Practices/Control-Flow/ConstantCondition.ql ql/csharp/ql/src/Dead Code/DeadStoreOfLocal.ql +ql/csharp/ql/src/Language Abuse/MissedReadonlyOpportunity.ql ql/csharp/ql/src/Likely Bugs/Collections/ContainerLengthCmpOffByOne.ql ql/csharp/ql/src/Likely Bugs/Collections/ContainerSizeCmpZero.ql ql/csharp/ql/src/Likely Bugs/DangerousNonShortCircuitLogic.ql diff --git a/csharp/ql/src/Language Abuse/MissedReadonlyOpportunity.ql b/csharp/ql/src/Language Abuse/MissedReadonlyOpportunity.ql index b794700e79b0..78cce5126dfc 100644 --- a/csharp/ql/src/Language Abuse/MissedReadonlyOpportunity.ql +++ b/csharp/ql/src/Language Abuse/MissedReadonlyOpportunity.ql @@ -8,6 +8,7 @@ * @id cs/missed-readonly-modifier * @tags maintainability * language-features + * quality */ import csharp @@ -19,13 +20,17 @@ predicate defTargetsField(AssignableDefinition def, Field f) { predicate isReadonlyCompatibleDefinition(AssignableDefinition def, Field f) { defTargetsField(def, f) and ( - def.getEnclosingCallable().(Constructor).getDeclaringType() = f.getDeclaringType() + def.getEnclosingCallable().(StaticConstructor).getDeclaringType() = f.getDeclaringType() + or + def.getEnclosingCallable().(InstanceConstructor).getDeclaringType() = f.getDeclaringType() and + def.getTargetAccess().(QualifiableExpr).getQualifier() instanceof ThisAccess or def instanceof AssignableDefinitions::InitializerDefinition ) } predicate canBeReadonly(Field f) { + exists(Type t | t = f.getType() | not t instanceof Struct or t.(Struct).isReadonly()) and forex(AssignableDefinition def | defTargetsField(def, f) | isReadonlyCompatibleDefinition(def, f)) } diff --git a/csharp/ql/src/change-notes/2025-05-22-missed-readonly-modifier.md b/csharp/ql/src/change-notes/2025-05-22-missed-readonly-modifier.md new file mode 100644 index 000000000000..ee3d60fe4ff8 --- /dev/null +++ b/csharp/ql/src/change-notes/2025-05-22-missed-readonly-modifier.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* The precision of the query `cs/missed-readonly-modifier` has been improved. Some false positives related to static fields and struct type fields have been removed. diff --git a/csharp/ql/test/query-tests/Language Abuse/MissedReadonlyOpportunity/MissedReadonlyOpportunity.cs b/csharp/ql/test/query-tests/Language Abuse/MissedReadonlyOpportunity/MissedReadonlyOpportunity.cs index 0ecb33abadcc..a365feec5e38 100644 --- a/csharp/ql/test/query-tests/Language Abuse/MissedReadonlyOpportunity/MissedReadonlyOpportunity.cs +++ b/csharp/ql/test/query-tests/Language Abuse/MissedReadonlyOpportunity/MissedReadonlyOpportunity.cs @@ -1,23 +1,27 @@ class MissedReadonlyOpportunity { - public int Bad1; - public T Bad2; + public int Bad1; // $ Alert + public T Bad2; // $ Alert + public Immutable Bad3; // $ Alert public readonly int Good1; public readonly int Good2 = 0; public const int Good3 = 0; public int Good4; public readonly T Good5; public T Good6; + public Mutable Good7; public MissedReadonlyOpportunity(int i, T t) { Bad1 = i; Bad2 = t; + Bad3 = new Immutable(); Good1 = i; Good2 = i; Good4 = i; Good5 = t; Good6 = t; + Good7 = new Mutable(); } public void M(int i) @@ -27,3 +31,54 @@ public void M(int i) x.Good6 = false; } } + +struct Mutable +{ + private int x; + public int Mutate() + { + x = x + 1; + return x; + } +} + +readonly struct Immutable { } + +class Tree +{ + private Tree? Parent; + private Tree? Left; // $ Alert + private readonly Tree? Right; + + public Tree(Tree left, Tree right) + { + this.Left = left; + this.Right = right; + left.Parent = this; + right.Parent = this; + } + + public Tree() + { + Left = null; + Right = null; + } +} + +class StaticFields +{ + static int X; // $ Alert + static int Y; + + // Static constructor + static StaticFields() + { + X = 0; + } + + // Instance constructor + public StaticFields(int y) + { + Y = y; + } +} diff --git a/csharp/ql/test/query-tests/Language Abuse/MissedReadonlyOpportunity/MissedReadonlyOpportunity.expected b/csharp/ql/test/query-tests/Language Abuse/MissedReadonlyOpportunity/MissedReadonlyOpportunity.expected index 680a571e7754..6a9b286a343c 100644 --- a/csharp/ql/test/query-tests/Language Abuse/MissedReadonlyOpportunity/MissedReadonlyOpportunity.expected +++ b/csharp/ql/test/query-tests/Language Abuse/MissedReadonlyOpportunity/MissedReadonlyOpportunity.expected @@ -1,3 +1,6 @@ | MissedReadonlyOpportunity.cs:3:16:3:19 | Bad1 | Field 'Bad1' can be 'readonly'. | | MissedReadonlyOpportunity.cs:4:14:4:17 | Bad2 | Field 'Bad2' can be 'readonly'. | +| MissedReadonlyOpportunity.cs:5:22:5:25 | Bad3 | Field 'Bad3' can be 'readonly'. | +| MissedReadonlyOpportunity.cs:50:19:50:22 | Left | Field 'Left' can be 'readonly'. | +| MissedReadonlyOpportunity.cs:70:16:70:16 | X | Field 'X' can be 'readonly'. | | MissedReadonlyOpportunityBad.cs:3:9:3:13 | Field | Field 'Field' can be 'readonly'. | diff --git a/csharp/ql/test/query-tests/Language Abuse/MissedReadonlyOpportunity/MissedReadonlyOpportunity.qlref b/csharp/ql/test/query-tests/Language Abuse/MissedReadonlyOpportunity/MissedReadonlyOpportunity.qlref index 28237dce311a..eb2e98d639dc 100644 --- a/csharp/ql/test/query-tests/Language Abuse/MissedReadonlyOpportunity/MissedReadonlyOpportunity.qlref +++ b/csharp/ql/test/query-tests/Language Abuse/MissedReadonlyOpportunity/MissedReadonlyOpportunity.qlref @@ -1 +1,2 @@ -Language Abuse/MissedReadonlyOpportunity.ql +query: Language Abuse/MissedReadonlyOpportunity.ql +postprocess: utils/test/InlineExpectationsTestQuery.ql diff --git a/csharp/ql/test/query-tests/Language Abuse/MissedReadonlyOpportunity/MissedReadonlyOpportunityBad.cs b/csharp/ql/test/query-tests/Language Abuse/MissedReadonlyOpportunity/MissedReadonlyOpportunityBad.cs index 7bd3d8d31cd9..912141bb862b 100644 --- a/csharp/ql/test/query-tests/Language Abuse/MissedReadonlyOpportunity/MissedReadonlyOpportunityBad.cs +++ b/csharp/ql/test/query-tests/Language Abuse/MissedReadonlyOpportunity/MissedReadonlyOpportunityBad.cs @@ -1,6 +1,6 @@ class Bad { - int Field; + int Field; // $ Alert public Bad(int i) {