This is an archive of the discontinued LLVM Phabricator instance.

[Flang] Semantics for SELECT TYPE
AbandonedPublic

Authored by sameeranjoshi on May 13 2020, 4:46 AM.

Diff Detail

Event Timeline

sameeranjoshi created this revision.May 13 2020, 4:46 AM
Herald added a project: Restricted Project. · View Herald Transcript
jdoerfert retitled this revision from Semantics for SELECT TYPE to [Flang] Semantics for SELECT TYPE.May 13 2020, 8:34 AM
klausler requested changes to this revision.May 13 2020, 10:14 AM
klausler added inline comments.
flang/lib/Semantics/check-select-type.cpp
42

success is not necessary.

64

Just return evaluate::DynamicType::From(typeSpec.declTypeSpec); for the whole function body.

74–84

Your code code has no comments anywhere to explain what you're doing, and they would be helpful here. It looks like you're using move semantics to suck the contents out of the derived type spec and transfer them to a new derived type spec in the scope of the original derived type spec. This seems likely to wreck the representation of the original type, and I can't tell why you want to do it.

85

Just return; the if is not necessary.

129

If derived can't be null, use a reference. If it can be null, check it.

140

Is struct TypeCase nested in class TypeCaseValues?

179

You're just coping a single string to a stream to get the same string back.

195

Should be a function, not a function object; it has no state, and is only called directly.

196

Can be static rather than const, I think.

209

These six lines could be a single return statement.

218

The "but" here isn't reassuring, since the cases where compilation time matters are non-error cases.

241

I don't think that this closing brace is ending the namespace, but rather a class.

flang/lib/Semantics/resolve-names.cpp
5096

s/statements/statement/

s/ value//

5103

This error message is unclear. Explain why the expression cannot be used with an associate-name.

This revision now requires changes to proceed.May 13 2020, 10:14 AM
sameeranjoshi marked 6 inline comments as done.

Addressed comments by @klausler.

sameeranjoshi marked 12 inline comments as done and an inline comment as not done.May 14 2020, 11:11 AM
This comment was removed by sameeranjoshi.
sameeranjoshi added inline comments.May 14 2020, 11:11 AM
flang/lib/Semantics/check-select-type.cpp
74–84

Ok, so wanted to extract the Evaluate::DynamicType from the parser::DerivedTypeSpec.
The only way I could figure out was,
step1:
convert from semantics::DerivedTypeSpec to semantics::DeclTypeSpec
step2:
further I knew that evaluate::DynamicType::From() converts semantics::DeclTypeSpec to Evaluate::DynamicType

Is there a better way you can suggest?

120

Given that derived is checked for null value, I will go with reference.

140

Yes, the similar way select-case has struct Case inside class CaseValues.

179

Why is it used in select case?
Does AsFortran in evaluate::Constant template it do something more ?

flang/lib/Semantics/resolve-names.cpp
5096

Is this what you wanna say?
replace statements => statement
and remove "value"

new message
"Selector '%s' in SELECT TYPE statement must be polymorphic"

5103

Probably because selector has a vector subscript.
`Neither 'associate-name =>' nor any subobject must appear in selector, if it has a vector subscript"

looks good?

sameeranjoshi marked 3 inline comments as done.May 14 2020, 11:12 AM

Addressed comments by @klausler.

@klausler if you can point me some high level pointers on implementing C1162, would be much helpful, so I can start working on it.
Thanks.

PeteSteinfeld requested changes to this revision.May 20 2020, 11:42 AM

Thanks for working on this!

Let me know if you have any questions.

flang/lib/Semantics/check-select-type.cpp
86

A more evocative name would be "PassesChecksOnGuard()"

93–94

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?

118

A more evocative name would be "PassesDerivedTypeChecks()".

122–123

It would be more consistent for the message to be "The type specification statement must not specify "

"a type with a SEQUENCE attribute or a BIND attribute"
183

I'd prefer a more evocative name, such as "TypesAreDifferent()"

194

I'd prefer a more evocative name such as "AreTypeKindCompatible()"

198

There's an extra "//" here

207

There should be single quotes around the name of the conflicting type specification -- as in "Type specification '%s' conflicts with previous type specification"_err_en_US,

210

There should be single quotes around the name of the conflicting type specification -- as in "Conflicting type specification '%s'"_en_US.

flang/lib/Semantics/resolve-names.cpp
5101–5102

C1158 also applies to selectors that are not variables. Can you please add a check and test for that situation?

flang/test/Semantics/selecttype01.f90
1

Can you please add tests for C1165?

This revision now requires changes to proceed.May 20 2020, 11:42 AM
sameeranjoshi added inline comments.May 20 2020, 12:14 PM
flang/lib/Semantics/resolve-names.cpp
5101–5102

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?

sameeranjoshi added inline comments.May 20 2020, 12:28 PM
flang/lib/Semantics/check-select-type.cpp
93–94

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

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

flang/lib/Semantics/check-select-type.cpp
93–94

Exactly.

flang/lib/Semantics/resolve-names.cpp
5101–5102

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.

PeteSteinfeld added inline comments.May 20 2020, 9:30 PM
flang/lib/Semantics/resolve-names.cpp
5101–5102

I've been looking at the existing code in the compiler. All that you may need to do is add some code to WhyNotModifiable() to handle SELECT TYPE associate-names. I see that there's already a test for a name that's construct associated. Start by writing tests that cover the two situations for C1158 and see if the appropriate things happen.

sameeranjoshi removed a reviewer: jdoerfert.
sameeranjoshi removed a subscriber: mgorny.

Addressed comments by @PeteSteinfeld

sameeranjoshi marked 17 inline comments as done.May 29 2020, 3:46 AM

All of my comments have been addressed. @klausler, if things look good to you, this is ready to merge.

klausler requested changes to this revision.May 29 2020, 9:43 AM
klausler added inline comments.
flang/lib/Semantics/check-select-type.cpp
135

C1162 is important; please implement it. There can't be TYPE IS or CLASS IS cases in a SELECT TYPE that are not possible for the selector.

140

But why is this necessary?

flang/test/Semantics/selecttype03.f90
48 ↗(On Diff #267164)

The comment states that the test is trying to force an error by associating a selector with a designator that has a vector subscript, but that's not what's happening in the code.

This revision now requires changes to proceed.May 29 2020, 9:43 AM
sameeranjoshi marked 2 inline comments as done.May 29 2020, 12:29 PM
sameeranjoshi added inline comments.
flang/lib/Semantics/check-select-type.cpp
135

I have a test already added in selecttype01.f90 in subroutine CheckC1162.
If @PeteSteinfeld @klausler that LG, I think IsExtensibleType when passed with parentDerived would help in code implementation?

Can we extract the ancestor chain of a particular type as well as the types which derive from it?

Can you point some test file if present or the code file which could be already handling this?

140

Here's how I see it.
TypeCase nested in TypeCaseValues creates a logical nesting which becomes easy to read code and figure out the similar functions which would operate on some particular data of struct.

The way select-type-construct in below snippet from standard

R1152 select-type-construct 
select-type-stmt
[ type-guard-stmt block ] ...
[ type-guard-stmt block ] ...
...
end-select-type-stmt

Tries to seperate type-guard-stmt block and created a list of such blocks, similarly I see for TypeCase .
Also various helper functions and data members which logically would come in mind when it comes to implementing type-guard-stmt block are grouped in same struct.

Do you suggest instead of having a nested struct better to have these functions and struct data members in the parent class?

PeteSteinfeld requested changes to this revision.Jun 3 2020, 1:59 PM

Thanks for working on this. There's still more to be done here.

flang/lib/Semantics/check-select-type.cpp
2

Does this file need to have clang-format run on it?

57

The & is not needed for any of these variants.

62

Specifying the return type is not necessary for this variant.

74–84

I'm not sure what the best answer here is. But the std::move() seems like a dangerous thing to do. I suggest looking around the rest of the files that do semantics checking for how they extract Evaluate::DynamicType objects and see if you can glean some wisdom from them.

92

The & is not needed.

98

Please make this fit into 80 columns.

120

Other methods are preceded by a blank line. Please insert one for consistency.

125

Please make this fit into 80 columns.

135

I don't understand your first question about "code implementation".

In resolve-names.cpp, there's a function called FindSymbol() that searches though a chain of extended derived types. Also, take a look at the Post() function for parser::DerivedTypeStmt. It processes the EXTENDS clause of a derived type definition.

207

Peter's comment about this is that the normal case is that there are no errors. So why have this comment at all?

237

Variable names should start with a lower-case letter. Please change this to selector. See flang//documentation/C++style.md for guidance.

flang/lib/Semantics/resolve-names.cpp
5096

This looks good to me.

flang/test/Semantics/selecttype01.f90
2

The RUN command has changed. It's now

! RUN: %S/test_errors.sh %s %t %f18
flang/test/Semantics/selecttype02.f90
1 ↗(On Diff #267164)

The RUN command has changed. It's now

! RUN: %S/test_errors.sh %s %t %f18
flang/test/Semantics/selecttype03.f90
1 ↗(On Diff #267164)

The RUN command has changed. It's now

! RUN: %S/test_errors.sh %s %t %f18
48 ↗(On Diff #267164)

There's a comment in selecttype01.f90 that says it tests c1158. But I couldn't find a test that tests vector subscripts.

sameeranjoshi marked 17 inline comments as done.Jun 8 2020, 12:12 PM
sameeranjoshi added inline comments.
flang/lib/Semantics/check-select-type.cpp
2

I ran git-clang-format and I wasn't seeing any changes in licence part.

sameeranjoshi edited the summary of this revision. (Show Details)

Update more review comments given by @PeteSteinfeld .
C1162 implemented.

kiranchandramohan added inline comments.
flang/lib/Semantics/check-select-type.cpp
140

Is it guaranteed that selectorType will be derived here? Bad code can have a selector which is not derived.

flang/lib/Semantics/resolve-names.cpp
5081–5083

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
flang/test/Semantics/selecttype02.f90
23–37 ↗(On Diff #269312)

try to use same number of spaces for alignment.

40–55 ↗(On Diff #269312)

alignment

flang/test/Semantics/selecttype03.f90
17–23 ↗(On Diff #269312)

Alignment

59–66 ↗(On Diff #269312)

Alignment

sameeranjoshi marked 6 inline comments as done.Jun 9 2020, 11:38 AM
PeteSteinfeld requested changes to this revision.Jun 10 2020, 2:27 PM

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
flang/lib/Semantics/check-select-type.cpp
67–79

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
124

There should be a ! preceding the pair.second.isAssumed().

144

Please make this fit into 80 columns.

234

Please make this fit into 80 columns.

flang/lib/Semantics/resolve-names.cpp
5081–5083

@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
flang/test/Semantics/selecttype01.f90
56–57

This is the line that's in error, since the LEN is not assumed (*).

57–59

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.

58–59

This is not an error. C1160 requires that the LEN be assumed, which is what this source code does.

218–224

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.

flang/test/Semantics/selecttype02.f90
1 ↗(On Diff #269312)

Please remove all tabs from this file.

flang/test/Semantics/selecttype03.f90
1 ↗(On Diff #269312)

Please remove all tabs from this file.

This revision now requires changes to proceed.Jun 10 2020, 2:27 PM
sameeranjoshi marked 15 inline comments as done.

Thanks @kiranchandramohan and @PeteSteinfeld for reviewing.
I have addressed them.

PeteSteinfeld accepted this revision.Jun 11 2020, 10:01 AM

Looks good!

Please make that change to check-select-type.cpp before pushing.

Thanks for sticking with this.

PeteSteinfeld added inline comments.Jun 11 2020, 10:45 AM
flang/lib/Semantics/check-select-type.cpp
127–128

You should embed the declaration of selDerivedTypeSpec into the if condition to reduce its scope.

sameeranjoshi abandoned this revision.Jun 16 2020, 7:01 AM

Note that a Bugzilla report has been filed on this -- https://bugs.llvm.org/show_bug.cgi?id=46789. @sameeranjoshi , can you please take a look?