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

go/types: inconsistent handling of untyped expressions #13061

Open
mdempsky opened this issue Oct 27, 2015 · 13 comments
Open

go/types: inconsistent handling of untyped expressions #13061

mdempsky opened this issue Oct 27, 2015 · 13 comments
Assignees
Labels
Milestone

Comments

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Oct 27, 2015

Currently if you type check this code with go/types:

var (
    _ int = 0
    _ = int(0)
    _ *int = nil
    _ = (*int)(nil)
)

both 0 expressions will have type "int", whereas the first nil expression will have type "untyped nil", and the second will have type "*int". This seems at the least inconsistent. See http://play.golang.org/p/cw8Ldz1U5D

My expectation was that the 0s would type check as "untyped int" and the nils would type check as "untyped nil". The current behavior of rewriting the type of untyped expressions seems to have two negative consequences that I've noticed trying to use go/types:

  1. It makes it difficult to identify useless conversion operations; i.e., expressions T(x) where x is already of type T. Expressions like int(0) will trigger as false positives.
  2. It causes types.TypeAndValue.IsNil to return false for the nil subexpression in (*int)(nil), because it doesn't have the type "untyped nil" anymore.

It also seems inconsistent with conversions of already typed expressions, where they're not rewritten.

However, I notice api_test.go has a bunch of tests that seem to explicitly test for this behavior, so it seems intentional?

CC @griesemer

@griesemer
Copy link
Contributor

@griesemer griesemer commented Oct 27, 2015

@mdempsky It is definitively intentional - untyped constants are given their "inferred" type so that a backend - which needs to materialize the constants - knows of what type and size a constant is.

My guess would have been - for the example above - that the types would be int, untyped int, *nil, and untyped nil, but I need to look into the logic again.

I suspect the reason for the inconsistency is that nil is not considered a constant and thus perhaps is handled in a different code path.

@griesemer griesemer self-assigned this Oct 27, 2015
@griesemer griesemer added this to the Go1.6 milestone Oct 27, 2015
@mdempsky
Copy link
Member Author

@mdempsky mdempsky commented Oct 27, 2015

I'd think the backend could determine the needed type and size for materializing the constants some other way (e.g., looking at the type of the variable they're being assigned to)? But at least if 0 in int(0) and nil in (*int)(nil) became "untyped int" and "untyped nil", respectively, like you suggest, that would help my immediate problem of wanting to identify unnecessary conversions.

@mdempsky
Copy link
Member Author

@mdempsky mdempsky commented Oct 29, 2015

I took a stab at this, but discovered the trivial fix (just don't propagate types to explicit conversion arguments) causes a regression on this test case in test/fixedbugs/bug193.go, because it no longer detects the expected errors:

func main() {
        s := uint(10)
        ss := 1 << s
        y1 := float64(ss)
        y2 := float64(1 << s) // ERROR "shift"
        y3 := string(1 << s)  // ERROR "shift"
        _, _, _, _, _ = s, ss, y1, y2, y3
}

I'm actually rather surprised to find out the above is not valid Go (whereas float64(1 << uint(10)) is valid), but it seems to be in the spec:

If the left operand of a non-constant shift expression is an untyped constant, it is first converted to the type it would assume if the shift expression were replaced by its left operand alone.

Hrm.

@griesemer
Copy link
Contributor

@griesemer griesemer commented Oct 29, 2015

@mdempsky yes, shifts are tricky business . i'll take care of this (let me know if this is blocking you)

@mdempsky
Copy link
Member Author

@mdempsky mdempsky commented Oct 29, 2015

Nope, not blocking me. I was just interested in better understanding the problem.

@rsc rsc added this to the Unplanned milestone Nov 30, 2015
@rsc rsc removed this from the Go1.6 milestone Nov 30, 2015
@griesemer
Copy link
Contributor

@griesemer griesemer commented Feb 25, 2017

Investigate and decide what to do.

@griesemer griesemer added this to the Go1.9 milestone Feb 25, 2017
@griesemer griesemer removed this from the Unplanned milestone Feb 25, 2017
@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Jun 19, 2017

Ping Robert. You'd labeled this Go 1.9.

@mdempsky
Copy link
Member Author

@mdempsky mdempsky commented Jun 19, 2017

I think this can be safely bumped to 1.10. Nothing changed during 1.9 with respect to this issue.

@mdempsky mdempsky added this to the Go1.10 milestone Jun 19, 2017
@mdempsky mdempsky removed this from the Go1.9 milestone Jun 19, 2017
@rsc rsc removed this from the Go1.10 milestone Nov 22, 2017
@rsc rsc added this to the Go1.11 milestone Nov 22, 2017
@griesemer
Copy link
Contributor

@griesemer griesemer commented Dec 21, 2017

The reason for the inconsistent behavior is that nil is indeed handled specially, see go/types/expr.go, Checker.convertUntyped function, at the end:

case *Pointer, *Signature, *Slice, *Map, *Chan:
	if !x.isNil() {
		goto Error
	}
	// keep nil untyped - see comment for interfaces, above
	target = Typ[UntypedNil]

Removing the special case for nil (deleting the line target = Typ[UntypedNil]) will produce consistent output where all untyped values in this case get registered with their "target" type; i.e., the untyped nil we see would become *int as well.

According to the comment, it's important to record the untyped nil here for tools like vet. We could do the same for non-nil values but as @mdempsky noticed, this causes problems with shifts.

There may be a solution, still. Leaving open for now.

@griesemer
Copy link
Contributor

@griesemer griesemer commented Dec 21, 2017

It does seem reasonable to record the untyped type for values that are explicitly converted. So we should definitively fix this.

@griesemer griesemer removed this from the Go1.11 milestone Jun 7, 2018
@griesemer griesemer added this to the Go1.12 milestone Jun 7, 2018
@griesemer griesemer removed this from the Go1.12 milestone Dec 6, 2018
@griesemer griesemer added this to the Unplanned milestone Dec 6, 2018
@griesemer
Copy link
Contributor

@griesemer griesemer commented Jan 15, 2021

CL 283872 is addressing this partly for types2 (in dev.typeparams branch): with this CL, untyped nils consistently report the type untyped nil (even in conversions). This makes the untyped nil behavior consistent across all operations involving nil.

For any other untyped operand we do report the "context type" though, which is different from the behavior for nil. This is necessary (at the moment) to make shifts behave correctly, e.g. int(1.0 << s) is valid because the type of 1.0 is int due to the conversion.

It may make sense to always report the context type for any untyped value (even untyped nils).

cc: @mdempsky

@gopherbot
Copy link

@gopherbot gopherbot commented Jan 15, 2021

Change https://golang.org/cl/284052 mentions this issue: [dev.typeparams] cmd/compile/internal/types2: consistently report nil type as "untyped nil"

gopherbot pushed a commit that referenced this issue Jan 15, 2021
… type as "untyped nil"

This fixes an inconsistency where the type for nil in code such as

	var x unsafe.Pointer = nil

and in conversions of the form

	T(nil)

(where T is a pointer, function, slice, map, channel, interface, or
unsafe.Pointer) was reported as (converted to) the respective type.
For all other operations that accept a nil value, we don't do this
conversion for nil.

(We never change the type of the untyped nil value, in contrast to
other untyped values where we give the values context-specific types.)

It may still be useful to change this behavior and - consistently -
report a converted nil type like we do for any other type, but for
now this CL simply fixes the existing inconsistency.

Added tests and fixed existing test harness.

Updates #13061.

Change-Id: Ia82832845c096e3cbc4a239ba3d6c8b9a9d274c9
Reviewed-on: https://go-review.googlesource.com/c/go/+/284052
Trust: Robert Griesemer <gri@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Robert Griesemer <gri@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
@gopherbot
Copy link

@gopherbot gopherbot commented Feb 4, 2021

Change https://golang.org/cl/289715 mentions this issue: [dev.regabi] go/types: consistently report nil type as "untyped nil"

gopherbot pushed a commit that referenced this issue Feb 8, 2021
This is a port of CL 284052 to go/types. The port is not entirely
faithful, as untyped conversion has been refactored in go/types.
Additionally, a comment was added to reference issue #13061 in the
implicitType method.

For #13061

Change-Id: Iec17611f6432c988624023d1d74121ff34eb0c83
Reviewed-on: https://go-review.googlesource.com/c/go/+/289715
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Robert Findley <rfindley@google.com>
Trust: Robert Griesemer <gri@golang.org>
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
Labels
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants