-
Notifications
You must be signed in to change notification settings - Fork 1.9k
C#: Special handling of unknown types in isMatchingConstant.
#18932
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
C#: Special handling of unknown types in isMatchingConstant.
#18932
Conversation
4451e6b to
93881f8
Compare
93881f8 to
3903a90
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Overview
This PR refines the matching logic for unknown types in the isMatchingConstant function to eliminate false positives, particularly those related to constant conditions and useless assignments.
- Adjusts handling of expressions that involve unknown types.
- Updates a test case to assert that unknown types do not trigger a constant condition alert.
Reviewed Changes
| File | Description |
|---|---|
| csharp/ql/test/query-tests/standalone/Bad Practices/Control-Flow/ConstantCondition/ConstantCondition.cs | Adds test cases to verify that a known type (int) triggers an alert while an unknown type (D) does not |
Copilot reviewed 13 out of 13 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
csharp/ql/test/query-tests/standalone/Bad Practices/Control-Flow/ConstantCondition/ConstantCondition.cs:21
- The type 'D' is used without a definition, which may cause compilation issues. If this is intentional to simulate an unknown type, consider adding a comment clarifying this or provide a dummy definition of D.
var d = new D();
Tip: Copilot only keeps its highest confidence comments to reduce noise and keep you focused. Learn more
hvitved
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
| --- | ||
| category: minorAnalysis | ||
| --- | ||
| * Increase query precision for `cs/useless-assignment-to-local` and `cs/constant-condition` when *unknown* types are involved (mostly relevant for build mode none databases). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do we reference buildess DBs in the docs, should it be build-mode: none?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will change it to that.
Maybe the abbreviation BMN is also acceptable? (What do you think @coadaflorin? )
In this PR we remove some false positives for various queries. The completion logic lead to wrong conclusions on matching (when unknown types are involved, the completion logic derived that a pattern would never match), which also affects the control flow graph succ/pred logic.
It appears that it is mostly the
cs/constant-conditionandcs/useless-assignment-to-localfor BMN databases that are affected by this (this problem became more evident after #18894 as we categorise more types as unknown).The DCA results for the powershell project were spot-checked and the removed results indeed appeared to be false positives. Prior to the change the matching against an unknown type would lead to
cs/constant-conditionand typically a subsequentcs/useless-assignment-to-localfor cases like