This is an archive of the discontinued LLVM Phabricator instance.

[Flang] Fix for the scenario when type guard has intrinsic type specification and Selector is NOT unlimited Polymorphic.
ClosedPublic

Authored by inderjeet-hcl on Jul 21 2020, 7:03 PM.

Details

Summary

There are 2 issues:

    • Bug 46789 : Expected compile time error is not coming for Select type specification. Compile time error should be generated if Selector is NOT unlimited Polymorphic and Intrinsic type specification is specified in Type Guard statement.
  • Bug 46830 : Semantic error is not coming for duplicate intrinsic type specification in type guard statement.

Bug 46789 : As per below Fortran 2018 specification, compile time error should be generated if Selector is NOT unlimited Polymorphic and Intrinsic type specification is specified in Type Guard statement.

C1162 (R1152) If selector is not unlimited polymorphic, each TYPE IS or CLASS IS type-guard-stmt shall specify an extension of the declared type of selector.

Test program: Error is expected at line #16 as selector is NOT class(*).

[root@localhost Select_type_fix]# cat -n selecttype04.f90

 1	type base
 2	 integer :: ii
 3	end type
 4	type,extends(base) :: ext
 5	 integer :: jj
 6	end type
 7	call CheckC1162()
 8	contains
 9	  subroutine CheckC1162()
10	   class(base),allocatable :: aobj
11	   allocate(ext::aobj)
12	   select type(sel=>aobj)
13	    ! OK
14	    class is(base)
15	    !ERROR: Intrinsic Type specification must not be specified
16	    type is(integer)
17	    ! OK
18	    class default
19	   end select
20	  end
21	end
  • Gfortran behavior is correct, expected compilation Error is coming.

[root@localhost Select_type_fix]# gfortran selecttype04.f90
selecttype04.f90:16:12:

type is(integer)
       1

Error: Unexpected intrinsic type ‘INTEGER’ at (1)

Diff Detail

Event Timeline

inderjeet-hcl created this revision.Jul 21 2020, 7:03 PM
Herald added a project: Restricted Project. · View Herald Transcript

Additional compilation error will come in selecttype01.f90 and symbol11.f90 test cases and it is expected with the patch. Also I am uploading patch file to add "RUN: line" in newly added test case selecttype04.f90.

  1. selecttype01.f90: Addition error will come at line 122 and 130 selecttype01.f90:122:3: error: If selector is not Unlimited Polymorphic, intrinsic type specification must not be specified in type guard statement type is (integer) ^^^^^^^^^^^^^^^^^

    selecttype01.f90:130:3: error: If selector is not Unlimited Polymorphic, intrinsic type specification must not be specified in type guard statement type is (integer) ^^^^^^^^^^^^^^^^^
  1. symbol11.f90: Addition error will come at line 74.

symbol11.f90:74:3: error: If selector is not Unlimited Polymorphic, intrinsic type specification must not be specified in type guard statement

type is (integer(kind=8))
^^^^^^^^^^^^^^^^^^^^^^^^^

sameeranjoshi added a comment.EditedJul 22 2020, 2:58 AM

Additional compilation error will come in selecttype01.f90 and symbol11.f90 test cases and it is expected with the patch. Also I am uploading patch file to add "RUN: line" in newly added test case selecttype04.f90.

  1. selecttype01.f90: Addition error will come at line 122 and 130 selecttype01.f90:122:3: error: If selector is not Unlimited Polymorphic, intrinsic type specification must not be specified in type guard statement type is (integer) ^^^^^^^^^^^^^^^^^

    selecttype01.f90:130:3: error: If selector is not Unlimited Polymorphic, intrinsic type specification must not be specified in type guard statement type is (integer) ^^^^^^^^^^^^^^^^^
  1. symbol11.f90: Addition error will come at line 74.

symbol11.f90:74:3: error: If selector is not Unlimited Polymorphic, intrinsic type specification must not be specified in type guard statement

type is (integer(kind=8))
^^^^^^^^^^^^^^^^^^^^^^^^^

Thank you for finding this! Some comments below:

Can you add checks with !ERROR: in the respective files?
Also I never saw someone sending a .patch file for review separately in llvm-project, you need to include the changes with the revision itself.

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

I tried to minimize the use of passing stmt as a parameter by using parser::FindSourceLocation(typeSpec) it was strange that I couldn't see the source location information printed.

Does parser::FindSourceLocation fail in some scenarios @PeteSteinfeld ?
If that's the case using stmt is completely fine here.

86

Why are u and p capitalized?
Read more on error message to user guideline:
https://github.com/llvm/llvm-project/blob/master/flang/documentation/C%2B%2Bstyle.md#error-messages

101–102

How about adding an else if statement here?

if (){
...
} else if (!selectorType_.IsUnlimitedPolymorphic() && spec->AsIntrinsic()) {  // C1162
  ...
}
flang/test/Semantics/selecttype04.f90
1 ↗(On Diff #279692)

There's already a file with a test for C1162, can you add these tests in it?

You could have reduced the effort of writing summary, by just pointing to the bugzilla issue.

sameeranjoshi retitled this revision from Fix for the scenario when type guard has intrinsic type specification and Selector is NOT unlimited Polymorphic. to [Flang] Fix for the scenario when type guard has intrinsic type specification and Selector is NOT unlimited Polymorphic..Jul 22 2020, 3:01 AM
sameeranjoshi added a subscriber: flang-commits.
inderjeet-hcl marked 4 inline comments as done.Jul 22 2020, 5:36 AM

@sameeranjoshi, Kindly find reply inline and let me know if it seems fine.

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

Kindly refer following case when line number information is not output in error message.
In case of intrinsic types 'integer','character' etc. parser::FindSourceLocation(typespec) is returning empty:

[root@localhost Github_compiler]# f18 selecttype.f90
error: The type specification statement must have LEN type parameter as assumed
f18: semantic errors in selecttype.f90
[root@localhost Github_compiler]# cat -n selecttype.f90

1	class(*),allocatable :: cptr
2	
3	select type(cptr)
4	 !ERROR: The type specification statement must have LEN type parameter as assumed
5	 type is(character) !<-- assumed length-type
6	end select
7	end
86

Thanks for sharing document reference. I will update it as follows:

context_.Say(stmt.source,
    "If selector is not unlimited polymorphic, "
    "intrinsic type specification must not be specified "
    "in type guard statement"_err_en_US);
101–102

I think either way is fine. I added 'if' condition at the beginning considering if type specification is of intrinsic type then that should be the first check to be performed.

flang/test/Semantics/selecttype04.f90
1 ↗(On Diff #279692)

I agree, I will update test program selecttype01.f90 to add tests.

@sameeranjoshi. I missed to reply for below points, kindly find the reply below.

Can you add checks with !ERROR: in the respective files?

I will add '!ERROR:' and share updated test programs.

Also I never saw someone sending a .patch file for review separately in llvm-project, you need to include the changes with the revision itself.

Please note that I referred following point in document(https://www.llvm.org/docs/Phabricator.html#code-reviews-with-phabricator) to submit the patch. Kindly let me know if any other method needs to be following to share modified source files and test programs.
"Note that you can upload patches created through git,"

sameeranjoshi added inline comments.Jul 22 2020, 5:51 AM
flang/lib/Semantics/check-select-type.cpp
101–102

I think adding at beginning would never check C1160.

inderjeet-hcl marked an inline comment as done.Jul 22 2020, 6:16 AM

@sameeranjoshi , Kindly refer comment inline.

flang/lib/Semantics/check-select-type.cpp
101–102

I agree, If added at the begging and selector is not uninlimited polymorphic then C1160 will not be checked and return will execute after generating NEW error message for C1162.

However, adding "elseif" will never generate new error if C1160 is generated. Kindly refer below case.
One solution might be to change code and add if condition for 'Intrinsic type' and perform error checks for C1160 and C1162 before returning false.
Kindly suggest, if I should modify source code.

[root@localhost Github_compiler]# ./f18 selecttype.f90
error: The type specification statement must have LEN type parameter as assumed
./f18: semantic errors in selecttype.f90
[root@localhost Github_compiler]# cat -n selecttype.f90

 1	type ty
 2	 integer :: ii
 3	end type
 4	class(ty),allocatable :: cptr
 5	
 6	select type(cptr)
 7	 !ERROR: The type specification statement must have LEN type parameter as assumed
 8	 type is(character) !<-- assumed length-type
 9	end select
10	end

Thanks for working on this!

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

It looks like you could simplify this by just passing the TypeGuardStmt. You can use it to find the TypeGuardStmt::Guard and the source location of the TypeGuardStmt.

86

Here's what I would prefer for the message:

context_.Say(stmt.source,
    "If the selector is not unlimited polymorphic, "
    "an intrinsic type specification must not be specified "
    "in the type guard statement"_err_en_US);

@PeteSteinfeld , Thanks for sharing review comments. I will share the modified source file to accommodate comments.

Kindly review modified sources and test program. Modifications contain fix for Bugs: 46789 and 46830.

Source file check-select-type.cpp is updated for following points:

  • updated text of new message.
  • modified function PassesChecksOnGuard to generate correct messages for Intrinsic type.
  • For better code readability, modified PassesChecksOnGuard to remove extra if/else conditions and added CHECK() .

Following test program are modified:
selecttype01.f90
symbol11.f90

Thank you for working !
This patch builds successfully but fails for check-flang for me.

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

Can you please use GetGuardFromStmt here?

103

DeclTypeSpec::AsDerived might return nullptr. Please wrap it in some conditional statement.

flang/test/Semantics/selecttype01.f90
122

Is this error correct?
I see check-flang failing for both the files.
Did you verify check-flang before submitting this patch?

inderjeet-hcl marked 2 inline comments as done.Jul 27 2020, 6:50 AM

Kindly check reply inline.

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

ok, I will update source code.

flang/test/Semantics/selecttype01.f90
122

sorry, I was not aware of the option check-flang. I will test it before submitting updated files.

Error seems correct, selector should be unlimited Polymorphic in case of Intrinsic type specification.
Function ReportConflictingTypeCases is generating 'INTEGER*4' in error message instead of 'integer' which is specified in test case. Do you have any idea how to update function ReportConflictingTypeCases to display correct name ?

sameeranjoshi added inline comments.Jul 27 2020, 9:25 AM
flang/test/Semantics/selecttype01.f90
122

4 specifies kind type parameter.
I am speaking of
!ERROR : Intrinsic type specification must not be specified
replaced with
!ERROR : If selector is not unlimited polymorphic, an intrinsic type specification must not be specified in the type guard statement

inderjeet-hcl marked 2 inline comments as done.Jul 27 2020, 5:12 PM

Kindly check reply inline and suggest.

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

GetGuardFromStmt is defined in struct TypeCase and can not be called from PassesChecksOnGuard as we do not have object of class TypeCase in function PassesChecksOnGuard.
Kindly suggest if I should pass object of class TypeCase as a parameter to PassesChecksOnGuard or we can keep above modifications without calling GetGuardFromStmt.

flang/test/Semantics/selecttype01.f90
122

I will update comment.

inderjeet-hcl marked an inline comment as done.Jul 27 2020, 5:21 PM

Kindly check comments inline and suggest.

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

I have added CHECK(spec); and CHECK(spec->AsIntrinsic() || spec->AsDerived()); at the start considering if code in block "[&](const parser::TypeSpec &typeSpec)" is executed then spec will always be one of intrinsic or derived type and one of AsIntrinsic() or AsDerived() will return non NULL value.
Considering above description, I think AsDerived() will not return NULL at this location as this code is present in 'else' of AsIntrinsic().
Kindly suggest.

sameeranjoshi added inline comments.Jul 27 2020, 10:53 PM
flang/lib/Semantics/check-select-type.cpp
77

Ok, please revert with the older way.

103

Sorry, I missed it.
Your point seems correct.

Kindly find updated test cases. There is no change in source file.

Kindly review and let me know if any other modification is required.

sameeranjoshi accepted this revision.Jul 28 2020, 2:02 AM

I can build and test successfully.
I have verified bugs Bug 46789 & Bug 46830.
Please wait for a day before you commit until other reviewers accept it.

Thanks for working on it.

This revision is now accepted and ready to land.Jul 28 2020, 2:02 AM
PeteSteinfeld accepted this revision.Jul 28 2020, 7:00 AM

All builds, tests, and looks good.

Thanks for doing this!

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

Please move the declaration of typeSpecRetVal below the calls to CHECK().

inderjeet-hcl edited the summary of this revision. (Show Details)

I have uploaded final patch.
This is my first time to commit changes in LLVM repository, kindly guide how to commit changes to LLVM github repository.
I have referred section "Committing a change" section in "https://www.llvm.org/docs/Phabricator.html#code-reviews-with-phabricator" and it seems I need to use "git push" to push my changes but I think I do not have write permission to the repository.

Here's the process for getting commit access to the repository -- https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access

Thanks for working @inderjeet-hcl. Do let us know if you still see any issues in committing changes.

@sameeranjoshi , thanks. I have received reply from Chris, I think now I can commit my changes. I will try and let you know in case of any issues.

@sameeranjoshi , I have committed modifications to LLVM/FLANG repository. Am I supposed to close PR46789 and PR46830 in Bugzilla ?