From 5941b3081c3a48084c21675fb961f8acabd89c0e Mon Sep 17 00:00:00 2001 From: Michael Nebel Date: Mon, 19 May 2025 13:44:09 +0200 Subject: [PATCH 1/7] C#: Convert tests for cs/missed-readonly-modifier to inline expectatations. --- .../MissedReadonlyOpportunity/MissedReadonlyOpportunity.cs | 4 ++-- .../MissedReadonlyOpportunity/MissedReadonlyOpportunity.qlref | 3 ++- .../MissedReadonlyOpportunity/MissedReadonlyOpportunityBad.cs | 2 +- 3 files changed, 5 insertions(+), 4 deletions(-) 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..bfe6b3243d4a 100644 --- a/csharp/ql/test/query-tests/Language Abuse/MissedReadonlyOpportunity/MissedReadonlyOpportunity.cs +++ b/csharp/ql/test/query-tests/Language Abuse/MissedReadonlyOpportunity/MissedReadonlyOpportunity.cs @@ -1,7 +1,7 @@ class MissedReadonlyOpportunity { - public int Bad1; - public T Bad2; + public int Bad1; // $ Alert + public T Bad2; // $ Alert public readonly int Good1; public readonly int Good2 = 0; public const int Good3 = 0; 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) { From 3a1cd3f734959ac38db20c97d4e1789c0ceed1a3 Mon Sep 17 00:00:00 2001 From: Michael Nebel Date: Mon, 19 May 2025 13:56:35 +0200 Subject: [PATCH 2/7] C#: Add cs/missed-readonly-modifier to the code-quality suite. --- .../posix/query-suite/csharp-code-quality.qls.expected | 1 + csharp/ql/src/Language Abuse/MissedReadonlyOpportunity.ql | 1 + 2 files changed, 2 insertions(+) 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..a71876e8c7da 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 From 28cd8a827a772bc9ce9eab91033852fa99bc6d78 Mon Sep 17 00:00:00 2001 From: Michael Nebel Date: Mon, 19 May 2025 14:12:29 +0200 Subject: [PATCH 3/7] C#: Add more test examples for cs/missing-readonly-modifier. --- .../MissedReadonlyOpportunity.cs | 55 +++++++++++++++++++ .../MissedReadonlyOpportunity.expected | 11 ++++ 2 files changed, 66 insertions(+) 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 bfe6b3243d4a..a365feec5e38 100644 --- a/csharp/ql/test/query-tests/Language Abuse/MissedReadonlyOpportunity/MissedReadonlyOpportunity.cs +++ b/csharp/ql/test/query-tests/Language Abuse/MissedReadonlyOpportunity/MissedReadonlyOpportunity.cs @@ -2,22 +2,26 @@ class MissedReadonlyOpportunity { 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..620b6b2dd111 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,14 @@ +#select | 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:12:20:12:24 | Good7 | Field 'Good7' can be 'readonly'. | +| MissedReadonlyOpportunity.cs:49:19:49:24 | Parent | Field 'Parent' 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'. | +| MissedReadonlyOpportunity.cs:71:16:71:16 | Y | Field 'Y' can be 'readonly'. | | MissedReadonlyOpportunityBad.cs:3:9:3:13 | Field | Field 'Field' can be 'readonly'. | +testFailures +| MissedReadonlyOpportunity.cs:12:20:12:24 | Field 'Good7' can be 'readonly'. | Unexpected result: Alert | +| MissedReadonlyOpportunity.cs:49:19:49:24 | Field 'Parent' can be 'readonly'. | Unexpected result: Alert | +| MissedReadonlyOpportunity.cs:71:16:71:16 | Field 'Y' can be 'readonly'. | Unexpected result: Alert | From 8108c72c17b945f5987fd6a730d5b1a263e8c721 Mon Sep 17 00:00:00 2001 From: Michael Nebel Date: Mon, 19 May 2025 14:15:13 +0200 Subject: [PATCH 4/7] C#: Exclude structs from being flagged in cs/missed-readonly-modifier. --- csharp/ql/src/Language Abuse/MissedReadonlyOpportunity.ql | 1 + 1 file changed, 1 insertion(+) diff --git a/csharp/ql/src/Language Abuse/MissedReadonlyOpportunity.ql b/csharp/ql/src/Language Abuse/MissedReadonlyOpportunity.ql index a71876e8c7da..6946e9b4894f 100644 --- a/csharp/ql/src/Language Abuse/MissedReadonlyOpportunity.ql +++ b/csharp/ql/src/Language Abuse/MissedReadonlyOpportunity.ql @@ -27,6 +27,7 @@ predicate isReadonlyCompatibleDefinition(AssignableDefinition def, Field f) { } 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)) } From 19e9197874cd585ce55e4cfbdc00ffb40b6764c1 Mon Sep 17 00:00:00 2001 From: Michael Nebel Date: Mon, 19 May 2025 16:37:14 +0200 Subject: [PATCH 5/7] C#: The field access should be on this for it to be compatible with readonly. --- csharp/ql/src/Language Abuse/MissedReadonlyOpportunity.ql | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/csharp/ql/src/Language Abuse/MissedReadonlyOpportunity.ql b/csharp/ql/src/Language Abuse/MissedReadonlyOpportunity.ql index 6946e9b4894f..78cce5126dfc 100644 --- a/csharp/ql/src/Language Abuse/MissedReadonlyOpportunity.ql +++ b/csharp/ql/src/Language Abuse/MissedReadonlyOpportunity.ql @@ -20,7 +20,10 @@ 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 ) From 008d5b7081b941a092e3b25320b037380154bfdb Mon Sep 17 00:00:00 2001 From: Michael Nebel Date: Wed, 21 May 2025 15:20:15 +0200 Subject: [PATCH 6/7] C#: Update test expected output. --- .../MissedReadonlyOpportunity.expected | 8 -------- 1 file changed, 8 deletions(-) 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 620b6b2dd111..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,14 +1,6 @@ -#select | 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:12:20:12:24 | Good7 | Field 'Good7' can be 'readonly'. | -| MissedReadonlyOpportunity.cs:49:19:49:24 | Parent | Field 'Parent' 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'. | -| MissedReadonlyOpportunity.cs:71:16:71:16 | Y | Field 'Y' can be 'readonly'. | | MissedReadonlyOpportunityBad.cs:3:9:3:13 | Field | Field 'Field' can be 'readonly'. | -testFailures -| MissedReadonlyOpportunity.cs:12:20:12:24 | Field 'Good7' can be 'readonly'. | Unexpected result: Alert | -| MissedReadonlyOpportunity.cs:49:19:49:24 | Field 'Parent' can be 'readonly'. | Unexpected result: Alert | -| MissedReadonlyOpportunity.cs:71:16:71:16 | Field 'Y' can be 'readonly'. | Unexpected result: Alert | From bae16f07ff5ae6c52e93924c5217eaa4f33f8cd9 Mon Sep 17 00:00:00 2001 From: Michael Nebel Date: Thu, 22 May 2025 08:42:37 +0200 Subject: [PATCH 7/7] C#: Change note. --- .../src/change-notes/2025-05-22-missed-readonly-modifier.md | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 csharp/ql/src/change-notes/2025-05-22-missed-readonly-modifier.md 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.