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
Labels
FrozenDueToAge LanguageChange okay-after-beta1 Used by release team to mark a release-blocker issue as okay to resolve either before or after beta1 Proposal-Accepted release-blocker
Milestone

Comments

@griesemer
Copy link
Contributor

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 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 commented Jul 30, 2013

Comment 2:

Labels changed: added go2.

@rsc
Copy link
Contributor

rsc commented Dec 4, 2013

Comment 3:

Labels changed: added repo-main.

@griesemer
Copy link
Contributor Author

Comment 4:

Status changed to LongTerm.

@griesemer
Copy link
Contributor Author

Comment 5:

Labels changed: added release-none.

@griesemer
Copy link
Contributor Author

Comment 6:

Issue #8610 has been merged into this issue.

@griesemer
Copy link
Contributor Author

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 added longterm LanguageChange v2 A language change or incompatible library change labels Aug 28, 2014
@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 Used by Ian and Robert for Go 2 organization. Unless you’re Ian or Robert, please do not use this. and removed LongTerm labels Dec 5, 2017
@dsnet
Copy link
Member

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

@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 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

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

@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

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

@ianlancetaylor ianlancetaylor removed the Go2Cleanup Used by Ian and Robert for Go 2 organization. Unless you’re Ian or Robert, please do not use this. label Mar 17, 2020
@ianlancetaylor ianlancetaylor modified the milestones: Unplanned, Proposal Mar 17, 2020
@ianlancetaylor
Copy link
Contributor

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 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 v2 A language change or incompatible library change label May 6, 2020
@rsc rsc added this to Accepted in Proposals (old) 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

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

@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

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

@ianlancetaylor
Copy link
Contributor

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

@ianlancetaylor ianlancetaylor added release-blocker okay-after-beta1 Used by release team to mark a release-blocker issue as okay to resolve either before or after beta1 labels May 14, 2020
@gopherbot
Copy link

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>
@golang golang locked and limited conversation to collaborators May 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge LanguageChange okay-after-beta1 Used by release team to mark a release-blocker issue as okay to resolve either before or after beta1 Proposal-Accepted release-blocker
Projects
No open projects
Development

No branches or pull requests

7 participants