Skip to content

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

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
merged 7 commits into from
May 27, 2025
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -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
7 changes: 6 additions & 1 deletion csharp/ql/src/Language Abuse/MissedReadonlyOpportunity.ql
Original file line number Diff line number Diff line change
@@ -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))
}

Original file line number Diff line number Diff line change
@@ -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.
Original file line number Diff line number Diff line change
@@ -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;
}
}
Original file line number Diff line number Diff line change
@@ -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'. |
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
Language Abuse/MissedReadonlyOpportunity.ql
query: Language Abuse/MissedReadonlyOpportunity.ql
postprocess: utils/test/InlineExpectationsTestQuery.ql
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
class Bad
{
int Field;
int Field; // $ Alert

public Bad(int i)
{