Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: ensure sniper pretty-printer prints modifiers and type separated by a space #4296

Merged
merged 11 commits into from
Nov 24, 2021
Merged

fix: ensure sniper pretty-printer prints modifiers and type separated by a space #4296

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
91c3bfa
Pass AST node to set position of type references
algomaster99 Nov 18, 2021
3a69748
Reproduce test case for fixing https://github.com/SpoonLabs/sorald/is…
algomaster99 Nov 19, 2021
fb5482f
Reproduce test case for fixing https://github.com/SpoonLabs/sorald/is…
algomaster99 Nov 19, 2021
40febc5
Add contract
algomaster99 Nov 19, 2021
9ccd262
Merge branch 'set-source-position' of github.com:algomaster99/spoon i…
algomaster99 Nov 19, 2021
c2f6d3d
Move test resources
algomaster99 Nov 19, 2021
3789b69
Fix styling issues
algomaster99 Nov 19, 2021
e2d0933
Assign position to type access before visiting it
algomaster99 Nov 22, 2021
99ee146
Revert style changes to ReferenceBuilder
algomaster99 Nov 22, 2021
3609586
Merge branch 'master' into set-source-position
algomaster99 Nov 23, 2021
1327bd1
Ignore AnnotationPositionTest
algomaster99 Nov 24, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/main/java/spoon/support/compiler/jdt/JDTTreeBuilderHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -445,7 +445,7 @@ CtTypeAccess<?> createTypeAccess(QualifiedNameReference qualifiedNameReference,
final TypeBinding receiverType = qualifiedNameReference.actualReceiverType;
if (receiverType != null) {
final CtTypeReference<Object> qualifiedRef = jdtTreeBuilder.getReferencesBuilder().getQualifiedTypeReference(
qualifiedNameReference.tokens, receiverType, qualifiedNameReference.fieldBinding().declaringClass.enclosingType(), new JDTTreeBuilder.OnAccessListener() {
qualifiedNameReference.tokens, qualifiedNameReference, receiverType, qualifiedNameReference.fieldBinding().declaringClass.enclosingType(), new JDTTreeBuilder.OnAccessListener() {
@Override
public boolean onAccess(char[][] tokens, int index) {
return !CharOperation.equals(tokens[index + 1], tokens[tokens.length - 1]);
Expand Down
16 changes: 11 additions & 5 deletions src/main/java/spoon/support/compiler/jdt/ReferenceBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ <T> CtTypeReference<T> buildTypeReference(QualifiedTypeReference type, Scope sco
CtTypeReference<T> accessedType = buildTypeReference((TypeReference) type, scope);
final TypeBinding receiverType = type != null ? type.resolvedType : null;
if (receiverType != null) {
final CtTypeReference<T> ref = getQualifiedTypeReference(type.tokens, receiverType, receiverType.enclosingType(), new JDTTreeBuilder.OnAccessListener() {
final CtTypeReference<T> ref = getQualifiedTypeReference(type.tokens, type, receiverType, receiverType.enclosingType(), new JDTTreeBuilder.OnAccessListener() {
@Override
public boolean onAccess(char[][] tokens, int index) {
return true;
Expand Down Expand Up @@ -260,12 +260,13 @@ private boolean isTypeArgumentExplicit(TypeReference[][] typeArguments) {
* Builds a type reference from a qualified name when a type specified in the name isn't available.
*
* @param tokens Qualified name.
* @param receiverNode AST node of last type in the qualified name.
* @param receiverType Last type in the qualified name.
* @param enclosingType Enclosing type of the type name.
* @param listener Listener to know if we must build the type reference.
* @return a type reference.
*/
<T> CtTypeReference<T> getQualifiedTypeReference(char[][] tokens, TypeBinding receiverType, ReferenceBinding enclosingType, JDTTreeBuilder.OnAccessListener listener) {
<T> CtTypeReference<T> getQualifiedTypeReference(char[][] tokens, ASTNode receiverNode, TypeBinding receiverType, ReferenceBinding enclosingType, JDTTreeBuilder.OnAccessListener listener) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a new parameter here is fine from my side, because this isn't real public api. Any other opinions?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I needed access to AST node to build the source position so for now, I just passed it as a parameter. I have opened an issue #4297 to find a more generic solution to this but that would require how CtTypeReference are built and inserted into the model. This PR is kind of like a workaround but it fixes a very important issue in Sorald.

I will come back to #4297 once I am done dealing with some important issues in sorald.

Copy link
Collaborator

@slarse slarse Nov 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's generally fine to add parameters to these internal methods (anything that isn't public, really), but here I think it's not the right solution. The added parameter is used in exactly the same way in two different places, which indicates to me that it should be used at the call site of the method rather than being passed in. So I think the code changes to this method should be reverted.

I think the place to solve this particular problem is in JDTTreeBuilder.visit(QualifiedTypeReference, BlockScope), and change the very end of the method like so:

-		context.enter(factory.Code().createTypeAccessWithoutCloningReference(references.buildTypeReference(qualifiedTypeReference, scope)), qualifiedTypeReference);
+		CtTypeReference<?> typeRef = references.buildTypeReference(qualifiedTypeReference, scope);
+		if (typeRef != null) {
+			typeRef.setPosition(position.buildPositionCtElement(typeRef, qualifiedTypeReference));
+		}
+		context.enter(factory.Code().createTypeAccessWithoutCloningReference(typeRef), qualifiedTypeReference);

It's there that we build a type reference for a type access, but never assign a source position to that type reference.

if (enclosingType != null && Collections.disjoint(PUBLIC_PROTECTED, JDTTreeBuilderQuery.getModifiers(enclosingType.modifiers, false, ModifierTarget.NONE))) {
String access = "";
int i = 0;
Expand All @@ -283,13 +284,18 @@ <T> CtTypeReference<T> getQualifiedTypeReference(char[][] tokens, TypeBinding re
final TypeBinding accessBinding = searchTypeBinding(access, units);
if (accessBinding != null && listener.onAccess(tokens, i)) {
final TypeBinding superClassBinding = searchTypeBinding(accessBinding.superclass(), CharOperation.charToString(tokens[i + 1]));
CtTypeReference<T> typeReference;
if (superClassBinding != null) {
return this.getTypeReference(superClassBinding.clone(accessBinding));
typeReference = this.getTypeReference(superClassBinding.clone(accessBinding));
} else {
return this.getTypeReference(receiverType);
typeReference = this.getTypeReference(receiverType);
}
typeReference.setPosition(this.jdtTreeBuilder.getPositionBuilder().buildPositionCtElement(typeReference, receiverNode));
return typeReference;
} else {
return this.getTypeReference(receiverType);
CtTypeReference<T> typeReference = this.getTypeReference(receiverType);
typeReference.setPosition(this.jdtTreeBuilder.getPositionBuilder().buildPositionCtElement(typeReference, receiverNode));
return typeReference;
}
}
return null;
Expand Down
5 changes: 5 additions & 0 deletions src/test/java/spoon/test/fieldaccesses/testclasses/SourcePartitionValidator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
package spoon.test.fieldaccesses.testclasses;

class SourcePartitionValidator {
public enum MatchingStrategy { }
}
5 changes: 5 additions & 0 deletions src/test/java/spoon/test/fieldaccesses/testclasses/SourcePosition.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
package spoon.test.fieldaccesses.testclasses;

public class SourcePosition {
private SourcePartitionValidator.MatchingStrategy pleaseAttachSourcePositionToMe;
}
17 changes: 17 additions & 0 deletions src/test/java/spoon/test/serializable/SourcePositionTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
*/
package spoon.test.serializable;

import static org.junit.Assert.assertThat;
import static org.junit.Assert.assertTrue;

import java.io.ByteArrayOutputStream;
Expand All @@ -25,9 +26,12 @@
import java.io.IOException;
import java.io.OutputStream;

import org.hamcrest.CoreMatchers;
import org.junit.Test;

import spoon.Launcher;
import spoon.reflect.CtModel;
import spoon.reflect.cu.position.NoSourcePosition;
import spoon.reflect.declaration.CtField;
import spoon.reflect.declaration.CtType;
import spoon.reflect.factory.Factory;
Expand Down Expand Up @@ -71,4 +75,17 @@ public void testSourcePosition() throws IOException {
CtField<?> elem2 = typeFromFile.getField("a");
assertTrue(elem1.getPosition().getFile().equals(elem2.getPosition().getFile()));
}

@Test
public void test_sourcePositionOfNestedTypeInFieldExists() {
final Launcher launcher = new Launcher();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you add a contract? thanks!

launcher.addInputResource("./src/test/java/spoon/test/fieldaccesses/testclasses/SourcePosition.java");
launcher.addInputResource("./src/test/java/spoon/test/fieldaccesses/testclasses/SourcePartitionValidator.java");
CtModel model = launcher.buildModel();
MartinWitt marked this conversation as resolved.
Show resolved Hide resolved

CtField field = (CtField) model.getElements(
MartinWitt marked this conversation as resolved.
Show resolved Hide resolved
element -> element instanceof CtField &&
((CtField) element).getSimpleName().equals("pleaseAttachSourcePositionToMe")).stream().findFirst().get();
assertThat(field.getType().getPosition(), CoreMatchers.not(CoreMatchers.instanceOf(NoSourcePosition.class)));
}
}