Fix type update behaviour in expression clones #138
Conversation
This avoids the standalone parse inserting DEFERRED types into the parent scope, which would override existing type information. Instead we force the new type info to be ignored when rescoping to the given scope.
|
Documentation for this branch can be viewed at https://sites.ecmwf.int/docs/loki/138/index.html |
We no longer ignore DEFERRED type setters...
This constitutes a small internal behaviour change, as we used to prefer local type info and thus override existing info in newly attached scopes.
Codecov Report
@@ Coverage Diff @@
## main #138 +/- ##
=======================================
Coverage 92.10% 92.11%
=======================================
Files 89 89
Lines 16546 16556 +10
=======================================
+ Hits 15240 15250 +10
Misses 1306 1306
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
@reuterbal As discussed offline, this branch now includes a small, but subtle behaviour change to get the expression tests back up and running. For this we now treat a So, please have another look and see if you agree. All other suggestions have been incorporated, let's see if CI can be convinced too. |
loki/expression/symbols.py
Outdated
| @@ -815,16 +819,10 @@ def __new__(cls, **kwargs): | |||
| scope = kwargs.get('scope') | |||
| _type = kwargs.get('type') | |||
|
|
|||
| if scope is not None and (_type is None or _type.dtype is BasicType.DEFERRED): | |||
| if scope is not None and _type is None: | |||
| # Try to determine stored type information if we have no or only deferred type | |||
There was a problem hiding this comment.
The comment is out of date. Given the complexity, it might be worth expanding this here a little bit in general?
There was a problem hiding this comment.
Good spot. Should be fixed now. Thanks!
Note the problem this PR intends to fix is the underlying problem to the symptoms reported in #128. Many thanks to @rolfhm for providing the MFE that helped triage this.
This PR adds a test and fixes certain corner cases for type updates of symbols via
symbol.clone(type-<...>). The key problem is that there were edge-cases where we would preserve existing type info stored in the associated scope of the symbol, if the new type was deemed inadequate. This PR now simplifies this logic and only ignores the incoming type if it isNone, as we do not allowNonetype info in the symbol table (BaseType.DEFERREDsignifies unknown types).In addition, a test was added that pins down the intended behaviour for plain scalar symbols. In the future this could be extended to procedures etc., but this felt overkill for the current problem.
I've also needed to change the scope handling logic in the fparser-based standalone expression parsing utility, where we can never gain type info (no declarations in expressions), but would wipe existing type info by setting it to DEFERRED. We now instead use a dummy to parse expressions and suppress that info in the subsequent rescoping (see inline comments).