Go: enable data flow consistency checks #20917
Conversation
There was a problem hiding this comment.
Pull request overview
This PR enables data flow consistency checks to run in CI for the Go codebase. The main changes include:
- Adding a new
localFlowIsLocalExcludepredicate to the shared data flow consistency library - Implementing Go-specific exclusions for known consistency check violations
- Moving consistency queries to a new
ql/consistency-queriesdirectory - Adding
.expectedfiles documenting current violations (to be fixed in future PRs)
Reviewed changes
Copilot reviewed 71 out of 72 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| shared/dataflow/codeql/dataflow/internal/DataFlowImplConsistency.qll | Adds localFlowIsLocalExclude predicate to allow language-specific exclusions for the localFlowIsLocal consistency check |
| go/ql/lib/semmle/go/dataflow/internal/DataFlowImplConsistency.qll | Implements Go-specific exclusion predicates for consistency checks including localFlowIsLocalExclude, missingLocationExclude, uniqueNodeLocationExclude, and argHasPostUpdateExclude |
| go/ql/consistency-queries/UnexpectedFrontendErrors.ql | New consistency query that reports all frontend extraction errors |
| go/Makefile | Updates test target to use new consistency query location ql/consistency-queries instead of ql/test/consistency |
| go/ql/test/consistency/dummy.ql | Adds dummy query to ensure consistency tests run in the test directory |
| go/ql/test/consistency/dummy.expected | Expected output for the dummy query |
| go/ql/test/consistency/UnexpectedFrontendErrors.expected | Removed (moved to CONSISTENCY subdirectory) |
| Multiple DataFlowConsistency.expected files | Documents current data flow consistency violations across test cases (reverseRead and identityLocalStep issues) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
owen-mc
removed
documentation
no-change-note-required
| predicate uniqueNodeLocationExclude(DataFlow::Node n) { missingLocationExclude(n) } | ||
|
|
||
| predicate localFlowIsLocalExclude(DataFlow::Node n1, DataFlow::Node n2) { | ||
| n1 instanceof DataFlow::FunctionNode and exists(n2) |
There was a problem hiding this comment.
Thats a large CP. Also, can you elaborate what such steps are for?
There was a problem hiding this comment.
I've fixed the CP by adding the constraint simpleLocalFlowStep(n1, n2, _), which is in the only place that this predicate is used.
I didn't immediatley know why we have these steps, but after thinking for a little bit I think I have figured it out. In go you can assign functions to variables and pass them around. It isn't that common but we should deal with it, at least locally. So we use local data flow to find possible callees - for example if you click through Function.getACall() you will get to this line. It's very useful to have steps from function definitions to uses of them as part of the local flow relation so that we can do this without having to use global flow.
go/ql/test/consistency/dummy.ql
Outdated
| @@ -0,0 +1 @@ | |||
| select "This is a dummy query which is only needed so that the consistency tests will run" | |||
There was a problem hiding this comment.
I don't understand why this is needed; consistency queries are supposed to run on all QL tests.
There was a problem hiding this comment.
This test folder exists just to check that UnexpectedFrontendErrors.ql, which used to be the only consistency query, is working, so there are no other .ql files in the folder, and by moving UnexpectedFrontendErrors.ql to be in the same folder as the data flow consistency queries I was leaving the folder without a test query file. I tried without adding another test query and it said it found no tests. I guess it needs a .ql or .qlref file to exist so that it will build a db and run a test, and then run the consistency queries on the db as well.
| predicate argHasPostUpdateExclude(DataFlow::ArgumentNode n) { | ||
| not DataFlow::insnHasPostUpdateNode(n.asInstruction()) | ||
| } | ||
| } |
There was a problem hiding this comment.
Did you forget to add the DataFlowConsistency.ql file?
There was a problem hiding this comment.
It already exists, so you won't see it in this PR. I made it a while ago but didn't set up CI to run it because there were so many results due to not using use-use flow at the time.
This is needed because a PR to another repo is needed to update the location of the consistency queries, and until that PR is merged we don't want to runny dummy.ql as a consistency query. After that PR is merged we should reinstate these files so that consistency tests are run on this test folder.
owen-mc
added
the
depends on internal PR
|
I didn't realise this, but the path to the consistency queries also needs to be updated in an internal PR. Until that is merged I have deleted |
owen-mc
removed
the
depends on internal PR
|
Correction: the consistency checks used in CI are set in an internal repository, so the change made to the makefile here is only relevant for local testing. However, the companion PR to this in the internal repo has all the go tests passing, which is evidence that I have committed the correct |
2 participants
This PR makes the data flow consistency checks run in CI, along with the existing consistency query
UnexpectedFrontendErrors.ql. It also adds some non-controversial exclusions to the tests.There is no particular need to review the .expected files, as they are just recording the current state of affairs. Fixing the go library to remove some of the results will be done in a follow-up PR. Hopefully we will eventually be able to remove all of the expected results for the data flow consistency checks.
One change is made to the shared library: allowing exclusions to the
localFlowIsLocalquery.