Dyno: Fix (callable variable, function) disambiguation

frontend/include/chpl/resolution/scope-types.h

@@ -234,6 +234,15 @@ class IdAndFlags {
     return (flags_ & TYPE) != 0;
   }
 
+  bool isFunctionLike() const {
+    // * functions are obviously function-like (shouldn't stop lookup)
+    // * parenless functions that are methods might not match the receiver,
+    //   so they should not stop the lookup either (treat them as function-like)
+    // * fields are effectively resolved as accessor functions, so they should
+    //   also not stop the lookup.
+    return isParenfulFunction() || isMethodOrField();
+  }
+
   /** Returns true if haveFlags matches filterFlags, and does not match
       the exclude flag set. See the comments on Flags and FlagSet for
       how the matching works.
@@ -262,6 +271,39 @@ class IdAndFlags {
   /// \endcond DO_NOT_DOCUMENT
 };
 
+/**
+  The result of looking up a name in a scope.
+ */
+class LookupResult {
+ private:
+  /* whether anything was found */
+  bool found_ = false;
+  /* whether one of the discovered symbols was not a function */
+  bool nonFunctions_ = false;
+
+  friend class OwnedIdsWithName;
+
+ public:
+  LookupResult(bool found, bool nonFunctions)
+    : found_(found), nonFunctions_(nonFunctions) {
+    CHPL_ASSERT(!nonFunctions_ || found);
+  }
+
+  static LookupResult empty() { return {false, false}; }
+
+  operator bool() const { return found_; }
+
+  LookupResult& operator|=(const LookupResult& other) {
+    found_ |= other.found_;
+    nonFunctions_ |= other.nonFunctions_;
+    return *this;
+  }
+
+  bool found() const { return found_; }
+
+  bool nonFunctions() const { return nonFunctions_; }
+};
+
 /**
   Collects IDs with a particular name.
  */
@@ -308,10 +350,10 @@ class OwnedIdsWithName {
 
   /** Append any entries that match filterFlags and aren't excluded
       by excludeFlagSet to a MatchingIdsWithName.
-      Returns 'true' if any matches were appended. */
-  bool gatherMatches(MatchingIdsWithName& dst,
-                     IdAndFlags::Flags filterFlags,
-                     const IdAndFlags::FlagSet& excludeFlagSet) const;
+      Returns whether any matches were appended. */
+  LookupResult gatherMatches(MatchingIdsWithName& dst,
+                             IdAndFlags::Flags filterFlags,
+                             const IdAndFlags::FlagSet& excludeFlagSet) const;
 
   int numIds() const {
     if (moreIdvs_.get() == nullptr) {
@@ -369,6 +411,7 @@ class MatchingIdsWithName {
 
  private:
   llvm::SmallVector<IdAndFlags, 1> idvs_;
+  bool encounteredFnNonFnConflict_ = false;
 
   /** Construct a MatchingIdsWithName referring to the same IDs
       as the passed OwnedIdsWithName.  */
@@ -432,6 +475,17 @@ class MatchingIdsWithName {
   /** Construct a empty MatchingIdsWithName containing no IDs. */
   MatchingIdsWithName() { }
 
+  /** Note that when populating this list, we found functions and non-functions
+      at the same scope level. */
+  void noteFnNonFnConflict() {
+    encounteredFnNonFnConflict_ = true;
+  }
+
+  /** Returns 'true' if we found functions and non-functions at the same scope level. */
+  bool encounteredFnNonFnConflict() const {
+    return encounteredFnNonFnConflict_;
+  }
+
   /** Append an IdAndFlags. */
   void append(IdAndFlags idv) {
     idvs_.push_back(std::move(idv));
@@ -502,7 +556,8 @@ class MatchingIdsWithName {
   }
 
   bool operator==(const MatchingIdsWithName& other) const {
-    return idvs_ == other.idvs_;
+    return idvs_ == other.idvs_ &&
+           encounteredFnNonFnConflict_ == other.encounteredFnNonFnConflict_;
   }
   bool operator!=(const MatchingIdsWithName& other) const {
     return !(*this == other);
@@ -513,6 +568,7 @@ class MatchingIdsWithName {
     for (const auto& x : idvs_) {
       ret = hash_combine(ret, chpl::hash(x));
     }
+    ret = hash_combine(ret, encounteredFnNonFnConflict_);
     return ret;
   }
 
@@ -536,15 +592,16 @@ class MatchingIdsWithName {
  */
 using DeclMap = std::unordered_map<UniqueString, OwnedIdsWithName>;
 
+
 /**
   Gather matches to 'name' that match 'filterFlags' and aren't
   excluded by 'excludeFlags'. Store any gathered into 'result'
  */
-bool lookupInDeclMap(const DeclMap& declared,
-                     UniqueString name,
-                     MatchingIdsWithName& result,
-                     IdAndFlags::Flags filterFlags,
-                     const IdAndFlags::FlagSet& excludeFlags);
+LookupResult lookupInDeclMap(const DeclMap& declared,
+                             UniqueString name,
+                             MatchingIdsWithName& result,
+                             IdAndFlags::Flags filterFlags,
+                             const IdAndFlags::FlagSet& excludeFlags);
 
 /**
   A scope roughly corresponds to a `{ }` block. Anywhere a new symbol could be
@@ -667,10 +724,10 @@ class Scope {
   /** If the scope contains IDs with the provided name,
       append the relevant BorrowedIdsToName the the vector.
       Returns true if something was appended. */
-  bool lookupInScope(UniqueString name,
-                     MatchingIdsWithName& result,
-                     IdAndFlags::Flags filterFlags,
-                     const IdAndFlags::FlagSet& excludeFlagSet) const {
+  LookupResult lookupInScope(UniqueString name,
+                             MatchingIdsWithName& result,
+                             IdAndFlags::Flags filterFlags,
+                             const IdAndFlags::FlagSet& excludeFlagSet) const {
     return lookupInDeclMap(declared_, name, result,
                            filterFlags, excludeFlagSet);
   }
@@ -1068,6 +1125,17 @@ enum {
     Include methods in the search results (they are excluded by default).
    */
   LOOKUP_METHODS = 2048,
+
+  /**
+    If proceeding through scopes (ie., not LOOKUP_INNERMOST), stop when
+    encountering a non-function. This encodes the behavior of the resolver
+    when encountering ambiguity between called functions and callable
+    non-function variables (e.g., tuple with 'proc this(x: int)').
+
+    This partially encodes the shadowing rules for function overload resolution:
+    "nearer" functions are always preferred to "further" ones.
+   */
+  LOOKUP_STOP_NON_FN = 4096,
 };
 /// \endcond
 
@@ -1314,10 +1382,10 @@ class ModulePublicSymbols {
 
   const DeclMap& syms() const { return syms_; }
 
-  bool lookupInModule(UniqueString name,
-                      MatchingIdsWithName& result,
-                      IdAndFlags::Flags filterFlags,
-                      const IdAndFlags::FlagSet& excludeFlags) const {
+  LookupResult lookupInModule(UniqueString name,
+                              MatchingIdsWithName& result,
+                              IdAndFlags::Flags filterFlags,
+                              const IdAndFlags::FlagSet& excludeFlags) const {
     return lookupInDeclMap(syms_, name, result, filterFlags, excludeFlags);
   }
 

frontend/lib/resolution/Resolver.cpp

@@ -3050,6 +3050,16 @@ void Resolver::issueAmbiguityErrorIfNeeded(const Identifier* ident,
   }
 }
 
+static LookupConfig identifierLookupConfig(bool resolvingCalledIdent) {
+  LookupConfig config = IDENTIFIER_LOOKUP_CONFIG;
+  if (resolvingCalledIdent) {
+    config |= LOOKUP_STOP_NON_FN;
+  } else {
+    config |= LOOKUP_INNERMOST;
+  }
+  return config;
+}
+
 MatchingIdsWithName Resolver::lookupIdentifier(
       const Identifier* ident,
       bool resolvingCalledIdent,
@@ -3066,8 +3076,7 @@ MatchingIdsWithName Resolver::lookupIdentifier(
                                       IdAndFlags::createForBuiltinVar());
   }
 
-  LookupConfig config = IDENTIFIER_LOOKUP_CONFIG;
-  if (!resolvingCalledIdent) config |= LOOKUP_INNERMOST;
+  LookupConfig config = identifierLookupConfig(resolvingCalledIdent);
 
   auto fHelper = getFieldDeclarationLookupHelper();
   auto helper = getMethodReceiverScopeHelper();
@@ -3315,9 +3324,8 @@ void Resolver::tryResolveParenlessCall(const ParenlessOverloadInfo& info,
       // Found both, it's an ambiguity after all. Issue the ambiguity error
       // late, for which we need to recover some context.
 
-      LookupConfig config = IDENTIFIER_LOOKUP_CONFIG;
       bool resolvingCalledIdent = nearestCalledExpression() == ident;
-      if (!resolvingCalledIdent) config |= LOOKUP_INNERMOST;
+      LookupConfig config = identifierLookupConfig(resolvingCalledIdent);
 
       issueAmbiguityErrorIfNeeded(ident, inScope, config);
       auto& rr = byPostorder.byAst(ident);
@@ -3406,10 +3414,18 @@ void Resolver::resolveIdentifier(const Identifier* ident) {
   auto parenlessInfo = ParenlessOverloadInfo();
   auto ids = lookupIdentifier(ident, resolvingCalledIdent, parenlessInfo);
 
-  // If we looked up a called identifier and found ambiguity between variables
-  // only, resolve as an implicit 'this' call on the innermost variable.
-  // TODO: replace this hacky solution with an adjustment to the scope
-  // resolution process
+  // If we requested IDs including potential overloads, and found
+  // both a variable and a function at the same point, we're not sure how to
+  // resolve the call (via regular call resolution or 'proc this' resolution).
+  // Issue an error.
+  if (ids.encounteredFnNonFnConflict()) {
+    result.setType(typeErr(ident, "ambiguity between function and non-function at the same scope level"));
+    return;
+  }
+
+  // If we looked up a called identifier and got back several variables,
+  // give up, since the lookup process only does that if they're defined
+  // in the same place.
   if (resolvingCalledIdent && ids.numIds() > 1) {
     bool onlyVars = true;
     for (auto idIt = ids.begin(); idIt != ids.end(); ++idIt) {
@@ -3419,8 +3435,10 @@ void Resolver::resolveIdentifier(const Identifier* ident) {
         break;
       }
     }
+
     if (onlyVars) {
-      ids.truncate(1);
+      result.setType(typeErr(ident, "ambiguous callable value in called expression"));
+      return;
     }
   }
 
@@ -3491,8 +3509,7 @@ void Resolver::resolveIdentifier(const Identifier* ident) {
           }
 
           if (otherThanParenless) {
-            LookupConfig config = IDENTIFIER_LOOKUP_CONFIG;
-            if (!resolvingCalledIdent) config |= LOOKUP_INNERMOST;
+            LookupConfig config = identifierLookupConfig(resolvingCalledIdent);
             issueAmbiguityErrorIfNeeded(ident, inScope, config);
           }
         } else {

frontend/lib/resolution/resolution-queries.cpp

@@ -4090,6 +4090,8 @@ lookupCalledExpr(Context* context,
   // For parenless non-method calls, only find the innermost match
   if (ci.isParenless() && !ci.isMethodCall()) {
     config |= LOOKUP_INNERMOST;
+  } else {
+    config |= LOOKUP_STOP_NON_FN;
   }
 
   if (ci.isMethodCall()) {
@@ -4107,6 +4109,21 @@ lookupCalledExpr(Context* context,
                                       /* receiverScopeHelper */ nullptr,
                                       name, config, visited);
 
+  // At this point, we don't allow "switching gears" to 'proc this'-based
+  // resolution of non-function names. The calling code is expected to
+  // have set up the CallInfo with 'name = "this"' if that's what it wanted.
+  // So, rule out non-functional IDs.
+  //
+  // This relies on the fact that under LOOKUP_STOP_NON_FN, the non-functional
+  // IDs are last. (except in the case of ambiguity, but that should've been
+  // ruled out by calling code as well).
+  if (config & LOOKUP_STOP_NON_FN) {
+    for (int i = 0; i < ret.numIds(); i++) {
+      auto& idv = ret.idAndFlags(i);
+      if (!idv.isFunctionLike() && !idv.isType()) ret.truncate(i);
+    }
+  }
+
   return ret;
 }
 

frontend/lib/resolution/resolution-types.cpp

@@ -416,10 +416,16 @@ CallInfo CallInfo::create(Context* context,
     auto calledExprType = tryGetType(calledExpr, byPostorder);
     auto dotReceiverType = tryGetType(dotReceiver, byPostorder);
 
-    if (calledExprType && isKindForFunctionalValue(calledExprType->kind())) {
+    if (calledExprType &&
+        (isKindForFunctionalValue(calledExprType->kind()) ||
+         calledExprType->isErroneousType())) {
       // If e.g. x is a value (and not a function, then x(0) translates to x.this(0)
       // Run this case even if the receiver is a module, since we might be
       // trying to invoke 'this' on value x in M.x.
+      //
+      // In the case of ErroneousType, assume that the called thing was
+      // a value (ambiguity or other "benign" UNKNOWN would not produce errors).
+      // Later, this can lead to skipping resolving the call altogether.
 
       name = USTR("this");
       // add the 'this' argument as well