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/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
Assignees
Labels
Milestone

Comments

@griesemer
Copy link
Contributor

@griesemer griesemer commented Mar 17, 2021

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 this to the Go1.17 milestone Mar 17, 2021
@griesemer griesemer self-assigned this Mar 17, 2021
@griesemer
Copy link
Contributor Author

@griesemer griesemer commented Mar 17, 2021

cc: @findleyr

Loading

@danscales
Copy link

@danscales danscales commented Mar 17, 2021

Loading

@findleyr
Copy link
Contributor

@findleyr findleyr commented Mar 17, 2021

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.

Loading

@findleyr
Copy link
Contributor

@findleyr findleyr commented Mar 17, 2021

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

Loading

@griesemer
Copy link
Contributor Author

@griesemer griesemer commented Mar 17, 2021

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

Loading

@findleyr
Copy link
Contributor

@findleyr findleyr commented Mar 17, 2021

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.

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 17, 2021

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

Loading

@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

@gopherbot gopherbot commented Mar 18, 2021

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

Loading

gopherbot pushed a commit that referenced this issue Mar 18, 2021
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 gopherbot closed this in 51e4bb2 Mar 18, 2021
@gopherbot
Copy link

@gopherbot gopherbot commented May 11, 2021

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

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented May 11, 2021

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

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented May 13, 2021

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

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented May 13, 2021

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

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented May 14, 2021

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

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Jul 6, 2021

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

Loading

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
4 participants