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: restore nested.go test case #54512

Closed
mdempsky opened this issue Aug 18, 2022 · 5 comments
Closed

cmd/compile: restore nested.go test case #54512

mdempsky opened this issue Aug 18, 2022 · 5 comments
Assignees
@mdempsky
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.20

Comments

@mdempsky
Copy link
Contributor

In CL 421821, I've commented out part of the test in nested.go, because it's incredibly contrived and runs into an awkward reentrancy issue with unified IR and shape types. I expect the issue will go away once #46731 is implemented, and unified IR updated to take advantage of it. This is a reminder issue to make sure that happens before Go 1.20.
@mdempsky mdempsky added the NeedsFix The path to resolution is known, but the work has not been done. label Aug 18, 2022
@mdempsky mdempsky added this to the Go1.20 milestone Aug 18, 2022
@mdempsky mdempsky self-assigned this Aug 18, 2022
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Aug 18, 2022
@mdempsky
Copy link
Contributor Author

Confirmed that changing this line:

go/src/cmd/compile/internal/noder/writer.go

Line 793 in 6dc7b06

w.typExpr(decl.Type)

to w.typ(named.Underlying()) fixes nested.go. (And does not affect the export data format.)

We can make that change as soon as the compiler no longer needs to support:

//go:notinheap
type A /* some type */

type B A // expected to also be NIH

(We don't need to have necessarily removed //go:notinheap entirely yet, just decided that we're not going to support that problematic use case anymore; in particular, removed any test cases that expect that.)

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/424854 mentions this issue: cmd/compile: restore test/nested.go test cases

@mdempsky
Copy link
Contributor Author

We reverted the fix.

@mdempsky mdempsky reopened this Aug 29, 2022
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/426335 mentions this issue: cmd/compile: fix unified IR shapifying recursive instantiated types

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/455279 mentions this issue: cmd/compile: restore test/nested.go test cases

gopherbot pushed a commit that referenced this issue Dec 7, 2022
@mdempsky
cmd/compile: restore test/nested.go test cases
dddc1ba 
[Re-land of CL 424854, which was reverted as CL 425214.]

When handling a type declaration like:

```
type B A
```

unified IR has been writing out that B's underlying type is A, rather
than the underlying type of A.

This is a bit awkward to implement and adds complexity to importers,
who need to handle resolving the underlying type themselves. But it
was necessary to handle when A was declared like:

```
//go:notinheap
type A int
```

Because we expected A's not-in-heap'ness to be conferred to B, which
required knowing that A was on the path from B to its actual
underlying type int.

However, since #46731 was accepted, we no longer need to support this
case. Instead we can write out B's actual underlying type.

One stumbling point though is the existing code for exporting
interfaces doesn't work for the underlying type of `comparable`, which
is now needed to implement `type C comparable`. As a bit of a hack, we
we instead export its underlying type as `interface{ comparable }`.

Fixes #54512.

Change-Id: I9aa087e0a277527003195ebc7f4fbba6922e788c
Reviewed-on: https://go-review.googlesource.com/c/go/+/455279
Run-TryBot: Matthew Dempsky <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Reviewed-by: Robert Griesemer <[email protected]>
@golang golang locked and limited conversation to collaborators Dec 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@mdempsky mdempsky

Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.20
Development

No branches or pull requests
2 participants
@mdempsky @gopherbot