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

Jump to bottom

go/types: inconsistent AssignableTo, ConvertibleTo behavior w/ invalid type #53595

Open
muirdm opened this issue Jun 29, 2022 · 14 comments
Open

go/types: inconsistent AssignableTo, ConvertibleTo behavior w/ invalid type #53595

muirdm opened this issue Jun 29, 2022 · 14 comments
Assignees
@griesemer
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.23

Comments

@muirdm
Copy link

muirdm commented Jun 29, 2022

What version of Go are you using (go version)?

$ go version
go version go1.18.3 darwin/arm64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="arm64"
GOBIN=""
GOCACHE="/Users/muir/Library/Caches/go-build"
GOENV="/Users/muir/Library/Application Support/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="arm64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/muir/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/muir/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_arm64"
GOVCS=""
GOVERSION="go1.18.3"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/muir/projects/tools/go.mod"
GOWORK=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/gs/s6cspwzn2gxgp826h7ld15800000gn/T/go-build2413164799=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

Checked if the invalid type was types.AssignableTo an interface and non-interface type.
https://go.dev/play/p/PQ7oklDR7lR

What did you expect to see?

I expected the invalid type to not be reported assignable to the error interface, as in Go 1.17.

What did you see instead?

The invalid type is reported as assignable to the error interface. (I believe it is reported assignable to all interface types.)

@findleyr This seems to be causing bad completion ordering in gopls. Namely, *types.PkgNames are getting a high completion score when the expected type is an interface. PkgName's type is invalid, and AssignableTo leads us to believe the invalid type is a good match for the expected type.
@shaoliming123
Copy link

confirmed that invalid type to not be reported assignable to the error interface in go 1.14
cc @griesemer

@griesemer griesemer added the NeedsFix The path to resolution is known, but the work has not been done. label Jun 29, 2022
@griesemer griesemer added this to the Go1.20 milestone Jun 29, 2022
@griesemer
Copy link
Contributor

In the type checker, to avoid follow-on errors caused by earlier errors in the source, we usually accept an invalid operand or type as "valid", without reporting an error.

What happens here is that an invalid type always implements any interface without error, hence invalid is assignable to error. What we don't do here (and probably we should, for consistency) is to permit assigning an invalid operand/type to some other (non-interface) type. This is a trivial fix and would lead to consistent behavior, but it's not clear to me if it solves the gopls issue.

That said, we probably don't want to change the behavior of invalid operands/types in the type checker dep. on whether AssignableTo was called externally or internally.

@muirdm It should be easy to use a wrapper around AssignableTo (and whatever other API entry points that need it) which checks that the arguments are valid types, and if not, return the correct negative result.

Internally, we should make the type checker behavior consistent and accept invalid in both cases.

@findleyr Other thoughts?

@muirdm
Copy link
Author

muirdm commented Jun 29, 2022 •
edited

It should be easy to use a wrapper around AssignableTo

Certainly yes. I wanted to understand the desired behavior of AssignabeTo before updating gopls.

an invalid type always implements any interface without error

This seems to be new behavior in Go 1.18, at least wrt AssignableTo. Ignoring the internal go/types use case of minimizing errors, why should an invalid type implement all interfaces? Intuitively, why is it more correct/useful for go/types to say that an invalid type can be assigned to/from anything?

@griesemer
Copy link
Contributor

The implementation of "implements" was completely rewritten for 1.18 as it needed to accommodate the new interface definition. So I'm not surprised the behavior changed slightly. Changing it back is trivial but it does lead to an unnecessary error in at least one of our (admittedly esoteric) test cases.

Usually, an invalid type is the result of an error in the package that's being type checked. That type may be stored/retained as the type of the erroneous variable or expression. It's standard practice throughout the type checker to ignore such invalid types. If we don't, a local error may lead to additional errors elsewhere where they are not helpful. We are not 100% consistent with this, which is ok because the primary impact (ignoring API calls for the moment) is that the type checker and thus compiler may report more errors than needed (only if there are errors in the first place).

Thus, being able to ignore invalid types or operands (which basically means, treating them as "correct") is a very useful feature and significantly reduces the number of follow-on errors in the presence of errors.

We could catch invalid types in the exported AssignableTo (rather than requiring clients to use a wrapper) but it would only work at the top level: in general an invalid type may appear deeply nested in some enclosing composite type, and we probably don't want to walk each type first.

In general, it's probably best for a client to not rely on a specific behavior for undefined types, but to exclude them as needed.

@findleyr
Copy link
Contributor

I don't have a strong opinion, but agree that we should be consistent in both cases. We don't have a coherent theory of Typ[Invalid], so it's hard to make a principled argument either way. I'd lean toward preserving the 1.17 behavior (and backporting the fix).

Two additional notes:

  • We have explicit handling for Typ[Invalid] in AssertableTo
  • The behavior of ConvertibleTo also changed in 1.18: https://go.dev/play/p/bBgW7xRuiMP (compare with selecting 1.17 in the backend dropdown)

Either way, it's probably a good idea to add explicit handling in gopls.

@findleyr findleyr changed the title go/types: inconsistent AssignableTo behavior w/ invalid type go/types: inconsistent AssignableTo, ConvertibleTo behavior w/ invalid type Jun 29, 2022
@griesemer
Copy link
Contributor

griesemer commented Jun 30, 2022 •
edited

@findleyr You make an excellent observation with AssertableTo: we explicitly check for invalid types: since this effectively says that an interface cannot have an invalid type as a dynamic type, we should also not let an invalid type implement an interface (at least with respect to how these exported functions behave).

So let's maintain 1.17 behavior. (see below)

@gopherbot
Copy link

Change https://go.dev/cl/415334 mentions this issue: types2: exported predicates to return false for invalid type arguments

@griesemer griesemer modified the milestones: Go1.20, Go1.19 Jun 30, 2022
@griesemer griesemer self-assigned this Jun 30, 2022
@griesemer
Copy link
Contributor

Per further discussion, maintaining 1.17 behavior is inconsistent as well. Decided to document that behavior is undefined for invalid types for now. We can consider applying https://go.dev/cl/415334 for 1.20.

@griesemer griesemer modified the milestones: Go1.19, Go1.20 Jun 30, 2022
@gopherbot
Copy link

Change https://go.dev/cl/415574 mentions this issue: go/types, types2: document that exported predicates are undefined for invalid type arguments

gopherbot pushed a commit that referenced this issue Jul 1, 2022
@griesemer
go/types, types2: document that exported predicates are unspecified f…
c847a2c 
…or invalid type arguments

Per discussion on the issue.

For #53595.

Change-Id: Iefd161e5c7e792d454652cbe831a0c2d769f748e
Reviewed-on: https://go-review.googlesource.com/c/go/+/415574
Reviewed-by: Robert Findley <rfindley@google.com>
Reviewed-by: Robert Griesemer <gri@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
@gopherbot
Copy link

Change https://go.dev/cl/415595 mentions this issue: lsp/completion: fix ranking of *types.PkgName candidates

gopherbot pushed a commit to golang/tools that referenced this issue Jul 1, 2022
@muirdm @findleyr
lsp/completion: fix ranking of *types.PkgName candidates
ffc70b9 
In Go 1.18 types.AssignableTo() started reporting that an invalid type
is assignable to any interface. *types.PkgName (i.e. an import at the
top of the file) has an invalid type for its Type(), so we started
thinking all in scope imports were great candidates when the expected
type was an interface.

Fix by wrapping the AssignableTo (and AssertableTo) to explicitly
return false if either operand is invalid.

Updates golang/go#53595

Change-Id: Ie5a84b7f410ff5c73c6b7870e052bafaf3e21e99
Reviewed-on: https://go-review.googlesource.com/c/tools/+/415595
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
jproberts pushed a commit to jproberts/go that referenced this issue Aug 10, 2022
@griesemer @jproberts
go/types, types2: document that exported predicates are unspecified f…
50ee0db 
…or invalid type arguments

Per discussion on the issue.

For golang#53595.

Change-Id: Iefd161e5c7e792d454652cbe831a0c2d769f748e
Reviewed-on: https://go-review.googlesource.com/c/go/+/415574
Reviewed-by: Robert Findley <rfindley@google.com>
Reviewed-by: Robert Griesemer <gri@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
@griesemer
Copy link
Contributor

Some tools depend on current behavior. We need to have a good answer for tools first (they may want to use their own wrappers with check for invalid types and forward to go/types otherwise). Moving to 1.21.

@griesemer griesemer modified the milestones: Go1.20, Go1.21 Nov 21, 2022
@griesemer griesemer added the early-in-cycle A change that should be done early in the 3 month dev cycle. label Nov 21, 2022
@gopherbot
Copy link

This issue is currently labeled as early-in-cycle for Go 1.21.
That time is now, so a friendly reminder to look at it again.

@griesemer
Copy link
Contributor

The comment here still applies. Moving to 1.22.

@griesemer griesemer modified the milestones: Go1.21, Go1.22 May 16, 2023
@griesemer griesemer removed the early-in-cycle A change that should be done early in the 3 month dev cycle. label May 16, 2023
@griesemer
Copy link
Contributor

Moving to 1.23.

@griesemer griesemer modified the milestones: Go1.22, Go1.23 Nov 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@griesemer griesemer

Labels
NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.23
Development

No branches or pull requests
5 participants
@griesemer @gopherbot @muirdm @findleyr @shaoliming123