Skip to content

Go: make data flow consistency checks available (and fix one)#14547

Merged
owen-mc merged 7 commits intogithub:mainfrom
owen-mc:go/enable-data-flow-consistency-checks
Oct 25, 2023
Merged

Go: make data flow consistency checks available (and fix one)#14547
owen-mc merged 7 commits intogithub:mainfrom
owen-mc:go/enable-data-flow-consistency-checks

Conversation

@owen-mc
Copy link
Copy Markdown
Contributor

@owen-mc owen-mc commented Oct 19, 2023

This PR makes the data flow consistency checks for go available. They currently have to be manually. When we have done the work to reduce the number of expected results to zero we can add them to the consistency checks so that we will notice when there is a regression.

This PR also fixes a problem which was highlighted that not all nodes have a result for nodeGetEnclosingCallable. Two steps were needed: (1) fixing an accidental exclusion of ImplicitVarArgsSlice nodes and (2) making a new branch of TDataFlowCallable called TExternalFileScope which is used only as a return value for nodeGetEnclosingCallable when a node exists in an external file (i.e. one which is not in the project being analyzed).

This can be run on an existing database to check for any assumptions
of the data flow library which do not hold.
It wasn't updated when MkImplicitVarargsSlice was added as a branch of
TNode. This meant that it gave no result for `ImplicitVarargsSlice`s
in function calls used to initialise variables declared at file level.
@owen-mc owen-mc requested a review from a team as a code owner October 19, 2023 15:52
@github-actions github-actions Bot added the Go label Oct 19, 2023
@owen-mc owen-mc requested a review from a team as a code owner October 20, 2023 10:20
@github-actions github-actions Bot added the C# label Oct 20, 2023
@github-actions github-actions Bot added documentation and removed C# labels Oct 20, 2023
Copy link
Copy Markdown
Member

@mbg mbg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable -- just a few minor questions and comments.

I also couldn't find any reference of this actually getting run in the build logs. Is there a way to verify that this works as expected other than deliberately introducing a failure locally?

Comment thread go/ql/lib/semmle/go/dataflow/internal/DataFlowNodes.qll
Comment thread go/ql/lib/change-notes/2023-10-20-enclosing-callable-for-external-files.md Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: why does this get its own qlpack?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reason 1: all the other languages do it...

…nal-files.md

Co-authored-by: Michael B. Gale <mbg@github.com>
@owen-mc
Copy link
Copy Markdown
Contributor Author

owen-mc commented Oct 20, 2023

It isn't currently run in CI. I'm not sure what other languages do. I stumbled across some experimental tests using it for python. It's a bit unclear to me what test code it should be run on. I've just been running it on some databases I already had handy.

@owen-mc
Copy link
Copy Markdown
Contributor Author

owen-mc commented Oct 20, 2023

It seems the standard way to run it in CI is to add it to the consistency queries, so it's run on all tests. But we can't really do that till we've made all the queries have zero results.

@owen-mc
Copy link
Copy Markdown
Contributor Author

owen-mc commented Oct 25, 2023

Performance testing showed so significant difference. I think this is ready to merge.

@owen-mc owen-mc changed the title Go: enable data flow consistency checks (and fix some) Go: make data flow consistency checks available (and fix some) Oct 25, 2023
@owen-mc owen-mc changed the title Go: make data flow consistency checks available (and fix some) Go: make data flow consistency checks available (and fix one) Oct 25, 2023
@owen-mc owen-mc merged commit 27646ce into github:main Oct 25, 2023
@owen-mc owen-mc deleted the go/enable-data-flow-consistency-checks branch October 25, 2023 10:15
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