Respond to reviewer feedback

Signed-off-by: Danila Fedorin <[email protected]>

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 {

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();