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.

Sign up for GitHub

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

Closed
wants to merge 7 commits into from
Closed

Resolving nil assignments in record initialization #26031

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
99f4c48
code for the resolution, but the opcall intents are still not sorted.
brandon-neth Oct 2, 2024
d6842be
simplified logic.
brandon-neth Oct 2, 2024
d530bde
test based on github issue code
brandon-neth Oct 2, 2024
bbc7c26
updated tests
brandon-neth Oct 2, 2024
538f734
fixed nullptr segfault
brandon-neth Oct 2, 2024
7f81b49
whitespace and matching lhs to rhs
brandon-neth Oct 2, 2024
0ee005a
Ben's feedback on PR
brandon-neth Oct 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions frontend/lib/resolution/InitResolver.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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

chapel/frontend/lib/resolution/InitResolver.cpp

Line 361 in 939475f

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.

Copy link
Contributor

@DanilaFe DanilaFe Oct 3, 2024 •
edited
Loading

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

auto lhsQt = reLhs.type();
auto lhsKind = lhsQt.kind();
if (lhsKind != QualifiedType::TYPE && lhsKind != QualifiedType::PARAM && isConstQualifier(lhsKind)) {
// Regardless of the field's intent, it is mutable in this expression.
lhsKind = QualifiedType::REF;
}
auto lhsType = QualifiedType(lhsKind, state->qt.type(), state->qt.param());
auto lhsType = QualifiedType(lhsKind, lhsQt.type(), lhsQt.param());
initResolver_.byPostorder.byAst(lhs).setType(lhsType);
initPoints.insert(node);

} else {
CHPL_ASSERT(0 == "Not handled yet!");
Expand Down
3 changes: 3 additions & 0 deletions frontend/lib/resolution/InitResolver.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.


static owned<InitResolver>
create(Context* context, Resolver& visitor, const uast::Function* fn);

Expand Down
6 changes: 4 additions & 2 deletions frontend/lib/resolution/call-init-deinit.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -899,11 +899,13 @@ void CallInitDeinit::handleAssign(const OpCall* ast, RV& rv) {

// check for use of deinited variables
processMentions(ast, rv);


bool isIniting = splitInited;
isIniting |= resolver.initResolver && resolver.initResolver->initPoints.count(ast) > 0;
if (lhsType.isType() || lhsType.isParam()) {
// these are basically 'move' initialization
resolveMoveInit(ast, rhsAst, lhsType, rhsType, rv);
} else if (splitInited) {
} else if (isIniting) {
processInit(frame, ast, lhsType, rhsType, rv);
} else {
// it is assignment, so resolve the '=' call
Expand Down
32 changes: 32 additions & 0 deletions frontend/test/resolution/testInitSemantics.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1627,6 +1627,37 @@ static void testInitGenericAfterConcrete() {
}
}

static void testNilFieldInit() {
std::string program = R"""(
class C { var x: int; }

record R {
var x: unmanaged C?;
proc init() {
x = nil;
}
}

var x: R;
)""";
Context ctx;
Context* context = &ctx;
ErrorGuard guard(context);
ResolutionContext rcval(context);
auto rc = &rcval;
auto m = parseModule(context, std::move(program));
auto recordDecl = m->stmt(1)->toAggregateDecl();
auto initFunc = recordDecl->declOrComment(1)->toFunction();
assert(initFunc);

auto untyped = UntypedFnSignature::get(context, initFunc);
auto typed = typedSignatureInitial(rc, untyped);
auto inFn = resolveFunction(rc, typed, nullptr);
assert(inFn);

assert(guard.errors().size() == 0);
};

// TODO:
// - test using defaults for types and params
// - also in conditionals
Expand Down Expand Up @@ -1673,6 +1704,7 @@ int main() {

testInitGenericAfterConcrete();

testNilFieldInit();
return 0;
}