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

algomaster99 · 2021-11-18T15:18:43Z

This patch is a consequence of what has been discussed here. The source position of a field's type was not being set correctly. I realised that the source position are stored in the AST nodes created by JDT compiler and the function in this patch did not have access to one. So, I have modified the function definition so I am able to build and set the position.

There are things left in this PR:

Tests for all the changes. I can assert the model built to check if one of its field's type contains the source position now.
- [ ] Look for an alternative way because just by reading the code, my intuition is telling me that ReferenceBuilder is a wrong place to set the position. It should probably be done somewhere in a related file.

EDIT: The second task shall be handled in #4297.

algomaster99 · 2021-11-19T09:12:11Z

For now, I have created a test case that will be sufficient to fix Sorald's issue. However, that is not the best way. There are still some type references whose source position is not set. For example, when we execute this test case, the model built misses out setting position of Foo (instance of CtTypeReference`) is this method declaration.

It is safe to say that this pull request will only fix Sorald's issue. For type references like above, a further inspection is needed. I can open a ticket for that and deal with that later.

EDIT: #4297

algomaster99 · 2021-11-19T09:32:26Z

@monperrus this is ready for review.

I am also having some trouble naming the PR. The current title is the final goal but it's not related to Spoon. Another title I came up with was "fix: ensure pretty-printer prints modifiers and type separated by a space", but that's also related to Sorald.

monperrus · 2021-11-19T11:13:05Z

src/test/java/spoon/test/serializable/SourcePositionTest.java

+
+	@Test
+	public void test_sourcePositionOfNestedTypeInFieldExists() {
+		final Launcher launcher = new Launcher();


could you add a contract? thanks!

monperrus · 2021-11-19T11:14:32Z

Overall LGTM, @slarse do you want to look at it and merge?

…nto set-source-position

src/test/java/spoon/test/serializable/SourcePositionTest.java

MartinWitt · 2021-11-19T15:54:04Z

src/main/java/spoon/support/compiler/jdt/ReferenceBuilder.java

 	 * @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) {


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

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.

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.

src/test/java/spoon/test/serializable/SourcePositionTest.java

MartinWitt

I added some small style comments otherwise looks fine for me. Adding a new parameter for a method is breaking behaviour, but the referencebuilder isn't part of the public api from my pov.

algomaster99 · 2021-11-19T16:16:50Z

@MartinWitt incorporated your changes.

MartinWitt · 2021-11-19T16:25:54Z

Merging from my side is fine. Let's wait the 24 hours and merge with fix(sniper): ensure sniper pretty-printer prints modifiers and type separated by a space ?

slarse · 2021-11-20T08:59:22Z

Having a peek.

slarse

This was a rather tricky one, took me a while to evaluate it.

I don't think this is the right solution. The ReferenceBuilder isn't concerned with setting source positions, and for good reason: references are often implicit. For example, a variable read has a type, which is represented as a type reference, but the type is contained in the declaration of the variable, and not at the site of the variable read. For that reason, I think it's pretty important we deal with setting source positions further up the stack.

See comment on the source code.

slarse · 2021-11-20T09:13:40Z

src/main/java/spoon/support/compiler/jdt/ReferenceBuilder.java

 	 * @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) {


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.

algomaster99 · 2021-11-22T09:00:53Z

@slarse , yes that makes sense. Thanks for the patch. I think we will need to replicate such a thing for many type references and I have kept that for #4297 .

algomaster99 · 2021-11-22T09:32:45Z

@slarse This is causing some problems. I inspected one of them. It turns out that the patch tries to build a source position of the element java.lang.Exception ~~which doesn't even exist in the test case resources~~. I am looking further into it.

EDIT:

So I found the problem with one of the test cases.

It tries to look up previous annotations while building source position for type reference (spoon element) java.lang.Exception however, sourceStart points to the beginning of the qualified type reference of the @TypeAnnotation and there are no annotations before that. findPrevAnnotations returns -1, and sourceStart is set to that, because it can't find any annotations before j in java.lang. ... @TypeAnnotation.

I am not sure how to go forward now. Maybe we can remove findPrevAnnotations which was introduced in #3359 as it only fails the test case introduced in that PR and doesn't affect any other. @nharrand also suggests writing a post-processor to include source positions of annotations in their respective type reference, parameter, etc rather than having a workaround in #3359.

algomaster99 · 2021-11-23T13:12:10Z

@nharrand need your opinions about disabling #3359 's fix because it is interfering with setting source position for Exception here. Reason:

It tries to look up previous annotations while building source position for type reference (spoon element) java.lang.Exception however, sourceStart points to the beginning of the qualified type reference of the @TypeAnnotation and there are no annotations before that. findPrevAnnotations returns -1, and sourceStart is set to that, because it can't find any annotations before j in java.lang. ... @TypeAnnotation.

monperrus · 2021-11-23T14:38:49Z

Works for me. Actions:

ignoring the failing assertion with a detailed comment
puts high priority on Annotations source position handling on parameters and parameter type annotation. #3358 on your backlog

algomaster99 · 2021-11-24T09:51:20Z

@monperrus I ignored the annotation position test for now. I will come back to it after we are done with sorald's paper submission.

monperrus · 2021-11-24T10:34:27Z

ack, thanks @algomaster99

algomaster99 · 2021-11-24T11:51:50Z

@MartinWitt Something broke in the code quality test after this PR was merged. Can you take a look at it?

MartinWitt · 2021-11-24T12:19:34Z

Sure, taking a look, but at the first glance this looks like some maven-central related problem.

algomaster99 · 2021-11-24T19:25:41Z

@MartinWitt thanks!

algomaster99 added 3 commits November 18, 2021 16:07

Pass AST node to set position of type references

91c3bfa

Reproduce test case for fixing ASSERT-KTH/sorald#603

3a69748

Reproduce test case for fixing ASSERT-KTH/sorald#603

fb5482f

algomaster99 marked this pull request as ready for review November 19, 2021 09:12

algomaster99 changed the title ~~fix: set source position of type references~~ fix: patch for fixing SpoonLabs/sorald#603 Nov 19, 2021

algomaster99 mentioned this pull request Nov 19, 2021

Inspect if all instances of CtTypeReferences in a model built from source have a set source position #4297

Open

monperrus reviewed Nov 19, 2021

View reviewed changes

algomaster99 added 2 commits November 19, 2021 12:58

Add contract

40febc5

Merge branch 'set-source-position' of github.com:algomaster99/spoon i…

9ccd262

…nto set-source-position

algomaster99 changed the title ~~fix: patch for fixing SpoonLabs/sorald#603~~ fix: ensure sniper pretty-printer prints modifiers and type separated by a space Nov 19, 2021

algomaster99 requested a review from monperrus November 19, 2021 12:40

MartinWitt reviewed Nov 19, 2021

View reviewed changes

src/test/java/spoon/test/serializable/SourcePositionTest.java Outdated Show resolved Hide resolved

algomaster99 mentioned this pull request Nov 19, 2021

fix: lookup absolute path to maven executable #4298

Merged

Move test resources

c2f6d3d

algomaster99 requested a review from MartinWitt November 19, 2021 15:29

MartinWitt reviewed Nov 19, 2021

View reviewed changes

src/test/java/spoon/test/serializable/SourcePositionTest.java Outdated Show resolved Hide resolved

MartinWitt reviewed Nov 19, 2021

View reviewed changes

src/test/java/spoon/test/serializable/SourcePositionTest.java Outdated Show resolved Hide resolved

MartinWitt approved these changes Nov 19, 2021

View reviewed changes

Fix styling issues

3789b69

slarse suggested changes Nov 20, 2021

View reviewed changes

Assign position to type access before visiting it

e2d0933

Revert style changes to ReferenceBuilder

99ee146

algomaster99 requested a review from slarse November 22, 2021 09:04

algomaster99 added 2 commits November 23, 2021 16:30

Merge branch 'master' into set-source-position

3609586

Ignore AnnotationPositionTest

1327bd1

monperrus merged commit 454dada into INRIA:master Nov 24, 2021

algomaster99 deleted the set-source-position branch November 24, 2021 11:51

This was referenced Dec 6, 2021

[Bug] SniperJavaPrettyPrinter - Invalid ouput on annotated default method #4330

Open

chore: upgrade Spoon to 10.0.1-beta-2 ASSERT-KTH/sorald#676

Merged

monperrus mentioned this pull request Mar 14, 2022

WIP release: next release #4636

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

algomaster99 commented Nov 18, 2021 •

edited

Loading

algomaster99 commented Nov 19, 2021 •

edited

Loading

algomaster99 commented Nov 19, 2021 •

edited

Loading

monperrus Nov 19, 2021

monperrus commented Nov 19, 2021

MartinWitt Nov 19, 2021

algomaster99 Nov 19, 2021

slarse Nov 20, 2021 •

edited

Loading

MartinWitt left a comment

algomaster99 commented Nov 19, 2021

MartinWitt commented Nov 19, 2021

slarse commented Nov 20, 2021

slarse left a comment

slarse Nov 20, 2021 •

edited

Loading

algomaster99 commented Nov 22, 2021

algomaster99 commented Nov 22, 2021 •

edited

Loading

algomaster99 commented Nov 23, 2021

monperrus commented Nov 23, 2021

algomaster99 commented Nov 24, 2021

monperrus commented Nov 24, 2021

algomaster99 commented Nov 24, 2021

MartinWitt commented Nov 24, 2021

algomaster99 commented Nov 24, 2021

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

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

Conversation

algomaster99 commented Nov 18, 2021 • edited Loading

algomaster99 commented Nov 19, 2021 • edited Loading

algomaster99 commented Nov 19, 2021 • edited Loading

monperrus Nov 19, 2021

Choose a reason for hiding this comment

monperrus commented Nov 19, 2021

MartinWitt Nov 19, 2021

Choose a reason for hiding this comment

algomaster99 Nov 19, 2021

Choose a reason for hiding this comment

slarse Nov 20, 2021 • edited Loading

Choose a reason for hiding this comment

MartinWitt left a comment

Choose a reason for hiding this comment

algomaster99 commented Nov 19, 2021

MartinWitt commented Nov 19, 2021

slarse commented Nov 20, 2021

slarse left a comment

Choose a reason for hiding this comment

slarse Nov 20, 2021 • edited Loading

Choose a reason for hiding this comment

algomaster99 commented Nov 22, 2021

algomaster99 commented Nov 22, 2021 • edited Loading

algomaster99 commented Nov 23, 2021

monperrus commented Nov 23, 2021

algomaster99 commented Nov 24, 2021

monperrus commented Nov 24, 2021

algomaster99 commented Nov 24, 2021

MartinWitt commented Nov 24, 2021

algomaster99 commented Nov 24, 2021

algomaster99 commented Nov 18, 2021 •

edited

Loading

algomaster99 commented Nov 19, 2021 •

edited

Loading

algomaster99 commented Nov 19, 2021 •

edited

Loading

slarse Nov 20, 2021 •

edited

Loading

slarse Nov 20, 2021 •

edited

Loading

algomaster99 commented Nov 22, 2021 •

edited

Loading