-
Notifications
You must be signed in to change notification settings - Fork 49
ssa: make recursive type conversion order-independent #2280
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,15 +28,27 @@ import ( | |
| // ----------------------------------------------------------------------------- | ||
|
|
||
| type goTypes struct { | ||
| typs map[unsafe.Pointer]unsafe.Pointer | ||
| typbg sync.Map | ||
| // typs and cvtneed are owned by the single lowering goroutine for one | ||
| // Program. typbg is populated during concurrent package syntax preloading, | ||
| // before lowering starts, so it remains a sync.Map. | ||
| typs map[unsafe.Pointer]unsafe.Pointer | ||
| cvtneed map[*types.Named]conversionRequirement | ||
| typbg sync.Map | ||
| } | ||
|
|
||
| func newGoTypes() goTypes { | ||
| typs := make(map[unsafe.Pointer]unsafe.Pointer) | ||
| return goTypes{typs: typs} | ||
| return goTypes{typs: typs, cvtneed: make(map[*types.Named]conversionRequirement)} | ||
| } | ||
|
|
||
| type conversionRequirement uint8 | ||
|
|
||
| const ( | ||
| conversionUnknown conversionRequirement = iota | ||
| conversionNotNeeded | ||
| conversionNeeded | ||
| ) | ||
|
|
||
| type Background int | ||
|
|
||
| const ( | ||
|
|
@@ -101,7 +113,7 @@ func (p goTypes) cvtType(typ types.Type) (raw types.Type, cvt bool) { | |
| } | ||
| return p.cvtStruct(t) | ||
| case *types.Named: | ||
| if v, ok := p.typbg.Load(namedLinkname(t)); ok && v.(Background) == InC { | ||
| if !p.shouldConvertNamed(t) { | ||
| break | ||
| } | ||
| return p.cvtNamed(t) | ||
|
|
@@ -161,37 +173,169 @@ func namedLinkname(t *types.Named) string { | |
| return obj.Name() | ||
| } | ||
|
|
||
| func (p goTypes) shouldConvertNamed(t *types.Named) bool { | ||
| v, ok := p.typbg.Load(namedLinkname(t)) | ||
| return !ok || v.(Background) != InC | ||
| } | ||
|
|
||
| func (p goTypes) cvtNamed(t *types.Named) (raw *types.Named, cvt bool) { | ||
| if v, ok := p.typs[unsafe.Pointer(t)]; ok { | ||
| raw = (*types.Named)(v) | ||
| cvt = t != raw | ||
| return | ||
| } | ||
| // Decide whether the complete recursive type graph needs conversion before | ||
| // installing the recursion placeholder. Previously the placeholder was the | ||
| // original type. For mutually recursive named types, that made the result | ||
| // depend on which member of the cycle happened to be converted first: a | ||
| // closure reachable through a later member could leave an earlier member | ||
| // permanently cached in its unconverted form. | ||
| if !p.namedNeedsTypeConversion(t) { | ||
| p.typs[unsafe.Pointer(t)] = unsafe.Pointer(t) | ||
| return t, false | ||
| } | ||
| n := t.NumMethods() | ||
| methods := make([]*types.Func, n) | ||
| for i := 0; i < n; i++ { | ||
| m := t.Method(i) // don't need to convert method signature | ||
| methods[i] = m | ||
| } | ||
| named := types.NewNamed(t.Obj(), types.Typ[types.Int], methods) | ||
| origin := types.NewNamed(t.Obj(), types.Typ[types.Int], methods) | ||
| if tp := t.TypeParams(); tp != nil { | ||
| list := make([]*types.TypeParam, tp.Len()) | ||
| for i := 0; i < tp.Len(); i++ { | ||
| param := tp.At(i) | ||
| list[i] = types.NewTypeParam(param.Obj(), param.Constraint()) | ||
| } | ||
| named.SetTypeParams(list) | ||
| origin.SetTypeParams(list) | ||
| } | ||
| p.typs[unsafe.Pointer(t)] = unsafe.Pointer(t) | ||
| if tund, cvt := p.cvtType(t.Underlying()); cvt { | ||
| named.SetUnderlying(tund) | ||
| if typ, ok := Instantiate(named, t); ok { | ||
| named = typ.(*types.Named) | ||
| named := origin | ||
| if typ, ok := Instantiate(origin, t); ok { | ||
| named = typ.(*types.Named) | ||
| } | ||
| // Publish the converted placeholder before descending so every back-edge in | ||
| // the cycle observes the same conversion decision. | ||
| p.typs[unsafe.Pointer(t)] = unsafe.Pointer(named) | ||
| tund, _ := p.cvtType(t.Underlying()) | ||
| // Generic instances derive their underlying type lazily from the origin. | ||
| // Fill the origin before any caller observes named.Underlying(), so a | ||
| // recursive My[T] back-edge resolves to the converted My[args] instance. | ||
| origin.SetUnderlying(tund) | ||
| return named, true | ||
| } | ||
|
|
||
| type conversionNeedState struct { | ||
| visiting bool | ||
| seen bool | ||
| } | ||
|
|
||
| type conversionNeedQuery map[*types.Named]conversionNeedState | ||
|
|
||
| func (p goTypes) namedNeedsTypeConversion(t *types.Named) bool { | ||
| if requirement := p.cvtneed[t]; requirement != conversionUnknown { | ||
| return requirement == conversionNeeded | ||
| } | ||
| query := make(conversionNeedQuery) | ||
| needed := p.needsTypeConversion(t, query) | ||
| if !needed { | ||
| // A complete negative query proves that every named type it reached is | ||
| // also conversion-free. Negative results observed only on a cycle | ||
| // back-edge are never stored here. | ||
| for named, state := range query { | ||
| if state.seen { | ||
| p.cvtneed[named] = conversionNotNeeded | ||
| } | ||
| } | ||
| p.typs[unsafe.Pointer(t)] = unsafe.Pointer(named) | ||
| return named, true | ||
| } | ||
| return t, false | ||
| return needed | ||
| } | ||
|
|
||
| // needsTypeConversion reports whether cvtType changes any part of typ. The | ||
| // recursion set deliberately belongs to one query: a cycle back-edge alone is | ||
| // not a conversion, but another member of that cycle may still require one. | ||
| // Keep its traversal and conversion predicates in lock-step with cvtType. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maintenance hazard:
Neither case panics, so drift would be hard to catch. Consider a shared traversal or a test that asserts |
||
| func (p goTypes) needsTypeConversion(typ types.Type, query conversionNeedQuery) bool { | ||
| if _, ok := cvtGoSSAOpaqueType(typ); ok { | ||
| return true | ||
| } | ||
| switch t := typ.(type) { | ||
| case *types.Basic: | ||
| return false | ||
| case *types.Pointer: | ||
| return p.needsTypeConversion(t.Elem(), query) | ||
| case *types.Interface: | ||
| for i := 0; i < t.NumExplicitMethods(); i++ { | ||
| sig := t.ExplicitMethod(i).Type().(*types.Signature) | ||
| if p.needsTypeConversion(sig.Params(), query) || p.needsTypeConversion(sig.Results(), query) { | ||
| return true | ||
| } | ||
| } | ||
| for i := 0; i < t.NumEmbeddeds(); i++ { | ||
| if p.needsTypeConversion(t.EmbeddedType(i), query) { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| case *types.Slice: | ||
| return p.needsTypeConversion(t.Elem(), query) | ||
| case *types.Map: | ||
| return p.needsTypeConversion(t.Key(), query) || p.needsTypeConversion(t.Elem(), query) | ||
| case *types.Struct: | ||
| if IsClosure(t) { | ||
| return false | ||
| } | ||
| for i := 0; i < t.NumFields(); i++ { | ||
| if p.needsTypeConversion(t.Field(i).Type(), query) { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| case *types.Named: | ||
| if !p.shouldConvertNamed(t) { | ||
| return false | ||
| } | ||
| if requirement := p.cvtneed[t]; requirement != conversionUnknown { | ||
| return requirement == conversionNeeded | ||
| } | ||
| state := query[t] | ||
| state.seen = true | ||
| if state.visiting { | ||
| query[t] = state | ||
| return false | ||
| } | ||
| state.visiting = true | ||
| query[t] = state | ||
| ret := p.needsTypeConversion(t.Underlying(), query) | ||
| state = query[t] | ||
| state.visiting = false | ||
| query[t] = state | ||
| if ret { | ||
| p.cvtneed[t] = conversionNeeded | ||
| } | ||
| return ret | ||
| case *types.Signature: | ||
| return true | ||
| case *types.Array: | ||
| return p.needsTypeConversion(t.Elem(), query) | ||
| case *types.Chan: | ||
| return p.needsTypeConversion(t.Elem(), query) | ||
| case *types.Tuple: | ||
| for i := 0; i < t.Len(); i++ { | ||
| if p.needsTypeConversion(t.At(i).Type(), query) { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| case *types.TypeParam: | ||
| return false | ||
| case *types.Alias: | ||
| return p.needsTypeConversion(types.Unalias(t), query) | ||
| case *types.Union: | ||
| // cvtUnion currently always creates a raw union. | ||
| return true | ||
| default: | ||
| panic(fmt.Sprintf("needsTypeConversion: unexpected type - %T", typ)) | ||
| } | ||
| } | ||
|
|
||
| func Instantiate(orig types.Type, t *types.Named) (types.Type, bool) { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
originvsnamedsplit here is load-bearing and non-obvious: for a generic instantiation,named = Instantiate(origin, t), andSetUnderlyingis called onorigin, notnamed. This works becausego/typesmaterializes the instance's underlying lazily by substituting type args into the origin's underlying — so setting the origin's underlying beforenamed.Underlying()is first forced reflects into the returned instance. Since this is exactly whatTestRecursiveGenericNamedTypeConversionguards, a one-line comment explaining whySetUnderlyingtargetsorigin(and that it must precede anynamed.Underlying()call) would help future readers.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in ef145c2: added a comment documenting lazy generic-instance underlying materialization and why SetUnderlying must target origin before named.Underlying is observed.