fix: print square brackets in array initalisation

[algomaster99]

Fixes #4315

[algomaster99]

I tried this approach today, but it fails to work for elements that have no source position. Maybe a better way to go about it is to just add metadata to CtArrayTypeReference elements using putMetadata while building the tree.

[algomaster99]

@slarse I wanted to ask a question. I managed to attach metadata in CtArrayTypeReference which would help us to distinguish the DECLARATION_STYLE. However, there were some test cases where the metadata is removed when a type inside a model is fetched. For example, consider

spoon/src/test/java/spoon/test/prettyprinter/TestSniperPrinter.java

Line 108 in 7c01e62

public void testClassRename(File tempdir, Consumer<CtType<?>> renameTransfo) throws Exception {

.

The model is built with metadata attached however, the type fetched using type = f.Class().get(testClass); does not have the above metadata present. Is this the intended behaviour?

Please note that f97fccd is not the perfect solution and that's why it hasn't been refactored yet.

[algomaster99]

I needed some help with this pull request. Only one test is failing, and I cannot figure out why. I shall first explain the changes to get better insights.

I have added a psuedo metamodel element in CtArrayTypeReferenceImpl to store the declaration style of array. I set the type of declaration in JDTTreeBuilder as metadata of CtArrayTypeReferenceImpl, and then I use this metadata for printing the brackets after type or identifier. Note that I have ignored cases like int[] array[] because it is a quite bizarre way to declare an array and @slarse, and I feel it's safe to ignore it.

However, this approach works for all the existing test cases and the new ones introduced except CtGenerationTest.testGenerateRoleHandler. I tried to debug the reason, and I found out that it is because the metadata of RoleHandler[] is removed while prettyprinting it. I cannot follow why it would have been erased because I am pretty sure (I checked too) that the metadata existed once the model was built, or until this line here.

@slarse @MartinWitt @I-Al-Istannen any thoughts?

---

I am calling this a pseudo metamodel attribute because it is essentially not part of the AST but just metadata. I did not add it to AST because I agreed with @slarse 's comment here.

[algomaster99]

The issue seems to be with the following line.

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

Line 500 in a47ada1

processing.process(Query.getElements(factory.getModel().getUnnamedModule(), typeFilter));

I think this invocation removes the metadata out of the type of field.

---

The metadata is lost here.

[algomaster99]

The problem was here. The metadata of the declaration style was lost here.

Should we consider copying the metadata as well? We can make it generic and do it for all ElementNodes.

[algomaster99]

Suggested change

	if (this.templateElement instanceof CtArrayTypeReferenceImpl) {
	((CtArrayTypeReferenceImpl<?>) clone).setDeclarationKind(((CtArrayTypeReferenceImpl<?>) this.templateElement).getDeclarationKind());
	}
	clone.setAllMetadata(this.templateElement.getAllMetadata());

Something like this can be done. What do you think?

[slarse]

I'm not sure that's a good idea, propagating metadata in a clone is a major change to semantics.

@monperrus is there a conscious decision for not "cloning" metadata?

[monperrus]

is there a conscious decision for not "cloning" metadata?

no, seems to me that it is reasonable to clone the metadata as well?

[slarse]

no, seems to me that it is reasonable to clone the metadata as well?

I'm unsure how it could affect e.g. gumtree-spoon-ast-diff, which uses metadata to map Gumtree nodes to Spoon nodes. And surely there is a reliance somewhere on not propagating metadata, so I'd label this as a breaking change if we use it.

[algomaster99]

I'm unsure how it could affect e.g. gumtree-spoon-ast-diff, which uses metadata to map Gumtree nodes to Spoon nodes.

I can check if gumtree-spoon is affected in any way.

And surely there is a reliance somewhere on not propagating metadata

That could be an issue but our test cases are not able to catch that. I was also wondering that there are so many places where we clone elements. We should also discuss if they need to be changed as well. Otherwise, the definition of clone shall vary from place to place.

EDIT:

Nothing breaks on gumtree-spoon.

I added topLevelClone.setAllMetadata(topLevelElement.getAllMetadata()) here and all the tests still pass. Retaining metadata does not seem to affect anything.

[slarse]

I'm unsure how it could affect e.g. gumtree-spoon-ast-diff, which uses metadata to map Gumtree nodes to Spoon nodes.

I can check if gumtree-spoon is affected in any way.

And surely there is a reliance somewhere on not propagating metadata

That could be an issue but our test cases are not able to catch that. I was also wondering that there are so many places where we clone elements.

I don't mean in Spoon, but in some other app or library that uses Spoon. It's a behavioral change, and thus potentially breaking to dependents as something may be relying on the old behavior.

We should also discuss if they need to be changed as well. Otherwise, the definition of clone shall vary from place to place.

It might be good to add an explicit test to check that metadata is propagated in a clone.

Nothing breaks on gumtree-spoon.

That's good.

I added topLevelClone.setAllMetadata(topLevelElement.getAllMetadata()) here and all the tests still pass. Retaining metadata does not seem to affect anything.

In libraries we control. Which is good but not sufficient to not mark this a breaking change.

[algomaster99]

It might be good to add an explicit test to check that metadata is propagated in a clone.

@Test
public void test_cloneCopiesMetadata() {
  String c1 = "public class A { }";
  CtType < ? > type = Launcher.parseClass(c1);
  type.putMetadata("meaning of life", 42);

  CtType < ? > typeClone = type.clone();

  assertEquals(1, typeClone.getAllMetadata().size());
}

This test case passed without the changes, which means that we have been cloning metadata as well, and this is the line that does it.

Cloning via clone API is different from cloning via ElementNode. Metadata was propagated for the former, however, it was ignored for the latter. I am unsure of the exact distinction between their semantics.

[algomaster99]

@slarse I added the change for cloning element via template here.

[algomaster99]

This is ready for review. @monperrus @slarse @MartinWitt @I-Al-Istannen , can you have a look?

Summary of the changes.

1. Add an enum in CtArrayTypeReferenceImpl. This enum is always used in the metadata.
2. Add two public APIs there which will set and get metadata.
3. Modify JDTTreeBuilder to set the declaration style.
4. Modify DefaultJavaPrettyPrinter to conditionally print square brackets.
5. Add tests.

A bit unrelated change:
Ensure metadata of all template elements is copied while copying them.

[monperrus]

LGTM, will merge, thanks @algomaster99

[algomaster99]

@monperrus note that I have added the contract for the nested case instead of individual test cases because all of them are testing the same thing but resources are different.