[go] go/types, types2: clean up Named assertions and establish RHS invariant
Mark Freeman (Gerrit)
Mark Freeman voted and added 1 comment![]( "Open in Gerrit")
Votes added by Mark Freeman
| Commit-Queue | +1 |
1 comment
Mark FreemanIs this ready for review? Happy to review all or part of it--let me know what would be most helpful.
Pardon my delay — this is ready for review. Please note the breakage of x/tools is due to a compiler bug that an x/tools test relies on.
Related details
- Robert Findley
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Freeman (Gerrit)
Mark Freeman added 1 comment![]( "Open in Gerrit")
Mark FreemanIs this ready for review? Happy to review all or part of it--let me know what would be most helpful.
Pardon my delay — this is ready for review. Please note the breakage of x/tools is due to a compiler bug that an x/tools test relies on.
Review of the entire CL would be appreciated.
Related details
- Robert Findley
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Freeman (Gerrit)
Mark Freeman voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Robert Findley
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Freeman (Gerrit)
Mark Freeman voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Robert Findley
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Robert Findley (Gerrit)
Robert Findley added 4 comments![]( "Open in Gerrit")
type has been deduced. One can check if the underlying is a
named type, but if the RHS was already a type literal, it's
still ambiguous.Don't understand this. My mental model is that the underlying is either forwarded, or not. So if it's not Named, then we've already run the `under` algorithm. What am I missing?
n.SetUnderlying(underlying)This calls resolve(). I can't reason about the consequences of that... is this intentional? I always think of resolve() as something that happens lazily, and yet this is eager. Won't the underlying already have been set in newNamed?
At the very least, warrants a big comment explaining why we're setting the underlying twice.
assert(n.loader == nil) // cannot import an instantiationYou certainly can import an instantiation, so this comment seems misleading.
n.resolve().under()This resolve is unnecessary. In general, we should only be calling resolve right before accessing the resolved information. In this case `Named.Underlying` calls resolve implicitly.
Related details
- Mark Freeman
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Freeman (Gerrit)
Mark Freeman added 2 comments![]( "Open in Gerrit")
n.SetUnderlying(underlying)This calls resolve(). I can't reason about the consequences of that... is this intentional? I always think of resolve() as something that happens lazily, and yet this is eager. Won't the underlying already have been set in newNamed?
At the very least, warrants a big comment explaining why we're setting the underlying twice.
The `newNamed` never sets the underlying type. It only establishes the RHS, which we just set immediately to the underlying, as if it were declared in source.
Calling `resolve()` is intentional — this CL enforces that only types which are resolved can have underlying types. It orders those operations.
n.resolve().under()This resolve is unnecessary. In general, we should only be calling resolve right before accessing the resolved information. In this case `Named.Underlying` calls resolve implicitly.
Calling `under` does not resolve the type for the caller. With this change, it asserts that the type has been resolved prior to being called.
In this way, the expectations of `under` are explicit and surfaces an "out of phase" error.
Related details
- Robert Findley
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Robert Findley (Gerrit)
Robert Findley added 1 comment![]( "Open in Gerrit")
Ack, clearly I missed the point in a superficial scan of this CL. It will take me several hours to review in earnest, which I likely won't have for some time; perhaps next week. Sorry.
Related details
- Mark Freeman
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Freeman (Gerrit)
Mark Freeman added 2 comments![]( "Open in Gerrit")
Ack, clearly I missed the point in a superficial scan of this CL. It will take me several hours to review in earnest, which I likely won't have for some time; perhaps next week. Sorry.
No worries, thanks for taking a look.
assert(n.loader == nil) // cannot import an instantiationYou certainly can import an instantiation, so this comment seems misleading.
I assume this is resolved via discussion on go.dev/cl/697697.
Related details
- Robert Findley
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Robert Griesemer (Gerrit)
Robert Griesemer added 6 comments![]( "Open in Gerrit")
Just some initial comments on the CL description.
More to come.
The RHS chain is a key feature for traversal of aliases; it refers toClearer:
The RHS chain used by Named types is a key feature...
type of a. At some point during type checking, a walks the chain of"a walks" is hard to read
If you use capital letters for types, this becomes clearer.
1. The underlying field may not refer to the underlying type,
depending on when it is observed.Whether the underlying field refers to the underlying type or not depends on when the field is accessed.
Operations derived from the underlying share similar caveats. For
example, the expandUnderlying() method, which substitutes type
arguments, returns an instantiated named type whose underlying
also may or may not be its underlying type.I think this could benefit from more precise prose. The problem is caused by the word "underlying" which is an adjective, a field name, and a specific type, depending on context. "underlying field" is slightly better, but also misleading for somebody not very familiar with the code. Since this is a longer exposition, it could make sense to introduce some terminology at the top for clarity. For instance, U-field for the field named underlying, U-type, for the underlying type, or something like this (better suggestions welcome).
The basic idea (if I understand correctly) is to use the RHS field consistently by Named and Alias types, and the use the underlying and actual fields to represent the "resolved" state. I'd start with this at the top, rather than with the problem description.
same purpose as alias types. The underlying is not set until its/as/as for/ ?
Related details
- Mark Freeman
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Freeman (Gerrit)
Mark Freeman uploaded new patchset![]( "Open in Gerrit")
Related details
- Mark Freeman
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Freeman (Gerrit)
Mark Freeman added 6 comments![]( "Open in Gerrit")
The RHS chain is a key feature for traversal of aliases; it refers toClearer:
The RHS chain used by Named types is a key feature...
I began with Aliases to then relate it to the underlying field for Named, showing that these two concepts are similar in function.
Do we want to jump straight to Named types at the start?
type of a. At some point during type checking, a walks the chain of"a walks" is hard to read
If you use capital letters for types, this becomes clearer.
Done
1. The underlying field may not refer to the underlying type,
depending on when it is observed.Whether the underlying field refers to the underlying type or not depends on when the field is accessed.
Done
type has been deduced. One can check if the underlying is a
named type, but if the RHS was already a type literal, it's
still ambiguous.Don't understand this. My mental model is that the underlying is either forwarded, or not. So if it's not Named, then we've already run the `under` algorithm. What am I missing?
Suppose `type t int`. Without running `under`, the `underlying` field of `t` points to `int`.
So the type not being `Named` does not imply `under` was run.
Operations derived from the underlying share similar caveats. For
example, the expandUnderlying() method, which substitutes type
arguments, returns an instantiated named type whose underlying
also may or may not be its underlying type.I think this could benefit from more precise prose. The problem is caused by the word "underlying" which is an adjective, a field name, and a specific type, depending on context. "underlying field" is slightly better, but also misleading for somebody not very familiar with the code. Since this is a longer exposition, it could make sense to introduce some terminology at the top for clarity. For instance, U-field for the field named underlying, U-type, for the underlying type, or something like this (better suggestions welcome).
The basic idea (if I understand correctly) is to use the RHS field consistently by Named and Alias types, and the use the underlying and actual fields to represent the "resolved" state. I'd start with this at the top, rather than with the problem description.
That's the correct idea. Working on the prose now.
same purpose as alias types. The underlying is not set until itMark Freemans/as/as for/ ?
Done
Related details
- Robert Findley
- Robert Griesemer
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Robert Griesemer (Gerrit)
Robert Griesemer added 1 comment![]( "Open in Gerrit")
The RHS chain is a key feature for traversal of aliases; it refers toMark FreemanClearer:
The RHS chain used by Named types is a key feature...
I began with Aliases to then relate it to the underlying field for Named, showing that these two concepts are similar in function.
Do we want to jump straight to Named types at the start?
I think it may be better to explain the goal: use of fromRHS consistently to express the RHS as we encounter it in the syntax tree, and the use underlying/actual once we found those respective types.
Maybe something like:
A type definition or alias declaration consists of a type name (LHS) which is bound to a type expression (RHS) by the declaration.
This CL consistently uses the fromRHS fields of Named and Alias types to represent that RHS type expression, and sets Named.underlying and Alias.actual only once those types have been computed.
Currently, Named types used their underlying field for some of this functionality, which made it difficult to understand.
Specifically, Named.underlying is not set until we have computed the underlying type; otherwise it is nil, providing a clear signal that we do not (yet) know that type.
(As an aside, I never liked 'fromRHS', I wish we had a better name. But that's something we can easily change down the road.)
Related details
- Mark Freeman
- Robert Findley
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Robert Griesemer (Gerrit)
Robert Griesemer added 9 comments![]( "Open in Gerrit")
Some initial comment. Need to study named.go in more detail.
// tags applied to named types in contexts where invariants can be applied only conditionallyThese are not tags, they are (boolean) flags:
// flags indicating temporary violations of the invariants for fromRHS and underlying
allowNilRHS boolline comment explaining when this is possible?
n.SetUnderlying(underlying)should we assert here that underlying is not a Named type?
// resolve resolves the type parameters, methods, and RHS of n.Perhaps the right name for the field is _RHS (for another CL).
t.fromRHS = underlyingIs this necessary? What does it mean if underlying is not the real RHS?
func setDefType(def *TypeName, typ Type) {Maybe this should become setRHS? (Separate CL ok)
y = ny.resolve().under()isn't under() calling resolve?
(ditto elsewhere)
Mark Freeman (Gerrit)
Mark Freeman uploaded new patchset![]( "Open in Gerrit")
- Mark Freeman
- Robert Griesemer
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Freeman (Gerrit)
Mark Freeman added 10 comments![]( "Open in Gerrit")
The RHS chain is a key feature for traversal of aliases; it refers toMark FreemanClearer:
The RHS chain used by Named types is a key feature...
Robert GriesemerI began with Aliases to then relate it to the underlying field for Named, showing that these two concepts are similar in function.
Do we want to jump straight to Named types at the start?
I think it may be better to explain the goal: use of fromRHS consistently to express the RHS as we encounter it in the syntax tree, and the use underlying/actual once we found those respective types.
Maybe something like:
A type definition or alias declaration consists of a type name (LHS) which is bound to a type expression (RHS) by the declaration.
This CL consistently uses the fromRHS fields of Named and Alias types to represent that RHS type expression, and sets Named.underlying and Alias.actual only once those types have been computed.
Currently, Named types used their underlying field for some of this functionality, which made it difficult to understand.Specifically, Named.underlying is not set until we have computed the underlying type; otherwise it is nil, providing a clear signal that we do not (yet) know that type.
(As an aside, I never liked 'fromRHS', I wish we had a better name. But that's something we can easily change down the road.)
Incorporated much of the suggestions here
Operations derived from the underlying share similar caveats. For
example, the expandUnderlying() method, which substitutes type
arguments, returns an instantiated named type whose underlying
also may or may not be its underlying type.Mark FreemanI think this could benefit from more precise prose. The problem is caused by the word "underlying" which is an adjective, a field name, and a specific type, depending on context. "underlying field" is slightly better, but also misleading for somebody not very familiar with the code. Since this is a longer exposition, it could make sense to introduce some terminology at the top for clarity. For instance, U-field for the field named underlying, U-type, for the underlying type, or something like this (better suggestions welcome).
The basic idea (if I understand correctly) is to use the RHS field consistently by Named and Alias types, and the use the underlying and actual fields to represent the "resolved" state. I'd start with this at the top, rather than with the problem description.
That's the correct idea. Working on the prose now.
Used Named.underlying to make it a bit clearer when we are referring to the field and the term underlying type to make it clear when we are talking about the type.
// tags applied to named types in contexts where invariants can be applied only conditionallyThese are not tags, they are (boolean) flags:
// flags indicating temporary violations of the invariants for fromRHS and underlying
Done
line comment explaining when this is possible?
Done
Mark FreemanThis calls resolve(). I can't reason about the consequences of that... is this intentional? I always think of resolve() as something that happens lazily, and yet this is eager. Won't the underlying already have been set in newNamed?
At the very least, warrants a big comment explaining why we're setting the underlying twice.
The `newNamed` never sets the underlying type. It only establishes the RHS, which we just set immediately to the underlying, as if it were declared in source.
Calling `resolve()` is intentional — this CL enforces that only types which are resolved can have underlying types. It orders those operations.
Acknowledged
n.SetUnderlying(underlying)should we assert here that underlying is not a Named type?
FWIW, `SetUnderlying` already makes this assertion, unless you're suggesting to duplicate this assertion?
Mark FreemanThis resolve is unnecessary. In general, we should only be calling resolve right before accessing the resolved information. In this case `Named.Underlying` calls resolve implicitly.
Calling `under` does not resolve the type for the caller. With this change, it asserts that the type has been resolved prior to being called.
In this way, the expectations of `under` are explicit and surfaces an "out of phase" error.
Acknowledged
t.fromRHS = underlyingIs this necessary? What does it mean if underlying is not the real RHS?
IIRC, this `SetUnderlying` should ideally only be called for cases where we know the underlying type already, such as for importers and things in the universe.
In that case, this just makes it follow the same flow as type checking from source — make a trivial RHS, resolve it, and set the underlying.
y = ny.resolve().under()isn't under() calling resolve?
(ditto elsewhere)
The function `under` indeed calls `resolve`. The method `Named.under` does not.
This CL changed the implementation of the method — it begins by asserting that the input Named type has been resolved. The thought was to surface this ordering explicitly to the caller, rather than implicitly handling it in under.
It's harder to do this for the function, since it's a) used in many more places and b) in contexts where we do not know if it's a named.
Related details
- Robert Findley
- Robert Griesemer
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Robert Griesemer (Gerrit)
Robert Griesemer added 34 comments![]( "Open in Gerrit")
Some more comments.
Overall looks pretty reasonable, but I have added some questions.
functionality, which made it difficult to understand. Operations whichs/made it/makes the code/
used Named.underlying for this functionality now use Named.fromRHS.s/for this functionality//
type has been deduced. One can check if Named.underlying is
a named type, but if the RHS was already a type literal, it's
still ambiguous.I don't understand this 2nd sentence here: if the RHS is a type literal, then underlying would be that type literal eventually. And it is nil before. At least for a type definition.
With this change, presence of Named.underlying becomes the canonicalClearer (w/o resorting to under() which you haven't mentioned in this CL desc yet):
With this change, Named.underlying is nil as long as it is unknown, and non-nil and not a named type once it is known.
1. A named type cannot populate Named.underlying until it has been1. Named.underlying is not set until Named has been resolved.
type to already be resolved. In that case, it's best to assert it.s/In that case, it's best to assert it.//
is exploited by a test (issue61678.go) in x/tools.So what is the action Here?
// The RHS of a named can be nil if there's a cycle of non-reified aliases. There is likely aThis is the first use of reify in this code base. It's not defined what it means. Also this comment applies to the next line.
Can you add a simple example to clarify this?
named.allowNilRHS = trueuse a defer here - this ensures correct behavior (invariants) in case one of the functions below panics (e.g. early exit after first error). It may not matter in this case but it's good code hygiene.
named.allowNilRHS = true
defer func() { named.allowNilRHS = false } ()
named.allowNilRHS = falseremove this in favor of defer above
unresolved namedState = iota // type parameters, RHS, and methods might be unavailable... RHS, underlying, and methods ...
(?) or, depending on my comment below (line 473), maybe just:
... underlying, and methods ...
(?)
n.SetUnderlying(underlying)Mark Freemanshould we assert here that underlying is not a Named type?
FWIW, `SetUnderlying` already makes this assertion, unless you're suggesting to duplicate this assertion?
ACK. Marked as resolved.
if n.state() > unresolved { // avoid locking belown.state() >= resolved
makes it clearer that n is already resolved (as is, we need to check if the next state after unresolved is indeed resolve)
This is also the code you use in line 556
assert(n.underlying == nil) // underlying comes after resolvingI don't understand this: aren't we past resolving (we set the state to complete in the next instruction)
t.fromRHS = underlyingMark FreemanIs this necessary? What does it mean if underlying is not the real RHS?
IIRC, this `SetUnderlying` should ideally only be called for cases where we know the underlying type already, such as for importers and things in the universe.
In that case, this just makes it follow the same flow as type checking from source — make a trivial RHS, resolve it, and set the underlying.
ACK. Since we now have a clear separation between RHS and underlying, couldn't we keep RHS nil if it doesn't actually exist in the source? That is, for imported types and things in the universe?
(I see that this would required changes to expndRHS below. Just trying to understand this code.)
// It does so by following type chains. If a type literal is found, each... by following RHS type chains. ...
// are skipped because their underlying type is not memoized.I don't understand this last sentence. Alias types don't have an underlying field.
var path []Objectdeclare path after seen, as it is used after seen in typical use below
for {var u Type
for u != nil {
...
and then you can remove some of the break's below and you don't need a loop label
switch curr := t.(type) {s/t/rhs/ (plus appropriate decl above)
s/curr/t/
break loopjust break (no need for loop)
break loop // any type literal worksno need for break
}Immediately after the loop, as a reminder you could add:
// u != nil
assert(u != nil)no need for this assertion, it's guaranteed by the loop termination condition
u = n.Underlying()nice to get the call to Underlying out of this loop!
// its origin type (which is a generic type).s/is/must be/ (?)
// type t[p any] struct {please use capital letters for type names and type parameters
assert(orig.state() > unresolved)`>= resolved`
return ifacewe can fall through here, can't we?
y = ny.resolve().under()Mark Freemanisn't under() calling resolve?
(ditto elsewhere)
The function `under` indeed calls `resolve`. The method `Named.under` does not.
This CL changed the implementation of the method — it begins by asserting that the input Named type has been resolved. The thought was to surface this ordering explicitly to the caller, rather than implicitly handling it in under.
It's harder to do this for the function, since it's a) used in many more places and b) in contexts where we do not know if it's a named.
Acknowledged
typ.fromRHS = itypAgain, I wonder if we can avoid this since there's no real RHS
Related details
- Mark Freeman
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Freeman (Gerrit)
Mark Freeman uploaded new patchset![]( "Open in Gerrit")
Related details
- Mark Freeman
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Freeman (Gerrit)
Mark Freeman added 10 comments![]( "Open in Gerrit")
type has been deduced. One can check if Named.underlying is
a named type, but if the RHS was already a type literal, it's
still ambiguous.I don't understand this 2nd sentence here: if the RHS is a type literal, then underlying would be that type literal eventually. And it is nil before. At least for a type definition.
This refers to Named.underlying as it was previously used — not with this change. As in, suppose `type B int`, even without calling `under`, `Named.underlying` would already be `int`.
is exploited by a test (issue61678.go) in x/tools.So what is the action Here?
No action, we dropped the test for 1.26 via go.dev/cl/700395.
if n.state() > unresolved { // avoid locking belown.state() >= resolved
makes it clearer that n is already resolved (as is, we need to check if the next state after unresolved is indeed resolve)
This is also the code you use in line 556
Done
assert(n.underlying == nil) // underlying comes after resolvingI don't understand this: aren't we past resolving (we set the state to complete in the next instruction)
I think complete is mostly orthogonal to N.underlying. We are still in N.resolve and so we cannot have called N.under.
t.fromRHS = underlyingMark FreemanIs this necessary? What does it mean if underlying is not the real RHS?
Robert GriesemerIIRC, this `SetUnderlying` should ideally only be called for cases where we know the underlying type already, such as for importers and things in the universe.
In that case, this just makes it follow the same flow as type checking from source — make a trivial RHS, resolve it, and set the underlying.
ACK. Since we now have a clear separation between RHS and underlying, couldn't we keep RHS nil if it doesn't actually exist in the source? That is, for imported types and things in the universe?
(I see that this would required changes to expndRHS below. Just trying to understand this code.)
We likely could for the cases you mentioned.
It just seemed simpler to me to fold one case into the other to reduce the cases that the rest of the code must consider. For example, for imported types, it seemed natural to consider the underlying type from export data as the RHS and thus get the invariant that RHS cannot be nil (outside the cases where we allow it).
// It does so by following type chains. If a type literal is found, each... by following RHS type chains. ...
Done
// are skipped because their underlying type is not memoized.I don't understand this last sentence. Alias types don't have an underlying field.
Correct, though they still need to be traversed and they are not set when we find our first type literal. We can only do that because they walk to the nearest `Named` type when their `Underlying` is called.
for {var u Type
for u != nil {
...
and then you can remove some of the break's below and you don't need a loop label
If `var u Type`, how do we enter the loop?
u = n.Underlying()nice to get the call to Underlying out of this loop!
- Robert Griesemer
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Freeman (Gerrit)
Mark Freeman uploaded new patchset![]( "Open in Gerrit")
Related details
- Robert Griesemer
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Freeman (Gerrit)
Mark Freeman added 6 comments![]( "Open in Gerrit")
functionality, which made it difficult to understand. Operations whichMark Freemans/made it/makes the code/
Done
used Named.underlying for this functionality now use Named.fromRHS.Mark Freemans/for this functionality//
Done
With this change, presence of Named.underlying becomes the canonicalClearer (w/o resorting to under() which you haven't mentioned in this CL desc yet):
With this change, Named.underlying is nil as long as it is unknown, and non-nil and not a named type once it is known.
Done
1. A named type cannot populate Named.underlying until it has been1. Named.underlying is not set until Named has been resolved.
Done
type to already be resolved. In that case, it's best to assert it.s/In that case, it's best to assert it.//
Done
assert(orig.state() > unresolved)Mark Freeman`>= resolved`
Done
Related details
- Robert Griesemer
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Robert Griesemer (Gerrit)
Robert Griesemer added 1 comment![]( "Open in Gerrit")
type has been deduced. One can check if Named.underlying is
a named type, but if the RHS was already a type literal, it's
still ambiguous.Mark FreemanI don't understand this 2nd sentence here: if the RHS is a type literal, then underlying would be that type literal eventually. And it is nil before. At least for a type definition.
This refers to Named.underlying as it was previously used — not with this change. As in, suppose `type B int`, even without calling `under`, `Named.underlying` would already be `int`.
I see. I was confused because `int` in this case is not a type literal. It is a named, albeit predeclared, type.
Related details
- Mark Freeman
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Robert Griesemer (Gerrit)
Robert Griesemer added 1 comment![]( "Open in Gerrit")
for {Mark Freemanvar u Type
for u != nil {
...
and then you can remove some of the break's below and you don't need a loop label
If `var u Type`, how do we enter the loop?
I meant
`for u == nil ...`
of course. Sorry.
Related details
- Mark Freeman
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Freeman (Gerrit)
Mark Freeman uploaded new patchset![]( "Open in Gerrit")
Related details
- Mark Freeman
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Freeman (Gerrit)
Mark Freeman added 13 comments![]( "Open in Gerrit")
Another batch of responses, just working on the remaining comments.
use a defer here - this ensures correct behavior (invariants) in case one of the functions below panics (e.g. early exit after first error). It may not matter in this case but it's good code hygiene.
named.allowNilRHS = true
defer func() { named.allowNilRHS = false } ()
Done
remove this in favor of defer above
Done
declare path after seen, as it is used after seen in typical use below
Done
for {Mark Freemanvar u Type
for u != nil {
...
and then you can remove some of the break's below and you don't need a loop label
Robert GriesemerIf `var u Type`, how do we enter the loop?
I meant
`for u == nil ...`
of course. Sorry.
Done
s/t/rhs/ (plus appropriate decl above)
s/curr/t/
Done
don't need the break
Done
just break (no need for loop)
Done
break loop // any type literal worksMark Freemanno need for break
Done
Immediately after the loop, as a reminder you could add:
// u != nil
Acknowledged, feel the loop condition is obvious enough
no need for this assertion, it's guaranteed by the loop termination condition
Done
// its origin type (which is a generic type).Mark Freemans/is/must be/ (?)
Done
please use capital letters for type names and type parameters
- Robert Griesemer
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Freeman (Gerrit)
Mark Freeman uploaded new patchset![]( "Open in Gerrit")
Related details
- Robert Griesemer
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Robert Griesemer (Gerrit)
Robert Griesemer added 5 comments![]( "Open in Gerrit")
Thanks.
I will give this another thorough review, hopefully tomorrow.
t.fromRHS = underlyingMark FreemanIs this necessary? What does it mean if underlying is not the real RHS?
Robert GriesemerIIRC, this `SetUnderlying` should ideally only be called for cases where we know the underlying type already, such as for importers and things in the universe.
In that case, this just makes it follow the same flow as type checking from source — make a trivial RHS, resolve it, and set the underlying.
Mark FreemanACK. Since we now have a clear separation between RHS and underlying, couldn't we keep RHS nil if it doesn't actually exist in the source? That is, for imported types and things in the universe?
(I see that this would required changes to expndRHS below. Just trying to understand this code.)
We likely could for the cases you mentioned.
It just seemed simpler to me to fold one case into the other to reduce the cases that the rest of the code must consider. For example, for imported types, it seemed natural to consider the underlying type from export data as the RHS and thus get the invariant that RHS cannot be nil (outside the cases where we allow it).
Acknowledged.
func (n *Named) under() Type {Re: our conversation today:
If we were following the RHS chain in a separate phase after the resolver and before we start type-checking objects, we could effectively move the work from this function out into that phase, I think.
I wonder if this would simplify some of the surrounding code that depends on calling this function?
(Perhaps for another CL.)
rhs := Type(n)nitpick: I think
```
var rhs Type = n
```
is clearer because there's no need to think about what Type(n) does.
nitpick: if you don't have an empty line after each case, I would not introduce empty lines within a case as it implies some kinds of break in the switch that doesn't exist
Related details
- Mark Freeman
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Freeman (Gerrit)
Mark Freeman added 5 comments![]( "Open in Gerrit")
type has been deduced. One can check if Named.underlying is
a named type, but if the RHS was already a type literal, it's
still ambiguous.Mark FreemanI don't understand this 2nd sentence here: if the RHS is a type literal, then underlying would be that type literal eventually. And it is nil before. At least for a type definition.
Robert GriesemerThis refers to Named.underlying as it was previously used — not with this change. As in, suppose `type B int`, even without calling `under`, `Named.underlying` would already be `int`.
I see. I was confused because `int` in this case is not a type literal. It is a named, albeit predeclared, type.
Correct, pardon the inaccuracy in wording — adjusted.
unresolved namedState = iota // type parameters, RHS, and methods might be unavailable... RHS, underlying, and methods ...
(?) or, depending on my comment below (line 473), maybe just:
... underlying, and methods ...
(?)
I believe the first is more correct. Unresolved instance and loaded types also do not have an RHS until `Named.resolve`.
rhs := Type(n)nitpick: I think
```
var rhs Type = n
```
is clearer because there's no need to think about what Type(n) does.
Done
nitpick: if you don't have an empty line after each case, I would not introduce empty lines within a case as it implies some kinds of break in the switch that doesn't exist
Done
typ.fromRHS = itypAgain, I wonder if we can avoid this since there's no real RHS
We might be able to do that via something similar to `Named.inst` and `Named.loader`, which also describe how the details should be filled in.
Perhaps a `Named.predeclared` to hold information for predeclared types until `resolve`?
Related details
- Robert Griesemer
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Freeman (Gerrit)
Mark Freeman uploaded new patchset![]( "Open in Gerrit")
Related details
- Robert Griesemer
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Freeman (Gerrit)
Mark Freeman added 1 comment![]( "Open in Gerrit")
// The RHS of a named can be nil if there's a cycle of non-reified aliases. There is likely aThis is the first use of reify in this code base. It's not defined what it means. Also this comment applies to the next line.
Can you add a simple example to clarify this?
Reworded, shifted, and added example
Related details
- Robert Griesemer
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Freeman (Gerrit)
Mark Freeman added 1 comment![]( "Open in Gerrit")
return ifacewe can fall through here, can't we?
I think we can only fall through if we say `rhs = iface`, but that seems mostly the same as returning here. No strong preference.
Related details
- Robert Griesemer
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Freeman (Gerrit)
Mark Freeman uploaded new patchset![]( "Open in Gerrit")
Related details
- Robert Griesemer
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Robert Griesemer (Gerrit)
Robert Griesemer voted and added 14 comments![]( "Open in Gerrit")
Votes added by Robert Griesemer
| Code-Review | +2 |
14 comments
LGTM with the notes per discussion. Thanks!
u, _ := named.resolve().under().(*Interface)just: named.Underlying().(*Interface) ?
unresolved namedState = iota // type parameters, RHS, and methods might be unavailable... RHS, underlying, and methods ...
(?) or, depending on my comment below (line 473), maybe just:
... underlying, and methods ...
(?)
Done
assert(n.underlying == nil) // underlying comes after resolvingI don't understand this: aren't we past resolving (we set the state to complete in the next instruction)
I think complete is mostly orthogonal to N.underlying. We are still in N.resolve and so we cannot have called N.under.
Acknowledged
rhs := Type(n)nitpick: I think
```
var rhs Type = n
```
is clearer because there's no need to think about what Type(n) does.
Done
nitpick: if you don't have an empty line after each case, I would not introduce empty lines within a case as it implies some kinds of break in the switch that doesn't exist
Done
return ifacewe can fall through here, can't we?
Done
switch u := T.resolve().under().(type) {T.Underlying()
if t == asNamed(universeComparable.Type()).resolve().under() {I think this is not needed for this CL
if t := asNamed(t); t != nil {
return t.resolve().under()
}get rid of the if fowr now - remove under() in a 2nd CL
typ.fromRHS = itypAgain, I wonder if we can avoid this since there's no real RHS
In another CL we may want to keep fromRHS nil if there's no actual source RHS.
Related details
- Robert Griesemer
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Robert Griesemer (Gerrit)
Robert Griesemer voted and added 6 comments![]( "Open in Gerrit")
Votes added by Robert Griesemer
| Code-Review | +2 |
6 comments
Please also make the following adjustments before submitting. Thanks!
go/types, types2: change and enforce lifecycle of named typesmaybe:
change and enforce lifecycle of Names.fromRHS and Named.underlying fields
(you don't really change the lifecycle of Named types)
While this approach works, it introduces some caveats:s/some caveats/problems/
Operations derived from Named.underlying share similar caveats. Fors/share similar caveats/have similar problems/
is exploited by a test (issue61678.go) in x/tools.Mark FreemanSo what is the action Here?
No action, we dropped the test for 1.26 via go.dev/cl/700395.
Done
package pPlease add the usual copyright notice here.
Related details
- Mark Freeman
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |