[flang-commits] [PATCH] D79851: [Flang] Semantics for SELECT TYPE

[Pete Steinfeld via Phabricator via flang-commits]

PeteSteinfeld requested changes to this revision.
PeteSteinfeld added a comment.
This revision now requires changes to proceed.

Thanks again for working on this.  Here's a summary of the changes I think are needed:

- Fix the creation of a DynamicType in GetGuardType()
- Fix the check for assumed length of derived types
- Add a test that uses `class is (derived(param=*))` as a guard
- Add a test where the selector is unlimited polymorphic (`class(*)`)
- Add test without a polymorphic selector: "select type (a => x)"
- Fix selecttype01.f90 to have the correct test for assumed length derived types
- Format the tests and remove all tabs





================
Comment at: flang/lib/Semantics/check-select-type.cpp:66-78
+                const semantics::Symbol &typeSymbol{
+                    derivedTypeSpec->typeSymbol()};
+                if (const semantics::Scope * scope{typeSymbol.scope()}) {
+                  auto &mutablederivedTypeSpec{
+                      *const_cast<semantics::DerivedTypeSpec *>(
+                          derivedTypeSpec)};
+                  auto &mutableConstScope{
----------------
You can create the required object as follows:
```
  return evaluate::DynamicType(*derivedTypeSpec);
```

I don't think that you have any tests that actually exercise this code.  The existing test resolve73.f90 does, though.  Here's a smaller test that will exercise the variant `parser::TypeSpec`:
```
subroutine s()
  type derived(param)
    integer, len :: param
    class(*), allocatable :: x
  end type

  select type (ax => a%x)
    class is (derived(param=*))
      print *, "hello"
  end select
end subroutine s
```


================
Comment at: flang/lib/Semantics/check-select-type.cpp:123
+    for (const auto &pair : derived.parameters()) {
+      if (pair.second.isLen() && pair.second.isAssumed()) { // C1160
+        context_.Say(sourceLoc,
----------------
There should be a `!` preceding the `pair.second.isAssumed()`.


================
Comment at: flang/lib/Semantics/check-select-type.cpp:143
+          context_.Say(sourceLoc,
+              "Type specification '%s' must be an extension of TYPE '%s'"_err_en_US,
+              derived.AsFortran(), selDerivedTypeSpec.AsFortran());
----------------
Please make this fit into 80 columns.


================
Comment at: flang/lib/Semantics/check-select-type.cpp:233
+            msg = &context_.Say(iter->stmt.source,
+                "Type specification '%s' conflicts with previous type specification"_err_en_US,
+                iter->AsFortran());
----------------
Please make this fit into 80 columns.


================
Comment at: flang/lib/Semantics/resolve-names.cpp:5143-5145
     // This isn't a name in the current scope, it is in each TypeGuardStmt
     MakePlaceholder(*name, MiscDetails::Kind::SelectTypeAssociateName);
     association.name = &*name;
----------------
kiranchandramohan wrote:
> The polymorphic selector check should happen here also to catch cases like the following.
> 
> ```
> program test
>   integer :: x
>   select type (a => x)
>   type is (integer)
>     print *,'integer ',a
>   end select
> end program
> ```
> 
> 
@kiranchandramohan is correct.  Also, test a version of the program that does not include the declaration of `integer :: x`.  That also fails.

Also, it would be good to include a test that actually has an `integer` type guard.  Here's an example:
```
subroutine s(arg)
  class(*) :: arg
    select type (arg)
        type is (integer)
    end select
end
```


================
Comment at: flang/test/Semantics/selecttype01.f90:55-56
+                type is ( character(len=*) )
+		! OK
+		type is ( t(n=10) )
+!ERROR: The type specification statement must have LEN type parameter as assumed
----------------
This is the line that's in error, since the LEN is not assumed (`*`).


================
Comment at: flang/test/Semantics/selecttype01.f90:56-58
+		type is ( t(n=10) )
+!ERROR: The type specification statement must have LEN type parameter as assumed
+		type is ( t(n=*) )   !<-- assumed length-type
----------------
Please use spaces rather than tabs.  There are other lines in this file that also contain tabs.

Also, please align the `!ERROR` and other comment lines with the associated lines that contain errors.

Also, please make the amount of indentation is inconsistent.  Please indent two spaces everywhere.

Please do all of this for the other `select type` tests, too.


================
Comment at: flang/test/Semantics/selecttype01.f90:57-58
+		type is ( t(n=10) )
+!ERROR: The type specification statement must have LEN type parameter as assumed
+		type is ( t(n=*) )   !<-- assumed length-type
+!ERROR: Derived type 'character' not found
----------------
This is not an error.  C1160 requires that the LEN be assumed, which is what this source code does.


================
Comment at: flang/test/Semantics/selecttype01.f90:217-223
+program main
+   use m, only : CheckC1160,t
+   type(t(n=3)) :: foo
+   call CheckC1160( "Hello!" )
+   call CheckC1160( foo )
+   call CheckC1160( 1.0 )
+end program
----------------
Note that tests do not need a main program.  We're only testing the semantic checking.  There's no need to pretend that we can create an executable program.


================
Comment at: flang/test/Semantics/selecttype02.f90:1
+! RUN: %S/test_errors.sh %s %t %f18
+module m1
----------------
Please remove all tabs from this file.


================
Comment at: flang/test/Semantics/selecttype03.f90:1
+! RUN: %S/test_errors.sh %s %t %f18
+! Test various conditions in C1158.
----------------
Please remove all tabs from this file.


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

https://reviews.llvm.org/D79851