Warn for surprising shadowing (#23511)

Resolves Cray/chapel-private#5220
Resolves #19780
Resolves #19782

This PR introduces a warning for cases where symbol shadowing might be
surprising. In particular, the warning is focused on cases where a `use`
statement brings in a symbol that conflicts with a more obviously
available symbol in an outer scope. This warning is motivated by the
example in #14014 and the the hijacking example in
#14014 (comment).
Additionally, in review of PR #19306, it was requested to make a warning
about the case that a `public use` symbol shadows a `private use` one
and this PR implements that request.

The new warning only applies to scope lookups for the innermost match;
that means that the new warning does not apply to functions and calls.

What is the logic for the warning?
* when a symbol is available through a bulk import from a `use`
statement, warn if it's defined in another available scope (another
shadow scope or outer scope) except:
* do not warn for locally defined symbols that shadow a symbol from a
private use (shadow scope 1 or 2)
* do not warn for conflicts between a name brought in by a private use
and the module name brought in by the same private use

The warning is implemented to consider all parent scopes and the names
of toplevel modules, if we are considering a `use`/`import`. Considering
the parent scopes seems natural to me but the toplevel module part is
necessary to give the warning in the case of
#14014 (comment)
. We considered avoiding warning in the case that the toplevel module in
question is in a different file but opted not to do that.

Since the warning is (for the most part) a superset of the warning for
hidden formals added in PR #21614, this PR removes the hidden formal
warning.
* The case where a function is found through a `use` statement but has
the same name as a formal is no longer warned about because the new
warning does not apply to function lookups
(test/functions/iterators/recursive/recursive-iter-findfilesfailure)
* The case of a local variable that shadows something from a `use`
statement that in turn shadows a formal is no longer warned about, by
the design of the new warning
(test/modules/diten/testIfModuleShadowsLocal)

Future Work:
 * consider a similar error for function calls

Reviewed by @DanilaFe and @lydia-duncan - thanks!

- [x] full comm=none testing

compiler/Makefile

@@ -167,10 +167,11 @@ frontend: FORCE $(CHPL_CONFIG_CHECK) $(COMPILER_BUILD)
 test-frontend: FORCE $(CHPL_CONFIG_CHECK) $(COMPILER_BUILD)
 	@echo "Making and running the frontend tests..."
 	@cd $(COMPILER_BUILD) && $(CMAKE) $(CHPL_MAKE_HOME) $(CMAKE_FLAGS_NO_NDEBUG) && $(MAKE) tests
-	JOBSFLAG=`echo "$$MAKEFLAGS" | sed -n 's/.*\(-j\|--jobs=\) *\([0-9][0-9]*\).*/-j\2/p'` ; \
-	  cd $(COMPILER_BUILD)/frontend/test && ctest $$JOBSFLAG . ;
 	@echo "Making symbolic link to frontend tests in build/frontend-test"
 	@cd ../build && rm -f frontend-test && ln -s $(COMPILER_BUILD)/frontend/test frontend-test
+	JOBSFLAG=`echo "$$MAKEFLAGS" | sed -n 's/.*\(-j\|--jobs=\) *\([0-9][0-9]*\).*/-j\2/p'` ; \
+	  cd $(COMPILER_BUILD)/frontend/test && ctest $$JOBSFLAG . ;
+	@echo "frontend tests are available in build/frontend-test"
 
 COPY_IF_DIFFERENT = $(CHPL_MAKE_PYTHON) $(CHPL_MAKE_HOME)/util/config/update-if-different --quiet --copy
 

frontend/include/chpl/framework/ErrorWriter.h

@@ -215,11 +215,11 @@ class ErrorWriterBase {
     BRIEF,
   };
  protected:
-  Context* context;
+  Context* context_; // note: this can sometimes be null
   OutputFormat outputFormat_;
 
   ErrorWriterBase(Context* context, OutputFormat outputFormat)
-    : context(context), outputFormat_(outputFormat) {}
+    : context_(context), outputFormat_(outputFormat) {}
 
   /**
     Makes tweaks to an error string depending on output format.
@@ -235,7 +235,7 @@ class ErrorWriterBase {
   void writeHeading(ErrorBase::Kind kind, ErrorType type, const uast::AstNode* ast, const std::string& message);
   template <typename T>
   void writeHeading(ErrorBase::Kind kind, ErrorType type, errordetail::LocationOnly<T> t, const std::string& message) {
-    writeHeading(kind, type, errordetail::locate(context, t.t), message);
+    writeHeading(kind, type, errordetail::locate(context_, t.t), message);
   }
 
   /**
@@ -255,7 +255,7 @@ class ErrorWriterBase {
   void writeNote(const uast::AstNode* ast, const std::string& message);
   template <typename T>
   void writeNote(errordetail::LocationOnly<T> t, const std::string& message) {
-    writeNote(errordetail::locate(context, t.t), message);
+    writeNote(errordetail::locate(context_, t.t), message);
   }
 
   /**
@@ -271,7 +271,7 @@ class ErrorWriterBase {
     std::ostringstream oss;
     auto write = [&](auto t) {
       errordetail::Writer<decltype(t)> writer;
-      writer(context, oss, t);
+      writer(context_, oss, t);
     };
 
     auto dummy = { (write(ts), 0)..., };
@@ -355,9 +355,9 @@ class ErrorWriterBase {
             const std::vector<LocHighlight>& toHighlight = {}) {
     std::vector<Location> ids(toHighlight.size());
     std::transform(toHighlight.cbegin(), toHighlight.cend(), ids.begin(), [&](auto node) {
-      return errordetail::locate(context, node);
+      return errordetail::locate(context_, node);
     });
-    writeCode(errordetail::locate(context, place), ids);
+    writeCode(errordetail::locate(context_, place), ids);
   }
 
   /**

frontend/include/chpl/resolution/resolution-error-classes-list.h

@@ -40,7 +40,6 @@ ERROR_CLASS(AsWithUseExcept, const uast::Use*, const uast::As*)
 WARNING_CLASS(Deprecation, std::string, const uast::AstNode*, const uast::NamedDecl*)
 ERROR_CLASS(DotExprInUseImport, const uast::VisibilityClause*, const uast::VisibilityClause::LimitationKind, const uast::Dot*)
 ERROR_CLASS(ExternCCompilation, ID, std::vector<std::pair<Location, std::string>>)
-WARNING_CLASS(HiddenFormal, const uast::Formal*, resolution::BorrowedIdsWithName, resolution::ResultVisibilityTrace)
 ERROR_CLASS(IfVarNonClassType, const uast::Conditional*, types::QualifiedType)
 WARNING_CLASS(ImplicitFileModule, const uast::AstNode*, const uast::Module*, const uast::Module*)
 ERROR_CLASS(IncompatibleIfBranches, const uast::Conditional*, types::QualifiedType, types::QualifiedType)
@@ -60,6 +59,7 @@ ERROR_CLASS(NestedClassFieldRef, const uast::AggregateDecl*, const uast::Aggrega
 ERROR_CLASS(NonIterable, const uast::AstNode*, const uast::AstNode*, types::QualifiedType)
 ERROR_CLASS(NotInModule, const uast::Dot*, ID, UniqueString, ID, bool)
 ERROR_CLASS(PhaseTwoInitMarker, const uast::AstNode*, std::vector<ID>)
+WARNING_CLASS(PotentiallySurprisingShadowing, ID, UniqueString, std::vector<resolution::BorrowedIdsWithName>, std::vector<resolution::ResultVisibilityTrace>, std::vector<resolution::BorrowedIdsWithName>, std::vector<resolution::ResultVisibilityTrace>)
 ERROR_CLASS(PrivateToPublicInclude, const uast::Include*, const uast::Module*)
 ERROR_CLASS(ProcDefExplicitAnonFormal, const uast::Function*, const uast::Formal*)
 ERROR_CLASS(ProcTypeUnannotatedFormal, const uast::FunctionSignature*, const uast::AnonFormal*)

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

@@ -66,6 +66,20 @@ namespace resolution {
                     UniqueString name,
                     LookupConfig config);
 
+  /**
+    Same as lookupNameInScope but can produce warnings based on
+    the ID passed in.
+   */
+
+  std::vector<BorrowedIdsWithName>
+  lookupNameInScopeWithWarnings(Context* context,
+                                const Scope* scope,
+                                llvm::ArrayRef<const Scope*> receiverScopes,
+                                UniqueString name,
+                                LookupConfig config,
+                                ID idForWarnings);
+
+
   /**
     Same as lookupNameInScope but traces how each symbol was found,
     for error messages.

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

@@ -1183,6 +1183,8 @@ struct ResultVisibilityTrace {
     ID visibilityClauseId;
     VisibilityStmtKind visibilityStmtKind = VIS_USE;
     UniqueString renameFrom;
+    UniqueString usedImportedThingName;
+    const Scope* usedImportedScope = nullptr;
     bool fromUseImport = false;
 
     // this indicates a method receiver scope
@@ -1203,6 +1205,8 @@ struct ResultVisibilityTrace {
              visibilityClauseId == other.visibilityClauseId &&
              visibilityStmtKind == other.visibilityStmtKind &&
              renameFrom == other.renameFrom &&
+             usedImportedThingName == other.usedImportedThingName &&
+             usedImportedScope == other.usedImportedScope &&
              fromUseImport == other.fromUseImport &&
              methodReceiverScope == other.methodReceiverScope &&
              parentScope == other.parentScope &&
@@ -1217,6 +1221,8 @@ struct ResultVisibilityTrace {
     void mark(Context* context) const {
       context->markPointer(resolvedVisibilityScope);
       renameFrom.mark(context);
+      usedImportedThingName.mark(context);
+      context->markPointer(usedImportedScope);
       visibilityClauseId.mark(context);
       context->markPointer(methodReceiverScope);
       context->markPointer(parentScope);

frontend/lib/framework/ErrorBase.cpp

@@ -58,7 +58,7 @@ class CompatibilityWriter : public ErrorWriterBase {
                     IdOrLocation idOrLoc, const std::string& message) override {
     // We may not have a context e.g. if we are just figuring out the error
     // message text. Trust that `computedLoc_` is not important for that.
-    if (context) this->computedLoc_ = errordetail::locate(context, idOrLoc);
+    if (context_) this->computedLoc_ = errordetail::locate(context_, idOrLoc);
     this->idOrLoc_ = std::move(idOrLoc);
     this->message_ = message;
   }

frontend/lib/framework/ErrorWriter.cpp

@@ -150,7 +150,7 @@ void ErrorWriter::writeHeading(ErrorBase::Kind kind, ErrorType type,
   // Printing the header prints the file path, so we need to update the
   // 'lastFilePath_' field.
   std::string printedPath;
-  writeFile(context, oss_, errordetail::locate(context, loc), &printedPath);
+  writeFile(context_, oss_, errordetail::locate(context_, loc), &printedPath);
   noteFilePath(std::move(printedPath));
 
   if (outputFormat_ == DETAILED) {
@@ -174,7 +174,7 @@ void ErrorWriter::writeNote(IdOrLocation loc, const std::string& str) {
   if (outputFormat_ == BRIEF) {
     // Indent notes in brief mode to make things easier to organize
     oss_ << "  note in ";
-    writeFile(context, oss_, errordetail::locate(context, loc));
+    writeFile(context_, oss_, errordetail::locate(context_, loc));
     oss_ << ": ";
   } else {
     // In detailed mode, the body is indented.
@@ -197,9 +197,9 @@ static void printLineNo(std::ostream& os, size_t gutterLength, int line) {
 
 void ErrorWriter::writeCode(const Location& location,
                            const std::vector<Location>& toHighlight) {
-  if (outputFormat_ != DETAILED || context == nullptr) return;
+  if (outputFormat_ != DETAILED || context_ == nullptr) return;
 
-  auto str = fileText(context, location);
+  auto str = fileText(context_, location);
   if (str.empty()) return;
 
   ssize_t startIndex = posToFileIndex(str.c_str(), location.firstLine(), 1);
@@ -233,7 +233,7 @@ void ErrorWriter::writeCode(const Location& location,
   // Print the file path if it's changed since the last code block. Printing
   // a code block will display the file path if needed, so lastFilePath_ needs
   // to be updated.
-  if (noteFilePath(locToPath(context, location))) {
+  if (noteFilePath(locToPath(context_, location))) {
     printBlank(oss_, codeIndent - 1);
     oss_ << "--> " << lastFilePath_ << std::endl;
   }

frontend/lib/parsing/parsing-queries.cpp

@@ -909,8 +909,8 @@ const Module* getIncludedSubmodule(Context* context,
   return getIncludedSubmoduleQuery(context, includeModuleId);
 }
 
-static const AstNode* const& astForIDQuery(Context* context, ID id) {
-  QUERY_BEGIN(astForIDQuery, context, id);
+static const AstNode* const& astForIdQuery(Context* context, ID id) {
+  QUERY_BEGIN(astForIdQuery, context, id);
 
   const AstNode* result = nullptr;
   const BuilderResult* r = parseFileContainingIdToBuilderResult(context, id);
@@ -927,7 +927,7 @@ const AstNode* idToAst(Context* context, ID id) {
     return nullptr;
   }
 
-  return astForIDQuery(context, id);
+  return astForIdQuery(context, id);
 }
 
 // TODO: could many of these get-property-of-ID queries
@@ -939,7 +939,7 @@ static const AstTag& idToTagQuery(Context* context, ID id) {
   AstTag result = asttags::AST_TAG_UNKNOWN;
 
   if (!id.isFabricatedId()) {
-    const AstNode* ast = astForIDQuery(context, id);
+    const AstNode* ast = astForIdQuery(context, id);
     if (ast != nullptr) {
       result = ast->tag();
     } else if (types::CompositeType::isMissingBundledRecordType(context, id)) {
@@ -963,7 +963,7 @@ static const bool& idIsParenlessFunctionQuery(Context* context, ID id) {
 
   AstTag tag = idToTag(context, id);
   if (asttags::isFunction(tag)) {
-    const AstNode* ast = astForIDQuery(context, id);
+    const AstNode* ast = astForIdQuery(context, id);
     if (ast != nullptr) {
       if (auto fn = ast->toFunction()) {
         result = fn->isParenless();
@@ -1026,7 +1026,7 @@ static const bool& idIsMethodQuery(Context* context, ID id) {
 
   AstTag tag = idToTag(context, id);
   if (asttags::isFunction(tag)) {
-    const AstNode* ast = astForIDQuery(context, id);
+    const AstNode* ast = astForIdQuery(context, id);
     if (ast != nullptr) {
       if (auto fn = ast->toFunction()) {
         result = fn->isMethod();
@@ -1045,7 +1045,7 @@ static const UniqueString& fieldIdToNameQuery(Context* context, ID id) {
   QUERY_BEGIN(fieldIdToNameQuery, context, id);
 
   UniqueString result;
-  if (auto ast = astForIDQuery(context, id)) {
+  if (auto ast = astForIdQuery(context, id)) {
     if (auto var = ast->toVariable()) {
       if (var->isField()) {
         result = var->name();
@@ -1663,7 +1663,7 @@ reportUnstableWarningForId(Context* context, ID idMention, ID idTarget) {
 static const Module::Kind& getModuleKindQuery(Context* context, ID moduleId) {
   Module::Kind ret = Module::Kind::DEFAULT_MODULE_KIND;
   QUERY_BEGIN(getModuleKindQuery, context, moduleId);
-  const AstNode* ast = astForIDQuery(context, moduleId);
+  const AstNode* ast = astForIdQuery(context, moduleId);
   CHPL_ASSERT(ast && "could not find AST for module ID");
   if (auto mod = ast->toModule()) {
     ret = mod->kind();

frontend/lib/resolution/Resolver.cpp

@@ -2405,8 +2405,9 @@ Resolver::lookupIdentifier(const Identifier* ident,
 
   if (!resolvingCalledIdent) config |= LOOKUP_INNERMOST;
 
-  auto vec =
-      lookupNameInScope(context, scope, receiverScopes, ident->name(), config);
+  auto vec = lookupNameInScopeWithWarnings(context, scope, receiverScopes,
+                                           ident->name(), config,
+                                           ident->id());
 
   bool notFound = vec.empty();
   bool ambiguous = !notFound && (vec.size() > 1 || vec[0].numIds() > 1);