Skip to content

Commit

Permalink
Fix dyno ambiguity issue between fields and secondary methods with wr…
Browse files Browse the repository at this point in the history 
…ong receiver (#25231)

Fix a dyno resolver bug, in which secondary methods on a different
receiver are considered candidates for resolving identifiers in methods,
causing ambiguity.

The fix is implemented by attempting method resolution with an implicit
`this.` before the identifier if we've found apparent ambiguity, since
method resolution already knows how to filter based on receiver type.

Also adds testing for the fixed cases.

Resolves Cray/chapel-private#6284.

Note to reviewer: The commit history on this one is a bit of a mess as I
went a few directions, so it's better to review the changes as a whole.

[reviewed by @mppf , thanks!]

Future work:
- #25376

Testing:
- [x] dyno tests
- [x] reproducer from original issue now resolves properly
  • Loading branch information
@riftEmber
riftEmber authored Jun 27, 2024
2 parents 770870b + dc83fe6 commit 953181f
Show file tree
Hide file tree
Showing 2 changed files with 188 additions and 37 deletions.
99 changes: 62 additions & 37 deletions frontend/lib/resolution/Resolver.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2658,10 +2658,9 @@ void Resolver::issueAmbiguityErrorIfNeeded(const Identifier* ident,
}
}

std::vector<BorrowedIdsWithName>
Resolver::lookupIdentifier(const Identifier* ident,
llvm::ArrayRef<const Scope*> receiverScopes,
ParenlessOverloadInfo& outParenlessOverloadInfo) {
std::vector<BorrowedIdsWithName> Resolver::lookupIdentifier(
const Identifier* ident, llvm::ArrayRef<const Scope*> receiverScopes,
ParenlessOverloadInfo& outParenlessOverloadInfo) {
CHPL_ASSERT(scopeStack.size() > 0);
const Scope* scope = scopeStack.back();
outParenlessOverloadInfo = ParenlessOverloadInfo();
Expand All @@ -2670,7 +2669,7 @@ Resolver::lookupIdentifier(const Identifier* ident,
// Super is a keyword, and should't be looked up in scopes. Return
// an empty ID to indicate that this identifier points to something,
// but that something has a special meaning.
return { BorrowedIdsWithName::createWithBuiltinId() };
return {BorrowedIdsWithName::createWithBuiltinId()};
}

bool resolvingCalledIdent = nearestCalledExpression() == ident;
Expand All @@ -2679,11 +2678,7 @@ Resolver::lookupIdentifier(const Identifier* ident,
if (!resolvingCalledIdent) config |= LOOKUP_INNERMOST;

auto vec = lookupNameInScopeWithWarnings(context, scope, receiverScopes,
ident->name(), config,
ident->id());

bool notFound = vec.empty();
bool ambiguous = !notFound && (vec.size() > 1 || vec[0].numIds() > 1);
ident->name(), config, ident->id());

if (!vec.empty()) {
// We might be ambiguous, but due to having found multiple parenless procs.
Expand All @@ -2692,30 +2687,10 @@ Resolver::lookupIdentifier(const Identifier* ident,
// one identifier is not a parenless proc, there's an ambiguity.
//
// outParenlessOverloadInfo will be falsey if we found non-parenless-proc
// IDs, in which case we should emit an ambiguity error.
outParenlessOverloadInfo = ParenlessOverloadInfo::fromBorrowedIds(context, vec);
}

// TODO: these errors should be enabled for scope resolution
// but for now, they are off, as a temporary measure to enable
// the production compiler handle these cases. To enable this,
// we will have to adjust the dyno scope resolver to handle 'domain',
// and probably a few other features.
if (!scopeResolveOnly) {
if (notFound) {
if (!resolvingCalledIdent) {
auto pair = namesWithErrorsEmitted.insert(ident->name());
if (pair.second) {
// insertion took place so emit the error
bool mentionedMoreThanOnce = identHasMoreMentions(ident);
CHPL_REPORT(context, UnknownIdentifier, ident, mentionedMoreThanOnce);
}
}
} else if (ambiguous &&
!outParenlessOverloadInfo.areCandidatesOnlyParenlessProcs() &&
!resolvingCalledIdent) {
issueAmbiguityErrorIfNeeded(ident, scope, receiverScopes, config);
}
// IDs. In that case we may emit an ambiguity error later, after filtering
// out incorrect receivers.
outParenlessOverloadInfo =
ParenlessOverloadInfo::fromBorrowedIds(context, vec);
}

return vec;
Expand Down Expand Up @@ -2952,18 +2927,68 @@ void Resolver::resolveIdentifier(const Identifier* ident,
auto parenlessInfo = ParenlessOverloadInfo();
auto vec = lookupIdentifier(ident, receiverScopes, parenlessInfo);

bool resolvingCalledIdent = nearestCalledExpression() == ident;
// TODO: these errors should be enabled for scope resolution
// but for now, they are off, as a temporary measure to enable
// the production compiler handle these cases. To enable this,
// we will have to adjust the dyno scope resolver to handle 'domain',
// and probably a few other features.
bool emitLookupErrors = !resolvingCalledIdent && !scopeResolveOnly;

if (parenlessInfo.areCandidatesOnlyParenlessProcs() && !scopeResolveOnly) {
// Ambiguous, but a special case: there are many parenless functions.
// This might be fine, if their 'where' clauses leave only one.
//
// Call resolution will issue an error if the overload selection fails.
tryResolveParenlessCall(parenlessInfo, ident, receiverScopes);
} else if (vec.size() == 0) {
if (emitLookupErrors) {
auto pair = namesWithErrorsEmitted.insert(ident->name());
if (pair.second) {
// insertion took place so emit the error
bool mentionedMoreThanOnce = identHasMoreMentions(ident);
CHPL_REPORT(context, UnknownIdentifier, ident, mentionedMoreThanOnce);
}
}

result.setType(QualifiedType());
} else if (vec.size() > 1 || vec[0].numIds() > 1) {
// can't establish the type. If this is in a function
// call, we'll establish it later anyway.
result.setType(QualifiedType());
// Ambiguous. Check if we can break ambiguity by performing function resolution with
// an implicit 'this' receiver, to filter based on receiver type.
QualifiedType receiverType;
ID receiverId;
if (getMethodReceiver(&receiverType, &receiverId) && receiverType.type()) {
std::vector<CallInfoActual> actuals;
actuals.push_back(CallInfoActual(receiverType, USTR("this")));
auto ci = CallInfo(/* name */ ident->name(),
/* calledType */ QualifiedType(),
/* isMethodCall */ true,
/* hasQuestionArg */ false,
/* isParenless */ true, actuals);
auto inScope = scopeStack.back();
auto inScopes = CallScopeInfo::forNormalCall(inScope, poiScope);
auto c = resolveGeneratedCall(context, ident, ci, inScopes);
// Ensure we error out for redeclarations within the method itself,
// which resolution with an implicit 'this' currently does not catch.
// TODO: Also catch redeclarations of formal names.
std::vector<BorrowedIdsWithName> redeclarations;
inScope->lookupInScope(ident->name(), redeclarations, IdAndFlags::Flags(),
IdAndFlags::FlagSet());
if (!redeclarations.empty()) {
context->error(ident, "parenless proc redeclares the field '%s'",
ident->name().c_str());
} else {
// Save result if successful
if (handleResolvedCallWithoutError(result, ident, ci, c) &&
emitLookupErrors) {
issueErrorForFailedCallResolution(ident, ci, c);
}
}
} else {
// Can't establish the type. If this is in a function
// call, we'll establish it later anyway.
result.setType(QualifiedType());
}
} else {
// vec.size() == 1 and vec[0].numIds() <= 1
const IdAndFlags& idv = vec[0].firstIdAndFlags();
Expand Down
126 changes: 126 additions & 0 deletions frontend/test/resolution/testMethodFieldAccess.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -725,6 +725,24 @@ static void testExample5a() {
"" /* ambiguity */);
}

// Like 5a but this.foo instead of foo
static void testExample5b() {
testCall("example5b.chpl",
R""""(
module M {
record r { }
proc r.test() {
var foo: int;
proc r.foo { }
this.foo;
}
}
)"""",
"M.test",
"M.test@5",
"M.test.foo");
}

static void testExample6() {
testCall("example6.chpl",
R""""(
Expand Down Expand Up @@ -1190,6 +1208,112 @@ static void testExample27() {
"" /* both false; no matches. */);
}

// Check diambiguation of identifiers in methods, between a
// field and a proc on a different receiver.
static void testExample28() {
{
testCall("example28a.chpl",
R""""(
module M {
record foo {
var size : int;
}
record bar {
}
proc bar.size do return 3;
proc foo.asdf() {
return size;
}
}
)"""",
"M.asdf",
"M.asdf@2",
"M.foo@1");
}

// Same as 28a but with primary calling method
{
testCall("example28b.chpl",
R""""(
module M {
record foo {
var size : int;
proc asdf() {
return size;
}
}
record bar {
}
proc bar.size do return 3;
}
)"""",
"M.foo.asdf",
"M.foo.asdf@1",
"M.foo@1");
}

// Same as 28a but with primary called method
{
testCall("example28c.chpl",
R""""(
module M {
record foo {
var size : int;
proc asdf() {
return size;
}
}
record bar {
proc size do return 3;
}
}
)"""",
"M.foo.asdf",
"M.foo.asdf@1",
"M.foo@1");
}

// Same as 28b but with primary called method
{
testCall("example28d.chpl",
R""""(
module M {
record foo {
var size : int;
proc asdf() {
return size;
}
}
record bar {
proc size do return 3;
}
}
)"""",
"M.foo.asdf",
"M.foo.asdf@1",
"M.foo@1");
}

// Sanity check: 2a but with same receiver, clearly ambiguous
{
testCall("example28e.chpl",
R""""(
module M {
record foo {
var size : int;
}
proc foo.size do return 3;
proc foo.asdf() {
return size;
}
}
)"""",
"M.asdf",
"M.asdf@2",
"" /* ambiguous */);
}
}

int main() {
test1r();
test1c();
Expand Down Expand Up @@ -1230,6 +1354,7 @@ int main() {
testExample4a();
testExample5();
testExample5a();
testExample5b();
testExample6();
testExample7();
testExample8();
Expand All @@ -1252,6 +1377,7 @@ int main() {
testExample25();
testExample26();
testExample27();
testExample28();

return 0;
}

0 comments on commit 953181f

Please sign in to comment.