Merge pull request #19520 from michaelnebel/csharp/missedreadonly

C#: Improve `cs/missed-readonly-modifier` and to code-quality suite.

Author: michaelnebel

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

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))
 }
 

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.

csharp/ql/test/query-tests/Language Abuse/MissedReadonlyOpportunity/MissedReadonlyOpportunity.cs

@@ -1,23 +1,27 @@
 class MissedReadonlyOpportunity<T>
 {
-    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;
+    }
+}

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'. |

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

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)
     {