-
Notifications
You must be signed in to change notification settings - Fork 13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Correct symbols after derived type enrichment #450
Conversation
Documentation for this branch can be viewed at https://sites.ecmwf.int/docs/loki/450/index.html |
41b37f6
to
9fcbb07
Compare
This is implicitly used by `SubstatituteExpressions`, but can also run a simple `LokiIdentityMapper` (the default) over any expression.
After the type information has been correctly attached, we can rebuild the epxression trees to replace `DeferredTypeSymbols` with `Scalar` and `Array` symbols.
605166f
to
8a975bf
Compare
We previously (ab)used `ProcedureSymbol` objects for this. The new symbol type also uses the exact same mechanics as procedure symbols for interfacing twith pymbolic types.
8a975bf
to
3aaef08
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #450 +/- ##
==========================================
+ Coverage 93.24% 93.28% +0.03%
==========================================
Files 220 220
Lines 41137 41202 +65
==========================================
+ Hits 38360 38436 +76
+ Misses 2777 2766 -11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Many thanks, this misclassification of imported type names was a bit of a sore point...
Implementation looks good to me. Out of curiosity, how are constructor calls represented now? For example, something like
use some_mod, only: type_name
...
type(type_name) :: obj
...
obj = type_name(arg1, arg2)
Is this an inline call with DerivedTypeSymbol('type_name')
?
assert isinstance(v, sym.ProcedureSymbol) | ||
assert isinstance(v.type.dtype, ProcedureType) | ||
assert v.type.external is True | ||
assert v.type.dtype.procedure == BasicType.DEFERRED |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Codecov thinks these lines are never executed, so it looks to me like the test isn't quite doing what it's meant to do.
This isn't related to this PR but maybe you can have a quick look if you don't mind?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, good spot for codecov! These lines were indeed implicitly skipped, as the test was attempting to check ProcedureDeclaration
nodes, but was iterating VariableDeclaration
, likely as the former had been introduced much later than the test. I've pushed a fix that should rectify that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"mumble, grumble, ... the system works(!), ... grumble"
Yes, the above example will become an |
This re-enables the actual check on the declaration symbols.
Currently sits on top of PR #448 , so needs rebase once that is merged.This PR aims to make the state of subroutine/modules after enrichment with derived type info to the a priori provision of type info via
definitions
. The key difference here is that, after type info has been attached to imports and symbol tables, the expression trees need to be rebuild, so thatDeferredTypeSymbol.clone()
yieldsArray
orScalar
variable symbols.For this purpose, I've introduced a base class
ExpressionTransformer
that takes only the basic expression re-build mechanism fromSubstituteExpressions
and exposes this generically via a staticexpr_mapper
attribute (similar to theExpressionFinder
base class. This defaults toLokiIdentityMapper
, which is enough to rebuild expressions.Along the way to getting this un-muddled in my head, I also got annoyed by derived-type symbols in
Import
nodes being typed asProcedureSymbols
, so I implemented a mechanically equivalentDerivedTypeSymbol
and added a test that uses it to the frontends, to explicitly test the use of type info viadefinitions
to create the right symbol types.Tow other piggy-backed housekeeping changes: