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: embedding comparable is not considered in calculating type sets #51472

Closed
go101 opened this issue Mar 4, 2022 · 5 comments
Closed

cmd/compile: embedding comparable is not considered in calculating type sets #51472

go101 opened this issue Mar 4, 2022 · 5 comments
Labels
NeedsFix release-blocker
Milestone

Comments

@go101
Copy link

@go101 go101 commented Mar 4, 2022

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

$ go version
go version go1.18rc1 linux/amd64

What did you do?

package main

func main() {
	foo("Abc")
	foo([]byte{}) // compiles okay, but should not
}

type C interface {
	comparable
	[]byte | string
}
	
func foo[T C](x T) {}

What did you expect to see?

Fails to compile.

What did you see instead?

Compiles okay.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Mar 4, 2022

CC @griesemer @findleyr

@ianlancetaylor ianlancetaylor added the NeedsInvestigation label Mar 4, 2022
@ianlancetaylor ianlancetaylor added this to the Go1.18 milestone Mar 4, 2022
@ianlancetaylor ianlancetaylor changed the title cmd/compile: it looks embedding comprable is not considered in calculating tyep sets cmd/compile: it looks embedding comparable is not considered in calculating type sets Mar 4, 2022
@ianlancetaylor ianlancetaylor changed the title cmd/compile: it looks embedding comparable is not considered in calculating type sets cmd/compile: embedding comparable is not considered in calculating type sets Mar 4, 2022
@griesemer griesemer added the NeedsFix label Mar 4, 2022
@griesemer griesemer self-assigned this Mar 4, 2022
@griesemer griesemer added release-blocker and removed NeedsInvestigation labels Mar 4, 2022
@griesemer
Copy link
Contributor

@griesemer griesemer commented Mar 4, 2022

Briefly looked at this. It appears we're forgetting to consider the "comparable" field in typesets under some circumstances. Appears like a trivial oversight, as the actual type set computations are ok. Will fix tomorrow.

@griesemer
Copy link
Contributor

@griesemer griesemer commented Mar 4, 2022

Quick update: fix is pending but given that we're so short before the release, I'll spend some time adding more tests; this shouldn't have slipped. CL probably later today or tomorrow.

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 4, 2022

Change https://go.dev/cl/390025 mentions this issue: types2: correctly include comparable in type set intersection

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 7, 2022

Change https://go.dev/cl/390419 mentions this issue: [release-branch.go1.18] go/types, types2: correctly include comparable in type set intersection

gopherbot pushed a commit that referenced this issue Mar 8, 2022
…e in type set intersection

The comparable bit was handled incorrectly. This CL establishes
a clear invariant for a type set's terms and its comparable bit
and correctly uses the bit when computing term intersections.

Relevant changes:

- Introduce a new function intersectTermLists that does the
  correct intersection computation.

Minor:

- Moved the comparable bit after terms in _TypeSet to make it
  clearer that they belong together.

- Simplify and clarify _TypeSet.IsAll predicate.

- Remove the IsTypeSet predicate which was only used for error
  reporting in union.go, and use the existing predicates instead.

- Rename/introduce local variables in computeInterfaceTypeSet
  for consistency and to avoid confusion.

- Update some tests whose output has changed because the comparable
  bit is now only set if we have have the set of all types.
  For instance, for interface{comparable; int} the type set doesn't
  set the comparable bit because the intersection of comparable and
  int is just int; etc.

- Add many more comments to make the code clearer.

Fixes #51472.

Change-Id: I8a5661eb1693a41a17ce5f70d7e10774301f38ab
Reviewed-on: https://go-review.googlesource.com/c/go/+/390025
Trust: Robert Griesemer <gri@golang.org>
Run-TryBot: Robert Griesemer <gri@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
(cherry picked from commit 7dc6c5e)
Reviewed-on: https://go-review.googlesource.com/c/go/+/390419
Trust: Dmitri Shuralyov <dmitshur@golang.org>
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsFix release-blocker
Projects
None yet
Development

No branches or pull requests

4 participants