Skip to content
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

cmd/vet: detect impossible interface-interface type assertions #4483

Closed
griesemer opened this issue Dec 3, 2012 · 22 comments
Closed

cmd/vet: detect impossible interface-interface type assertions #4483

griesemer opened this issue Dec 3, 2012 · 22 comments

Comments

@griesemer
Copy link
Contributor

@griesemer griesemer commented Dec 3, 2012

For:

$ cat test.go
package main

import "io"

func main() {
    var y interface {
        Read()
    }
    _ = y.(io.Reader)
}

Neither compiler complains:

$ gccgo test.go
$ go tool 6g test.go

And according to the spec it is a valid program.

However, any concrete value assigned to y must implement Read(), and thus cannot
possibly implement io.Reader.Read. Thus, this type assertion will fail and we know this
at compile time.

The spec should require this check and compilers should implement it. This would be a
backward-incompatible language change, albeit with a very small chance of actually
breaking existing code.
@rsc
Copy link
Contributor

@rsc rsc commented Dec 6, 2012

Comment 1:

For what it's worth, I don't believe this is something we can change
during Go 1.
Russ
@rsc
Copy link
Contributor

@rsc rsc commented Jul 30, 2013

Comment 2:

Labels changed: added go2.

@rsc
Copy link
Contributor

@rsc rsc commented Dec 4, 2013

Comment 3:

Labels changed: added repo-main.

@griesemer
Copy link
Contributor Author

@griesemer griesemer commented Feb 27, 2014

Comment 4:

Status changed to LongTerm.

@griesemer
Copy link
Contributor Author

@griesemer griesemer commented Aug 25, 2014

Comment 5:

Labels changed: added release-none.

@griesemer
Copy link
Contributor Author

@griesemer griesemer commented Aug 28, 2014

Comment 6:

Issue #8610 has been merged into this issue.

@griesemer
Copy link
Contributor Author

@griesemer griesemer commented Aug 28, 2014

Comment 7:

From issue #8610, a point to consider:
When it comes to interface assignments ("implements" relation), conflicting method
signatures are not permitted.
@griesemer griesemer self-assigned this Aug 28, 2014
@rsc rsc added this to the Unplanned milestone Apr 10, 2015
@rsc rsc changed the title spec: more stringent type assertions spec: detect impossible interface-interface type assertions Jun 17, 2017
@rsc rsc changed the title spec: detect impossible interface-interface type assertions proposal: spec: detect impossible interface-interface type assertions Jun 17, 2017
@ianlancetaylor ianlancetaylor added Go2Cleanup and removed LongTerm labels Dec 5, 2017
@dsnet
Copy link
Member

@dsnet dsnet commented Nov 7, 2018

I believe this is more suitable for a vet check than a language change.

Here's a real use-case for impossible type assertions:

In generated code, where users may inject custom methods to a type via a separate file. Since the generated code cannot know what the users may inject, it conservatively produces type assertions checking for the user's methods. In the event that the user does not inject methods, the type assertion is an impossible one (i.e., always fails).

Example code:

// In my_proto.pb.go, which is generated code

func (m *MyMessage) ProtoUnmarshal(b []byte, o protoiface.UnmarshalOptions) (protoiface.SupportFlags, error) {
    // For legacy support check if the user manually injected their own unmarshal method.
    // If so, call it to maintain backwards compatibility.
    if u, ok := (interface{})(m).(protoV1.Unmarshaler); ok {
        return 0, u.Unmarshal(b)
    }
    ...
}

// This file does not define a MyMessage.Unmarshal method.
// In custom_methods.go, which is user created

func (m *MyMessage) Unmarshal(b []byte) error {
    ...
}

Depending on whether custom_methods.go exists or not, the type assertion in my_proto.pb.go will always succeed or always fail.

A similar pattern may exist for methods that are only present if a build tag is present. However, that situation isn't as bad since the user can add a tag-protected boolean constant that is used to perform control flow.

@griesemer
Copy link
Contributor Author

@griesemer griesemer commented Nov 8, 2018

@dsnet I may be misunderstanding your case but I believe this issue addresses a different situation: A type assertion of the form x.(T) where x's type and T are both interfaces, and where both the type of x and T have a method with the same name m, but different signatures. That is, all the information is known to the compiler at all times (no matter what value is in x) - such an assertion will always fail.

For instance, the type assertion in https://play.golang.org/p/PEevF5j55Et will always fail no matter what, yet the compiler doesn't complain.

@dsnet
Copy link
Member

@dsnet dsnet commented Nov 8, 2018

both the type of x and T have a method with the same name m, but different signatures

Ah, I see. Ignore my comment above then. I have no objection then.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Nov 8, 2018

I have a slightly different objection. One can imagine using build tags to construct various possible interfaces, and then check them in shared code. Such a program may have type assertions that can never succeed in one build configuration but will succeed in a different configuration.

I feel that compiler enforced checks of this sort are unhelpful because they make certain unusual kinds of programs very hard to write, but the benefit of the checks is very small. The benefit is small because programmers rarely make this kind of mistake, and when they do it is normally discovered quite quickly in the course of ordinary testing. I wouldn't even make this a vet check. I think it might be appropriate for lint.

@griesemer
Copy link
Contributor Author

@griesemer griesemer commented Nov 8, 2018

@ianlancetaylor I'd agree that the benefit is small overall (this is not a common bug), but the scenario you're describing wouldn't be much more difficult to express either: It would still be trivially possible to assign those interfaces to an empty interface and do the subsequent type assertion at that point: https://play.golang.org/p/IPDncMPg3z4

In summary, the benefit may be small, but the fix is also tiny (one line). Code that uses build tags to create various interfaces is not unlikely, but also not common. The specific scenario you're describing is easily dealt with using an empty interface (which may be the interface of choice anyway in those situations). It seems to me at least, that if a type assertion is obviously impossible because signatures don't match, we shouldn't permit them in the first place. Note that we don't allow them for type assertions using concrete (target) types, and arguably the same argument using build-tag affected code could be made there (and it's not a problem). See: https://play.golang.org/p/sbCjdIxvuAV .

@gopherbot
Copy link

@gopherbot gopherbot commented Feb 10, 2020

Change https://golang.org/cl/218779 mentions this issue: go/analysis: add pass to check for impossible interface-interface type assertions

@gopherbot
Copy link

@gopherbot gopherbot commented Feb 25, 2020

Change https://golang.org/cl/220977 mentions this issue: cmd/vet: include ifaceassert and stringintconv checks from upstream x/tools

@gopherbot
Copy link

@gopherbot gopherbot commented Feb 27, 2020

Change https://golang.org/cl/221339 mentions this issue: cmd/vet: add ifaceassert and stringintconv checks

gopherbot pushed a commit that referenced this issue Mar 10, 2020
This change re-vendors x/tools to add the ifaceassert and stringintconv
checks to cmd/vet.

Fixes #32479.
Updates #4483.

Change-Id: I6bd30b0a3278592dfab4bd247036404ddaff09e4
Reviewed-on: https://go-review.googlesource.com/c/go/+/221339
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
@ianlancetaylor ianlancetaylor modified the milestones: Unplanned, Proposal Mar 17, 2020
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Mar 17, 2020

This is expected to be a vet check for 1.14. For a future language version we will consider moving this into the language proper.

@rsc
Copy link
Contributor

@rsc rsc commented May 6, 2020

As a reminder, we introduced a new process for these Go 2-related language changes in our blog post blog.golang.org/go2-here-we-come. The Go 1.15 development cycle is now over and it’s time for the final decision for the issues described at https://blog.golang.org/go1.15-proposals, including this one.

This has been implemented since March 9 in the development tree, but we haven't yet enabled it by default in the go command (in cmd/go/internal/test/test.go).

There has been no negative feedback otherwise, so accepting this proposal for Go 1.15.

— rsc for proposal review

@rsc rsc changed the title proposal: spec: detect impossible interface-interface type assertions proposal: cmd/vet: detect impossible interface-interface type assertions May 6, 2020
@rsc rsc removed the Go2 label May 6, 2020
@rsc rsc added this to Accepted in Proposals May 6, 2020
@rsc rsc changed the title proposal: cmd/vet: detect impossible interface-interface type assertions cmd/vet: detect impossible interface-interface type assertions May 6, 2020
@rsc rsc modified the milestones: Proposal, Go1.15 May 6, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented May 6, 2020

Change https://golang.org/cl/232660 mentions this issue: cmd/go: enable stringintconv and ifaceassert vet checks by default

gopherbot pushed a commit that referenced this issue May 13, 2020
As per discussion on the accepted proposals, enable these vet checks by
default in the go command. Update corresponding documentation as well.

Updates #32479.
Updates #4483.

Change-Id: Ie93471930c24dbb9bcbf7da5deaf63bc1a97a14f
Reviewed-on: https://go-review.googlesource.com/c/go/+/232660
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@toothrot
Copy link
Contributor

@toothrot toothrot commented May 14, 2020

@smasher164 @ianlancetaylor @griesemer: Does this issue need to be updated now that https://golang.org/cl/232660 is merged? It's labeled as blocking the Go 1.15 beta.

@smasher164
Copy link
Member

@smasher164 smasher164 commented May 14, 2020

@toothrot Since CL 232660 has been merged, this issue is resolved, and no longer a release blocker.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented May 14, 2020

I'm planning to review this and #32479 to make sure they are done and to write release notes.

@gopherbot
Copy link

@gopherbot gopherbot commented May 18, 2020

Change https://golang.org/cl/234518 mentions this issue: doc/go1.15: mention vet warning for impossible type assertions

gopherbot pushed a commit that referenced this issue May 18, 2020
For #4483

Change-Id: Iab76baf50b79eda1e3acfd662d0e7830c7962f5d
Reviewed-on: https://go-review.googlesource.com/c/go/+/234518
Reviewed-by: Robert Griesemer <gri@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Accepted
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
7 participants
You can’t perform that action at this time.