FIX (GraphEngine): @W-12696959@: UnusedMethodRule now fully supports constructors. #1020
Conversation
| // This means we need to do more, but what that "more" is depends on whether we're looking | ||
| // at an inner or outer type. | ||
| boolean typeIsInner = constructedType.contains("."); | ||
| if (typeIsInner) { |
There was a problem hiding this comment.
The contents of this if branch are very similar to those of the one at line 316, but I couldn't see a way to cleanly refactor them into a single method. In the fullness of time, we may want to revisit that.
There was a problem hiding this comment.
How about extracting a method and passing in NodeType.<vertex_type>, inner class name, and outer class name as input?
The name-splitting could become a util method in ClassUtil.
| * @param referencedType - The outer type with which inner types may collide | ||
| * @return - {@code unfilteredResults}, with all colliding references removed | ||
| */ | ||
| private <T extends BaseSFVertex> List<T> removeInnerTypeCollisions( |
There was a problem hiding this comment.
The else-branch from getInvocationsOnType and getConstructionsOfType are identical, and have been refactored into their own method.
| */ | ||
| @ValueSource(ints = {0, 1}) | ||
| @ParameterizedTest(name = "{displayName}: Arity {0}") | ||
| public void constructorCalledViaNew_expectNoViolation(Integer arity) { |
There was a problem hiding this comment.
New test for simply calling a constructor for one class in another class. This sort of thing is arguably covered by the tests for inner classes, but I felt that having an explicit test for it did no harm.
| @ValueSource(strings = {"OuterClass.InnerClass", "InnerClass"}) | ||
| @ParameterizedTest(name = "{displayName}: OuterClass.InnerClass constructed as new {0}()") |
There was a problem hiding this comment.
The syntax with which the variable was declared ended up not impacting how the rule executes, so I removed the ability to configure that and cut the number of cases.
There was a problem hiding this comment.
How about we leave the initial set of ways of calling so that we'll know if there's ever a regression while making changes?
| @Disabled | ||
| public void innerConstructorUsedBySiblingInner_expectNoViolation( | ||
| String variable, String constructor) { | ||
| @ValueSource(strings = {"OuterClass.InnerClass", "InnerClass"}) |
There was a problem hiding this comment.
As above, variable declaration didn't terribly matter, so it was hardcoded.
There was a problem hiding this comment.
Same as above. If we have a change in mechanism, the additional cases will be helpful to look for regressions.
| */ | ||
| @ValueSource(ints = {0, 1}) | ||
| @ParameterizedTest(name = "{displayName}: Constructor of arity {0}") | ||
| public void externalReferenceSyntaxCollision_expectViolation(Integer arity) { |
There was a problem hiding this comment.
It seemed valuable to have tests for this kind of name ambiguity, so I added one.
| + " }\n" | ||
| // Use the engine directive to prevent this method from tripping the rule. | ||
| + " /* sfge-disable-stack UnusedMethodRule */" | ||
| + " /* sfge-disable-stack UnusedMethodRule */\n" |
There was a problem hiding this comment.
The missing newline meant that this directive was actually on the same line as the constructor and therefore wasn't being applied. Since public constructors weren't supported yet, that caused no problems. Adding support for public constructors caused the tests to start failing, and adding the newline fixes that.
This PR does the following:
UnusedMethodRulenow properly detects invocations of private/protected constructors via thenewkeyword (Thereby also resolving issue [BUG] SFGE reports false positives for constructors only called within a given class #1001)UnusedMethodRulenow fully support public constructors