FIX (GraphEngine): @W-13363157@: Handles loop exclusions more effectively #1085
Conversation
rmohan20
commented
Jun 2, 2023
rmohan20
commented
- Handles method invocations from static blocks as permanent loop exclusions.
- Handles nested loops around method-calls inside for-each loop definition. These are now treated as overridable loop exclusions.
There was a problem hiding this comment.
The most active part of the change is here.
| } | ||
|
|
||
| @Test | ||
| public void testNestedLookupFromForEachValueWithEffectiveExclusion() { |
There was a problem hiding this comment.
Extended test case to make sure the exclusion works across many layers.
rmohan20
changed the title
| * @param <T> Stack's generic | ||
| * @return last element added to the stack | ||
| */ | ||
| public static <T> Optional<T> peek(Stack<T> stack) { |
There was a problem hiding this comment.
Moving this here since the logic is needed on a couple of different Stacks and would be helpful to reuse in future as well.
| // TODO: Edge case where a user also creates a method of the same name in their class. | ||
| // Ideally, we need to check if the method definition associated with this call has | ||
| // isSynthetic as true. | ||
| return methodName.startsWith(SYNTHETIC_STATIC_BLOCK_METHOD_BASE); |
There was a problem hiding this comment.
The edge case is pretty far off. We're not even doing a case-insensitive match.
There was a problem hiding this comment.
A potential workaround could be using double-underscore in the name of the synthetic method, since real methods aren't allowed to do t hat.
| return this._isInsideLoop(copyStack); | ||
| } | ||
|
|
||
| private Optional<? extends SFVertex> _isInsideLoop(Stack<? extends LoopBoundary> boundaryStack) { |
There was a problem hiding this comment.
Performs recursive calls until it finds the boundary that actually applies.
| // TODO: Edge case where a user also creates a method of the same name in their class. | ||
| // Ideally, we need to check if the method definition associated with this call has | ||
| // isSynthetic as true. | ||
| return methodName.startsWith(SYNTHETIC_STATIC_BLOCK_METHOD_BASE); |
There was a problem hiding this comment.
A potential workaround could be using double-underscore in the name of the synthetic method, since real methods aren't allowed to do t hat.
| createPermanentLoopExclusionIfApplicable(vertex, loopBoundaryItem); | ||
| createOverridableLoopExclusionIfApplicable(vertex, loopBoundaryItem); |
There was a problem hiding this comment.
Is it possible that the same vertex could produce both a permanent and overridable exclusion? If so, could you please add a comment explaining how?
There was a problem hiding this comment.
Thanks for bringing this up. Missed adding the explanation.
| } | ||
|
|
||
| @Test | ||
| public void testLoopFromStaticBlock_nestedLoop() { |
There was a problem hiding this comment.
Is this loop actually nested?
There was a problem hiding this comment.
Looks like a code merge issue. Good catch.
