[flang-commits] [PATCH] D123927: [flang] Do not ICE on recursive function definition in function result

[Daniil Dudkin via Phabricator via flang-commits]

unterumarmung marked 2 inline comments as not done.
unterumarmung added inline comments.


================
Comment at: flang/lib/Semantics/check-declarations.cpp:1789
     if (!details) {
+      if (generic.test(Symbol::Flag::Function)) {
+        Characterize(generic);
----------------
klausler wrote:
> unterumarmung wrote:
> > klausler wrote:
> > > unterumarmung wrote:
> > > > klausler wrote:
> > > > > When `generic` is not `GenericDetails`, what is it?
> > > > Well, the `generic` was written not by me. If I understand correctly, `generic` is a `Symbol` that meant to have the `GenericDetails`.
> > > Yes, but you're going out of your way to pass it to Characterize(), so I assume that you've encountered this case in a test somewhere.  If you keep this call here, why does it apply only to functions?  Fortran has generic subroutines as well.
> > Well, if we delete this chunk of code and run the test, we will get an ICE in the lowering part of the compiler because we do not call the `Characterize` on `foo` or `r` (from the test) because they do not have `GenericDetails`. And because there's not any other compile errors, the execution of compiler successfully goes to the lowering, where it tries to generate `Procedure` for `foo`  via `Characterize` and crashes there. I added the code here specifically for functions, to enable at least some semantic checks for the `generic` symbol.  
> > 
> > This is not the best solution and I'd be glad to consider your (or anyone else's) advices how to make it better, if you have any.
> Ok, but I don't think that the `Function` flag on the symbol means what you think.  Try testing for `.has<SubprogramDetails>()` so that it works for functions and subroutines.
I tried and it creates a lot of new error messages for `resolve102.f90`
For example, for `sub1`  in this chunk of test: https://github.com/llvm/llvm-project/blob/main/flang/test/Semantics/resolve102.f90#L51-L65 it generates 3 errors instead of 1:
```
./flang/test/Semantics/resolve102.f90:48:16: error: Procedure 'sub1' is recursively defined.  Procedures in the cycle: 'sub1', 'arg', 'sub', 'p2'
      Subroutine sub1(arg)
                 ^^^^
./flang/test/Semantics/resolve102.f90:48:16: error: Procedure 'sub1' is recursively defined.  Procedures in the cycle: 'sub1', 'arg'
      Subroutine sub1(arg)
                 ^^^^
./flang/test/Semantics/resolve102.f90:48:16: error: Procedure 'sub1' is recursively defined.  Procedures in the cycle: 'p', 'sub1', 'arg'
      Subroutine sub1(arg)
                 ^^^^
```
All the errors seem fair. Is it an acceptable behavior? If it is, I'll proceed with the change.
Personally, I think that "more errors is better" - because it can help fix erroneous code faster.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123927/new/

https://reviews.llvm.org/D123927