Skip to content

Conversation

@michaelnebel
Copy link
Contributor

@michaelnebel michaelnebel commented Mar 5, 2025

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-condition and cs/useless-assignment-to-local for 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-condition and typically a subsequent cs/useless-assignment-to-local for cases like

if (x is MyUnknownType y) {
   Use(y);
}

@github-actions github-actions bot added the C# label Mar 5, 2025
@michaelnebel michaelnebel force-pushed the csharp/ismatchingconstantunknowtype branch from 4451e6b to 93881f8 Compare March 7, 2025 09:01
@michaelnebel michaelnebel force-pushed the csharp/ismatchingconstantunknowtype branch from 93881f8 to 3903a90 Compare March 7, 2025 12:24
@michaelnebel michaelnebel marked this pull request as ready for review March 10, 2025 11:58
Copilot AI review requested due to automatic review settings March 10, 2025 11:58
@michaelnebel michaelnebel requested a review from a team as a code owner March 10, 2025 11:58
Copy link
Contributor

Copilot AI left a 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
hvitved previously approved these changes Mar 10, 2025
Copy link
Contributor

@hvitved hvitved left a 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).
Copy link
Contributor

@tamasvajk tamasvajk Mar 10, 2025

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?

Copy link
Contributor Author

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

@michaelnebel michaelnebel merged commit ca553bf into github:main Mar 10, 2025
20 checks passed
@michaelnebel michaelnebel deleted the csharp/ismatchingconstantunknowtype branch March 11, 2025 08:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants