-
Notifications
You must be signed in to change notification settings - Fork 20
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add tests for type assertions and type switches #149
Conversation
In passing: this sounds to me like something that would be straightforward to implement within an interpretation-based approach. (Which is not to say that there isn't a straightforward way to do it under the current approach.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great idea! I like that you opened a PR with code so we can comment on specific lines.
func TestTypeSwitchInline(i interface{}) { | ||
switch i.(type) { | ||
case core.Innocuous, *core.Source, core.Source: | ||
core.Sink(i) // TODO/discuss - do we want "a source has reached a sink" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so.
If this case is taken, then we know there's a possibility that i
could be a Source
, so to me this code is unsafe and should be fixed. Of course, it is possible that TestTypeSwitchInline
is only ever called with core.Innocuous
values, so producing a report would be "incorrect" when considering the whole program. However, to me that would be a programming error that I don't think we should accomodate.
// See the License for the specific language governing permissions and | ||
// limitations under the License. | ||
|
||
package _switch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this is a draft, and my question is besides the point, but: why the leading underscore? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since switch
is a keyword, package switch
refuses to compile.
I probably should've gone with package name typeswitch
anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense, didn't cross my mind to remove the underscore and see. 😅
core.Sink(i) | ||
core.Sink(t) | ||
case *core.Source: | ||
core.Sink(i) // TODO/discuss - do we want "a source has reached a sink" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should, since within that branch we know that i
is a Source
.
core.Sink(i) // TODO/discuss - do we want "a source has reached a sink" | ||
core.Sink(t) // want "a source has reached a sink" | ||
case core.Source: | ||
core.Sink(i) // TODO/discuss - do we want "a source has reached a sink" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above. I think it should, since within that branch we know that i
is a Source
.
case core.Source: | ||
core.Sink(i) // TODO/discuss - do we want "a source has reached a sink" | ||
core.Sink(t) // want "a source has reached a sink" | ||
default: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is more an interface{}
problem than a type switch
problem, but it is possible that i
is a source that just wasn't caught by any of the previous cases (e.g., i has type Foo
and Foo
was configured to be a source) and that this code is in fact unsafe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is more an interface{} problem
Yeah, my interpretation here is that just circles back to the interface
problems. All we know for sure is that it definitely isn't one of the above by type identity. That leaves a lot of options.
An observation: at the SSA level type switches are implemented as a series of comma-ok Honing in on this case: case core.Source:
core.Sink(i) // TODO/discuss - do we want "a source has reached a sink"
core.Sink(t) // want "a source has reached a sink" This is the SSA for
There is a clear path from This is the SSA for
It seems like since |
@@ -0,0 +1,45 @@ | |||
// Copyright 2020 Google LLC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To add: test case from discussion in 159.
Tests updated and moved to a more sensible package. Issue opened for intended work. |
Vaguely related to the [edit: Nevermind. That was fixed since the last time I merged with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. These are all important tests/test targets for us to have.
Sorry I didn't approve earlier, this fell off my radar.
It crossed my mind that we don't have test coverage for type switches. We appear to catch any source when it's reassigned to an identifyable type, but don't identify an source obfuscated by interface, even when we're in a block that would require that type.
That make sense given our current implementation and limitations. This might be trivialized by our general conversations surrounding how we plan to puncture the
interface
obfuscation problem. If not, we might need a way to mark an item as a source only within the scope of specificBlock
s.We have a good priority set right now. I just wanted to get a thread stated while it was top of mind.