C#: Improve cs/missed-readonly-modifier and to code-quality suite.
#19520
Conversation
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
cs/missed-readonly-modifier and to code-quality suite.
36998db to
2c8fa06
Compare
2c8fa06 to
f1805b1
Compare
f1805b1 to
008d5b7
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the cs/missed-readonly-modifier query to avoid changing semantics for mutable structs, eliminates additional false positives (non-public field assignments and static-field writes in instance constructors), and integrates the query into the code-quality suite.
- Exclude mutable struct types from readonly-modifier suggestions
- Refine detection to skip non-public argument-field assignments and static-field initializations
- Add the improved query to the
code-qualitysuite and update tests
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| csharp/ql/test/query-tests/Language Abuse/MissedReadonlyOpportunity/MissedReadonlyOpportunityBad.cs | Marked the bad-case field to trigger the new alert |
| csharp/ql/test/query-tests/Language Abuse/MissedReadonlyOpportunity/MissedReadonlyOpportunity.qlref | Updated reference to include postprocessing for inline expectations |
| csharp/ql/test/query-tests/Language Abuse/MissedReadonlyOpportunity/MissedReadonlyOpportunity.expected | Added new expected alerts for Bad3, Left, and X fields |
| csharp/ql/test/query-tests/Language Abuse/MissedReadonlyOpportunity/MissedReadonlyOpportunity.cs | Extended the test scenario with mutable/immutable structs, tree and static-fields cases |
| csharp/ql/src/change-notes/2025-05-22-missed-readonly-modifier.md | Added changelog entry for the improved precision of the query |
| csharp/ql/src/Language Abuse/MissedReadonlyOpportunity.ql | Updated the main QL predicate to exclude mutable structs and refine constructor checks |
| csharp/ql/integration-tests/posix/query-suite/csharp-code-quality.qls.expected | Added MissedReadonlyOpportunity.ql to the code-quality suite |
| --- | ||
| 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. |
There was a problem hiding this comment.
[nitpick] Consider updating the changelog entry to also mention the fix for false positives when assigning non-public fields on arguments to give users a complete summary of all improvements.
Suggested change
| * 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. | |
| * The precision of the query `cs/missed-readonly-modifier` has been improved. Some false positives related to static fields, struct type fields, and assignments to non-public fields on arguments have been removed. |
Copilot uses AI. Check for mistakes.
Positive FeedbackNegative Feedback
Comment on lines
32
to
+33
| predicate canBeReadonly(Field f) { | ||
| exists(Type t | t = f.getType() | not t instanceof Struct or t.(Struct).isReadonly()) and |
There was a problem hiding this comment.
[nitpick] For clarity and to avoid any ambiguity around operator precedence, consider extracting the struct-check into a named predicate or adding explicit parentheses (e.g. (not t instanceof Struct) or t.(Struct).isReadonly()) and document that this step excludes mutable structs.
Suggested change
| predicate canBeReadonly(Field f) { | |
| exists(Type t | t = f.getType() | not t instanceof Struct or t.(Struct).isReadonly()) and | |
| /** | |
| * Determines if a type is compatible with being readonly. | |
| * A type is readonly-compatible if it is not a struct or if it is a readonly struct. | |
| */ | |
| predicate isReadonlyCompatibleType(Type t) { | |
| not t instanceof Struct or t.(Struct).isReadonly() | |
| } | |
| predicate canBeReadonly(Field f) { | |
| exists(Type t | t = f.getType() | isReadonlyCompatibleType(t)) and |
Copilot uses AI. Check for mistakes.
Positive FeedbackNegative Feedback
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
In this PR we
cs/missed-readonly-modifier. The reason is, that it can alter the semantics of the program for mutable structs. Example can be seen herecode-qualitysuite.DCA looks good.