cmd/vet: detect impossible interface-interface type assertions

[griesemer]

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]

Comment 1:

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

[rsc]

Comment 2:

Labels changed: added go2.

[rsc]

Comment 3:

Labels changed: added repo-main.

[griesemer]

Comment 4:

Status changed to LongTerm.

[griesemer]

Comment 5:

Labels changed: added release-none.

[griesemer]

Comment 6:

Issue #8610 has been merged into this issue.

[griesemer]

Comment 7:

From issue #8610, a point to consider:
When it comes to interface assignments ("implements" relation), conflicting method
signatures are not permitted.

[dsnet]

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]

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

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]

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]

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

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

[gopherbot]

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

[gopherbot]

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

[ianlancetaylor]

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]

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

[gopherbot]

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

[toothrot]

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

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

[ianlancetaylor]

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

[gopherbot]

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