-
Notifications
You must be signed in to change notification settings - Fork 424
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
Resolving nil assignments in record initialization #26031
Conversation
--- Signed-off-by: Brandon Neth <brandon.neth@hpe.com>
--- Signed-off-by: Brandon Neth <brandon.neth@hpe.com>
--- Signed-off-by: Brandon Neth <brandon.neth@hpe.com>
--- Signed-off-by: Brandon Neth <brandon.neth@hpe.com>
--- Signed-off-by: Brandon Neth <brandon.neth@hpe.com>
--- Signed-off-by: Brandon Neth <brandon.neth@hpe.com>
@@ -129,6 +129,9 @@ class InitResolver { | |||
|
|||
public: | |||
|
|||
//initialization points to guide handling `=` operators | |||
std::set<const uast::AstNode*> initPoints; |
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.
I think it would be better to add a method on InitResolver that returns true if a given AstNode is field initialization.
@@ -769,13 +769,16 @@ bool InitResolver::handleAssignmentToField(const OpCall* node) { | |||
// field doesn't contribute to the receiver type. | |||
updateResolverVisibleReceiverType(); | |||
|
|||
auto lhsKind = state->qt.kind(); | |||
if (lhsKind != QualifiedType::TYPE && lhsKind != QualifiedType::PARAM) { | |||
auto& reLhs = initResolver_.byPostorder.byId(lhs->id()); |
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.
Notably (see above) the state qt
is still determined from reRhs
type, which I don't think is right. Later on in the process (see computeReceiverTypeConsideringState
, the QT of the state is used for constructing instantiations explicitly. I don't think this is right, since in this particular case, this will lead to using null
(the type) as the field type.
Does call-init-deinit re-run field resolution with results of init=
? If not, I think this will lead to incorrect instantiations.
To see this, try with a generic class field.
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.
I don't think call-init-deinit reruns field resolution; it looks like it assumes the field types have been resolved already.
Could you elaborate on why a generic class field would expose this behavior? I think there's holes in my understanding of the process for generics here.
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.
Well, there's a bunch of logic that computes the field types / field substitutions for a generic type when it's being constructed. E.g., so that:
proc init() {
self.myTypeField = int;
}
Leads to a type where myTypeField
has an int
substitution. This is only necessary for generic fields (because concrete fields don't need instantiations). This is why I'm telling you to try with a generic field (and it needs to be a class field so that nil
can be legally assigned to it).
if (!isFieldSyntacticallyGeneric(ctx_, id)) continue; |
When inserting the substitutions, it looks like the type of state->qt
is used directly.
chapel/frontend/lib/resolution/InitResolver.cpp
Lines 364 to 365 in 939475f
if (isValidQtForSubstitutions(state->qt)) { | |
subs.insert({id, state->qt}); |
This means that, since your PR doesn't change state->qt
from its original (RHS) value, a generic field being instantiated using nil
will get the type of nil
. I don't think that is what you want.
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 thing I'm concretely telling you to try is something like the following:
class C {
type typeField; // C is now generic
}
record R {
var myC: owned C(?)?;
proc init() {
this.myC = nil; // here, state->qt for myC is NullType, so you'll get R(NullType) instead of R(C(...))
}
}
--- Signed-off-by: Brandon Neth <brandon.neth@hpe.com>
This has been replaced by #26219 |
Resolves https://github.com/Cray/chapel-private/issues/6660.
Adds support for resolving field initialization where the field is initialized to
nil
, as inThis involved 2 changes. First, changing the
InitResolver
to use the declared type of an LHS for the LHS rather than the type of the RHS expression. Second, adding aninitPoints
field to theInitResolver
that tracks the initialization points of fields. Then, incall-init-deinit
, assignments are handled as initializations if the assignment is one of the initialization points.