This repository has been archived by the owner on Jan 5, 2023. It is now read-only.
Refine potential targets for method call through interface #219
Conversation
This file contains 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
added 4 commits
June 22, 2020 09:22
The call target must belong to the method set of a type that implements the interface type of the method call receiver, if any. For example, assume `h` has type `hash.Hash`, then `h.Write(...)` should only be resolved to implementations of `Write` in types implementing `hash.Hash`, not arbitrary other `Writer`s.
max-schaefer
force-pushed
the
refine-virtual-dispatch
branch
from
584bd10
to
1f68a32
Compare
owen-mc
approved these changes
Jun 22, 2020
|
CodeQL analysis will fail eventually, but that's unrelated to this PR (cf #220), so I'll just go ahead and merge. |
ceh
pushed a commit
to ceh-forks/codeql-go
that referenced
this pull request
Jul 22, 2020
Teach extractor about CodeQL environment variables.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
When faced with a call
x.m()where the type ofxis some interfaceI,getACallee()does the conservative thing and resolves the call to all overriding definitions ofm.For the data-flow analysis, we try a bit harder: the
viableCalleepredicate checks whether it can by local reasoning determine a concrete (non-interface) type forx, or perhaps a set of such types, and then resolvesmon those.This PR adds an orthogonal refinement: even if we can't determine a concrete type for
x, we can still require that the overriding definition ofmwe select must belong to a type that implementsI. This isn't a trivial condition, sinceImight inheritmfrom another interfaceJ, and this way we might be able to exclude some definitions ofmin types that implementJbut notI.Evaluation (internal link) shows no performance change and one alert change. The alert in question had 56 paths; I didn't check all of them, but the ones I did inspect all had a spurious call step of the kind described above in them, so I'm inclined to believe that this was a false positive that is now fixed.
It also fixes a wrong call step observed by @owen-mc, but it does not, by itself, fix the false positive that gave rise to. There is another imprecision involved, which I will fix in a separate PR.
Commit-by-commit review is encouraged. The first commit is not logically related to the rest of the PR, but it's useful for debugging and I've wanted to add it for a while.