Fixes for missing position information and invocation ordering problem #262
Conversation
|
Hey! Thank you for your contribution. I am sorry it slipped my mind to look at this after I approved the workflow run. I will try to look at it by the week's end. I hope you can use the snapshot build of the project to satisfy your use case for the time being. :) |
There was a problem hiding this comment.
Hi @benmss ! Sorry, it took so long, but I thank you again for the contribution. However, there are some changes that I would suggest.
Firstly, since this PR does two things - Missing Position Information and Invocation Ordering. We should have two separate PRs for it. It makes reviewing efficient.
Secondly, I have looked at the first issue thoroughly and suggested the change in my comments. Please have a look at that and let me know if you disagree with any.
Finally, I have had a cursory look at the second fix because I thought we should focus on the "missing position" issue first.
| CompilationUnit cu = null; | ||
| Integer sourceStart = null; | ||
| int sourceEnd = 0; | ||
| for (var position : positions) { | ||
| if (position instanceof NoSourcePosition) { continue; } | ||
| if (sourceStart == null || position.getSourceStart() < sourceStart) { | ||
| sourceStart = position.getSourceStart(); | ||
| } | ||
| if (position.getSourceEnd() > sourceEnd) { | ||
| sourceEnd = position.getSourceEnd(); | ||
| } | ||
| if (cu == null) { cu = position.getCompilationUnit(); } | ||
| } | ||
| if (sourceStart != null) { | ||
| SourcePosition virtualPosition = new SourcePositionImpl(cu, sourceStart, sourceEnd, cu.getLineSeparatorPositions()); | ||
| virtualElement.setPosition(virtualPosition); | ||
| } |
There was a problem hiding this comment.
I think you can get the sourceStart from the first modifier and sourceEnd from the last modifier.
@I-Al-Istannen the set returned by method.getExtendendedModifiers is ordered by the source position of the the modifier, right?
There was a problem hiding this comment.
Preetty sure that this would be accidental. The CtModifierHandler creates a hashset. See also INRIA/spoon#4033 for reference. Just don't try to fix that in spoon if you can't spare a week :^)
| @@ -171,7 +214,9 @@ public <A extends Annotation> void visitCtAnnotation(CtAnnotation<A> annotation) | |||
| for (Map.Entry<String, CtExpression> entry: annotation.getValues().entrySet()) { | |||
| Tree annotationValueNode = builder.createNode("ANNOTATION_VALUE", entry.toString()); | |||
| annotationNode.addChild(annotationValueNode); | |||
| annotationValueNode.setMetadata(SpoonGumTreeBuilder.SPOON_OBJECT, new CtWrapper(entry, annotation, CtRole.VALUE)); | |||
| var wrapper = new CtWrapper(entry, annotation, CtRole.VALUE); | |||
There was a problem hiding this comment.
I personally avoid using var as I think it hampers readability. I think it is more readable when you can directly figure out the type.
There was a problem hiding this comment.
Not a problem, I will remove all added uses.
| /* | ||
| /* ***************************************************************************** | ||
| * Copyright 2016 Matias Martinez | ||
| * Copyright (c) 2022, Oracle and/or its affiliates. | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| * *****************************************************************************/ |
There was a problem hiding this comment.
This is unrelated to the review process, but I am curious about adding license. @monperrus, do you know on what basis we should add names to license? For example, this file has been revised by many contributors, so why only Matias' name be there?
There was a problem hiding this comment.
Matias' name was added only because it already exists as a copyright header in one of the test files. Can change it to whatever you feel best represents the work done.
As to why the headers were added: it is a requirement set by Oracle for this kind of contribution.
| String left = "class A { static void a() {} }"; | ||
| String right = "class A { private static synchronized void a() {} }"; | ||
| Diff diff = new AstComparator().compare(left, right); | ||
| diff.getRootOperations().forEach(e -> assertFalse(e.getSrcNode().getPosition() instanceof NoSourcePosition)); |
There was a problem hiding this comment.
I like the test case overall. I would suggest that you compare the exact position like /test:1 or the expected source line, character start, and character end of each modifier. It makes the test stronger.
| @@ -601,4 +619,19 @@ public void test_nestedReturnTypeOfMethodShouldGetParsed() throws Exception { | |||
| assertEquals(1, returnType.getDescendants().size()); | |||
| assertEquals("java.lang.Integer", returnType.getChild(0).getLabel()); | |||
| } | |||
|
|
|||
| @Test | |||
| public void test_AccessModifierShouldHaveSourcePosition() { | |||
There was a problem hiding this comment.
There are two ways to pass code to compare. You can use the string code like you have done or you could pass a path to a file. I suggest adding a test for the latter case as well.
| * This class wraps any node that appears as an invocation target to provide a means of differentiating | ||
| * between that node and any other child nodes the invocation may have. | ||
| */ | ||
| public class CtTargetWrapper extends CtElementImpl { |
There was a problem hiding this comment.
Can we use CtWrapper instead to wrap the target? CtTargetWrapper seems to be designed exactly like CtWrapper. You could also look at the proposed solution here.
But we can think more about this later. Let's focus on the other issue first.
|
Hi @algomaster99, thanks for taking the time to look through it all. I will separate the second fix out of this PR as you suggest, and makes the other changes as above where applicable. |
|
@benmss time to move to the second PR :) |
Two issues are fixed in this pull request:
Missing Position Information
CtVirtualElement and CtWrapper objects do not contain the related position information. As this information is still useful, especially for debugging purposes, this fix correctly maps the objects to their position information during creation. For the case where CtWrapper objects represent more than one code element, the positions are merged.
Relates to: Issue 199 and Issue 132
Invocation Ordering
In the AST, invocation nodes have two types of child nodes: Targets and Arguments. During matching these two types of child nodes are considered to be equal, leading to problems where reordering the target or arguments of an invocation produces no AST change. This fix creates a new class, CtTargetWrapper, that behaves similarly to CtVirtualElement, wrapping any invocation targets so they are treated differently to other invocation child nodes.
Relates to: Issue 126