DUC unsupported type checks - #3492
Conversation
…rted types correctly
…pported types as they could be pointers and target keyword symbols
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3492 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 397 397
Lines 55587 55687 +100
==========================================
+ Hits 55587 55687 +100 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
… of assignments being ignored sometimes
…e into duc_unsupported_type_checks
|
@sergisiso @arporter This is ready for a first look - it handles edge cases in the DUC where we need to force any UnsupportedType reference to match to any UnsupportedType reference as we can't guarantee there is no aliasing between them (due to |
arporter
left a comment
arporter
left a comment
There was a problem hiding this comment.
Thanks Aidan. I have to confess that I don't really understand the implementation (although I'm getting there) so I've mainly focused on whether it looks sensible. I do think it could do with being broken up a bit as the methods are huge - see inline.
Thanks for all the new tests - I've requested quite a few changes to those to help me to understand them. I'll do the ITs next time.
| assert reaches[3] is routine.walk(Assignment)[4].rhs.children[0] | ||
|
|
||
|
|
||
| def test_definition_use_chains_forward_accesses_unsupported_type( |
There was a problem hiding this comment.
I've not yet looked at these tests but if you could update them in similar ways to the 'backward' tests then I'll look next time.
|
@arporter Should be ready for another look now - some of the comments you had were repeats so I just put done on a single comment instead of each. |
arporter
left a comment
arporter
left a comment
There was a problem hiding this comment.
Looking much better now, thanks Aidan. I think I'm getting my head around it. My main concern now is that the new methods update state both via an argument and via internal object state. I think it would be better if they did exclusively one or the other but am open to be persuaded. A little bit of renaming might help make this clearer too.
Apart from that, it's just tidying. There's a merge conflict so I won't run any tests this time.
… commit if needed
|
@arporter Ready for another look - I didn't comment on repeats necessarily inline but I should have hit them all. |
arporter
left a comment
arporter
left a comment
There was a problem hiding this comment.
Thanks very much @LonelyCat124, this is proving very educational for me :-)
There are a couple of places where you only look at the first node in the parse tree of a CodeBlock and I'm not convinced that that's safe. Apart from that, it's mostly a bit more clarification and tidying.
| # CodeBlocks only find symbols, so we can only do as good | ||
| # as checking the symbol - this means we can get false | ||
| # positives for structure accesses inside CodeBlocks. | ||
| if isinstance(reference.parse_tree_nodes[0], Goto_Stmt): |
There was a problem hiding this comment.
Is only looking at the first parse-tree node sufficient?
There was a problem hiding this comment.
It is not, good spot, I forgot we would combine codeblocks so if we had e.g. print then goto the current test fails.
| # If we find an Exit or Cycle statement, we can't | ||
| # reach further in this code region so we can return. | ||
| if isinstance( | ||
| reference.parse_tree_nodes[0], (Exit_Stmt, Cycle_Stmt) |
There was a problem hiding this comment.
Again, is only looking at the first parse-tree node sufficient?
|
|
||
| :param reference: The CodeBlock being analysed. | ||
|
|
||
| :returns: whether the calling function should terminate. |
| self._defsout[sig] = [] | ||
| self._defsout[sig].append(reference) | ||
| else: | ||
| # Reference outside an Assignment - read only. This could be |
There was a problem hiding this comment.
What about loop variables (now that they're part of the tree)?
There was a problem hiding this comment.
Sergi added #3486 about this - loop variables are handled otherwise, so we should hit the loop itself still at the moment.
| assign = reference.ancestor(Assignment) | ||
| if assign is not None: | ||
| if assign.lhs is reference: | ||
| for i, sig in enumerate(self._reference_signatures): |
There was a problem hiding this comment.
Aha! I think I had worked that out by the end of my last review, thanks to the tests. Please could you add a comment. Perhaps "every signature" at L697/8 is "every signature of a reference to an UnsupportedType"?
| end subroutine test""" | ||
| psyir = fortran_reader.psyir_from_source(code) | ||
| assign = psyir.walk(Assignment)[-1] | ||
| sig = assign.rhs.get_signature_and_indices()[0] |
| psyir = fortran_reader.psyir_from_source(code) | ||
| assigns = psyir.walk(Assignment) | ||
| assign = assigns[-1] | ||
| sig = assign.lhs.get_signature_and_indices()[0] |
|
|
||
| def test_backward_accesses_unsupported_type_lhs_ends_chain(fortran_reader): | ||
| """Test that if the lhs of a found assignment is an UnsupportedType | ||
| then we skip any UnsupportedTypes on the rhs""" |
There was a problem hiding this comment.
To make sure I understand: we're going backwards so the lhs comes first and if it's an UnsupportedType, then it ends(?) the chain for the assigned-to signature?
| routine = psyir.walk(Routine)[1] | ||
| assigns = routine.walk(Assignment) | ||
| assign = assigns[-1] | ||
| sig = assign.lhs.get_signature_and_indices()[0] |
| assign = psyir.walk(Assignment)[-1] | ||
| a_sig = assign.lhs.get_signature_and_indices()[0] | ||
| c_sig = assign.rhs.get_signature_and_indices()[0] | ||
| chains = DefinitionUseChain([assign.lhs, assign.rhs]) |
There was a problem hiding this comment.
Might be a silly question but could you do DefinitionUseChain(assign) instead?
arporter
added
ready for review
reviewed with actions
and removed
under review
ready for review
labels
2 participants
No description provided.