Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| from LocalName local | ||
| where isUnusedLocal(local) | ||
| select local, "Unused " + getKind(local) + " '" + local.getName() + "'" |
The special-cased labelExpr rules were problematic. They are now coveered by a combination of more general rules.
f9e102a to
84fe0cc
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The query has misleading precision metadata and incorrectly treats plain assignment targets as reads.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
Open (2)
What changed in this PR
Adds a Unified unused-variable query and fixes Swift AST extraction for patterns and nested trailing closures.
Changes:
- Adds unused-variable query logic and regression tests.
- Improves Swift pattern and argument translation.
- Updates extractor and dataflow expectations.
| File | Description |
|---|---|
unified/ql/test/query-tests/unusedentities/UnusedVariable.qlref |
Registers the query test. |
unified/ql/test/query-tests/unusedentities/UnusedVariable.expected |
Records expected query results. |
unified/ql/test/query-tests/unusedentities/test.swift |
Provides query regression cases. |
unified/ql/test/library-tests/dataflow/test.expected |
Updates generated dataflow expectations. |
unified/ql/src/queries/unusedentities/UnusedVariable.ql |
Implements the unused-variable query. |
unified/extractor/tests/corpus/swift/control-flow/nested-enum-case-pattern.output |
Updates generated pattern output. |
unified/extractor/tests/corpus/swift/closures/nested-trailing-closure.swift |
Adds nested trailing-closure coverage. |
unified/extractor/tests/corpus/swift/closures/nested-trailing-closure.output |
Records generated AST output. |
unified/extractor/src/languages/swift/swift.rs |
Fixes Swift pattern and argument translation. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| * @kind problem | ||
| * @problem.severity recommendation | ||
| * @id unified/unused-variable | ||
| * @precision high |
There was a problem hiding this comment.
I tend to agree - and past experience suggests it takes quite a lot of effort to get the false positive rate on an unused variable query to an acceptable level for high precision.
| * @precision high | |
| * @precision medium |
There was a problem hiding this comment.
I think this was perhaps to ensure that the query is included in the default DCA suite?
There was a problem hiding this comment.
What @hvitved said. In prod this would not be high and in its current state not even medium. But right now we just need to make sure the alerts are seen.
| callback() | ||
| return "df" | ||
| } | ||
| // Note: This currently fails because the trailing closure is not extracted correctly |
| private predicate isUnusedLocal(LocalName local) { | ||
| local.getABinding().fromSource() and // ignore unused implicit locals, and ignore locals in built-ins | ||
| not ignoreDeclaration(local.getABinding().getDeclaration()) and // ignore fields, method, etc | ||
| not local.getName().regexpMatch("_.*") and |
There was a problem hiding this comment.
not local.getName().charAt(0) = "_".
There was a problem hiding this comment.
Isn't regexpMatch more efficient though? I seem to recall we did a mass replacement of string matching expressions to regexpMatch and I've just defaulted to using it since.
|
|
||
| private import unified | ||
|
|
||
| private predicate isUnusedLocal(LocalName local) { |
There was a problem hiding this comment.
Why not use LocalVariable instead of LocalName? Then it should not be necessary to filter away fields and methods.
There was a problem hiding this comment.
Mainly because it's a debugging query and we don't have the corresponding UnusedType.ql. It would also mean we don't catch things like unused local type aliases, or if there's a bug affecting the LocalVariable charpred it might not be revealed by this query.
| access = local.getAnAccess() and | ||
| not access instanceof NameBinding | ||
| ) and | ||
| not local = any(UnqualifiedMemberAccess access).getImplicitQualifierVariable() |
There was a problem hiding this comment.
With #22666 this can instead be a call to isImplicitReceiverParameter; that should also eliminate FPs when a function doesn't have any implicit self receivers.
There was a problem hiding this comment.
Implicit variables are always ignored so I'm not sure which FPs you mean?
Anyway, with this formulation we can detect FPs from guard let self or capture-declarations [self] without a subsequent use of self which is actually a nice stress test of the machinery around self.
This query has a lot of FPs currently but is great at pointing out bugs in our analysis. The PR also fixes one of these bugs, there are many more to find.