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.

Sign up for GitHub

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

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

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

zigo101 opened this issue Mar 4, 2022 · 5 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Milestone
Go1.18

Comments

@zigo101
Copy link

zigo101 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
Member

CC @griesemer @findleyr

@ianlancetaylor ianlancetaylor added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. 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 The path to resolution is known, but the work has not been done. label Mar 4, 2022
@griesemer griesemer self-assigned this Mar 4, 2022
@griesemer griesemer added release-blocker and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Mar 4, 2022
@griesemer
Copy link
Contributor

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

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
Contributor

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

@dmitshur dmitshur mentioned this issue Mar 5, 2022
Closed
33 tasks
@gopherbot
Copy link
Contributor

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
@griesemer @dmitshur
[release-branch.go1.18] go/types, types2: correctly include comparabl…
f913f9d 
…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>
@golang golang locked and limited conversation to collaborators Jun 22, 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. release-blocker
Projects
None yet
Milestone
Go1.18
Development

No branches or pull requests
4 participants
@ianlancetaylor @griesemer @gopherbot @zigo101