Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The checked-in schema statistics must be regenerated to match the updated schema.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Updates QL-for-QL parsing to support file-level module; declarations and prevent related dead-code false positives.
Changes:
- Upgrades Tree-sitter and the QL grammar revision.
- Regenerates the QL schema and AST bindings.
- Adds regression coverage for parameterized modules.
| File | Description |
|---|---|
ql/extractor/Cargo.toml |
Updates parser dependencies. |
ql/Cargo.lock |
Locks updated dependencies. |
ql/ql/src/ql.dbscheme |
Reflects the revised module grammar. |
ql/ql/src/codeql_ql/ast/internal/TreeSitter.qll |
Updates generated module accessors. |
ql/ql/test/queries/style/DeadCode/Foo.qll |
Adds regression scenarios. |
ql/ql/test/queries/style/DeadCode/Parameterized.qll |
Adds parameterized-module fixture. |
ql/ql/test/queries/style/DeadCode/DeadCode.expected |
Updates expected locations. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Many languages clear this and fix any bad join orders that arise, to avoid having to update it with every dbscheme change.
Give anonymous module declarations a source location so file-level overlay annotations apply to their declarations. Exclude valid top-level anonymous modules from the missing-name consistency check. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
Author
|
I've just pushed a commit which should fix them. |
hvitved
requested changes
Sep 29, 2026
hvitved
left a comment
hvitved
left a comment
Contributor
There was a problem hiding this comment.
The removal of DB stats likely introduced a bad join:
[2/65 eval 2.3s] Evaluation done; writing results to codeql/ql/queries/diagnostics/SuccessfullyExtractedFiles.bqrs.
[3/65 eval 7.7s] Evaluation done; writing results to codeql/ql/queries/reports/OutdatedDeprecations.bqrs.
[4/65 eval 27.6s] Evaluation done; writing results to codeql/ql/queries/style/QlRefInlineExpectations.bqrs.
[5/65 eval 2m7s] Evaluation done; writing results to codeql/ql/queries/bugs/MissingSanitizerGuardCase.bqrs.
[6/65 eval 2m7s] Evaluation done; writing results to codeql/ql/queries/bugs/OrderByConst.bqrs.
[7/65 eval 2m7s] Evaluation done; writing results to codeql/ql/queries/bugs/PathProblemQuery.bqrs.
[8/65 eval 2m7s] Evaluation done; writing results to codeql/ql/queries/bugs/SumWithoutDomain.bqrs.
[9/65 eval 2m7s] Evaluation done; writing results to codeql/ql/queries/performance/DontUseGetAQlClass.bqrs.
[10/65 eval 2m7s] Evaluation done; writing results to codeql/ql/queries/diagnostics/EmptyConsistencies.bqrs.
[11/65 eval 2m42s] Evaluation done; writing results to codeql/ql/queries/performance/MissingNoinline.bqrs.
[12/65 eval 2m42s] Evaluation done; writing results to codeql/ql/queries/performance/NonInitialStdLibImport.bqrs.
[13/65 eval 2m42s] Evaluation done; writing results to codeql/ql/queries/style/AcronymsShouldBeCamelCase.bqrs.
[14/65 eval 2m43s] Evaluation done; writing results to codeql/ql/queries/style/AndroidIdPrefix.bqrs.
[15/65 eval 2m43s] Evaluation done; writing results to codeql/ql/queries/style/ConsistentAlertMessage.bqrs.
[16/65 eval 2m43s] Evaluation done; writing results to codeql/ql/queries/style/ConsistentCasing.bqrs.
[17/65 eval 2m43s] Evaluation done; writing results to codeql/ql/queries/style/CountingToZero.bqrs.
[18/65 eval 2m43s] Evaluation done; writing results to codeql/ql/queries/style/DataFlowConfigModuleNaming.bqrs.
[19/65 eval 2m43s] Evaluation done; writing results to codeql/ql/queries/style/DBTypeInNonLib.bqrs.
[20/65 eval 2m44s] Evaluation done; writing results to codeql/ql/queries/style/GetAPrimaryQlClassConsistency.bqrs.
[21/65 eval 2m44s] Evaluation done; writing results to codeql/ql/queries/style/MissingQualityMetadata.bqrs.
[22/65 eval 2m44s] Evaluation done; writing results to codeql/ql/queries/style/LibraryAnnotation.bqrs.
[23/65 eval 2m44s] Evaluation done; writing results to codeql/ql/queries/style/IfWithElseNone.bqrs.
[24/65 eval 2m44s] Evaluation done; writing results to codeql/ql/queries/style/MissingSecurityMetadata.bqrs.
[25/65 eval 2m44s] Evaluation done; writing results to codeql/ql/queries/style/Misspelling.bqrs.
[26/65 eval 2m44s] Evaluation done; writing results to codeql/ql/queries/style/NameCasing.bqrs.
[27/65 eval 2m44s] Evaluation done; writing results to codeql/ql/queries/style/NonDocBlock.bqrs.
[28/65 eval 2m44s] Evaluation done; writing results to codeql/ql/queries/style/RankOne.bqrs.
[29/65 eval 2m44s] Evaluation done; writing results to codeql/ql/queries/style/RegexpInsteadOfPattern.bqrs.
[30/65 eval 2m44s] Evaluation done; writing results to codeql/ql/queries/style/RepeatedWord.bqrs.
[31/65 eval 2m44s] Evaluation done; writing results to codeql/ql/queries/style/SingletonSetLiteral.bqrs.
[32/65 eval 2m45s] Evaluation done; writing results to codeql/ql/queries/style/RedundantImport.bqrs.
[33/65 eval 2m49s] Evaluation done; writing results to codeql/ql/queries/bugs/InconsistentDeprecation.bqrs.
[34/65 eval 2m49s] Evaluation done; writing results to codeql/ql/queries/bugs/NameClashInSummarizedCallable.bqrs.
[35/65 eval 2m51s] Evaluation done; writing results to codeql/ql/queries/performance/UnusedField.bqrs.
[36/65 eval 2m55s] Evaluation done; writing results to codeql/ql/queries/performance/AbstractClassImport.bqrs.
[37/65 eval 3m14s] Evaluation done; writing results to codeql/ql/queries/style/SwappedParameterNames.bqrs.
[38/65 eval 3m21s] Evaluation done; writing results to codeql/ql/queries/style/OverrideAny.bqrs.
Match builtin predicate calls by name before computing their arity, and calculate the arity directly from builtin parameters. This avoids a broad virtual getArity dispatch that produced tens of billions of intermediate tuples without dbscheme statistics. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Use directly local declarations as witnesses when propagating file-level overlay annotations. File equality already provides the required closure, while the recursive formulation materialized a multi-billion-row location join. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
|
I think there is still a performance issue. Here are the timings from before (from this run) vs on this PR: |
hvitved
requested changes
Sep 30, 2026
|
|
||
| pragma[noinline] | ||
| private predicate builtinCandidate(BuiltinClassless pred, PredicateCall call) { | ||
| pred.getName() = call.getPredicateName() |
Contributor
There was a problem hiding this comment.
This is still joins on name before joining on arity; it would be better to join on both columns simultaneously:
pragma[noinline]
private predicate builtin(BuiltinClassless pred, string name, int arity) {
name = pred.getName() and
arity = count(int i | exists(pred.getParameterType(i)))
}
pragma[noinline]
private predicate call(PredicateCall call, string name, int arity) {
name = call.getPredicateName() and
arity = call.getNumberOfArguments()
}
private predicate resolveBuiltinPredicateCall(PredicateCall call, BuiltinClassless pred) {
exists(string name, int arity |
builtin(pred, name, arity) and
call(call, name, arity)
)
}This branch has not been deployed
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
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.
I was getting FPs, which turned out to be because the QL extractor couldn't parse
overlay[local] module;. The first commit adds tests demonstrating the FP. The second commit updates the tree-sitter grammar version we are using, which fixes the tests.This was done by copilot. I'm not very familiar with QL-for-QL. I've reviewed it as best I can. It seems low risk as it's just a dependency update.
I assume this doesn't need a change note.