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

go/types, cmd/compile/internal/types2: ensure that Named.Orig is indempotent #46794

Open
mdempsky opened this issue Jun 16, 2021 · 14 comments
Open

go/types, cmd/compile/internal/types2: ensure that Named.Orig is indempotent #46794

mdempsky opened this issue Jun 16, 2021 · 14 comments
Assignees
Labels
early-in-cycle NeedsInvestigation
Milestone

Comments

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Jun 16, 2021

In both unified IR and types2, an operation that seems useful is to take an instantiated Named and decompose it into the (original) TypeName and type arguments.

Currently there's a Named.Orig method, but it's not always the case that Named.Orig is idempotent. E.g., there are at least test cases where it's necessary to apply it twice.

This is a placeholder issue to look into this further before Go 1.18.

/cc @griesemer @findleyr

@mdempsky mdempsky added NeedsInvestigation release-blocker labels Jun 16, 2021
@mdempsky mdempsky added this to the Go1.18 milestone Jun 16, 2021
@mdempsky
Copy link
Member Author

@mdempsky mdempsky commented Jun 16, 2021

noder/writer.go code:

case *types2.Named:
// Type aliases can refer to uninstantiated generic types, so we
// might see len(TParams) != 0 && len(TArgs) == 0 here.
// TODO(mdempsky): Revisit after #46477 is resolved.
assert(len(typ.TParams()) == len(typ.TArgs()) || len(typ.TArgs()) == 0)
// TODO(mdempsky): Why do we need to loop here?
orig := typ
for orig.TArgs() != nil {
orig = orig.Orig()
}
w.code(typeNamed)
w.obj(orig.Obj(), typ.TArgs())

types2/typestring.go code:

if instanceHashing != 0 {
// For local defined types, use the (original!) TypeName's position
// to disambiguate. This is overkill, and could probably instead
// just be the pointer value (if we assume a non-moving GC) or
// a unique ID (like cmd/compile uses). But this works for now,
// and is convenient for debugging.
// TODO(mdempsky): I still don't fully understand why typ.orig.orig
// can differ from typ.orig, or whether looping more than twice is
// ever necessary.
typ := obj.typ.(*Named)
for typ.orig != typ {
typ = typ.orig
}
if orig := typ.obj; orig.pkg != nil && orig.parent != orig.pkg.scope {
fmt.Fprintf(buf, "@%q", orig.pos)
}
}

@toothrot
Copy link
Contributor

@toothrot toothrot commented Sep 22, 2021

Checking in on this issue as it's labeled a release blocker for Go 1.18. Is there any update?

@griesemer griesemer changed the title go/types, cmd/compile/internal/types2: provide unique ID to identify instantiations of the same original type go/types, cmd/compile/internal/types2: ensure that Named.Orig is indempotent Oct 28, 2021
@griesemer
Copy link
Contributor

@griesemer griesemer commented Oct 28, 2021

Leaving as release blocker as this might hide more serious issues.

@griesemer griesemer added the okay-after-beta1 label Nov 10, 2021
@toothrot
Copy link
Contributor

@toothrot toothrot commented Dec 8, 2021

Checking in on this as a release-blocking issue. Are there any updates?

@findleyr
Copy link
Contributor

@findleyr findleyr commented Dec 8, 2021

No updates, other than it's near the top of my queue? This got push backed by not-okay-after-beta1 release blockers.

We suspect there isn't a bug here any more, but I need to prove it.

@cherrymui cherrymui removed the okay-after-beta1 label Dec 14, 2021
@findleyr
Copy link
Contributor

@findleyr findleyr commented Jan 13, 2022

Will address this by next week.

@griesemer griesemer assigned griesemer and unassigned findleyr Jan 19, 2022
@griesemer
Copy link
Contributor

@griesemer griesemer commented Jan 20, 2022

Removing this from release-blockers. Still something we need to investigate but we have no evidence that this is causing us troubles at the moment.

@griesemer
Copy link
Contributor

@griesemer griesemer commented Feb 9, 2022

The loop

for typ.orig != typ { 
	typ = typ.orig 
}

doesn't exist anymore in types2 and go/types, and the unified build (where the other loop is) is not enabled yet for 1.18.

It's not clear that this is still a problem. Should investigate early in 1.19 development by trying to identify code for which the loop in the unified noder triggers.

@griesemer griesemer assigned mdempsky and unassigned griesemer Feb 9, 2022
@griesemer griesemer added the early-in-cycle label Feb 9, 2022
@griesemer griesemer removed this from the Go1.18 milestone Feb 9, 2022
@griesemer griesemer added this to the Go1.19 milestone Feb 9, 2022
@gopherbot
Copy link

@gopherbot gopherbot commented Mar 16, 2022

This issue is currently labeled as early-in-cycle for Go 1.19.
That time is now, so a friendly reminder to look at it again.

@griesemer
Copy link
Contributor

@griesemer griesemer commented Mar 16, 2022

[edit: this turned out to be a separate issue - see https://go.dev/cl/393436]

Possibly related reproducer (extracted from https://storage.googleapis.com/go-build-log/a42378ee/linux-amd64-unified_c6889d3a.log):

  • use current tip (go version devel go1.19-81d3c25c6c)
  • in cmd/compile/internal/types2/check.go, set the debug flag to true
  • in $GOROOT/src, run: GOEXPERIMENT=unified ./make.bash

The build crashes with a panic:

<unknown line number>: internal compiler error: panic: validType0(nil)

The root cause is that t.orig.fromRHS is nil in some cases (cmd/compile/internal/types2/validtype.go:74); yet fromRHS should always be set.

@findleyr
Copy link
Contributor

@findleyr findleyr commented Mar 16, 2022

The root cause is that t.orig.fromRHS is nil in some cases (cmd/compile/internal/types2/validtype.go:74); yet fromRHS should always be set.

This seems like a separate bug, right? Am I missing some way in which that implies that Origin is not idempotent?

@griesemer
Copy link
Contributor

@griesemer griesemer commented Mar 16, 2022

It may be a separate bug, or it may be related in some cases. I don't know yet for sure.

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 16, 2022

Change https://go.dev/cl/393368 mentions this issue: go/types, types2: add an assertion that named type origin is idempotent

@griesemer
Copy link
Contributor

@griesemer griesemer commented Mar 16, 2022

It turns out that this is a separate issue (https://go.dev/cl/393436).

gopherbot pushed a commit that referenced this issue Mar 21, 2022
For #46794

Change-Id: I19edc19640a2dfa6bc7504dd8e1742a261ba29f1
Reviewed-on: https://go-review.googlesource.com/c/go/+/393368
Trust: Robert Findley <rfindley@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
Reviewed-by: Robert Griesemer <gri@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
early-in-cycle NeedsInvestigation
Projects
None yet
Development

No branches or pull requests

6 participants