Go: Remove redundant code in IR::ExtractTupleElementInstruction.getResultType() and expand tests#19484
Merged
owen-mc merged 3 commits intogithub:mainfrom May 14, 2025
Merged
Conversation
The case of a CallExpr is actually covered by the next disjunct. Note that the CallExpr case had a subtle bug: `c.getTarget()` is not defined when we are calling a variable. Better to use `c.getCalleeType()`. But in this case we can just delete the code.
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR removes obsolete branching in ExtractTupleElementInstruction.getResultType() and extends IR tests to cover nested function calls, including calls via variables.
- Deleted redundant
CallExpr-specific logic ingetResultType() - Added
testNestedFunctionCallsintest.goto verify direct and variable-based calls - Updated
test.qlandtest.expectedto select and assert on base instruction, index, and result type
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| go/ql/test/library-tests/semmle/go/IR/test.ql | Expanded query to bind base, index, and resultType |
| go/ql/test/library-tests/semmle/go/IR/test.go | Added nested call test covering variable callee |
| go/ql/test/library-tests/semmle/go/IR/test.expected | Updated expected results for new test cases |
| go/ql/lib/semmle/go/controlflow/IR.qll | Removed redundant CallExpr branch in getResultType() |
smowton
approved these changes
May 14, 2025
Contributor
smowton
left a comment
smowton
left a comment
There was a problem hiding this comment.
While in theory target-type could be more specific, I don't think that can happen specifically with getTarget which only gives the type of the direct referee; if we used getACallee instead that could have potentially yielded a tighter bound on the actual result type.
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 saw a minor edge case bug (
CallExpr.getTarget()is not defined when we are calling a variable - we can instead useCallExpr.getCalleeType()when we just want the type of the target), but it turned out to be in redundant code, so I expanded our tests to confirm this and deleted the redundant code.