PSyclone icon indicating copy to clipboard operation
PSyclone copied to clipboard

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

Open JulienRemy opened this issue 1 year ago • 10 comments

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.

JulienRemy avatar Jul 02 '24 10:07 JulienRemy

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 avatar Jul 02 '24 10:07 JulienRemy

@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.

JulienRemy avatar Jul 16 '24 01:07 JulienRemy

Codecov Report

:white_check_mark: All modified and coverable lines are covered by tests. :white_check_mark: Project coverage is 99.88%. Comparing base (8736ae3) to head (cbc5857). :warning: Report is 3167 commits behind head on master.

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     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

codecov[bot] avatar Nov 29 '24 18:11 codecov[bot]

@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.

JulienRemy avatar Nov 29 '24 22:11 JulienRemy

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

arporter avatar Dec 02 '24 10:12 arporter

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: image 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: image

arporter avatar Dec 02 '24 11:12 arporter

Thanks @arporter I had missed a few things related to psyGen (kernels get renamed, so procedure pointers must as well). All --compile tests now pass locally. Hopefully the integration tests will as well... or they'll reveal new bugs :)

JulienRemy avatar Dec 02 '24 17:12 JulienRemy

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

JulienRemy avatar Dec 11 '24 16:12 JulienRemy

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

JulienRemy avatar Dec 19 '24 14:12 JulienRemy

@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?

arporter avatar Dec 20 '24 10:12 arporter