(Closes #2772) fix for case where base of structure access is an array.

[arporter]

No description provided.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.90%. Comparing base (2611d11) to head (fc54d33).
⚠️ Report is 37 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #3182   +/-   ##
=======================================
  Coverage   99.90%   99.90%           
=======================================
  Files         376      376           
  Lines       53048    53070   +22     
=======================================
+ Hits        52996    53018   +22     
  Misses         52       52           

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[arporter]

Ready for a first look. Unfortunately we can't currently run the integration tests and that will need to be done before this gets merged.

[sergisiso]

@arporter I am not sure I agree with this fix. Or at least I want to discuss it first.

The proposed fix is inconsistent with what happens with Reference. In reference we allow References to array symbol. And then we apply Reference2ArrayRange. For a similar solution here we just need to allow the check at src/psyclone/psyir/nodes/structure_reference.py to pass with Array<DataTypeSymbol or Unknown/unresolved types.

Another question is if we should go the opposite direction an always add ranges when we know it is an array (for both Reference and StructureReferences). I don't particularly disagree with this, in fact we already do this in "where constructs" in the _array_syntax_to_indexed method, and having this is a prerequisite of other transformations. So should we do it everywhere?

The advantage of the second is that we don't need to apply at the beginning of scripts as we do with NEMO, the disadvantage is that bug in this logic will become passthrough bugs - technically already is if it happens inside the where, and it will be harder to manually create this PSyIR constructs. (To be clear, none of the options allow us to assume the References/StructureReference do not point to an array, as they can still have unresolved symbols. We have a bugs regarding this because some of our transformations assume that if there are no indices is an scalar, which is not true.)

So, even if we don't fix the whole thing here, where do we want to go with this?

[arporter]

I was going to argue in favour of changing other places in the frontend so that they do the same thing. However, I've just tried and immediately realised that it's going to be difficult to avoid re-introducing the bug where we pass array(:) to a call when it was previously just array. It's much easier to make sure we don't make this mistake when we apply the transformation so I'll change the solution I've adopted here.

[arporter]

I've undone my change to the frontend and am now working at resolving #1858 by updating Reference2ArrayRangeTrans to support structures.