From 5c8d6ce07386bd12c6acac9ba54876e7c3ab866d Mon Sep 17 00:00:00 2001 From: Danila Fedorin Date: Fri, 17 Jan 2025 16:45:39 -0800 Subject: [PATCH] Respond to reviewer feedback Signed-off-by: Danila Fedorin --- frontend/lib/resolution/Resolver.cpp | 21 +++++-- frontend/test/resolution/testManage.cpp | 77 ++++++++++++++++++++----- 2 files changed, 78 insertions(+), 20 deletions(-) diff --git a/frontend/lib/resolution/Resolver.cpp b/frontend/lib/resolution/Resolver.cpp index 0520145bf5e5..e7eade1bcb8a 100644 --- a/frontend/lib/resolution/Resolver.cpp +++ b/frontend/lib/resolution/Resolver.cpp @@ -3905,11 +3905,21 @@ bool Resolver::enter(const uast::Manage* manage) { auto& contextReturnType = witness->associatedTypes().at(contextReturnTypeId); + // This is effectively a variable declaration with an initialization + // expression being 'enterContext()'. The same semantics + // are expected. As a result, we call resolveNamedDecl to handle this. + // // Performance: We are taking a very simple path through resolveNamedDecl - // here, but it seems good to share the logic. + // here (most conditions it checks for are 'false': finding substitutions, + // detecting 'this' formals, handling 'extern' variables). However, + // it seems good to share the general logic. // - // Normal declarations don't do 'ref' and 'const ref' checking because - // it's not implemented, and we follow suit. + // It's possible for the enterContext() method to e.g., provide a value + // when the declaration is a ref. We don't check for that here specifically, + // since the behavior ought to be the same as trying to assign a value to + // a ref variable. However, 'resolveNamedDecl' does not handle that case + // at the time of writing, and therefore we effectively don't handle it + // here either. resolveNamedDecl(asVar, contextReturnType); accessContext = accessForQualifier(byPostorder.byAst(asVar).type().kind()); } @@ -3931,11 +3941,12 @@ bool Resolver::enter(const uast::Manage* manage) { // a ResolvedExpression. auto overloadsIt = witness->returnIntentOverloads().find(id); if (overloadsIt != witness->returnIntentOverloads().end()) { - bool ignoreAmbiguity; + bool ambiguity; auto bestCandidate = determineBestReturnIntentOverload(overloadsIt->second, accessContext, - ignoreAmbiguity); + ambiguity); + CHPL_ASSERT(!ambiguity); CHPL_ASSERT(bestCandidate); enterSig = bestCandidate->fn(); } else { diff --git a/frontend/test/resolution/testManage.cpp b/frontend/test/resolution/testManage.cpp index 50d632ef1b84..6cd5fdc13aee 100644 --- a/frontend/test/resolution/testManage.cpp +++ b/frontend/test/resolution/testManage.cpp @@ -69,8 +69,31 @@ static void testResourceByVar() { assert(x.type()->isIntType()); } +static void testResourceByVarExplicit() { + // test saving the resource into a variable, specifying the 'var' intent explicitly + auto ctx = buildStdContext(); + ErrorGuard guard(ctx); + std::string program = + R"""( + record R : contextManager { + proc type contextReturnType type do return int; + + proc enterContext() { + return 42; + } + proc exitContext(in error: owned Error?) {} + } + manage new R() as var res { + var x = res; + } + )"""; + + auto x = resolveTypesOfVariables(ctx, program, {"x"}).at("x"); + assert(x.type()->isIntType()); +} + static void testResourceByRef() { - // test saving the resource into a variable. + // test saving the resource into a variable by reference auto ctx = buildStdContext(); ErrorGuard guard(ctx); std::string program = @@ -96,7 +119,7 @@ static void testResourceByRef() { } static void testResourceByConstRef() { - // test saving the resource into a variable. + // test saving the resource into a variable, by const reference auto ctx = buildStdContext(); ErrorGuard guard(ctx); std::string program = @@ -122,7 +145,7 @@ static void testResourceByConstRef() { } static void testInferReturn() { - // test saving the resource into a variable. + // test inferring 'contextReturnType' auto ctx = buildStdContext(); ErrorGuard guard(ctx); std::string program = @@ -141,7 +164,8 @@ static void testInferReturn() { } static void testReturnIntentOverload() { - // test saving the resource into a variable. + // test the same context manager being able to call different functions + // depending on the return intent and expected kind of the variable. auto ctx = buildStdContext(); ErrorGuard guard(ctx); std::string program = @@ -173,49 +197,59 @@ static void testReturnIntentOverload() { assert(modules.size() == 1); auto& rr = resolveModule(ctx, modules[0]->id()); - bool found; + bool foundEnter, foundExit; auto byVal = findVariable(modules[0], "byVal"); auto byValParent = parsing::parentAst(ctx, byVal)->toAs()->symbol(); assert(byValParent); assert(rr.hasAst(byValParent)); - found = false; + foundEnter = foundExit = false; for (auto aa : rr.byAst(byValParent).associatedActions()) { if (aa.action() == AssociatedAction::ENTER_CONTEXT) { - found = true; + foundEnter = true; assert(aa.fn()->id().symbolPath() == "M.R.enterContext"); + } else if (aa.action() == AssociatedAction::EXIT_CONTEXT) { + foundExit = true; + assert(aa.fn()->id().symbolPath() == "M.R.exitContext"); } } - assert(found); + assert(foundEnter && foundExit); auto byRef = findVariable(modules[0], "byRef"); auto byRefParent = parsing::parentAst(ctx, byRef)->toAs()->symbol(); assert(byRefParent); assert(rr.hasAst(byRefParent)); - found = false; + foundEnter = foundExit = false; for (auto aa : rr.byAst(byRefParent).associatedActions()) { if (aa.action() == AssociatedAction::ENTER_CONTEXT) { - found = true; + foundEnter = true; assert(aa.fn()->id().symbolPath() == "M.R.enterContext#1"); + } else if (aa.action() == AssociatedAction::EXIT_CONTEXT) { + foundExit = true; + assert(aa.fn()->id().symbolPath() == "M.R.exitContext"); } } - assert(found); + assert(foundEnter && foundExit); auto byConstRef = findVariable(modules[0], "byConstRef"); auto byConstRefParent = parsing::parentAst(ctx, byConstRef)->toAs()->symbol(); assert(byConstRefParent); assert(rr.hasAst(byConstRefParent)); - found = false; + foundEnter = foundExit = false; for (auto aa : rr.byAst(byConstRefParent).associatedActions()) { if (aa.action() == AssociatedAction::ENTER_CONTEXT) { - found = true; + foundEnter = true; assert(aa.fn()->id().symbolPath() == "M.R.enterContext#2"); + } else if (aa.action() == AssociatedAction::EXIT_CONTEXT) { + foundExit = true; + assert(aa.fn()->id().symbolPath() == "M.R.exitContext"); } } - assert(found); + assert(foundEnter && foundExit); } static void testNoExplicitImplements() { + // We complain about not having an explicit ': contextManager', but still resolve. auto ctx = buildStdContext(); ErrorGuard guard(ctx); std::string program = @@ -234,11 +268,14 @@ static void testNoExplicitImplements() { } static void testWithoutInterfaceMatch() { + // Unlike in production, we like the 'owned Error' formal to be called 'error', + // and not anything else. This is a consequence of leaning on interfaces + // for context managers. auto ctx = buildStdContext(); ErrorGuard guard(ctx); std::string program = R"""( - record R { + record R : contextManager { proc enterContext() { return 42; } @@ -249,12 +286,22 @@ static void testWithoutInterfaceMatch() { auto x = resolveTypesOfVariables(ctx, program, {"res"}).at("res"); assert(x.isUnknownOrErroneous()); + + bool foundError = false; + for (auto& error : guard.errors()) { + if (error->type() == ErrorType::InterfaceMissingFn) { + foundError = true; + break; + } + } + assert(foundError); assert(guard.realizeErrors()); } int main() { testBasic(); testResourceByVar(); + testResourceByVarExplicit(); testResourceByRef(); testResourceByConstRef(); testInferReturn();