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

Resolving nil assignments in record initialization #26031

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

brandon-neth
Copy link
Contributor

Resolves https://github.com/Cray/chapel-private/issues/6660.

Adds support for resolving field initialization where the field is initialized to nil, as in

class C { var x : int; }

record R {
  var x : unmanaged C?;

  proc init() {
    x = nil;
  }
}

var r : R;

This 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 an initPoints field to the InitResolver that tracks the initialization points of fields. Then, in call-init-deinit, assignments are handled as initializations if the assignment is one of the initialization points.

---
Signed-off-by: Brandon Neth <[email protected]>
---
Signed-off-by: Brandon Neth <[email protected]>
---
Signed-off-by: Brandon Neth <[email protected]>
---
Signed-off-by: Brandon Neth <[email protected]>
---
Signed-off-by: Brandon Neth <[email protected]>
@brandon-neth brandon-neth marked this pull request as ready for review October 2, 2024 22:20
@benharsh benharsh self-requested a review October 2, 2024 22:25
@@ -129,6 +129,9 @@ class InitResolver {

public:

//initialization points to guide handling `=` operators
std::set<const uast::AstNode*> initPoints;
Copy link
Member

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());
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

@DanilaFe DanilaFe Oct 3, 2024

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.

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.

Copy link
Contributor

@DanilaFe DanilaFe Oct 3, 2024

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(...))
  }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants