[flang] Bug fix for ambiguous references to data and functions

[PeteSteinfeld]

Summary

A program may erroneously reference the same name as both a data object and as a function. Some of these references were causing an internal error in expression analysis.

It was already the case that a symbol referenced in a parse tree for a call was changed from an Entity to a ProcEntity. I added code to detect when a symbol was referenced in a parse tree as an array element gets changed from an Entity to an ObjectEntity. Then, if an ObjectEntity gets called as a function or a ProcEntity gets referenced as a data object, errors get emitted.

This analysis was previously confined to the name resolution of the
specification part of a ProgramTree. I added a pass to the execution part of
a ProgramTree to catch names declared in blocks.

[tskeith]

Would it be worthwhile adding flags to symbols to indicate that they are assumed to be functions or data based on their usage (as opposed to explicit declarations)?

Then we could give better error messages like "Cannot access 'foo' as an array as it was previously called as a function" and "Cannot call 'foo' as a function as it was previous accessed as an array".

flang/lib/Semantics/resolve-names.cpp

5886

Why is this change needed? When would FindSymbol find the wrong symbol?

7029

Why does this require an extra pass over the parse tree. Can't this check be done as part of ResolveExecutionParts where ArrayElement nodes are handled?

flang/test/Semantics/resolve93.f90

11

It's odd that this is the same situation as line 6 but we get a different error message.

[PeteSteinfeld]

flang/lib/Semantics/resolve-names.cpp

5886

Yes. The call to currScope() misses block scopes. So if the same name is declared in a block and in the global scope, the wrong one will be found.

7029

The problem I ran into was dealing with blocks and their associated scopes. When processing the specification parts, I couldn't find symbols declared in blocks. I tried eliminating the pass in ResolveSpecificationParts, but that broke processing of subprograms as dummy arguments.

flang/test/Semantics/resolve93.f90

11

In the first case, the problem is discovered during the first skimming code walk, where the name str is classified as an ObjectEntity because we find it in a parse tree that's an ArrayElement.

Then, the execution part is walked to resolve names. During this walk, a Pre() function for FunctionReference gets executed and it converts the name str2 to a ProcEntity. This happens before the code that I added. Then the skimming walk that I added gets executed. It sees the parse tree for the ArrayElement, but the name str2 has already been classified as a ProcEntity at that point. This causes the error message to be emitted.

[tskeith]

flang/lib/Semantics/resolve-names.cpp

5886

So you can't call FindSymbol if the name is in a block scope. But then how do you know it won't be called in that case? Is there a guarantee that every name in a block scope will be resolved by this point and so FindSymbol won't be called on it?

7029

I was referring to this call, not the one in ResolveSpecificationParts. Immediately above here we walk the execution parts. The will include the ArrayElement node. Why can't NoteExecutablePartArrayElement be done as part of that walk?

[PeteSteinfeld]

flang/lib/Semantics/resolve-names.cpp

5886

I'm not sure I understand your question. This code gets executed twice, once during processing of the specification part and then again at the end of processing of the execution part. In either case, we won't find names declared in block scopes. During the second pass, we won't find names that are declared in block scopes that have not had their symbol filled in previously. I'm not sure if this latter case is possible for an error-free symbol.

7029

Thanks for clarifying. It may be possible to do that. I'll investigate.

[PeteSteinfeld]

flang/lib/Semantics/resolve-names.cpp

7029

@tskeith , you were correct that we can handle detection of ArrayElement references in the main ResolveNames pass of the execution part. I'll be pushing a change soon with this implementation.

[PeteSteinfeld]

I eliminated the extra skimmer pass during name resolution of the execution
part. I also reverted the processing of calls to its original form.

[tskeith]

flang/lib/Semantics/resolve-names.cpp

6300

Do you have an example where this is needed?

[PeteSteinfeld]

flang/lib/Semantics/resolve-names.cpp

6300

You're right. It's not needed! Eliminating it has the result of always causing references to functions being recognized before references to array elements. This changes the error messages where the array reference precedes the function reference, but I think that's OK. I'll get rid of it.

[PeteSteinfeld]

Removed an unneeded visitor for ArrayElement.

[tskeith]

flang/lib/Semantics/resolve-names.cpp

5907

I think if you do this in ResolveDataRef when we see an ArrayElement you don't need to call FindScope. The name will be resolved if it can be.

[tskeith]

flang/lib/Semantics/resolve-names.cpp

5910

symbol->has<ProcEntityDetails>() is better.

[PeteSteinfeld]

flang/lib/Semantics/resolve-names.cpp

5907

Good thought. But I checked, and the symbol field of the parser::Name has not yet been filled in at the point. On the other hand, I realized that the lookup via the call to FindSymbol() always succeeds at the point because the current scope is correctly set. This let me simplify the code by eliminating the unnecessary reference to name.symbol.

5910

Thanks, Tim. I'll change it.

[PeteSteinfeld]

Simplified the code for getting the symbol of an ArrayElement.

[tskeith]

flang/lib/Semantics/resolve-names.cpp

5907

ResolveDataRef is where the symbol is filled in. I'm suggesting something like this for the ArrayElement case in ResolveDataRef:

[&](const Indirection<parser::ArrayElement> &y) {
  Walk(y.value().subscripts);
  const parser::Name *name{ResolveDataRef(y.value().base)};
  if (!name) {
  } else if (!name->symbol->has<ProcEntityDetails>()) {
    ConvertToObjectEntity(*name->symbol);
  } else if (!context().HasError(*name->symbol)) {
    SayWithDecl(*name, *name->symbol,
        "Cannot reference function '%s' as data"_err_en_US);
  }
  return name;
},

[PeteSteinfeld]

flang/lib/Semantics/resolve-names.cpp

5907

Ahhh. Thanks for leading me by the nose. This is much better than my implementation.

[PeteSteinfeld]

At Tim's suggestion, I moved the code to detect an illegal use of a
ProcEntity and to convert an Entity to an ObjectEntity to the existing
walk that processes an ArrayElement.

flang/lib/Semantics/expression.cpp

 
 MaybeExpr ExpressionAnalyzer::Analyze(const parser::ArrayElement &ae) {
   if (MaybeExpr baseExpr{Analyze(ae.base)}) {
     if (ae.subscripts.empty()) {
       // will be converted to function call later or error reported
       return std::nullopt;
     } else if (baseExpr->Rank() == 0) {
       if (const Symbol * symbol{GetLastSymbol(*baseExpr)}) {
+        if (!context_.HasError(symbol)) {
           Say("'%s' is not an array"_err_en_US, symbol->name());
+          context_.SetError(const_cast<Symbol &>(*symbol));
+        }
       }
     } else if (std::optional<DataRef> dataRef{
                    ExtractDataRef(std::move(*baseExpr))}) {
       return ApplySubscripts(
           std::move(*dataRef), AnalyzeSectionSubscripts(ae.subscripts));
     } else {
       Say("Subscripts may be applied only to an object, component, or array constant"_err_en_US);
     }

flang/lib/Semantics/resolve-names.cpp

public:
   bool Pre(const parser::CallStmt &);
   bool Pre(const parser::ImportStmt &);
   void Post(const parser::TypeGuardStmt &);
   bool Pre(const parser::StmtFunctionStmt &);
   bool Pre(const parser::DefinedOpName &);
   bool Pre(const parser::ProgramUnit &);
   void Post(const parser::AssignStmt &);
   void Post(const parser::AssignedGotoStmt &);
+  void Post(const parser::ArrayElement &);
 
   // These nodes should never be reached: they are handled in ProgramUnit
   bool Pre(const parser::MainProgram &) {
     llvm_unreachable("This node is handled in ProgramUnit");
   }
   bool Pre(const parser::FunctionSubprogram &) {
     llvm_unreachable("This node is handled in ProgramUnit");
   }
public:
   bool Pre(const parser::Submodule &) {
     llvm_unreachable("This node is handled in ProgramUnit");
   }
   bool Pre(const parser::BlockData &) {
     llvm_unreachable("This node is handled in ProgramUnit");
   }
 
   void NoteExecutablePartCall(Symbol::Flag, const parser::Call &);
+  void NoteExecutablePartArrayElement(const parser::Name &);
 
   friend void ResolveSpecificationParts(SemanticsContext &, const Symbol &);
 
 private:
   // Kind of procedure we are expecting to see in a ProcedureDesignator
   std::optional<Symbol::Flag> expectedProcFlag_;
   std::optional<SourceName> prevImportStmt_;
 
void ResolveNamesVisitor::NoteExecutablePartCall(
     Symbol::Flag flag, const parser::Call &call) {
   auto &designator{std::get<parser::ProcedureDesignator>(call.t)};
   if (const auto *name{std::get_if<parser::Name>(&designator.u)}) {
     // Subtlety: The symbol pointers in the parse tree are not set, because
     // they might end up resolving elsewhere (e.g., construct entities in
     // SELECT TYPE).
     if (Symbol * symbol{currScope().FindSymbol(name->source)}) {
       Symbol::Flag other{flag == Symbol::Flag::Subroutine
               ? Symbol::Flag::Function
               : Symbol::Flag::Subroutine};
       if (!symbol->test(other)) {
         ConvertToProcEntity(*symbol);
         if (symbol->has<ProcEntityDetails>()) {
           symbol->set(flag);
           if (IsDummy(*symbol)) {
             symbol->attrs().set(Attr::EXTERNAL);
           }
           ApplyImplicitRules(*symbol);
         }
       }
     }
   }
 }
 
+void ResolveNamesVisitor::NoteExecutablePartArrayElement(
+    const parser::Name &name) {
+  // See the "subtlety" comment above for why we're getting the symbol this way
+  Symbol *symbol{name.symbol};
+  if (!symbol) {
+    symbol = currScope().FindSymbol(name.source);
+  }
+  if (symbol) {
+    if (symbol->detailsIf<ProcEntityDetails>()) {
+      if (!context().HasError(*symbol)) {
+        SayWithDecl(
+            name, *symbol, "Cannot reference function '%s' as data"_err_en_US);
+      }
+    } else {
+      ConvertToObjectEntity(*symbol);
+    }
+  }
+}
+
 // Check and set the Function or Subroutine flag on symbol; false on error.
 bool ResolveNamesVisitor::SetProcFlag(
     const parser::Name &name, Symbol &symbol, Symbol::Flag flag) {
   if (symbol.test(Symbol::Flag::Function) && flag == Symbol::Flag::Subroutine) {
     SayWithDecl(
         name, symbol, "Cannot call function '%s' like a subroutine"_err_en_US);
     return false;
   } else if (symbol.test(Symbol::Flag::Subroutine) &&
void ResolveNamesVisitor::Post(const parser::AssignStmt &x) {
   }
 }
 void ResolveNamesVisitor::Post(const parser::AssignedGotoStmt &x) {
   if (auto *name{ResolveName(std::get<parser::Name>(x.t))}) {
     ConvertToObjectEntity(DEREF(name->symbol));
   }
 }
 
+void ResolveNamesVisitor::Post(const parser::ArrayElement &elem) {
+  // To avoid converting the name of the array to a function call, note that
+  // the name we're referencing is an object entity
+  const parser::DataRef &dataRef{elem.base};
+  if (const parser::Name * name{std::get_if<parser::Name>(&dataRef.u)}) {
+    NoteExecutablePartArrayElement(*name);
+  }
+}
+
 bool ResolveNamesVisitor::Pre(const parser::ProgramUnit &x) {
   auto root{ProgramTree::Build(x)};
   SetScope(context().globalScope());
   ResolveSpecificationParts(root);
   FinishSpecificationParts(root);
   inExecutionPart_ = true;
   ResolveExecutionParts(root);
   inExecutionPart_ = false;
public:
   }
 
   template <typename A> bool Pre(const A &) { return true; }
   template <typename A> void Post(const A &) {}
   void Post(const parser::FunctionReference &fr) {
     resolver_.NoteExecutablePartCall(Symbol::Flag::Function, fr.v);
   }
   void Post(const parser::CallStmt &cs) {
     resolver_.NoteExecutablePartCall(Symbol::Flag::Subroutine, cs.v);
   }
 
 private:
   ResolveNamesVisitor &resolver_;
 };
 
 // Build the scope tree and resolve names in the specification parts of this
 // node and its children
if (const auto *exec{node.exec()}) {
     Walk(*exec);
   }
   PopScope(); // converts unclassified entities into objects
   for (const auto &child : node.children()) {
     ResolveExecutionParts(child);
   }
 }
 
 void ResolveNamesVisitor::ResolveOmpParts(const parser::ProgramUnit &node) {
   OmpAttributeVisitor{context(), *this}.Walk(node);
   if (!context().AnyFatalError()) {
     // The data-sharing attribute of the loop iteration variable for a
     // sequential loop (2.15.1.1) can only be determined when visiting
     // the corresponding DoConstruct, a second walk is to adjust the
     // symbols for all the data-refs of that loop iteration variable
     // prior to the DoConstruct.
     OmpAttributeVisitor{context(), *this}.Walk(node);

flang/test/Semantics/resolve93.f90

+! RUN: %S/test_errors.sh %s %t %f18
+subroutine s1()
+  character(10) str
+  character(10) str1
+  !ERROR: Cannot reference function 'str' as data
+  print *, str(1:9), str(7)
+  block
+    character(10) str2
+    character(10) str3
+    !ERROR: Cannot reference function 'str1' as data
+    print *, str1(1:9), str1(7)
+    !ERROR: 'str2' is not an array
+    print *, str2(1:9), str2(7)
+    !ERROR: Cannot reference function 'str3' as data
+    print *, str3(7), str3(1:9)
+  end block
+end subroutine s1
+
+subroutine s2()
+  character(10) func
+  !ERROR: Cannot reference function 'func' as data
+  print *, func(7), func(1:9)
+end subroutine s2
+
+subroutine s3()
+  real(8) :: func
+  !ERROR: Cannot reference function 'func' as data
+  print *, func(7), func(1:6)
+end subroutine s3
+
+subroutine s4()
+  real(8) :: local
+  real(8) :: local1
+  !ERROR: Cannot reference function 'local' as data
+  print *, local(1:6), local(7)
+  !ERROR: Cannot reference function 'local1' as data
+  print *, local1(7), local1(1:6)
+end subroutine s4
+
+subroutine s5(arg)
+  integer :: iVar
+  external :: arg
+  iVar = loc(arg)
+end subroutine s5