[PATCH] D79851: [Flang] Semantics for SELECT TYPE

[Pete Steinfeld via Phabricator via llvm-commits]

PeteSteinfeld added a comment.

There's a lot to digest here.  Let me know if you have questions.



================
Comment at: flang/lib/Semantics/check-select-type.cpp:93-94
+              if (const DeclTypeSpec * spec{typeSpec.declTypeSpec}) {
+                if (spec->category() == DeclTypeSpec::Character &&
+                    !guardDynamicType.IsAssumedLengthCharacter()) { // C1160
+                  context_.Say(parser::FindSourceLocation(typeSpec),
----------------
sameeranjoshi wrote:
> PeteSteinfeld wrote:
> > C1160 applies to LEN type parameters for both character and derived types.  It doesn't look like you're checking or testing for LEN parameters on derived types.  Can you please add a check and test?
> Do you mean a test like below?
>    type :: t(n)
>       integer, len :: n
>    end type
> 
> And inside `SELECT TYPE` something like :
>  type is ( t(n=*) )   !<-- assumed length-type
Exactly.


================
Comment at: flang/lib/Semantics/resolve-names.cpp:5101-5102
+
+    } else if (evaluate::HasVectorSubscript(
+                   *association.selector.expr)) { // C1158
+      Say(association.selector.source,
----------------
sameeranjoshi wrote:
> PeteSteinfeld wrote:
> > C1158 also applies to selectors that are not variables.  Can you please add a check and test for that situation?
> I was confused from the wordings of standard.
> This is what I understand,
> If selector is not a variable => throw respective error
> if selector is a variable that has a vector subscript -> throw error
> 
> Is this what you meant, that I am missing the first check?
Here's my understanding of C1158 in pseudo code:
 
```
 if (!selector-is-variable || selector-is-variable-with-vector-subscript) {
    if (associate-name-appears-in-variable-definition-context ||
      associate-name-subobject-appears-in-variable-definition-context) {
      emit error message;
    }
  }
```

Suppose you have a function that returns a polymorphic type.  Then, you call that function in a select-type-stmt and associate it with an associate-name.  Then, suppose the associate-name appears on the left-hand side of an assignment statement or you use it as an actual argument to a procedure that has a dummy argument with INTENT(OUT) or INTENT(INOUT).  I think that's the situation that C1158 is trying to avoid.

So it's OK to use something other than a variable, as long as you don't try to change the value of the associate-name.  In standards-speak, that means that the associate-name can't appear in a "variable definition context" (see section 19.6.7 for details).  Note also that if the selector **is ** a variable, it's OK to use it in a variable definition context.

In fact, now that I've explained this in more detail.  I don't think that your existing check or test is correct.  To trigger an error, you should have a test where the associate-name appears on the left hand side of an assignment statement in the code of the construct of the SELECT TYPE statement.  The test should look something like this:


```
  select type(func_call(x) => func_result)
  type is (xxx)
    call sub_with_inout_param(func_result) ! Bad since func-result is in a variable definition context
  end select
```
So to implement this check, you'll need to look at all occurrences of the associate-name in the body of the select-type construct and see if the associate-name is associated with something other than a variable **and** if it is used in a variable definition context.  In such cases, you should emit an error message.

As a model for implementation, you can look in in check-do-forall.cpp.  Constraint C1130 requires that a variable used in a DO CONCURRENT construct must have its locality explicitly specified.  So there's code in there that walks the DO CONCURRENT construct looking for variables that violate this requirement.

You might also find it useful to call the function WhyNotModifiable() in tools.h to figure out whether a Symbol is modifiable in a Scope.  See section 19.6.7 for details on variable definition contexts.



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

https://reviews.llvm.org/D79851