(Closes #2642, initial step towards #2643) Implement `extends(type)`, `procedure` in derived type, `class` in declarations. #2644

JulienRemy · 2024-07-02T10:48:27Z

This adds the following object- and inheritance-oriented features:

an extends attribute in StructureType for inheritance of derived types, which avoids relying on UnsupportedFortranType and its declaration: str attribute,
a procedure_components attribute in StructureType for procedures in a contains block of a derived type, including visibility and initial value,
support for class (versus type) in routine declarations, by introducing a boolean attribute in DataTypeSymbol.

Note that as this makes some derived types supported there is for now a workaround using FortranWriter in both GOceanKernelMetadata and LFRicKernelMetadata, which rely on UnsupportedFortranType().declaration and fparser, see #2643.

…ord.

JulienRemy · 2024-07-02T10:51:53Z

This will require me to edit the documentation, including (but maybe not limited to) https://psyclone-dev.readthedocs.io/en/latest/psyir_symbols.html#datatypes

JulienRemy · 2024-07-16T01:07:28Z

@arporter @sergisiso This works fine in practice on our project.
I’ll need to add some more specific tests, see what’s up with the master branch conflicts (Aidan’s routines things?) and refactor this thing for sure so it’s going to stay a draft until next week at least.
But if you have time for a broad overview at some point I’m definitely interested in it :) Don’t bother with the details, but if something doesn’t agree with what you’d want in there, feel very free to point it out and I’ll rework it. Most especially: I’m not so sure the way I’m getting the fparser attributes in the frontend is very clean at times.

codecov · 2024-11-29T18:58:35Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.88%. Comparing base (8736ae3) to head (cbc5857).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2644      +/-   ##
==========================================
- Coverage   99.88%   99.88%   -0.01%     
==========================================
  Files         357      357              
  Lines       49823    50214     +391     
==========================================
+ Hits        49767    50156     +389     
- Misses         56       58       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

JulienRemy · 2024-11-29T22:03:20Z

@sergisiso @arporter Two remaining lines still to be tested for codecov (I’ll do this on Monday) but this is otherwise ready for review.
It might be nice to fire the integration tests to be sure it breaks nothing in lfric due to the metadata extraction workaround. I’m arguably going a bit blind on that front.
Also interested on your opinion on my having to xfail one test in due to added support: src/psyclone/tests/psyad/domain/lfric/test_lfric_adjoint_harness.py There’s probably some better way to deal with the PSyclone error on these two routines not being imported in the TL source code.

arporter · 2024-12-02T10:33:19Z

Thanks @JulienRemy. I've set the integration tests going...

arporter · 2024-12-02T11:19:04Z

Unfortunately both the LFRic integration tests and the compilation tests failed. You should be able to reproduce the latter by running pytest with the --compile flag. The error is:

One of the tests that failed is:

tests/psyir/frontend/fparser2_select_type_test.py

The LFRic integration tests failed when linking the LFRic executable:

arporter

Many thanks for this Julien, it's a big change, especially since it impacts the metadata handling. In general I think it's good although there are a few things to discuss and I can't remember all of them now as it's taken me several days to get this first review done. However, it's all in the comments so we can work through them in the usual way.

src/psyclone/domain/lfric/kernel/lfric_kernel_metadata.py

src/psyclone/domain/gocean/kernel/psyir.py

src/psyclone/domain/lfric/kernel/lfric_kernel_metadata.py

src/psyclone/psyGen.py

src/psyclone/tests/psyir/frontend/fparser2_derived_type_test.py

src/psyclone/tests/psyir/frontend/fparser2_test.py

arporter · 2024-12-10T16:39:59Z

src/psyclone/tests/psyir/frontend/fparser2_test.py

+        "  use kernel_mod, only : parent_type\n"
+        "  type, extends(parent_type) :: test_type\n"
+        "    integer :: i = 1\n"
+        "    contains\n"


Please could you extend this test so that the type contains a private statement. The visibility of the test_code procedure should then become private.

Done, it works :)

Not quite and unfortunately I think it's broken (see below) ! :-)

src/psyclone/tests/psyir/symbols/datatype_test.py

JulienRemy · 2024-12-11T16:20:17Z

Thanks for your review @arporter :)
I've pushed my edits and detailed some answers above as well.

arporter · 2024-12-18T10:09:07Z

src/psyclone/psyir/frontend/fparser2.py

+                        if type(extends_symbol) is Symbol:
+                            extends_symbol.specialise(DataTypeSymbol)
+                            extends_symbol.datatype = StructureType()
+                    # If it is not in the symbol table, create a new


Please move this comment into the else: block as that's what it applies to. If we are creating a Symbol then it should be added to a SymbolTable. Given that the lookup hasn't found it, it will have to have an Unresolved interface. That means we know it exists somewhere but we don't know how it is brought into scope.

You're right. This is done!

src/psyclone/domain/gocean/kernel/psyir.py

arporter · 2024-12-18T20:02:41Z

src/psyclone/psyir/frontend/fparser2.py

@@ -3268,6 +3339,10 @@ def _add_target_attribute(var_name, table):
            (e.g. a routine argument or an imported symbol).

        '''
+        # pylint: disable=import-outside-toplevel


Sorry to be a pain but please could you move the pylint disable down too.. In fact, could you also change it to be # pylint: disable-next=... as that's even tighter.

arporter · 2024-12-18T20:08:01Z

src/psyclone/psyir/frontend/fparser2.py

+        :param tsymbol: the DataTypeSymbol representing the derived-type.
+        :type tsymbol: :py:class:`psyclone.psyir.symbols.DataTypeSymbol`
+
+        :returns: None


No need for a :returns: if it doesn't return anything :-)

arporter · 2024-12-18T20:09:06Z

src/psyclone/psyir/frontend/fparser2.py

+        '''
+
+        contains_blocks = walk(decl, Fortran2003.Type_Bound_Procedure_Part)
+        if contains_blocks:


To reduce subsequent indentation, I suggest changing this to:

if not contains_blocks: return

arporter · 2024-12-18T20:13:34Z

src/psyclone/psyir/frontend/fparser2.py

+                # `testkern_code` in `procedure :: code => testkern_code`
+                if procedure.items[4] is not None:
+                    initial_value_name = procedure.items[4].string
+                    initial_value_symbol = parent.symbol_table.lookup(


I think find_or_create (https://psyclone-ref.readthedocs.io/en/latest/_static/html/classpsyclone_1_1psyir_1_1symbols_1_1symbol__table_1_1SymbolTable.html#ada706dd884505330c3d24b9b9a738042) would simplify things here. You can pass the symboltype and data_type that you want (if it doesn't exist) as arguments.

Done, it's indeed clearer.

src/psyclone/psyir/frontend/fparser2.py

arporter · 2024-12-18T20:54:58Z

src/psyclone/psyir/symbols/datatypes.py

+        :raises TypeError: if the new value is not a Reference to a
+                           RoutineSymbol.
+
+        :returns: None


Please remove this line.

arporter · 2024-12-18T20:57:25Z

src/psyclone/psyir/symbols/datatypes.py

+                    new_datatype,
+                    procedure_component.visibility,
+                    new_value)
+                return


Can a datatype contain more than one procedure with the same initial value? I would guess that it can. Same goes for the early return at L1216. Please also add a test for this case.

Agreed about the early returns, there could be multiple procedure, ... => old_name to replace in there. They're removed and I've added a test with multiple procedure components (supported and not).

arporter · 2024-12-18T20:58:48Z

src/psyclone/psyir/symbols/datatypes.py

+                or procedure_component.initial_value.name.lower()
+                    != old_value_name.lower()):
+                continue
+            self._procedure_components[name] = self.ComponentType(


If we made ComponentType mutable (i.e. not frozen) this would be a bit simpler - in case that swings the argument in your other PR that @sergisiso is reviewing.

I guess keeping it frozen makes sense, otherwise we get a Symbol-equivalent that's mutable? Feels a bit dangerous.

arporter · 2024-12-18T21:04:40Z

src/psyclone/tests/psyGen_test.py

+    ''' Check that the CodedKern rename method renames the kernel in the PSyIR
+    tree. '''
+    # pylint: disable=protected-access, too-many-statements
+    _, invoke_info = parse(os.path.join(BASE_PATH, "1_single_invoke.f90"),


I'll give you that but they are old tests :-) It's:

psy, invoke_info = get_invoke("1_single_invoke.f90", "lfric", dist_mem=False)

arporter · 2024-12-19T10:02:04Z

src/psyclone/tests/psyir/frontend/fparser2_test.py

+    assert isinstance(sym.datatype, DataTypeSymbol)
+    assert sym.datatype.is_class is True
+    assert sym.name == "urgh"
+    assert sym.datatype.name == "*"


As discussed in the implementation, I think this should either be UnsupportedFortranType (the easiest thing to do) or an UnresolvedFortranType with an is_class==True attribute.

arporter · 2024-12-19T10:10:56Z

src/psyclone/tests/psyir/frontend/fparser2_test.py

+        "  end type test_type\n" in result)
+
+    # private type that contains procedures
+    # the `test_code` procedure should thus become private


I don't think this is correct. You've put the private attribute on the declaration of test_type so test_type will be private - this says nothing about the visibility of the components within it. What I meant was:

type my_type private contains procedure :: this_should_be_private end type

Oops, I had misunderstood. Indeed, components should not inherit the derived type visibility and my implementation was plain wrong. Thanks for spotting this!

I checked the standard and implemented visibility for procedure components using a "standalone" private in the contains part.
However, note that private statements in the data components and procedure components parts are independent (see e.g. NOTE 4.44 p.74 of https://dci.dci-gitlab.cines.fr/webextranet/_downloads/91b30eb00b39f6896fae528e5d0d66fc/Fortran_2003_N1601.pdf) so that your code above would not modify the visibility of this_should_be_private whereas this would:

type my_type contains private procedure :: this_should_be_private end type

I've fixed this and added tests where the standalone private statement either does or does not impact the visibility of a procedure and of a data component.

arporter · 2024-12-19T10:12:11Z

src/psyclone/tests/psyir/frontend/fparser2_test.py

+        "  type, extends(parent_type), private :: test_type\n"
+        "    integer, public :: i = 1\n"
+        "    contains\n"
+        "      procedure, private :: test_code\n"


This should remain public I think? Worth keeping this test in addition to one where you add a private statement inside the type.

You're totally right, see above.

arporter · 2024-12-19T10:12:48Z

src/psyclone/tests/psyir/frontend/fparser2_test.py

+        "      procedure (my_interface) :: test_code\n"
+        "  end type test_type\n" in result)
+
+    # type that contains procedures with pass, nopass and deferred


As with other tests elsewhere, please could you break this one up into separate tests, as indicated by the comments that you have already.

arporter · 2024-12-19T10:14:29Z

src/psyclone/tests/psyir/frontend/fparser2_test.py

+        "  use kernel_mod, only : parent_type\n"
+        "  type, extends(parent_type) :: test_type\n"
+        "    integer :: i = 1\n"
+        "    contains\n"


Not quite and unfortunately I think it's broken (see below) ! :-)

src/psyclone/tests/psyir/symbols/datatype_test.py

arporter

Looking pretty good now, thanks Julien.
We will need to update the frontend a little to handle class(*) - I've suggested a couple of options.
Also, I think the visibility of members of a type needs a little work.
I'll set the integration tests running now in case that shows any other issues.

JulienRemy · 2024-12-19T14:44:49Z

@arporter All edits except for those about class(*) have been done, pending a decision about these.

arporter · 2024-12-20T10:14:16Z

@hiker and @sergisiso - we could do with your opinion on how to handle declarations of type class(*) - these are "unlimited polymorphic type". My thought is to have these as UnresolvedType and extend that type with an is_class property. That's perhaps going to be non intuitive though as it will mean we then need to generate a declaration for such a Symbol but only if is_class is True (and also, the meaning of UnresolvedType is currently that we can't resolve it but could in principle). We may therefore be better off adding an entirely new type, possibly DeferredType? What do you think?

JulienRemy added 4 commits June 25, 2024 16:02

Add type, extends and procedure support in StructureType.

a527543

Add support for class keyword in declarations.

b95abdc

Cleanup.

349bd2b

#2642 Implement extends, procedure in derived types, class keyw…

f3bb061

…ord.

JulienRemy requested review from sergisiso and arporter July 2, 2024 10:48

JulienRemy marked this pull request as draft July 2, 2024 10:50

JulienRemy added 2 commits July 15, 2024 17:08

#2642 Add missing class keyword in Fortran backend.

db3d952

#2642 Fix replace calls for #2643 workaround.

6d0bb74

Merge branch 'master' into 2642_implement_extends_procedure_and_class

a449c31

JulienRemy marked this pull request as ready for review November 28, 2024 11:05

JulienRemy added 7 commits November 28, 2024 12:44

#2642 Fix flake8 formatting and some tests.

16ed2e4

#2642 Some codecov progress, some tests still missing.

e182c18

Merge branch 'master' into 2642_implement_extends_procedure_and_class

ff993ea

#2642 More tests.

8c789b7

Merge branch 'master' into 2642_implement_extends_procedure_and_class

4247a00

#2642 Edit dev doc

1cd37e4

#2642 Cleanup

0719559

stfc deleted a comment from codecov bot Nov 29, 2024

JulienRemy added 2 commits November 29, 2024 19:41

#2642 More codecov

6698427

#2642 Flake8

cc6fb67

JulienRemy added the ready for review label Nov 29, 2024

JulienRemy linked an issue Nov 29, 2024 that may be closed by this pull request

Unimplemented features: extends inheritance, procedures as components of derived types, class keyword in declarations. #2642

Open

arporter temporarily deployed to integration December 2, 2024 10:32 — with GitHub Actions Inactive

arporter added the under review label Dec 3, 2024

arporter requested changes Dec 10, 2024

View reviewed changes

arporter added reviewed with actions and removed under review labels Dec 10, 2024

JulienRemy added 4 commits December 11, 2024 17:20

#2642 Edits w.r.t. Andy's review

b72336b

#2642 codecov

809ebf2

#2642 flake8...

6f18710

#2642 Refactor, codecov

9419a05

JulienRemy added the ready for review label Dec 17, 2024

arporter added under review and removed ready for review reviewed with actions labels Dec 18, 2024

Merge branch 'master' into 2642_implement_extends_procedure_and_class

421e546

arporter reviewed Dec 18, 2024

View reviewed changes

#2642 Fix parent type to UnresolvedInterface and add to symbol table.

a862a21

arporter reviewed Dec 18, 2024

View reviewed changes

arporter reviewed Dec 19, 2024

View reviewed changes

arporter added reviewed with actions and removed under review labels Dec 19, 2024

arporter requested changes Dec 19, 2024

View reviewed changes

arporter temporarily deployed to integration December 19, 2024 10:22 — with GitHub Actions Inactive

#2642 Edits wrt Andy's second review (all except class(*))

cbc5857

arporter added the question label Dec 20, 2024

arporter assigned arporter, hiker and JulienRemy Dec 20, 2024

(Closes #2642, initial step towards #2643) Implement extends(type), procedure in derived type, class in declarations. #2644

Are you sure you want to change the base?

(Closes #2642, initial step towards #2643) Implement extends(type), procedure in derived type, class in declarations. #2644

Conversation

JulienRemy commented Jul 2, 2024

JulienRemy commented Jul 2, 2024

JulienRemy commented Jul 16, 2024

codecov bot commented Nov 29, 2024 • edited Loading

Codecov Report

JulienRemy commented Nov 29, 2024

arporter commented Dec 2, 2024

arporter commented Dec 2, 2024

arporter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JulienRemy commented Dec 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JulienRemy Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arporter left a comment

Choose a reason for hiding this comment

JulienRemy commented Dec 19, 2024

arporter commented Dec 20, 2024

(Closes #2642, initial step towards #2643) Implement `extends(type)`, `procedure` in derived type, `class` in declarations. #2644

(Closes #2642, initial step towards #2643) Implement `extends(type)`, `procedure` in derived type, `class` in declarations. #2644

codecov bot commented Nov 29, 2024 •

edited

Loading

JulienRemy Dec 19, 2024 •

edited

Loading