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: miscompilation of comparison between type parameter and interface #51522

Closed
mdempsky opened this issue Mar 7, 2022 · 17 comments
Closed
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Milestone

Comments

@mdempsky
Copy link
Member

mdempsky commented Mar 7, 2022

The program below should run quietly, but instead it prints:

FAIL: if i != t
FAIL: switch i
FAIL: switch t

Marking as a release blocker, because this seems like a miscompilation bug that users could plausibly actually run into. But I think it might be reasonable to punt fixing it to a point release.

A possible workaround here is to replace each use of t with any(t).

package main

func f[T comparable](i any) {
	var t T

	if i != t {
		println("FAIL: if i != t")
	}

	switch i {
	case t:
		// ok
	default:
		println("FAIL: switch i")
	}

	switch t {
	case i:
		// ok
	default:
		println("FAIL: switch t")
	}
}

func main() {
	f[int](int(0))
}

/cc @griesemer @ianlancetaylor @randall77

@mdempsky mdempsky added NeedsFix The path to resolution is known, but the work has not been done. release-blocker labels Mar 7, 2022
@mdempsky mdempsky added this to the Go1.18 milestone Mar 7, 2022
@mdempsky
Copy link
Member Author

mdempsky commented Mar 7, 2022

More comprehensive test case, that also exercises non-empty interfaces:

package main

type I interface { M() }

type myint int
func (myint) M() {}

func f[T comparable, U interface{ comparable; I }](a any, i I) {
        var t T
        var u U

        if a != t {
                println("FAIL: if a != t")
        }
        switch a {
        case t:
        default:
                println("FAIL: switch a, case t")
        }
        switch t {
        case a:
        default:
                println("FAIL: switch t, case a")
        }

        if a != u {
                println("FAIL: if a != u")
        }
        switch a {
        case u:
        default:
                println("FAIL: switch a, case u")
        }
        switch u {
        case a:
        default:
                println("FAIL: switch u, case a")
        }

        if i != u {
                println("FAIL: if i != u")
        }
        switch i {
        case u:
        default:
                println("FAIL: switch i, case u")
        }
        switch u {
        case i:
        default:
                println("FAIL: switch u, case i")
        }
}

func main() {
        f[myint, myint](myint(0), myint(0))
}

@ianlancetaylor
Copy link
Contributor

CC @randall77

@findleyr
Copy link
Contributor

findleyr commented Mar 8, 2022

It seems plausible that this could lead to subtle bugs in real code, for example:
https://go.dev/play/p/MQ1qi27IjUJ?v=gotip

@aclements
Copy link
Member

Do we understand the underlying issue here? If so, do we have a plan to fix it?

@eliben
Copy link
Member

eliben commented Mar 9, 2022

Do we understand the underlying issue here? If so, do we have a plan to fix it?

We chatted about this on Monday and decided to flag this as an error in the compiler for 1.18, with a hope for a more fundamental fix in 1.19

@jeremyfaller
Copy link
Contributor

From a conversation with @eliben, it looks like he's counting on @mdempsky or @randall77 to take a look. From Eli – "We decided to flag it as an error because we don't have a good undestanding of how to compare interface values with generic values at this time"

Should be done by release time.

@gopherbot
Copy link

Change https://go.dev/cl/391315 mentions this issue: go/types, types2: disallow interface vs type parameter comparison

@griesemer
Copy link
Contributor

griesemer commented Mar 10, 2022

Acceptance of https://go.dev/cl/391315 will change this from a release blocker to a bug for Go 1.19.

A better temp. solution might be to report this issue in the compiler proper; this CL is just here as a last resort.

@ianlancetaylor
Copy link
Contributor

I sent https://go.dev/cl/391374 that fixes some of the cases and give an error for a case that is harder to handle.

I'll let @randall77 and @mdempsky decide which approach to take.

@gopherbot
Copy link

Change https://go.dev/cl/391374 mentions this issue: cmd/compile: use correct type when comparing generic to interface

@randall77
Copy link
Contributor

Sorry, I've been away for a few days.
The problem here, at least for the != test, is that there's an implicit OCONVIFACE that the stenciler needs to see in order to do the comparison properly. I think we actually fixed that at one point, in CL 336992 perhaps? Not sure if there was a regression since. Looks like that CL didn't have a test, unfortunately. I'll look some more.

@gopherbot
Copy link

Change https://go.dev/cl/391475 mentions this issue: cmd/compile: fix transform of OEQ/ONE when one arg is a type param

@gopherbot
Copy link

Change https://go.dev/cl/391594 mentions this issue: cmd/compile: fix expression switches using type parameters

gopherbot pushed a commit that referenced this issue Mar 10, 2022
At this point in stenciling, we have shape types, not raw type parameters.
The code was correct in the other part of this function.

Update #51522

Change-Id: Ife495160a2be5f6af5400363c3efb68dda518b5f
Reviewed-on: https://go-review.googlesource.com/c/go/+/391475
Trust: Keith Randall <khr@golang.org>
Run-TryBot: Keith Randall <khr@golang.org>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@randall77
Copy link
Contributor

Reopening for backport to 1.18 release branch. There are 2 CLs, 391475 and 391594.

@gopherbot
Copy link

Change https://go.dev/cl/391794 mentions this issue: [release-branch.go1.18] cmd/compile: fix transform of OEQ/ONE when one arg is a type param

@gopherbot
Copy link

Change https://go.dev/cl/391795 mentions this issue: [release-branch.go1.18] cmd/compile: fix expression switches using type parameters

gopherbot pushed a commit that referenced this issue Mar 14, 2022
…e arg is a type param

At this point in stenciling, we have shape types, not raw type parameters.
The code was correct in the other part of this function.

Update #51522

Change-Id: Ife495160a2be5f6af5400363c3efb68dda518b5f
Reviewed-on: https://go-review.googlesource.com/c/go/+/391475
Trust: Keith Randall <khr@golang.org>
Run-TryBot: Keith Randall <khr@golang.org>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
(cherry picked from commit 8cf1169)
Reviewed-on: https://go-review.googlesource.com/c/go/+/391794
Trust: Dmitri Shuralyov <dmitshur@golang.org>
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
gopherbot pushed a commit that referenced this issue Mar 14, 2022
…pe parameters

Both the thing we're switching on, as well as the cases we're switching for.
Convert anything containing a type parameter to interface{} before the
comparison happens.

Fixes #51522

Change-Id: I97ba9429ed332cb7d4240cb60f46d42226dcfa5f
Reviewed-on: https://go-review.googlesource.com/c/go/+/391594
Trust: Keith Randall <khr@golang.org>
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
(cherry picked from commit 2e46a0a)
Reviewed-on: https://go-review.googlesource.com/c/go/+/391795
Trust: Dmitri Shuralyov <dmitshur@golang.org>
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
@dmitshur
Copy link
Contributor

Closed by merging commit 1edc1cc (CL 391794) and commit 2c6a889 (CL 391795) to release-branch.go1.18.

@golang golang locked and limited conversation to collaborators Mar 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Projects
None yet
Development

No branches or pull requests

10 participants