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

dev.go2go: translator generates incorrect code for type switches, leading to duplicate cases #42758

Open
tdakkota opened this issue Nov 21, 2020 · 10 comments
Open

dev.go2go: translator generates incorrect code for type switches, leading to duplicate cases #42758

tdakkota opened this issue Nov 21, 2020 · 10 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Unreleased

Comments

@tdakkota
Copy link

tdakkota commented Nov 21, 2020 •
edited

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

$ go version
go version devel +165ceb09f9 Tue Nov 3 12:31:40 2020 -0500 windows/amd64

Does this issue reproduce with the latest release?

n/a

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

  • windows/amd64
  • go2go playground

What did you do?

https://go2goplay.golang.org/p/dTkkesE4GCX.
Error is printing when go1 compiler tries to compile translated code.
Also I ran on dev.typeparams

go tool compile -G ./issue.go

where issue.go is code from playground

Here instantiated createRequest[io.Writer] look something like:

func createRequest_ioWriter(w io.Writer) {
	switch (interface{})(w).(type) {
	case io.Writer:
		fmt.Println("W")
	case io.Writer:
		fmt.Println("io.Writer")
	}

}

What did you expect to see

An error from go2go like type parameter cannot be case of type switch.

What did you see instead?

Code passes go2go and dev.typeparams type checker.
@tdakkota tdakkota changed the title dev.typeparams, dev.go2go: type switch with generic type case can be a duplicate dev.typeparams, dev.go2go: type checker permits type switch with generic type case which can be a duplicate Nov 21, 2020
@tdakkota
Copy link
Author

CC @griesemer

@griesemer
Copy link
Contributor

The typeparams draft doesn't disallow type parameters in type switch cases (or type assertions for that matter). A type parameter is a type like any other, so it's not obvious that this should be prohibited.

At the moment I consider this a limitation of the source-to-source translator (prototype).

@ianlancetaylor ianlancetaylor added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Nov 24, 2020
@ianlancetaylor ianlancetaylor added this to the Unreleased milestone Nov 24, 2020
@ianlancetaylor
Copy link
Contributor

I agree, but I don't plan to do any more work on this experimental tool. If anybody else wants to send a fix, that would be fine.

@gopherbot
Copy link

Change https://golang.org/cl/273127 mentions this issue: [dev.go2go] go/types, cmd/compile/internal/types2: a type parameter is a valid type case in a type switch

gopherbot pushed a commit that referenced this issue Nov 25, 2020
@griesemer
[dev.go2go] go/types, cmd/compile/internal/types2: a type parameter i…
a940233 
…s a valid type case in a type switch

Likewise for type assertions.

Updates #42758.

Change-Id: Iea727639f348c6997b4d3aaba420ea242e985002
Reviewed-on: https://go-review.googlesource.com/c/go/+/273127
Trust: Robert Griesemer <gri@golang.org>
Run-TryBot: Robert Griesemer <gri@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
@gopherbot
Copy link

Change https://golang.org/cl/273128 mentions this issue: [dev.typeparams] cmd/compile/internal/types2: a type parameter is a valid type case in a type switch

gopherbot pushed a commit that referenced this issue Nov 25, 2020
@griesemer
[dev.typeparams] cmd/compile/internal/types2: a type parameter is a v…
2c25cd5 
…alid type case in a type switch

Likewise for type assertions.

This is a port of https://golang.org/cl/273127 to dev.typeparams.

Updates #42758.

Change-Id: If93246371c3555e067b0043f0caefaac99101ebc
Reviewed-on: https://go-review.googlesource.com/c/go/+/273128
Trust: Robert Griesemer <gri@golang.org>
Run-TryBot: Robert Griesemer <gri@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
@griesemer
Copy link
Contributor

Just for the record: The CLs above don't address this issue - they only clean up related handling of type parameters in type switches (which are carried forward into the dev.typeparams branch).

@griesemer griesemer changed the title dev.typeparams, dev.go2go: type checker permits type switch with generic type case which can be a duplicate dev.go2go: translator generates incorrect code for type switches, leading to duplicate cases Nov 25, 2020
@tdakkota
Copy link
Author

The typeparams draft doesn't disallow type parameters in type switch cases (or type assertions for that matter). A type parameter is a type like any other, so it's not obvious that this should be prohibited.

I didn't fully understand,

var t io.Writer
switch t.(type) {
case io.Writer:
   return
case W:
    panic("W")
}

what behavior is expected when W is io.Writer?

@ianlancetaylor
Copy link
Contributor

The first case should be chosen, and the function should return.

We recently updated the design draft for this. See https://go.googlesource.com/proposal/+/refs/heads/master/design/go2draft-type-parameters.md#generic-types-as-type-switch-cases .

@mdempsky
Copy link
Member

Relatedly, naive type parameter substitution can result in duplicate constants in value switch statements and map literals: https://go2goplay.golang.org/p/fB_3R56ivQE I know cmd/go2go isn't likely to be updated to address these issues, so I'm commenting this more for posterity.

I think it may be worthwhile to keep a doc with implementation notes about cases like these to beware of when implementing generics, esp. adding generics to an existing implementation. E.g., unified IR currently has these same issues, because it works somewhat similarly to cmd/go2go (naive type substitution during the frontend, and then handing off the rewritten AST to an existing compiler backend; just it doesn't have to deal with the limitations of using Go source as an intermediate representation). I knew to expect these limitations (and that the backend would eventually need some additional features), but it could save future implementers some trouble to document them up front.

@gopherbot
Copy link

Change https://go.dev/cl/390474 mentions this issue: cmd/compile: remove duplicate const logic from typecheck

gopherbot pushed a commit that referenced this issue Mar 7, 2022
@mdempsky
cmd/compile: remove duplicate const logic from typecheck
dcb6547 
Now that we always use types2 to validate user source code, we can
remove the constSet logic from typecheck for detecting duplicate
expression switch cases and duplicate map literal keys. This logic is
redundant with types2, and currently causes unified IR to report
inappropriate duplicate constant errors that only appear after type
substitution.

Updates #42758.

Change-Id: I51ee2c5106eec9abf40eba2480dc52603c68ba21
Reviewed-on: https://go-review.googlesource.com/c/go/+/390474
Trust: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gopher Robot <gobot@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
Assignees
No one assigned
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
5 participants
@mdempsky @ianlancetaylor @griesemer @gopherbot @tdakkota