From 5f10468041a17a4cf2a3bb631b10e403c725c706 Mon Sep 17 00:00:00 2001 From: Danila Fedorin Date: Fri, 20 Dec 2024 14:08:29 -0800 Subject: [PATCH 01/13] Track whether or not variables were found to handle some disambiguation cases Signed-off-by: Danila Fedorin --- .../include/chpl/resolution/scope-types.h | 77 ++++-- frontend/lib/resolution/Resolver.cpp | 19 +- .../lib/resolution/resolution-queries.cpp | 2 + frontend/lib/resolution/scope-queries.cpp | 254 ++++++++++-------- frontend/lib/resolution/scope-types.cpp | 21 +- 5 files changed, 229 insertions(+), 144 deletions(-) diff --git a/frontend/include/chpl/resolution/scope-types.h b/frontend/include/chpl/resolution/scope-types.h index 3aeb383eac40..8912fef6985a 100644 --- a/frontend/include/chpl/resolution/scope-types.h +++ b/frontend/include/chpl/resolution/scope-types.h @@ -262,6 +262,37 @@ 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) {} + + 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 +339,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) { @@ -536,15 +567,16 @@ class MatchingIdsWithName { */ using DeclMap = std::unordered_map; + /** 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 +699,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 +1100,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 +1357,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); } diff --git a/frontend/lib/resolution/Resolver.cpp b/frontend/lib/resolution/Resolver.cpp index b947214fd362..f648b661edf4 100644 --- a/frontend/lib/resolution/Resolver.cpp +++ b/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); @@ -3491,8 +3499,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 { diff --git a/frontend/lib/resolution/resolution-queries.cpp b/frontend/lib/resolution/resolution-queries.cpp index 3e82c365041e..3f6934913af8 100644 --- a/frontend/lib/resolution/resolution-queries.cpp +++ b/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()) { diff --git a/frontend/lib/resolution/scope-queries.cpp b/frontend/lib/resolution/scope-queries.cpp index 345f3e570c7c..7314b1eee1a4 100644 --- a/frontend/lib/resolution/scope-queries.cpp +++ b/frontend/lib/resolution/scope-queries.cpp @@ -660,38 +660,39 @@ struct LookupHelper { allowCached(allowCached) { } - bool doLookupInImportsAndUses(const Scope* scope, - const ResolvedVisibilityScope* cur, - UniqueString name, - LookupConfig config, - IdAndFlags::Flags filterFlags, - const IdAndFlags::FlagSet& excludeFilter, - VisibilitySymbols::ShadowScope shadowScope, - bool* foundInAllContents, - std::unordered_set* foundInClauses, - std::unordered_set* ignoreClauses); - - bool doLookupInAutoModules(const Scope* scope, - UniqueString name, - bool onlyInnermost, - bool skipPrivateVisibilities, - bool onlyMethodsFields, - bool includeMethods); - - bool doLookupEnclosingModuleName(const Scope* scope, UniqueString name); - - bool doLookupInToplevelModules(const Scope* scope, UniqueString name); - - bool doLookupInReceiverScopes(const Scope* scope, - const MethodLookupHelper* receiverScopes, - UniqueString name, - LookupConfig config); - - bool doLookupInExternBlocks(const Scope* scope, UniqueString name); - - bool doLookupInScope(const Scope* scope, - UniqueString name, - LookupConfig config); + LookupResult doLookupInImportsAndUses(const Scope* scope, + const ResolvedVisibilityScope* cur, + UniqueString name, + LookupConfig config, + IdAndFlags::Flags filterFlags, + const IdAndFlags::FlagSet& excludeFilter, + VisibilitySymbols::ShadowScope shadowScope, + bool* foundInAllContents, + std::unordered_set* foundInClauses, + std::unordered_set* ignoreClauses); + + LookupResult doLookupInAutoModules(const Scope* scope, + UniqueString name, + bool onlyInnermost, + bool stopNonFn, + bool skipPrivateVisibilities, + bool onlyMethodsFields, + bool includeMethods); + + LookupResult doLookupEnclosingModuleName(const Scope* scope, UniqueString name); + + LookupResult doLookupInToplevelModules(const Scope* scope, UniqueString name); + + LookupResult doLookupInReceiverScopes(const Scope* scope, + const MethodLookupHelper* receiverScopes, + UniqueString name, + LookupConfig config); + + LookupResult doLookupInExternBlocks(const Scope* scope, UniqueString name); + + LookupResult doLookupInScope(const Scope* scope, + UniqueString name, + LookupConfig config); }; static const Scope* const& scopeForAutoModuleQuery(Context* context) { @@ -746,7 +747,7 @@ idAndFlagsForScopeSymbol(Context* context, const Scope* scope) { // config has settings for this part of the search // filterFlags has the filter used when considering the module name itself -bool LookupHelper::doLookupInImportsAndUses( +LookupResult LookupHelper::doLookupInImportsAndUses( const Scope* scope, const ResolvedVisibilityScope* cur, UniqueString name, @@ -758,13 +759,14 @@ bool LookupHelper::doLookupInImportsAndUses( std::unordered_set* foundInClauses, std::unordered_set* ignoreClauses) { bool onlyInnermost = (config & LOOKUP_INNERMOST) != 0; + bool stopNonFn = (config & LOOKUP_STOP_NON_FN) != 0; bool skipPrivateVisibilities = (config & LOOKUP_SKIP_PRIVATE_VIS) != 0; bool onlyMethodsFields = (config & LOOKUP_ONLY_METHODS_FIELDS) != 0; bool checkExternBlocks = (config & LOOKUP_EXTERN_BLOCKS) != 0; bool skipPrivateUseImport = (config & LOOKUP_SKIP_PRIVATE_USE_IMPORT) != 0; bool includeMethods = (config & LOOKUP_METHODS) != 0; bool trace = (traceCurPath != nullptr && traceResult != nullptr); - bool found = false; + auto found = LookupResult::empty(); if (cur != nullptr) { // check to see if it's mentioned in names/renames @@ -817,6 +819,9 @@ bool LookupHelper::doLookupInImportsAndUses( if (onlyInnermost) { newConfig |= LOOKUP_INNERMOST; } + if (stopNonFn) { + newConfig |= LOOKUP_STOP_NON_FN; + } if (onlyMethodsFields) { newConfig |= LOOKUP_ONLY_METHODS_FIELDS; } @@ -849,7 +854,7 @@ bool LookupHelper::doLookupInImportsAndUses( traceCurPath->push_back(std::move(elt)); } - bool foundHere = doLookupInScope(symScope, nameToLookUp, newConfig); + auto foundHere = doLookupInScope(symScope, nameToLookUp, newConfig); found |= foundHere; // note if we found it from the contents in a bulk // operation like 'use M' @@ -870,7 +875,8 @@ bool LookupHelper::doLookupInImportsAndUses( auto idv = idAndFlagsForScopeSymbol(context, is.scope()); if (idv.matchesFilter(filterFlags, excludeFlagSet)) { result.append(idv); - found = true; + found |= LookupResult(/* found */ true, + /* nonFunctions */ !idv.isParenfulFunction()); if (trace) { ResultVisibilityTrace t; t.scope = cur->scope(); @@ -900,14 +906,15 @@ bool LookupHelper::doLookupInImportsAndUses( return found; } -bool LookupHelper::doLookupInAutoModules(const Scope* scope, - UniqueString name, - bool onlyInnermost, - bool skipPrivateVisibilities, - bool onlyMethodsFields, - bool includeMethods) { +LookupResult LookupHelper::doLookupInAutoModules(const Scope* scope, + UniqueString name, + bool onlyInnermost, + bool stopNonFn, + bool skipPrivateVisibilities, + bool onlyMethodsFields, + bool includeMethods) { bool trace = (traceCurPath != nullptr && traceResult != nullptr); - bool found = false; + auto found = LookupResult::empty(); if (scope->autoUsesModules() && !skipPrivateVisibilities) { const Scope* autoModScope = scopeForAutoModule(context); @@ -920,6 +927,10 @@ bool LookupHelper::doLookupInAutoModules(const Scope* scope, newConfig |= LOOKUP_INNERMOST; } + if (stopNonFn) { + newConfig |= LOOKUP_STOP_NON_FN; + } + if (onlyMethodsFields) { newConfig |= LOOKUP_ONLY_METHODS_FIELDS; } @@ -946,20 +957,20 @@ bool LookupHelper::doLookupInAutoModules(const Scope* scope, return found; } -bool LookupHelper::doLookupEnclosingModuleName(const Scope* scope, - UniqueString name) { +LookupResult LookupHelper::doLookupEnclosingModuleName(const Scope* scope, + UniqueString name) { // this code assumes that 'scope' is a module scope CHPL_ASSERT(scope && scope->moduleScope() == scope); if (name != scope->name()) - return false; + return LookupResult::empty(); // Ignore this match for enclosing module names that aren't valid // Chapel file names. This avoids compilation failures for // implicit modules created from a filename of the form .chpl // e.g. domain.chpl if (getReservedIdentifier(name)) - return false; + return LookupResult::empty(); // the name matches! record the match // @@ -976,14 +987,14 @@ bool LookupHelper::doLookupEnclosingModuleName(const Scope* scope, traceResult->push_back(std::move(t)); } - return true; + return LookupResult(/* found */ true, /* nonFunctions */ true); } -bool LookupHelper::doLookupInToplevelModules(const Scope* scope, - UniqueString name) { +LookupResult LookupHelper::doLookupInToplevelModules(const Scope* scope, + UniqueString name) { const Module* mod = parsing::getToplevelModule(context, name); if (mod == nullptr) - return false; + return LookupResult::empty(); if (traceCurPath && traceResult) { ResultVisibilityTrace t; @@ -996,7 +1007,7 @@ bool LookupHelper::doLookupInToplevelModules(const Scope* scope, result.append(IdAndFlags::createForModule(mod->id(), /* isPublic */ true)); - return true; + return LookupResult(/* found */ true, /* nonFunctions */ true); } // Receiver scopes support two cases: @@ -1006,13 +1017,13 @@ bool LookupHelper::doLookupInToplevelModules(const Scope* scope, // // This method searches parents scopes (for secondary methods) // if LOOKUP_PARENTS is included in 'config'. -bool LookupHelper::doLookupInReceiverScopes( +LookupResult LookupHelper::doLookupInReceiverScopes( const Scope* scope, const MethodLookupHelper* receiverScopes, UniqueString name, LookupConfig config) { if (receiverScopes == nullptr) { - return false; + return LookupResult::empty(); } bool checkParents = (config & LOOKUP_PARENTS) != 0; @@ -1106,6 +1117,7 @@ bool LookupHelper::doLookupInReceiverScopes( // using receiverScopes->isReceiverApplicable. int endSize = result.numIds(); int cur = startParentsSize; + bool nonFunctions = false; for (int i = startParentsSize; i < endSize; i++) { IdAndFlags& idv = result.idAndFlags(i); if (receiverScopes->isReceiverApplicable(context, idv.id())) { @@ -1113,6 +1125,7 @@ bool LookupHelper::doLookupInReceiverScopes( if (cur != i) { result.idAndFlags(cur) = idv; } + nonFunctions |= !idv.isParenfulFunction(); cur++; } } @@ -1124,7 +1137,7 @@ bool LookupHelper::doLookupInReceiverScopes( traceResult->resize(cur); } - return cur > startSize; + return LookupResult(/* found */ cur > startSize, nonFunctions); } // returns IDs of all extern blocks directly contained within scope @@ -1143,11 +1156,13 @@ static const std::vector& gatherExternBlocks(Context* context, ID scopeID) { return QUERY_END(result); } -bool LookupHelper::doLookupInExternBlocks(const Scope* scope, - UniqueString name) { +LookupResult LookupHelper::doLookupInExternBlocks(const Scope* scope, + UniqueString name) { // Which are the IDs of the contained extern block(s)? const std::vector& exbIds = gatherExternBlocks(context, scope->id()); + bool found = false; + // Consider each extern block in turn. Does it have a symbol with that name? for (const auto& exbId : exbIds) { if (externBlockContainsName(context, exbId, name)) { @@ -1167,6 +1182,7 @@ bool LookupHelper::doLookupInExternBlocks(const Scope* scope, /* isType */ false); // maybe a lie result.append(std::move(idv)); + found = true; if (traceCurPath && traceResult) { ResultVisibilityTrace t; @@ -1179,7 +1195,7 @@ bool LookupHelper::doLookupInExternBlocks(const Scope* scope, } } - return true; + return LookupResult(found, /* nonFunctions; might be a lie; see above */ true); } // Returns the IdAndFlags for a common builtin, or nullptr. @@ -1226,6 +1242,12 @@ static const IdAndFlags* getReservedIdentifier(UniqueString name) { return nullptr; } +static bool doneLooking(const LookupResult& got, bool onlyInnermost, bool stopNonFn) { + if (got && onlyInnermost) return true; + if (got.nonFunctions() && stopNonFn) return true; + return false; +} + // appends to result // // traceCurPath and traceResult support gathering additional information @@ -1239,14 +1261,15 @@ static const IdAndFlags* getReservedIdentifier(UniqueString name) { // if both tracing arguments are not nullptr, traceResult will be updated to // have one entry for each of the elements in result, saving the traceCurPath // that provided that element in result. -bool LookupHelper::doLookupInScope(const Scope* scope, - UniqueString name, - LookupConfig config) { +LookupResult LookupHelper::doLookupInScope(const Scope* scope, + UniqueString name, + LookupConfig config) { bool checkDecls = (config & LOOKUP_DECLS) != 0; bool checkUseImport = (config & LOOKUP_IMPORT_AND_USE) != 0; bool checkParents = (config & LOOKUP_PARENTS) != 0; bool checkToplevel = (config & LOOKUP_TOPLEVEL) != 0; bool onlyInnermost = (config & LOOKUP_INNERMOST) != 0; + bool stopNonFn = (config & LOOKUP_STOP_NON_FN) != 0; bool skipPrivateVisibilities = (config & LOOKUP_SKIP_PRIVATE_VIS) != 0; bool goPastModules = (config & LOOKUP_GO_PAST_MODULES) != 0; bool onlyMethodsFields = (config & LOOKUP_ONLY_METHODS_FIELDS) != 0; @@ -1260,7 +1283,7 @@ bool LookupHelper::doLookupInScope(const Scope* scope, if (checkParents && !onlyMethodsFields) { if (const IdAndFlags* got = getReservedIdentifier(name)) { result.append(*got); - return true; + return LookupResult(/* found */ true, /* nonFunctions */ true); } } @@ -1306,7 +1329,7 @@ bool LookupHelper::doLookupInScope(const Scope* scope, // (which, because these are filters, means that foundFilter is // less restricted / more general / subsumes curFilter), // there is no need to visit this scope further. - return false; + return LookupResult::empty(); } // ok, we can search for curFilter but exclude what was already found @@ -1361,9 +1384,9 @@ bool LookupHelper::doLookupInScope(const Scope* scope, // gather non-shadow scope information // (declarations in this scope as well as public use / import) + auto got = LookupResult::empty(); + auto gotBeforeWarning = LookupResult::empty(); { - bool got = false; - // lookup in the module's public symbols if we have it if (modPublicSyms) { got |= @@ -1405,35 +1428,39 @@ bool LookupHelper::doLookupInScope(const Scope* scope, checkMoreForWarning = true; onlyInnermost = false; firstResultForWarning = result.numIds(); + gotBeforeWarning = got; } } } - if (onlyInnermost && got) return true; + if (doneLooking(got, onlyInnermost, stopNonFn)) return got; } // now check shadow scope 1 (only relevant for 'private use') if (checkUseImport && !skipShadowScopes) { - bool got = false; + auto gotInSS1 = LookupResult::empty(); bool foundInAll = false; - got |= doLookupInImportsAndUses(scope, r, name, config, - curFilter, excludeFilter, - VisibilitySymbols::SHADOW_SCOPE_ONE, - &foundInAll, - &foundInShadowScopeOneClauses, - /* ignoreClauses */ nullptr); + gotInSS1 |= doLookupInImportsAndUses(scope, r, name, config, + curFilter, excludeFilter, + VisibilitySymbols::SHADOW_SCOPE_ONE, + &foundInAll, + &foundInShadowScopeOneClauses, + /* ignoreClauses */ nullptr); // treat the auto-used modules as if they were 'private use'd - got |= doLookupInAutoModules(scope, name, - onlyInnermost, - skipPrivateVisibilities, - onlyMethodsFields, - includeMethods); - if (got && canCheckMoreForWarning && !checkMoreForWarning && foundInAll) { + gotInSS1 |= doLookupInAutoModules(scope, name, + onlyInnermost, + stopNonFn, + skipPrivateVisibilities, + onlyMethodsFields, + includeMethods); + if (gotInSS1 && canCheckMoreForWarning && !checkMoreForWarning && foundInAll) { checkMoreForWarning = true; onlyInnermost = false; firstResultForWarning = result.numIds(); } - if (onlyInnermost && got) return true; + + got |= gotInSS1; + if (doneLooking(got, onlyInnermost, stopNonFn)) return got; } std::unordered_set* ignoreClausesForShadowScopeTwo = nullptr; @@ -1445,19 +1472,21 @@ bool LookupHelper::doLookupInScope(const Scope* scope, // now check shadow scope 2 (only relevant for 'private use') if (checkUseImport && !skipShadowScopes) { - bool got = false; - got = doLookupInImportsAndUses(scope, r, name, config, - curFilter, excludeFilter, - VisibilitySymbols::SHADOW_SCOPE_TWO, - /* foundInAllContents */ nullptr, - /* foundInClauses */ nullptr, - ignoreClausesForShadowScopeTwo); - if (got && canCheckMoreForWarning && !checkMoreForWarning) { + auto gotInSS2 = LookupResult::empty(); + gotInSS2 = doLookupInImportsAndUses(scope, r, name, config, + curFilter, excludeFilter, + VisibilitySymbols::SHADOW_SCOPE_TWO, + /* foundInAllContents */ nullptr, + /* foundInClauses */ nullptr, + ignoreClausesForShadowScopeTwo); + if (gotInSS2 && canCheckMoreForWarning && !checkMoreForWarning) { checkMoreForWarning = true; onlyInnermost = false; firstResultForWarning = result.numIds(); } - if (onlyInnermost && got) return true; + + got |= gotInSS2; + if (doneLooking(got, onlyInnermost, stopNonFn)) return got; } // If we are at a method scope, consider receiver scopes now @@ -1467,8 +1496,8 @@ bool LookupHelper::doLookupInScope(const Scope* scope, if (scope->isMethodScope() && receiverScopeHelper != nullptr) { auto rcvScopes = receiverScopeHelper->methodLookupForMethodId(context, scope->id()); - bool got = doLookupInReceiverScopes(scope, rcvScopes, name, config); - if (onlyInnermost && got) return true; + got |= doLookupInReceiverScopes(scope, rcvScopes, name, config); + if (doneLooking(got, onlyInnermost, stopNonFn)) return got; } // consider the outer scopes due to nesting @@ -1508,22 +1537,21 @@ bool LookupHelper::doLookupInScope(const Scope* scope, // search without considering receiver scopes // (considered separately below) - bool got = doLookupInScope(cur, name, newConfig); + got |= doLookupInScope(cur, name, newConfig); if (trace) { traceCurPath->pop_back(); } - if (onlyInnermost && got) return true; + if (doneLooking(got, onlyInnermost, stopNonFn)) return got; // and then search only considering receiver scopes // as if the receiver scope were just outside of this scope. if (cur->isMethodScope() && receiverScopeHelper != nullptr) { auto rcvScopes = receiverScopeHelper->methodLookupForMethodId(context, cur->id()); - bool got = doLookupInReceiverScopes(scope, rcvScopes, - name, newConfig); - if (onlyInnermost && got) return true; + got |= doLookupInReceiverScopes(scope, rcvScopes, name, newConfig); + if (doneLooking(got, onlyInnermost, stopNonFn)) return got; } } } @@ -1536,13 +1564,13 @@ bool LookupHelper::doLookupInScope(const Scope* scope, traceCurPath->push_back(std::move(elt)); } - bool got = doLookupInScope(cur, name, newConfig); + got |= doLookupInScope(cur, name, newConfig); if (trace) { traceCurPath->pop_back(); } - if (onlyInnermost && got) return true; + if (doneLooking(got, onlyInnermost, stopNonFn)) return got; } if (!goPastModules && (reachedModule || isModule(scope->tag()))) { @@ -1560,13 +1588,13 @@ bool LookupHelper::doLookupInScope(const Scope* scope, traceCurPath->push_back(std::move(elt)); } - bool got = doLookupEnclosingModuleName(modScope, name); + got |= doLookupEnclosingModuleName(modScope, name); if (trace) { traceCurPath->pop_back(); } - if (onlyInnermost && got) return true; + if (doneLooking(got, onlyInnermost, stopNonFn)) return got; } // check also in the root scope if this isn't already the root scope @@ -1586,19 +1614,19 @@ bool LookupHelper::doLookupInScope(const Scope* scope, traceCurPath->push_back(std::move(elt)); } - bool got = doLookupInScope(rootScope, name, newConfig); + got |= doLookupInScope(rootScope, name, newConfig); if (trace) { traceCurPath->pop_back(); } - if (onlyInnermost && got) return true; + if (doneLooking(got, onlyInnermost, stopNonFn)) return got; } } if (checkToplevel) { - bool got = doLookupInToplevelModules(scope, name); - if (onlyInnermost && got) return true; + got |= doLookupInToplevelModules(scope, name); + if (doneLooking(got, onlyInnermost, stopNonFn)) return got; } // If LOOKUP_EXTERN_BLOCKS is set, and this scope has an extern block, @@ -1608,7 +1636,7 @@ bool LookupHelper::doLookupInScope(const Scope* scope, if (checkExternBlocks && scope->containsExternBlock() && !checkMoreForWarning) { foundExternBlock = true; - doLookupInExternBlocks(scope, name); + got |= doLookupInExternBlocks(scope, name); } if (checkMoreForWarning) { @@ -1633,6 +1661,7 @@ bool LookupHelper::doLookupInScope(const Scope* scope, } result.truncate(firstResultForWarning); + got = gotBeforeWarning; if (trace) { traceResult->resize(firstResultForWarning); } @@ -1644,10 +1673,10 @@ bool LookupHelper::doLookupInScope(const Scope* scope, CHPL_ASSERT(traceCurPath->size() == traceCurPathSize); } - return result.numIds() > startSize; + return got; } -static bool +static LookupResult helpLookupInScope(Context* context, const Scope* scope, const ResolvedVisibilityScope* resolving, @@ -1664,6 +1693,7 @@ helpLookupInScope(Context* context, std::vector* traceShadowed=nullptr) { bool onlyInnermost = (config & LOOKUP_INNERMOST) != 0; + bool stopNonFn = (config & LOOKUP_STOP_NON_FN) != 0; bool checkExternBlocks = (config & LOOKUP_EXTERN_BLOCKS) != 0; bool foundExternBlock = false; CheckedScopes savedCheckedScopes; @@ -1682,7 +1712,7 @@ helpLookupInScope(Context* context, savedCheckedScopes = checkedScopes; } - bool got = false; + auto got = LookupResult::empty(); if (scope) { got |= helper.doLookupInScope(scope, name, config); @@ -1691,7 +1721,7 @@ helpLookupInScope(Context* context, // When resolving a Dot expression like myRecord.foo, we might not be inside // of a method at all, but we should still search the definition point // of the relevant record. - if (methodLookupHelper != nullptr && !(got && onlyInnermost)) { + if (methodLookupHelper != nullptr && !(doneLooking(got, onlyInnermost, stopNonFn))) { got |= helper.doLookupInReceiverScopes(scope, methodLookupHelper, name, config); } @@ -1703,7 +1733,7 @@ helpLookupInScope(Context* context, checkedScopes = savedCheckedScopes; if (scope) { - got = helper.doLookupInScope(scope, name, config); + got |= helper.doLookupInScope(scope, name, config); } } @@ -1717,7 +1747,7 @@ helpLookupInScope(Context* context, // similar to helpLookupInScope but also emits a shadowing warning // in some cases. -static bool helpLookupInScopeWithShadowingWarning( +static LookupResult helpLookupInScopeWithShadowingWarning( Context* context, const Scope* scope, const ResolvedVisibilityScope* resolving, @@ -1733,7 +1763,7 @@ static bool helpLookupInScopeWithShadowingWarning( CheckedScopes checkedScopesForRetry = checkedScopes; MatchingIdsWithName shadowed; - bool got = helpLookupInScope(context, scope, resolving, + auto got = helpLookupInScope(context, scope, resolving, methodLookupHelper, receiverScopeHelper, name, config, checkedScopes, vec, allowCached, diff --git a/frontend/lib/resolution/scope-types.cpp b/frontend/lib/resolution/scope-types.cpp index 9155cb502cff..cd2c33015aa8 100644 --- a/frontend/lib/resolution/scope-types.cpp +++ b/frontend/lib/resolution/scope-types.cpp @@ -199,7 +199,7 @@ void FlagSet::mark(Context* context) const { // nothing, because flags don't need to be marked. } -bool +LookupResult OwnedIdsWithName::gatherMatches(MatchingIdsWithName& dst, IdAndFlags::Flags filterFlags, const IdAndFlags::FlagSet& excludeFlagSet) const @@ -209,9 +209,11 @@ OwnedIdsWithName::gatherMatches(MatchingIdsWithName& dst, // Are all of the filter flags present in flagsOr? // If not, it is not possible for this to match. if ((ownedIds.flagsOr_ & filterFlags) != filterFlags) { - return false; + return LookupResult::empty(); } + bool anyNonFunctions = false; + const IdAndFlags* beginIds = nullptr; const IdAndFlags* endIds = nullptr; if (ownedIds.moreIdvs_ == nullptr) { @@ -229,10 +231,11 @@ OwnedIdsWithName::gatherMatches(MatchingIdsWithName& dst, if (cur->matchesFilter(filterFlags, excludeFlagSet)) { dst.idvs_.push_back(*cur); anyAppended = true; + anyNonFunctions |= !cur->isParenfulFunction() && !cur->isMethodOrField(); } } - return anyAppended; + return LookupResult(/* found */ anyAppended, /* nonFunctions */ anyNonFunctions); } @@ -309,16 +312,16 @@ void MatchingIdsWithName::stringify(std::ostream& ss, } } -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) { auto search = declared.find(name); if (search != declared.end()) { return search->second.gatherMatches(result, filterFlags, excludeFlags); } - return false; + return LookupResult::empty(); } From c0a5d3025c91033c047f9ffd6d1f66acae242a61 Mon Sep 17 00:00:00 2001 From: Danila Fedorin Date: Mon, 23 Dec 2024 11:12:00 -0800 Subject: [PATCH 02/13] Add a method to check if an ID should be treated like a function For the purposes of stopping the search etc. Signed-off-by: Danila Fedorin --- frontend/include/chpl/resolution/scope-types.h | 4 ++++ frontend/lib/resolution/scope-queries.cpp | 4 ++-- frontend/lib/resolution/scope-types.cpp | 2 +- 3 files changed, 7 insertions(+), 3 deletions(-) diff --git a/frontend/include/chpl/resolution/scope-types.h b/frontend/include/chpl/resolution/scope-types.h index 8912fef6985a..b71f91d2d73d 100644 --- a/frontend/include/chpl/resolution/scope-types.h +++ b/frontend/include/chpl/resolution/scope-types.h @@ -234,6 +234,10 @@ class IdAndFlags { return (flags_ & TYPE) != 0; } + bool isFunctionLike() const { + 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. diff --git a/frontend/lib/resolution/scope-queries.cpp b/frontend/lib/resolution/scope-queries.cpp index 7314b1eee1a4..51ee5600785f 100644 --- a/frontend/lib/resolution/scope-queries.cpp +++ b/frontend/lib/resolution/scope-queries.cpp @@ -875,8 +875,8 @@ LookupResult LookupHelper::doLookupInImportsAndUses( auto idv = idAndFlagsForScopeSymbol(context, is.scope()); if (idv.matchesFilter(filterFlags, excludeFlagSet)) { result.append(idv); - found |= LookupResult(/* found */ true, - /* nonFunctions */ !idv.isParenfulFunction()); + found |= LookupResult(/* found */ true, + /* nonFunctions */ !idv.isFunctionLike()); if (trace) { ResultVisibilityTrace t; t.scope = cur->scope(); diff --git a/frontend/lib/resolution/scope-types.cpp b/frontend/lib/resolution/scope-types.cpp index cd2c33015aa8..52e70e44ca57 100644 --- a/frontend/lib/resolution/scope-types.cpp +++ b/frontend/lib/resolution/scope-types.cpp @@ -231,7 +231,7 @@ OwnedIdsWithName::gatherMatches(MatchingIdsWithName& dst, if (cur->matchesFilter(filterFlags, excludeFlagSet)) { dst.idvs_.push_back(*cur); anyAppended = true; - anyNonFunctions |= !cur->isParenfulFunction() && !cur->isMethodOrField(); + anyNonFunctions |= !cur->isFunctionLike(); } } From 80e4045f3259e42f0a64a83543fc71f54e089041 Mon Sep 17 00:00:00 2001 From: Danila Fedorin Date: Mon, 23 Dec 2024 11:12:33 -0800 Subject: [PATCH 03/13] Convert 'doneLooking' into a method on LookupHelper Signed-off-by: Danila Fedorin --- frontend/lib/resolution/scope-queries.cpp | 74 +++++++++++++++++------ 1 file changed, 57 insertions(+), 17 deletions(-) diff --git a/frontend/lib/resolution/scope-queries.cpp b/frontend/lib/resolution/scope-queries.cpp index 51ee5600785f..e8bec20fb9de 100644 --- a/frontend/lib/resolution/scope-queries.cpp +++ b/frontend/lib/resolution/scope-queries.cpp @@ -632,6 +632,8 @@ struct LookupHelper { CheckedScopes& checkedScopes; MatchingIdsWithName& result; bool& foundExternBlock; + bool& fnAndNonFnAtSameLevel; + int prevNumResults = 0; std::vector* traceCurPath = nullptr; std::vector* traceResult = nullptr; MatchingIdsWithName* shadowedResults = nullptr; @@ -645,6 +647,8 @@ struct LookupHelper { CheckedScopes& checkedScopes, MatchingIdsWithName& result, bool& foundExternBlock, + bool& fnAndNonFnAtSameLevel, + int prevNumResults, std::vector* traceCurPath, std::vector* traceResult, MatchingIdsWithName* shadowedResults, @@ -654,12 +658,27 @@ struct LookupHelper { receiverScopeHelper(receiverScopeHelper), checkedScopes(checkedScopes), result(result), foundExternBlock(foundExternBlock), + fnAndNonFnAtSameLevel(fnAndNonFnAtSameLevel), + prevNumResults(prevNumResults), traceCurPath(traceCurPath), traceResult(traceResult), shadowedResults(shadowedResults), traceShadowedResults(traceShadowedResults), allowCached(allowCached) { } + /* There are points at which lookups check if they can be "finished". + E.g., if we're performing a lookup with LOOKUP_INNERMOST, we check + if we found something after each shadow scope level of uses/imports. + This method is called each time we reach such a point, and determines + whether we are done with the lookup. + + For the purposes of tracking erroneous overloading cases (e.g., found + a function and variable with the same name at the same level), we also + track some mutable state (prevNumResults records the number of elements + in result at the point of the last call to this method). + */ + bool shouldFinishLookup(const LookupResult& got, bool onlyInnermost, bool stopNonFn); + LookupResult doLookupInImportsAndUses(const Scope* scope, const ResolvedVisibilityScope* cur, UniqueString name, @@ -695,6 +714,29 @@ struct LookupHelper { LookupConfig config); }; +bool LookupHelper::shouldFinishLookup(const LookupResult& got, bool onlyInnermost, bool stopNonFn) { + int itemsBefore = prevNumResults; + prevNumResults = result.numIds(); + + if (got && onlyInnermost) return true; + + if (got.nonFunctions() && stopNonFn) { + // check if we also found a function + for (int i = itemsBefore; i < result.numIds(); i++) { + auto& idv = result.idAndFlags(i); + if (idv.isFunctionLike()) { + fnAndNonFnAtSameLevel = true; + break; + } + } + + // we did find a variable, so we're done + return true; + } + + return false; +} + static const Scope* const& scopeForAutoModuleQuery(Context* context) { QUERY_BEGIN(scopeForAutoModuleQuery, context); @@ -1242,12 +1284,6 @@ static const IdAndFlags* getReservedIdentifier(UniqueString name) { return nullptr; } -static bool doneLooking(const LookupResult& got, bool onlyInnermost, bool stopNonFn) { - if (got && onlyInnermost) return true; - if (got.nonFunctions() && stopNonFn) return true; - return false; -} - // appends to result // // traceCurPath and traceResult support gathering additional information @@ -1432,7 +1468,7 @@ LookupResult LookupHelper::doLookupInScope(const Scope* scope, } } } - if (doneLooking(got, onlyInnermost, stopNonFn)) return got; + if (shouldFinishLookup(got, onlyInnermost, stopNonFn)) return got; } // now check shadow scope 1 (only relevant for 'private use') @@ -1460,7 +1496,7 @@ LookupResult LookupHelper::doLookupInScope(const Scope* scope, } got |= gotInSS1; - if (doneLooking(got, onlyInnermost, stopNonFn)) return got; + if (shouldFinishLookup(got, onlyInnermost, stopNonFn)) return got; } std::unordered_set* ignoreClausesForShadowScopeTwo = nullptr; @@ -1486,7 +1522,7 @@ LookupResult LookupHelper::doLookupInScope(const Scope* scope, } got |= gotInSS2; - if (doneLooking(got, onlyInnermost, stopNonFn)) return got; + if (shouldFinishLookup(got, onlyInnermost, stopNonFn)) return got; } // If we are at a method scope, consider receiver scopes now @@ -1497,7 +1533,7 @@ LookupResult LookupHelper::doLookupInScope(const Scope* scope, auto rcvScopes = receiverScopeHelper->methodLookupForMethodId(context, scope->id()); got |= doLookupInReceiverScopes(scope, rcvScopes, name, config); - if (doneLooking(got, onlyInnermost, stopNonFn)) return got; + if (shouldFinishLookup(got, onlyInnermost, stopNonFn)) return got; } // consider the outer scopes due to nesting @@ -1543,7 +1579,7 @@ LookupResult LookupHelper::doLookupInScope(const Scope* scope, traceCurPath->pop_back(); } - if (doneLooking(got, onlyInnermost, stopNonFn)) return got; + if (shouldFinishLookup(got, onlyInnermost, stopNonFn)) return got; // and then search only considering receiver scopes // as if the receiver scope were just outside of this scope. @@ -1551,7 +1587,7 @@ LookupResult LookupHelper::doLookupInScope(const Scope* scope, auto rcvScopes = receiverScopeHelper->methodLookupForMethodId(context, cur->id()); got |= doLookupInReceiverScopes(scope, rcvScopes, name, newConfig); - if (doneLooking(got, onlyInnermost, stopNonFn)) return got; + if (shouldFinishLookup(got, onlyInnermost, stopNonFn)) return got; } } } @@ -1570,7 +1606,7 @@ LookupResult LookupHelper::doLookupInScope(const Scope* scope, traceCurPath->pop_back(); } - if (doneLooking(got, onlyInnermost, stopNonFn)) return got; + if (shouldFinishLookup(got, onlyInnermost, stopNonFn)) return got; } if (!goPastModules && (reachedModule || isModule(scope->tag()))) { @@ -1594,7 +1630,7 @@ LookupResult LookupHelper::doLookupInScope(const Scope* scope, traceCurPath->pop_back(); } - if (doneLooking(got, onlyInnermost, stopNonFn)) return got; + if (shouldFinishLookup(got, onlyInnermost, stopNonFn)) return got; } // check also in the root scope if this isn't already the root scope @@ -1620,13 +1656,13 @@ LookupResult LookupHelper::doLookupInScope(const Scope* scope, traceCurPath->pop_back(); } - if (doneLooking(got, onlyInnermost, stopNonFn)) return got; + if (shouldFinishLookup(got, onlyInnermost, stopNonFn)) return got; } } if (checkToplevel) { got |= doLookupInToplevelModules(scope, name); - if (doneLooking(got, onlyInnermost, stopNonFn)) return got; + if (shouldFinishLookup(got, onlyInnermost, stopNonFn)) return got; } // If LOOKUP_EXTERN_BLOCKS is set, and this scope has an extern block, @@ -1696,11 +1732,15 @@ helpLookupInScope(Context* context, bool stopNonFn = (config & LOOKUP_STOP_NON_FN) != 0; bool checkExternBlocks = (config & LOOKUP_EXTERN_BLOCKS) != 0; bool foundExternBlock = false; + bool fnAndNonFnAtSameLevel = false; + int prevNumResults = 0; CheckedScopes savedCheckedScopes; auto helper = LookupHelper(context, resolving, receiverScopeHelper, checkedScopes, result, foundExternBlock, + fnAndNonFnAtSameLevel, + prevNumResults, traceCurPath, traceResult, shadowed, traceShadowed, allowCached && traceResult==nullptr); @@ -1721,7 +1761,7 @@ helpLookupInScope(Context* context, // When resolving a Dot expression like myRecord.foo, we might not be inside // of a method at all, but we should still search the definition point // of the relevant record. - if (methodLookupHelper != nullptr && !(doneLooking(got, onlyInnermost, stopNonFn))) { + if (methodLookupHelper != nullptr && !(helper.shouldFinishLookup(got, onlyInnermost, stopNonFn))) { got |= helper.doLookupInReceiverScopes(scope, methodLookupHelper, name, config); } From 5a6cf03d04199ccebab04588183050e1a7afeb36 Mon Sep 17 00:00:00 2001 From: Danila Fedorin Date: Mon, 23 Dec 2024 12:00:06 -0800 Subject: [PATCH 04/13] Comment 'isFunctionLike' Signed-off-by: Danila Fedorin --- frontend/include/chpl/resolution/scope-types.h | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/frontend/include/chpl/resolution/scope-types.h b/frontend/include/chpl/resolution/scope-types.h index b71f91d2d73d..0dae81a4fb11 100644 --- a/frontend/include/chpl/resolution/scope-types.h +++ b/frontend/include/chpl/resolution/scope-types.h @@ -235,6 +235,11 @@ class IdAndFlags { } 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(); } From be8c73aa66905f68344a2bb26ddad576b5f69c07 Mon Sep 17 00:00:00 2001 From: Danila Fedorin Date: Mon, 23 Dec 2024 12:00:38 -0800 Subject: [PATCH 05/13] Note function-variable ambiguity in MatchingIds Signed-off-by: Danila Fedorin --- frontend/include/chpl/resolution/scope-types.h | 12 ++++++++++++ frontend/lib/resolution/Resolver.cpp | 9 +++++++++ frontend/lib/resolution/scope-queries.cpp | 7 +------ 3 files changed, 22 insertions(+), 6 deletions(-) diff --git a/frontend/include/chpl/resolution/scope-types.h b/frontend/include/chpl/resolution/scope-types.h index 0dae81a4fb11..95064c2144c0 100644 --- a/frontend/include/chpl/resolution/scope-types.h +++ b/frontend/include/chpl/resolution/scope-types.h @@ -409,6 +409,7 @@ class MatchingIdsWithName { private: llvm::SmallVector idvs_; + bool encounteredFnNonFnConflict_ = false; /** Construct a MatchingIdsWithName referring to the same IDs as the passed OwnedIdsWithName. */ @@ -472,6 +473,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)); diff --git a/frontend/lib/resolution/Resolver.cpp b/frontend/lib/resolution/Resolver.cpp index f648b661edf4..77e124b88a20 100644 --- a/frontend/lib/resolution/Resolver.cpp +++ b/frontend/lib/resolution/Resolver.cpp @@ -3414,6 +3414,15 @@ void Resolver::resolveIdentifier(const Identifier* ident) { auto parenlessInfo = ParenlessOverloadInfo(); auto ids = lookupIdentifier(ident, resolvingCalledIdent, parenlessInfo); + // 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 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 diff --git a/frontend/lib/resolution/scope-queries.cpp b/frontend/lib/resolution/scope-queries.cpp index e8bec20fb9de..ac862e576de4 100644 --- a/frontend/lib/resolution/scope-queries.cpp +++ b/frontend/lib/resolution/scope-queries.cpp @@ -632,7 +632,6 @@ struct LookupHelper { CheckedScopes& checkedScopes; MatchingIdsWithName& result; bool& foundExternBlock; - bool& fnAndNonFnAtSameLevel; int prevNumResults = 0; std::vector* traceCurPath = nullptr; std::vector* traceResult = nullptr; @@ -647,7 +646,6 @@ struct LookupHelper { CheckedScopes& checkedScopes, MatchingIdsWithName& result, bool& foundExternBlock, - bool& fnAndNonFnAtSameLevel, int prevNumResults, std::vector* traceCurPath, std::vector* traceResult, @@ -658,7 +656,6 @@ struct LookupHelper { receiverScopeHelper(receiverScopeHelper), checkedScopes(checkedScopes), result(result), foundExternBlock(foundExternBlock), - fnAndNonFnAtSameLevel(fnAndNonFnAtSameLevel), prevNumResults(prevNumResults), traceCurPath(traceCurPath), traceResult(traceResult), shadowedResults(shadowedResults), @@ -725,7 +722,7 @@ bool LookupHelper::shouldFinishLookup(const LookupResult& got, bool onlyInnermos for (int i = itemsBefore; i < result.numIds(); i++) { auto& idv = result.idAndFlags(i); if (idv.isFunctionLike()) { - fnAndNonFnAtSameLevel = true; + result.noteFnNonFnConflict(); break; } } @@ -1732,14 +1729,12 @@ helpLookupInScope(Context* context, bool stopNonFn = (config & LOOKUP_STOP_NON_FN) != 0; bool checkExternBlocks = (config & LOOKUP_EXTERN_BLOCKS) != 0; bool foundExternBlock = false; - bool fnAndNonFnAtSameLevel = false; int prevNumResults = 0; CheckedScopes savedCheckedScopes; auto helper = LookupHelper(context, resolving, receiverScopeHelper, checkedScopes, result, foundExternBlock, - fnAndNonFnAtSameLevel, prevNumResults, traceCurPath, traceResult, shadowed, traceShadowed, From 362b55b01d9a0101dc52ca03c97e6c599c02a2de Mon Sep 17 00:00:00 2001 From: Danila Fedorin Date: Mon, 23 Dec 2024 12:00:57 -0800 Subject: [PATCH 06/13] Detect ErroneousType as receiver in CallInfo::create Signed-off-by: Danila Fedorin --- frontend/lib/resolution/resolution-types.cpp | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/frontend/lib/resolution/resolution-types.cpp b/frontend/lib/resolution/resolution-types.cpp index 04452a83c245..96f1fdc63e23 100644 --- a/frontend/lib/resolution/resolution-types.cpp +++ b/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 From e273802ec0d81c9e01708cbd6bc9b8f2d2a24ad7 Mon Sep 17 00:00:00 2001 From: Danila Fedorin Date: Mon, 23 Dec 2024 12:20:27 -0800 Subject: [PATCH 07/13] Resolve Anna's TODO using the new logic Signed-off-by: Danila Fedorin --- frontend/lib/resolution/Resolver.cpp | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/frontend/lib/resolution/Resolver.cpp b/frontend/lib/resolution/Resolver.cpp index 77e124b88a20..1db26ac50c9d 100644 --- a/frontend/lib/resolution/Resolver.cpp +++ b/frontend/lib/resolution/Resolver.cpp @@ -3423,10 +3423,9 @@ void Resolver::resolveIdentifier(const Identifier* ident) { return; } - // 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 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) { @@ -3436,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; } } From 1a73a7eef77e6fac7854d0cd859c442b3ab0b731 Mon Sep 17 00:00:00 2001 From: Danila Fedorin Date: Mon, 23 Dec 2024 12:23:40 -0800 Subject: [PATCH 08/13] Avoid 'non-function where function was expected' errors Signed-off-by: Danila Fedorin --- frontend/lib/resolution/resolution-queries.cpp | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/frontend/lib/resolution/resolution-queries.cpp b/frontend/lib/resolution/resolution-queries.cpp index 3f6934913af8..c02048548d28 100644 --- a/frontend/lib/resolution/resolution-queries.cpp +++ b/frontend/lib/resolution/resolution-queries.cpp @@ -4109,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; } From 5cd6a2b1dc5a4bbe41e3a3441c3d8957ed7b998f Mon Sep 17 00:00:00 2001 From: Danila Fedorin Date: Thu, 2 Jan 2025 09:52:39 -0800 Subject: [PATCH 09/13] Lock down behavior in new tests Signed-off-by: Danila Fedorin --- frontend/test/resolution/testResolve.cpp | 81 ++++++++++++++++++++++++ 1 file changed, 81 insertions(+) diff --git a/frontend/test/resolution/testResolve.cpp b/frontend/test/resolution/testResolve.cpp index 7ae6419ae3af..bf7b835c3c77 100644 --- a/frontend/test/resolution/testResolve.cpp +++ b/frontend/test/resolution/testResolve.cpp @@ -1739,6 +1739,83 @@ static void testInfiniteCycleBug() { std::ignore = resolveQualifiedTypeOfX(context, program1); } +// a callable foraml (like a tuple) is preferred to functions in outer +// scopes. +static void testFormalFunctionShadowing() { + std::string program = + R"""( + record R { proc this(x: int) do return 42; } + + proc foo(x) do return 1.0; + proc foo(x: int) do return new R(); + proc bar(foo: R) do return foo(0); + + var x = bar(new R()); + )"""; + + Context ctx; + Context* context = &ctx; + ErrorGuard guard(context); + + auto t = resolveTypeOfXInit(context, program); + CHPL_ASSERT(!t.isUnknownOrErroneous()); + CHPL_ASSERT(t.type()->isIntType()); +} + +// a callable foraml (like a tuple) interrupts the search for functions as +// an optimization for "distance" (any functions beyond the callable formal +// are further away than any functions we've already found). +static void testFunctionFormalShadowing() { + std::string program = + R"""( + record R { proc this(x: int) do return 42; } + + proc foo(x: int) do return new R(); + proc bar(foo: R) { + proc foo(x) do return 1.0; + return foo(0); + } + + var x = bar(new R()); + )"""; + + Context ctx; + Context* context = &ctx; + ErrorGuard guard(context); + + auto t = resolveTypeOfXInit(context, program); + CHPL_ASSERT(!t.isUnknownOrErroneous()); + CHPL_ASSERT(t.type()->isRealType()); +} + +// Today, we don't perform overload selection for callable objects. Instead, +// expect an error to be issued. +static void testCallableAmbiguity() { + std::string program = + R"""( + module Lib { + record R { + proc this() do return 42; + } + } + module M1 { use Lib; var x: R; } + module M2 { use Lib; var x: R; } + module M3 { + use M1; + use M2; + + var y = x(0); + } + )"""; + + Context ctx; + Context* context = &ctx; + ErrorGuard guard(context); + + std::ignore = resolveTypesOfVariables(context, program, { "y" }); + assert(guard.realizeErrors()); +} + int main() { test1(); test2(); @@ -1770,5 +1847,9 @@ int main() { testInfiniteCycleBug(); + testFormalFunctionShadowing(); + testFunctionFormalShadowing(); + testCallableAmbiguity(); + return 0; } From e45a393d1874d7e9b11a970d785db656fd52dd5e Mon Sep 17 00:00:00 2001 From: Danila Fedorin Date: Thu, 2 Jan 2025 10:51:48 -0800 Subject: [PATCH 10/13] Ensure all fields are used in hash functions etc. Signed-off-by: Danila Fedorin --- frontend/include/chpl/resolution/scope-types.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/frontend/include/chpl/resolution/scope-types.h b/frontend/include/chpl/resolution/scope-types.h index 95064c2144c0..68e7e4d33b37 100644 --- a/frontend/include/chpl/resolution/scope-types.h +++ b/frontend/include/chpl/resolution/scope-types.h @@ -554,7 +554,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); @@ -565,6 +566,7 @@ class MatchingIdsWithName { for (const auto& x : idvs_) { ret = hash_combine(ret, chpl::hash(x)); } + ret = hash_combine(ret, encounteredFnNonFnConflict_); return ret; } From 41f491381df599ba4c8971cf5028f6ef7da0471b Mon Sep 17 00:00:00 2001 From: Danila Fedorin Date: Tue, 7 Jan 2025 14:42:12 -0800 Subject: [PATCH 11/13] Incorporate reviewer feedback Signed-off-by: Danila Fedorin --- .../include/chpl/resolution/scope-types.h | 4 ++- frontend/lib/resolution/scope-queries.cpp | 32 ++++++++++--------- frontend/test/resolution/testResolve.cpp | 4 +-- 3 files changed, 22 insertions(+), 18 deletions(-) diff --git a/frontend/include/chpl/resolution/scope-types.h b/frontend/include/chpl/resolution/scope-types.h index 68e7e4d33b37..6e550338aa70 100644 --- a/frontend/include/chpl/resolution/scope-types.h +++ b/frontend/include/chpl/resolution/scope-types.h @@ -285,7 +285,9 @@ class LookupResult { public: LookupResult(bool found, bool nonFunctions) - : found_(found), nonFunctions_(nonFunctions) {} + : found_(found), nonFunctions_(nonFunctions) { + CHPL_ASSERT(!nonFunctions_ || found); + } static LookupResult empty() { return {false, false}; } diff --git a/frontend/lib/resolution/scope-queries.cpp b/frontend/lib/resolution/scope-queries.cpp index ac862e576de4..b2cd442cb86e 100644 --- a/frontend/lib/resolution/scope-queries.cpp +++ b/frontend/lib/resolution/scope-queries.cpp @@ -674,7 +674,7 @@ struct LookupHelper { track some mutable state (prevNumResults records the number of elements in result at the point of the last call to this method). */ - bool shouldFinishLookup(const LookupResult& got, bool onlyInnermost, bool stopNonFn); + bool shouldStopLookup(const LookupResult& got, bool onlyInnermost, bool stopNonFn); LookupResult doLookupInImportsAndUses(const Scope* scope, const ResolvedVisibilityScope* cur, @@ -711,7 +711,7 @@ struct LookupHelper { LookupConfig config); }; -bool LookupHelper::shouldFinishLookup(const LookupResult& got, bool onlyInnermost, bool stopNonFn) { +bool LookupHelper::shouldStopLookup(const LookupResult& got, bool onlyInnermost, bool stopNonFn) { int itemsBefore = prevNumResults; prevNumResults = result.numIds(); @@ -1164,7 +1164,7 @@ LookupResult LookupHelper::doLookupInReceiverScopes( if (cur != i) { result.idAndFlags(cur) = idv; } - nonFunctions |= !idv.isParenfulFunction(); + nonFunctions |= !idv.isFunctionLike(); cur++; } } @@ -1234,7 +1234,9 @@ LookupResult LookupHelper::doLookupInExternBlocks(const Scope* scope, } } - return LookupResult(found, /* nonFunctions; might be a lie; see above */ true); + /* might be a lie, because we're lying about isParenfulFunction above */ + bool nonFunctions = true; + return LookupResult(found, nonFunctions); } // Returns the IdAndFlags for a common builtin, or nullptr. @@ -1465,7 +1467,7 @@ LookupResult LookupHelper::doLookupInScope(const Scope* scope, } } } - if (shouldFinishLookup(got, onlyInnermost, stopNonFn)) return got; + if (shouldStopLookup(got, onlyInnermost, stopNonFn)) return got; } // now check shadow scope 1 (only relevant for 'private use') @@ -1493,7 +1495,7 @@ LookupResult LookupHelper::doLookupInScope(const Scope* scope, } got |= gotInSS1; - if (shouldFinishLookup(got, onlyInnermost, stopNonFn)) return got; + if (shouldStopLookup(got, onlyInnermost, stopNonFn)) return got; } std::unordered_set* ignoreClausesForShadowScopeTwo = nullptr; @@ -1519,7 +1521,7 @@ LookupResult LookupHelper::doLookupInScope(const Scope* scope, } got |= gotInSS2; - if (shouldFinishLookup(got, onlyInnermost, stopNonFn)) return got; + if (shouldStopLookup(got, onlyInnermost, stopNonFn)) return got; } // If we are at a method scope, consider receiver scopes now @@ -1530,7 +1532,7 @@ LookupResult LookupHelper::doLookupInScope(const Scope* scope, auto rcvScopes = receiverScopeHelper->methodLookupForMethodId(context, scope->id()); got |= doLookupInReceiverScopes(scope, rcvScopes, name, config); - if (shouldFinishLookup(got, onlyInnermost, stopNonFn)) return got; + if (shouldStopLookup(got, onlyInnermost, stopNonFn)) return got; } // consider the outer scopes due to nesting @@ -1576,7 +1578,7 @@ LookupResult LookupHelper::doLookupInScope(const Scope* scope, traceCurPath->pop_back(); } - if (shouldFinishLookup(got, onlyInnermost, stopNonFn)) return got; + if (shouldStopLookup(got, onlyInnermost, stopNonFn)) return got; // and then search only considering receiver scopes // as if the receiver scope were just outside of this scope. @@ -1584,7 +1586,7 @@ LookupResult LookupHelper::doLookupInScope(const Scope* scope, auto rcvScopes = receiverScopeHelper->methodLookupForMethodId(context, cur->id()); got |= doLookupInReceiverScopes(scope, rcvScopes, name, newConfig); - if (shouldFinishLookup(got, onlyInnermost, stopNonFn)) return got; + if (shouldStopLookup(got, onlyInnermost, stopNonFn)) return got; } } } @@ -1603,7 +1605,7 @@ LookupResult LookupHelper::doLookupInScope(const Scope* scope, traceCurPath->pop_back(); } - if (shouldFinishLookup(got, onlyInnermost, stopNonFn)) return got; + if (shouldStopLookup(got, onlyInnermost, stopNonFn)) return got; } if (!goPastModules && (reachedModule || isModule(scope->tag()))) { @@ -1627,7 +1629,7 @@ LookupResult LookupHelper::doLookupInScope(const Scope* scope, traceCurPath->pop_back(); } - if (shouldFinishLookup(got, onlyInnermost, stopNonFn)) return got; + if (shouldStopLookup(got, onlyInnermost, stopNonFn)) return got; } // check also in the root scope if this isn't already the root scope @@ -1653,13 +1655,13 @@ LookupResult LookupHelper::doLookupInScope(const Scope* scope, traceCurPath->pop_back(); } - if (shouldFinishLookup(got, onlyInnermost, stopNonFn)) return got; + if (shouldStopLookup(got, onlyInnermost, stopNonFn)) return got; } } if (checkToplevel) { got |= doLookupInToplevelModules(scope, name); - if (shouldFinishLookup(got, onlyInnermost, stopNonFn)) return got; + if (shouldStopLookup(got, onlyInnermost, stopNonFn)) return got; } // If LOOKUP_EXTERN_BLOCKS is set, and this scope has an extern block, @@ -1756,7 +1758,7 @@ helpLookupInScope(Context* context, // When resolving a Dot expression like myRecord.foo, we might not be inside // of a method at all, but we should still search the definition point // of the relevant record. - if (methodLookupHelper != nullptr && !(helper.shouldFinishLookup(got, onlyInnermost, stopNonFn))) { + if (methodLookupHelper != nullptr && !(helper.shouldStopLookup(got, onlyInnermost, stopNonFn))) { got |= helper.doLookupInReceiverScopes(scope, methodLookupHelper, name, config); } diff --git a/frontend/test/resolution/testResolve.cpp b/frontend/test/resolution/testResolve.cpp index bf7b835c3c77..9275588ab654 100644 --- a/frontend/test/resolution/testResolve.cpp +++ b/frontend/test/resolution/testResolve.cpp @@ -1739,7 +1739,7 @@ static void testInfiniteCycleBug() { std::ignore = resolveQualifiedTypeOfX(context, program1); } -// a callable foraml (like a tuple) is preferred to functions in outer +// a callable formal (like a tuple) is preferred to functions in outer // scopes. static void testFormalFunctionShadowing() { std::string program = @@ -1762,7 +1762,7 @@ static void testFormalFunctionShadowing() { CHPL_ASSERT(t.type()->isIntType()); } -// a callable foraml (like a tuple) interrupts the search for functions as +// a callable formal (like a tuple) interrupts the search for functions as // an optimization for "distance" (any functions beyond the callable formal // are further away than any functions we've already found). static void testFunctionFormalShadowing() { From 6982655467e815a827345e79a49872e8abe6bed7 Mon Sep 17 00:00:00 2001 From: Danila Fedorin Date: Tue, 7 Jan 2025 15:40:24 -0800 Subject: [PATCH 12/13] Properly detect 'multiple variables' case Signed-off-by: Danila Fedorin --- frontend/lib/resolution/Resolver.cpp | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/frontend/lib/resolution/Resolver.cpp b/frontend/lib/resolution/Resolver.cpp index 1db26ac50c9d..b663c87df72d 100644 --- a/frontend/lib/resolution/Resolver.cpp +++ b/frontend/lib/resolution/Resolver.cpp @@ -3428,9 +3428,8 @@ void Resolver::resolveIdentifier(const Identifier* ident) { // in the same place. if (resolvingCalledIdent && ids.numIds() > 1) { bool onlyVars = true; - for (auto idIt = ids.begin(); idIt != ids.end(); ++idIt) { - if (!parsing::idToAst(context, idIt.curIdAndFlags().id()) - ->isVarLikeDecl()) { + for (int i = 0; i < ids.numIds(); i++) { + if (ids.idAndFlags(i).isFunctionLike() ){ onlyVars = false; break; } From 0406158d44df9c7bb9894182d9785e2c4249a28c Mon Sep 17 00:00:00 2001 From: Danila Fedorin Date: Tue, 7 Jan 2025 16:03:29 -0800 Subject: [PATCH 13/13] Fix broken tests Signed-off-by: Danila Fedorin --- test/deprecated/dmapType.good | 2 +- .../redefinition/parenless-and-parenful-fns.1.good | 1 + .../redefinition/parenless-and-parenful-fns.2.good | 3 +++ test/functions/bradc/resolution/arrayVsFn.good | 1 + 4 files changed, 6 insertions(+), 1 deletion(-) diff --git a/test/deprecated/dmapType.good b/test/deprecated/dmapType.good index a2dc08019fe9..0d9e708009f2 100644 --- a/test/deprecated/dmapType.good +++ b/test/deprecated/dmapType.good @@ -19,11 +19,11 @@ dmapType.chpl:72: warning: 'BlockCyclic' is deprecated, please use 'blockCycDist dmapType.chpl:72: warning: 'BlockCyclic' is deprecated, please use 'blockCycDist' instead dmapType.chpl:84: warning: 'DimensionalDist2D' is deprecated, please use 'dimensionalDist2D' instead dmapType.chpl:85: warning: 'DimensionalDist2D' is deprecated, please use 'dimensionalDist2D' instead +dmapType.chpl:92: warning: 'DimensionalDist2D' is deprecated, please use 'dimensionalDist2D' instead dmapType.chpl:93: warning: 'DimensionalDist2D' is deprecated, please use 'dimensionalDist2D' instead dmapType.chpl:99: warning: 'Hashed' is deprecated, please use 'hashedDist' instead dmapType.chpl:103: warning: 'Hashed' is deprecated, please use 'hashedDist' instead dmapType.chpl:103: warning: 'Hashed' is deprecated, please use 'hashedDist' instead -dmapType.chpl:92: warning: 'DimensionalDist2D' is deprecated, please use 'dimensionalDist2D' instead dmapType.chpl:4: warning: The use of 'dmap' is deprecated for this distribution; please replace 'new dmap(new ())' with 'new ()' dmapType.chpl:8: warning: The use of 'dmap' is deprecated for this distribution; please replace 'dmap(())' with '()' dmapType.chpl:14: warning: The use of 'dmap' is deprecated for this distribution; please replace 'new dmap(new ())' with 'new ()' diff --git a/test/errors/scope-resolution/redefinition/parenless-and-parenful-fns.1.good b/test/errors/scope-resolution/redefinition/parenless-and-parenful-fns.1.good index 33a57487c0a9..cb77699237bc 100644 --- a/test/errors/scope-resolution/redefinition/parenless-and-parenful-fns.1.good +++ b/test/errors/scope-resolution/redefinition/parenless-and-parenful-fns.1.good @@ -1,3 +1,4 @@ parenless-and-parenful-fns.chpl:1: In module 'M': parenless-and-parenful-fns.chpl:2: error: 'foo' has multiple definitions parenless-and-parenful-fns.chpl:3: note: redefined here +parenless-and-parenful-fns.chpl:5: error: ambiguity between function and non-function at the same scope level diff --git a/test/errors/scope-resolution/redefinition/parenless-and-parenful-fns.2.good b/test/errors/scope-resolution/redefinition/parenless-and-parenful-fns.2.good index 1b95d837bc06..6bb961713be0 100644 --- a/test/errors/scope-resolution/redefinition/parenless-and-parenful-fns.2.good +++ b/test/errors/scope-resolution/redefinition/parenless-and-parenful-fns.2.good @@ -11,3 +11,6 @@ | ⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺⎺ | +─── error in parenless-and-parenful-fns.chpl:5 ─── + Ambiguity between function and non-function at the same scope level + diff --git a/test/functions/bradc/resolution/arrayVsFn.good b/test/functions/bradc/resolution/arrayVsFn.good index 1a172c2ff763..4d33b713c14f 100644 --- a/test/functions/bradc/resolution/arrayVsFn.good +++ b/test/functions/bradc/resolution/arrayVsFn.good @@ -1,2 +1,3 @@ arrayVsFn.chpl:1: error: 'A' has multiple definitions arrayVsFn.chpl:3: note: redefined here +arrayVsFn.chpl:8: error: ambiguity between function and non-function at the same scope level