Add shadowed formal warning to dyno scope resolver (#21614)

This PR adds a warning when a use/import statement brings in a symbol
that shadows a function formal. This helps to get the following test
working with `--dyno`:

    test/modules/diten/moduleShadowFormal.chpl

While there, I noticed that `--no-warnings` was still causing an "In
function" header to be printed, so I also fixed that.

The implementation approach here is to add checking in a new query run
at the end of `resolveVisibilityStmts` for use/import clauses within
functions.

Current error messages:

```
$ chpl --dyno --detailed-errors test/modules/diten/moduleShadowFormal.chpl
─── warning in test/modules/diten/moduleShadowFormal.chpl:10 [HiddenFormal] ───
  Module-level symbol is hiding function argument 'a'
  The formal argument:
      |
    9 |   proc foo(a: real) {
      |            ⎺⎺⎺⎺⎺⎺⎺⎺
      |
  is shadowed by a symbol provided by the following 'use' statement:
       |
    10 |     use M1;
       |         ⎺⎺
       |

```

```
$ chpl --dyno  test/modules/diten/moduleShadowFormal.chpl
test/modules/diten/moduleShadowFormal.chpl:9: In function 'foo':
test/modules/diten/moduleShadowFormal.chpl:10: warning: module-level symbol is hiding function argument 'a'
```

Reviewed by @DanilaFe - thanks!

- [x] full local testing
- [x] no unexpected failures for `--dyno` testing

compiler/parser/parser.cpp

@@ -910,16 +910,27 @@ static bool dynoRealizeErrors(void) {
   INT_ASSERT(gDynoErrorHandler);
   bool hadErrors = false;
   for (auto& err : gDynoErrorHandler->errors()) {
-    hadErrors = true;
-    // skip issuing errors that have already been issued
+    const chpl::ErrorBase* e = err.get();
+
+    if (e->kind() == chpl::ErrorBase::SYNTAX ||
+        e->kind() == chpl::ErrorBase::ERROR) {
+      // make a note if we found any errors
+      hadErrors = true;
+    }
+
+    // skip emitting warnings for '--no-warnings'
+    if (ignore_warnings && e->kind() == chpl::ErrorBase::WARNING) {
+      continue;
+    }
+
     if (fDetailedErrors) {
-      chpl::Context::defaultReportError(gContext, err.get());
+      chpl::Context::defaultReportError(gContext, e);
       // Use production compiler's exit-on-error functionality for errors
       // reported via new Dyno mechanism
-      setupDynoError(err->kind());
+      setupDynoError(e->kind());
     } else {
       // Try to maintain compatibility with the old reporting mechanism
-      dynoDisplayError(gContext, err->toErrorMessage(gContext));
+      dynoDisplayError(gContext, e->toErrorMessage(gContext));
     }
   }
   gDynoErrorHandler->clear();

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

@@ -128,3 +128,7 @@ WARNING_CLASS(Unstable,
     std::string,
     const uast::AstNode*,
     const uast::NamedDecl*)
+WARNING_CLASS(HiddenFormal,
+    const uast::Formal*,
+    const uast::VisibilityClause*,
+    const resolution::VisibilityStmtKind)

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

@@ -314,6 +314,10 @@ class BorrowedIdsWithName {
     return idv_.id_;
   }
 
+  /** Returns 'true' if the list contains only IDs that represent
+      methods or fields. */
+  bool containsOnlyMethodsOrFields() const;
+
   BorrowedIdsWithNameIter begin() const {
     return BorrowedIdsWithNameIter(this, beginIdAndVis());
   }
@@ -450,20 +454,13 @@ class Scope {
   bool lookupInScope(UniqueString name,
                      std::vector<BorrowedIdsWithName>& result,
                      bool arePrivateIdsIgnored,
-                     bool onlyMethodsFields) const {
-    auto search = declared_.find(name);
-    if (search != declared_.end()) {
-      // There might not be any IDs that are visible to us, so borrow returns
-      // an optional list.
-      auto borrowedIds = search->second.borrow(arePrivateIdsIgnored,
-                                               onlyMethodsFields);
-      if (borrowedIds.hasValue()) {
-        result.push_back(std::move(borrowedIds.getValue()));
-        return true;
-      }
-    }
-    return false;
-  }
+                     bool onlyMethodsFields) const;
+
+  /** Check to see if the scope contains IDs with the provided name. */
+  bool contains(UniqueString name) const;
+
+  /** Gathers all of the names of symbols declared directly within this scope */
+  std::set<UniqueString> gatherNames() const;
 
   bool operator==(const Scope& other) const {
     return parentScope_ == other.parentScope_ &&
@@ -581,13 +578,16 @@ class VisibilitySymbols {
   };
 
  private:
-  const Scope* scope_; // Scope of the Module etc
+  const Scope* scope_; // Scope of the Module used/imported
                        // This could technically be an ID but basically
                        // anything we do with it needs a Scope* anyway.
   Kind kind_ = SYMBOL_ONLY;
   bool isPrivate_ = true;
   int8_t shadowScopeLevel_ = REGULAR_SCOPE;
 
+  ID visibilityClauseId_; // ID of the uAST that generated this
+                          // (this is only needed to support error messages)
+
   // the names/renames:
   //  pair.first is the name as declared
   //  pair.second is the name here
@@ -597,9 +597,11 @@ class VisibilitySymbols {
   VisibilitySymbols() { }
   VisibilitySymbols(const Scope* scope, Kind kind,
                     bool isPrivate, ShadowScope shadowScopeLevel,
+                    ID visibilityClauseId,
                     std::vector<std::pair<UniqueString,UniqueString>> names)
     : scope_(scope), kind_(kind),
       isPrivate_(isPrivate), shadowScopeLevel_(shadowScopeLevel),
+      visibilityClauseId_(visibilityClauseId),
       names_(std::move(names))
   {
     CHPL_ASSERT(shadowScopeLevel == REGULAR_SCOPE ||
@@ -621,6 +623,13 @@ class VisibilitySymbols {
     return (ShadowScope) shadowScopeLevel_;
   }
 
+  /**
+    Returns the ID of the use/import clause that this VisibilitySymbols was
+    created to represent. */
+  const ID& visibilityClauseId() const {
+    return visibilityClauseId_;
+  }
+
   /** Lookup the declared name for a given name
       Returns true if `name` is found in the list of renamed names and
       stores the declared name in `declared`
@@ -639,9 +648,10 @@ class VisibilitySymbols {
   bool operator==(const VisibilitySymbols &other) const {
     return scope_ == other.scope_ &&
            kind_ == other.kind_ &&
-           names_ == other.names_ &&
            isPrivate_ == other.isPrivate_ &&
-           shadowScopeLevel_ == other.shadowScopeLevel_;
+           shadowScopeLevel_ == other.shadowScopeLevel_ &&
+           visibilityClauseId_ == other.visibilityClauseId_ &&
+           names_ == other.names_;
   }
   bool operator!=(const VisibilitySymbols& other) const {
     return !(*this == other);
@@ -650,9 +660,10 @@ class VisibilitySymbols {
   void swap(VisibilitySymbols& other) {
     std::swap(scope_, other.scope_);
     std::swap(kind_, other.kind_);
-    names_.swap(other.names_);
     std::swap(isPrivate_, other.isPrivate_);
     std::swap(shadowScopeLevel_, other.shadowScopeLevel_);
+    names_.swap(other.names_);
+    visibilityClauseId_.swap(other.visibilityClauseId_);
   }
 
   static bool update(VisibilitySymbols& keep,
@@ -662,6 +673,7 @@ class VisibilitySymbols {
 
   void mark(Context* context) const {
     context->markPointer(scope_);
+    visibilityClauseId_.mark(context);
     for (auto p : names_) {
       p.first.mark(context);
       p.second.mark(context);
@@ -702,10 +714,12 @@ class ResolvedVisibilityScope {
   void addVisibilityClause(const Scope* scope, VisibilitySymbols::Kind kind,
                            bool isPrivate,
                            VisibilitySymbols::ShadowScope shadowScopeLevel,
+                           ID visibilityClauseId,
                            std::vector<std::pair<UniqueString,UniqueString>> n)
   {
     auto elt = VisibilitySymbols(scope, kind,
                                  isPrivate, shadowScopeLevel,
+                                 std::move(visibilityClauseId),
                                  std::move(n));
     visibilityClauses_.push_back(std::move(elt));
   }

frontend/lib/framework/resolution-error-classes-list.cpp

@@ -811,6 +811,23 @@ void ErrorUnstable::write(ErrorWriterBase& wr) const {
   return;
 }
 
+void ErrorHiddenFormal::write(ErrorWriterBase& wr) const {
+  auto formal = std::get<const uast::Formal*>(info);
+  auto visibilityClause = std::get<const uast::VisibilityClause*>(info);
+  auto useOrImport = std::get<const resolution::VisibilityStmtKind>(info);
+  CHPL_ASSERT(formal && visibilityClause);
+
+  wr.heading(kind_, type_, visibilityClause,
+             "module-level symbol is hiding function argument '",
+             formal->name(), "'");
+  wr.message("The formal argument:");
+  wr.code(formal, { formal });
+  wr.message("is shadowed by a symbol provided by the following '",
+             useOrImport, "' statement:");
+  wr.code(visibilityClause, { visibilityClause });
+  return;
+}
+
 /* end resolution errors */
 
 } // end namespace 'chpl'

frontend/lib/resolution/Resolver.cpp

@@ -1952,40 +1952,45 @@ void Resolver::validateAndSetToId(ResolvedExpression& r,
   if (id.isEmpty()) return;
 
   // Validate the newly set to ID.
+  auto idTag = parsing::idToTag(context, id);
 
   // It shouldn't refer to a module unless the node is an identifier in one of
   // the places where module references are allowed (e.g. imports).
-  auto toAst = parsing::idToAst(context, id);
-  if (toAst != nullptr) {
-    if (auto mod = toAst->toModule()) {
-      auto parentId = parsing::idToParentId(context, node->id());
-      if (!parentId.isEmpty()) {
+  if (asttags::isModule(idTag)) {
+    auto parentId = parsing::idToParentId(context, node->id());
+    if (!parentId.isEmpty()) {
+      auto parentTag = parsing::idToTag(context, parentId);
+      if (asttags::isUse(parentTag) || asttags::isImport(parentTag) ||
+          asttags::isAs(parentTag) || asttags::isVisibilityClause(parentTag) ||
+          asttags::isDot(parentTag)) {
+        // OK
+      } else {
+        auto toAst = parsing::idToAst(context, id);
+        auto mod = toAst->toModule();
         auto parentAst = parsing::idToAst(context, parentId);
-        if (!parentAst->isUse() && !parentAst->isImport() &&
-            !parentAst->isAs() && !parentAst->isVisibilityClause() &&
-            !parentAst->isDot()) {
-          CHPL_REPORT(context, ModuleAsVariable, node, parentAst, mod);
-        }
+        CHPL_REPORT(context, ModuleAsVariable, node, parentAst, mod);
       }
     }
   }
 
   // If we're in a nested class, it shouldn't refer to an outer class' field.
   auto scope = scopeForId(context, id);
   auto parentId = scope->id();
-  auto parentAst = parsing::idToAst(context, parentId);
-  if (parentAst && parentAst->isAggregateDecl() &&
-      parentId.contains(node->id())) {
+  auto parentTag = parsing::idToTag(context, parentId);
+  if (asttags::isAggregateDecl(parentTag) && parentId.contains(node->id())) {
     // Referring to a field of a class that's surrounding the current node.
     // Loop upwards looking for a composite type.
     auto searchId = parsing::idToParentId(context, node->id());
     while (!searchId.isEmpty()) {
-      auto searchAst = parsing::idToAst(context, searchId);
-      if (searchAst == parentAst) {
+      auto searchTag = parsing::idToTag(context, searchId);
+      if (searchId == parentId) {
         // We found the aggregate type in which the to-ID is declared,
         // so there's no nested class issues.
         break;
-      } else if (auto searchAD = searchAst->toAggregateDecl()) {
+      } else if (asttags::isAggregateDecl(searchTag)) {
+        auto parentAst = parsing::idToAst(context, parentId);
+        auto searchAst = parsing::idToAst(context, searchId);
+        auto searchAD = searchAst->toAggregateDecl();
         // It's an error!
         CHPL_REPORT(context, NestedClassFieldRef, parentAst->toAggregateDecl(),
                     searchAD, node, id);