Go: fix DataFlow::ResultNode and some related things#21957
Open
owen-mc wants to merge 8 commits into
Open
Conversation
It was the number of result declarations, which is different from the number of results when one result declaration declares more than one variable, as in `x, y int`.
It is unused in this library. It could easily be used incorrectly and silently omit results when `getNumResult() > 1`.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the Go dataflow and type-model libraries to correctly model result nodes for functions with named result parameters, and to align FuncTypeExpr result-counting APIs with how Go result parameters are declared and used.
Changes:
- Stop creating
DataFlow::ResultNodes for explicitreturn <expr>expressions when the function has named result parameters (use the final result-variable reads instead). - Fix
FuncTypeExpr.getNumResult()to count result parameters (names), not result declarations, and deprecate the arity-0getResultDecl(). - Add/extend Go library tests and update expected CFG/dataflow baselines accordingly.
Show a summary per file
| File | Description |
|---|---|
| go/ql/test/library-tests/semmle/go/dataflow/Nodes/resultParameters.go | Adds regression coverage for result nodes with and without named result parameters. |
| go/ql/test/library-tests/semmle/go/dataflow/Nodes/ResultNode.qlref | Wires the new test query into the inline expectations harness. |
| go/ql/test/library-tests/semmle/go/dataflow/Nodes/ResultNode.ql | Test query enumerating DataFlow::ResultNodes for inline expectation checks. |
| go/ql/test/library-tests/semmle/go/dataflow/Nodes/ResultNode.expected | Captures expected locations/indices for result nodes, including named-result implicit reads. |
| go/ql/test/library-tests/semmle/go/dataflow/Nodes/main.go | Adds an inline expectation for multi-result returns in the existing Nodes test pack. |
| go/ql/test/library-tests/semmle/go/controlflow/ControlFlowGraph/main.go | Adds a named-result function variant to exercise CFG behavior around returns. |
| go/ql/test/library-tests/semmle/go/controlflow/ControlFlowGraph/ControlFlowNode_getASuccessor.expected | Updates expected CFG successor edges/baselines to reflect the added function. |
| go/ql/lib/semmle/go/Expr.qll | Fixes result-counting semantics for FuncTypeExpr and deprecates the arity-0 getResultDecl(). |
| go/ql/lib/semmle/go/dataflow/internal/DataFlowNodes.qll | Adjusts DataFlow::ResultNode construction to avoid duplicating result nodes for named-result returns. |
| go/ql/lib/change-notes/2026-06-08-functypeexpr-getnumresult.md | Documents the getNumResult() behavior change (currently with a naming typo). |
| go/ql/lib/change-notes/2026-06-08-fix-result-nodes.md | Documents the result-node modeling change (currently with an IR identifier typo). |
| go/ql/lib/change-notes/2026-06-08-deprecate-functypeexpr-getresultdecl.md | Announces the deprecation of FuncTypeExpr.getResultDecl() in favor of the indexed overload. |
Copilot's findings
- Files reviewed: 12/12 changed files
- Comments generated: 3
| --- | ||
| category: minorAnalysis | ||
| --- | ||
| * `FuncTypeExpr.getNumResults()` now gets the number of result parameters. It previously got the number of result declarations, which is different when one result declaration declares more than one variable, as in `x, y int`. All uses of it expected the number of result parameters. Its QLDoc has been updated. |
| --- | ||
| category: minorAnalysis | ||
| --- | ||
| * `DataFlow::ResultNode`s are no longer created for returned expressions in functions with named result parameters. In this case there are already result nodes corresponding to `IR::ResultReadInstruction`s at the end of the function body. |
Comment on lines
+935
to
+937
| // When a function has any (named) result variables, then the | ||
| // `ReadResultInstruction`s at the end of the function are the correct | ||
| // result nodes. |
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.
DataFlow::ResultNodes are no longer created for returned expressions in functions with named result parameters. In this case there are already result nodes corresponding toIR::ResultReadInstructions at the end of the function body.FuncTypeExpr.getNumResults()now gets the number of result parameters. It previously got the number of result declarations, which is different when one result declaration declares more than one variable, as inx, y int. All uses of it expected the number of result parameters. Its QLDoc has been updated.FuncTypeExpr.getResultDecl()has been deprecated. It was unused and could silently omit results.