[Flang] Fix IsProcedure() for procedure-valued function references

Project: llvm/llvm-project | Language: C++ | Concepts: error-handling, testing-strategy, refactoring | Difficulty: mid
Link: https://github.com/llvm/llvm-project/pull/216322

The problem

The IsProcedure() function in Flang did not recognize function references whose results are procedures. This led to assertion failures during intrinsic argument checking if such expressions were passed as actual arguments, instead of producing user-facing errors.

Why it's non-trivial

A naive fix might only check if the expression itself is a procedure or procedure pointer, missing the case where a function returns a procedure (an invalid but representable construct in error recovery). Properly handling this requires recursively inspecting the result of a function reference to determine if it is a procedure, not just looking at the top-level expression.

The approach

1. Extend IsProcedure() to handle function results that are procedures

-bool IsProcedure(const Expr<SomeType> &expr) {
-  return IsProcedureDesignator(expr) || IsProcedurePointer(expr);
+bool IsProcedure(const Expr<SomeType> &expr) {
+  if (IsProcedureDesignator(expr) || IsProcedurePointer(expr)) {
+    return true;
+  }
+  if (const auto *funcRef{UnwrapProcedureRef(expr)}) {
+    if (const Symbol *proc{funcRef->proc().GetSymbol()}) {
+      if (const Symbol *result{FindFunctionResult(*proc)}) {
+        return IsProcedure(*result);
+      }
+    }
+  }
+  return false;
 }

• What this does:
  • First, it checks if the expression is already recognized as a procedure or a procedure pointer.
  • If not, it tries to unwrap the expression as a function reference (UnwrapProcedureRef).
  • If successful, it looks up the function's result symbol (FindFunctionResult).
  • It then recursively checks if the result is itself a procedure, using IsProcedure.
• Why:
  • This ensures that function references whose results are procedures are detected, fixing cases where such constructs would otherwise slip through and cause assertion failures.

2. Add regression and behavior tests

Example from flang/test/Semantics/procedure-function-result01.f90:

function s(x) result(i)
  ! ERROR: A function result may not be a procedure unless it is a procedure pointer
  procedure() :: i
  ! ERROR: Actual argument for 'array=' may not be a procedure
  print *, size(S(dd))
end

• What this does:
  • Defines a function returning a procedure (invalid in Fortran).
  • Passes the result to an intrinsic expecting a variable, not a procedure.
  • Expects two errors: one for the invalid function result, another for the invalid argument.
• Why:
  • Verifies that the new logic in IsProcedure() triggers the correct user-facing errors, not internal assertion failures.

Example from flang/test/Semantics/procedure-function-result02.f90:

function s(x) result(i)
  procedure(), pointer :: i
  ! ERROR: Actual argument for 'array=' may not be a procedure
  print *, size(S(dd))
end

• What this does:
  • Tests a function with a procedure pointer result, which is valid, but checks that passing it as an array argument still triggers an error.
• Why:
  • Ensures the fix doesn't overgeneralize and still enforces argument restrictions.

Additional tests (appended to existing files) verify new error messages for output items and select type selectors:

function s2(x) result(i)
  !ERROR: A function result may not be a procedure unless it is a procedure pointer
  procedure() :: i
  !ERROR: Output item must not be a procedure
  print *, s2(dd)
end

function s3(x) result(i)
  !ERROR: A function result may not be a procedure unless it is a procedure pointer
  procedure() :: i
  !ERROR: Selector may not be a procedure
  select type (y => s3(dd))
  end select
end

• What this does:
  • Ensures that other contexts (output items, select type selectors) now also emit accurate errors for procedure-valued function results.

From the review thread

Besides fixing the assertion failure, this change has two other user-visible effects that are currently untested: the same invalid shape now gets an accurate error as an output item ("Output item must not be a procedure", from check-io.cpp) and as a SELECT TYPE selector ("Selector may not be a procedure", from check-select-type.cpp). Both were silent before this PR (only the declaration error was emitted). Since these are behavior changes introduced by this PR, please pin them too — e.g. by appending (verified against this PR):

Done! I've added the tests. Thanks for the review.

Takeaways

• When checking properties of expressions, recursively inspect the results of function references, not just the top-level type.
• Always add regression tests for both the original failure mode (e.g., assertion failure) and for new user-visible behaviors introduced by a fix.
• Error-handling code should be robust to invalid constructs that may arise during error recovery, not just valid language constructs.