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

mdempsky · 2021-06-16T20:56:06Z

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

The text was updated successfully, but these errors were encountered:

mdempsky · 2021-06-16T21:41:57Z

noder/writer.go code:

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

Lines 235 to 248 in dd95a4e

    
           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:

go/src/cmd/compile/internal/types2/typestring.go

Lines 362 to 379 in 1ba2074

    
           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 · 2021-09-22T16:22:16Z

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

griesemer · 2021-10-28T03:33:10Z

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

toothrot · 2021-12-08T17:13:02Z

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

findleyr · 2021-12-08T17:17:03Z

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.

findleyr · 2022-01-13T15:25:00Z

Will address this by next week.

griesemer · 2022-01-20T17:39:19Z

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 · 2022-02-09T17:17:32Z

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.

gopherbot · 2022-03-16T14:38:33Z

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 · 2022-03-16T20:16:57Z

[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 · 2022-03-16T20:34:27Z

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 · 2022-03-16T20:45:47Z

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

gopherbot · 2022-03-16T20:56:21Z

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

griesemer · 2022-03-16T21:16:07Z

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

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>

ianlancetaylor · 2022-06-24T19:12:12Z

@griesemer This issue is in the 1.19 milestone. Should it be 1.20? Backlog? Thanks.

griesemer · 2022-06-24T19:22:55Z

@findleyr is this still relevant?

findleyr · 2022-07-07T20:04:42Z

I don't think this is relevant anymore, but defer to mdempsky to confirm that this no longer repros. Bumping to 1.20, since that's when we plan to land unified IR.

gopherbot · 2022-08-12T14:51:20Z

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

mdempsky · 2022-08-12T17:10:16Z

I think this has been fixed too. In unified IR I added extra asserts to make sure it's idempotent and they're passing. I'll reopen if I run into any issues again. Thanks!

mdempsky added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker labels Jun 16, 2021

mdempsky added this to the Go1.18 milestone Jun 16, 2021

findleyr assigned griesemer Oct 7, 2021

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 assigned findleyr Nov 4, 2021

griesemer added the okay-after-beta1 Used by release team to mark a release-blocker issue as okay to resolve either before or after beta1 label Nov 10, 2021

findleyr unassigned griesemer Nov 18, 2021

cherrymui removed the okay-after-beta1 Used by release team to mark a release-blocker issue as okay to resolve either before or after beta1 label Dec 14, 2021

griesemer assigned griesemer and unassigned findleyr Jan 19, 2022

griesemer removed the release-blocker label Jan 20, 2022

thanm mentioned this issue Jan 25, 2022

Go compiler and runtime meeting notes #43930

Open

griesemer assigned mdempsky and unassigned griesemer Feb 9, 2022

griesemer added the early-in-cycle A change that should be done early in the 3 month dev cycle. label Feb 9, 2022

griesemer modified the milestones: Go1.18, Go1.19 Feb 9, 2022

findleyr modified the milestones: Go1.19, Go1.20 Jul 7, 2022

mdempsky closed this as completed Aug 12, 2022

golang locked and limited conversation to collaborators Aug 12, 2023

gopherbot added the FrozenDueToAge label Aug 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

mdempsky commented Jun 16, 2021

mdempsky commented Jun 16, 2021

toothrot commented Sep 22, 2021

griesemer commented Oct 28, 2021

toothrot commented Dec 8, 2021

findleyr commented Dec 8, 2021

findleyr commented Jan 13, 2022

griesemer commented Jan 20, 2022

griesemer commented Feb 9, 2022

gopherbot commented Mar 16, 2022

griesemer commented Mar 16, 2022 •

edited

findleyr commented Mar 16, 2022

griesemer commented Mar 16, 2022

gopherbot commented Mar 16, 2022

griesemer commented Mar 16, 2022

ianlancetaylor commented Jun 24, 2022

griesemer commented Jun 24, 2022

findleyr commented Jul 7, 2022

gopherbot commented Aug 12, 2022

mdempsky commented Aug 12, 2022

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

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

Comments

mdempsky commented Jun 16, 2021

mdempsky commented Jun 16, 2021

toothrot commented Sep 22, 2021

griesemer commented Oct 28, 2021

toothrot commented Dec 8, 2021

findleyr commented Dec 8, 2021

findleyr commented Jan 13, 2022

griesemer commented Jan 20, 2022

griesemer commented Feb 9, 2022

gopherbot commented Mar 16, 2022

griesemer commented Mar 16, 2022 • edited

findleyr commented Mar 16, 2022

griesemer commented Mar 16, 2022

gopherbot commented Mar 16, 2022

griesemer commented Mar 16, 2022

ianlancetaylor commented Jun 24, 2022

griesemer commented Jun 24, 2022

findleyr commented Jul 7, 2022

gopherbot commented Aug 12, 2022

mdempsky commented Aug 12, 2022

griesemer commented Mar 16, 2022 •

edited