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

cmd/compile/internal/types2: incorrect recorded type for untyped values that convert to a type parameter #45096

Closed
griesemer opened this issue Mar 17, 2021 · 14 comments
Closed

cmd/compile/internal/types2: incorrect recorded type for untyped values that convert to a type parameter #45096

griesemer opened this issue Mar 17, 2021 · 14 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.17

Comments

@griesemer
Copy link
Contributor

When the type checker encounters an untyped value (0 in this example) that needs to operate against a value of type parameter type (x in this case, in the expression x < 0), it makes sure that the operation (x < 0) is valid for any possible type in the type list of the type parameter. The existing algorithm assumed that it can update the type of the untyped operand to the type of the other operand (as there's just one, usually), and then record that type. But if there's a type parameter with a type list, there may possibly be many types in that list, and the algorithm is run against all of them. But because we record the "final" type as soon as we have a "match", the first entry in a type parameter list "wins" (if the program is correct in the first place).

package p

func _[T interface{ type int8, int16, int32  }](x T) {
      _ = x < 0
}

For this example, the reported type for 0 is the first entry in the type list, which is int8. But the correct type to be reported should be T (or possibly untyped int in this case, but that would probably break assumptions about what is reported by go/types and we likely can't change that).

There may be a simple solution that special-cases this type parameter case. Longer-term, a better approach would be to eliminate "state" in this algorithm and have a more functional approach. That would be much safer, and likely easier to understand.
@griesemer griesemer added the NeedsFix The path to resolution is known, but the work has not been done. label Mar 17, 2021
@griesemer griesemer added this to the Go1.17 milestone Mar 17, 2021
@griesemer griesemer self-assigned this Mar 17, 2021
@griesemer
Copy link
Contributor Author

cc: @findleyr

@danscales
Copy link
Contributor

/cc @danscales

@findleyr
Copy link
Contributor

I believe this is fixed in go/types, where untyped conversion is handled differently. The implicit type for a type param should be the type param itself, and due to the refactoring of untyped conversion, types are not recorded eagerly when checking convertibility.

@findleyr
Copy link
Contributor

See also https://golang.org/cl/284256 for more context on the decision in go/types.

@griesemer
Copy link
Contributor Author

@findleyr Ok, I will check against go/types and update this bug.

@findleyr
Copy link
Contributor

Note that originally in that CL, I was recording the type as a Sum type, which is another option, but reverted to the target type:
https://go-review.googlesource.com/c/go/+/284256/4..8/src/go/types/expr.go
(unfortunately, it seems I did not update the commit message).

A Sum type could be another possibility, as it contains slightly more information than the type param itself. But that is probably unnecessary.

I'd be happy to port something like this to types2. Feel free to assign to me.

@gopherbot
Copy link

Change https://golang.org/cl/302754 mentions this issue: go/types: add test case for issue #45096

@griesemer griesemer changed the title go/types, types2: incorrect recorded type for untyped values that convert to a type parameter cmd/compile/internal/types2: incorrect recorded type for untyped values that convert to a type parameter Mar 17, 2021
@gopherbot
Copy link

Change https://golang.org/cl/302755 mentions this issue: cmd/compile/internal/types2: delay recording types of untyped operands when checking against type parameters

gopherbot pushed a commit that referenced this issue Mar 18, 2021
@griesemer
go/types: add test case for issue #45096
2583c1b 
This verifies that issue #45096 is not an issue for go/types.

Updates #45096.

Change-Id: I4e987b5d4928f0c864d0d2c0379149443beb4d5c
Reviewed-on: https://go-review.googlesource.com/c/go/+/302754
Trust: Robert Griesemer <gri@golang.org>
Run-TryBot: Robert Griesemer <gri@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
@gopherbot
Copy link

Change https://golang.org/cl/318850 mentions this issue: [dev.typeparams] all: merge master (9b84814) into dev.typeparams

@gopherbot
Copy link

Change https://golang.org/cl/318989 mentions this issue: [dev.fuzz] all: merge master (9b84814) into dev.fuzz

@gopherbot
Copy link

Change https://golang.org/cl/319749 mentions this issue: [dev.fuzz] all: merge master (2a61b3c) into dev.fuzz

@gopherbot
Copy link

Change https://golang.org/cl/319890 mentions this issue: [dev.fuzz] all: merge master (7a7624a) into dev.fuzz

@gopherbot
Copy link

Change https://golang.org/cl/320050 mentions this issue: [dev.fuzz] all: merge master (d137b74) into dev.fuzz

@gopherbot
Copy link

Change https://golang.org/cl/333013 mentions this issue: [dev.cmdgo] all: merge master (912f075) into dev.cmdgo

@golang golang locked and limited conversation to collaborators Jun 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.17
Development

No branches or pull requests
4 participants
@danscales @griesemer @gopherbot @findleyr