review: feature: Add support for local interfaces

[SirYwell]

In regards of #4074 and #3653, this adds support for local interfaces and enums.

I also added tests for local classes as this doesn't seem to be covered pretty well for now (see below).

Enums are basically already supported, but they are printed wrong, as the leading number is not removed.

I also added the layout for a future test covering local records, this needs to be finished at some point. I disabled it therefore for now.

Note that the only test that succeeds currently is the testLocalClassExists one. This is wrong if the javadocs are right (see mentioned issue above) as there is no number prefixing the actual name. Looking into that, I found out that JDT reports the names somewhat inconsistently, MyClass has the constant pool name X$1$MyClass while MyEnum and MyInterface have the constant pool names X$1MyEnum and X$1MyInterface respectively...

Not sure if this makes it better or worse, but the constant pool name only differs if the compliance level is set to <=4. Are those lower levels relevant for spoon?

TODO:

• if the number prefix is kept in the simple name, the DefaultJavaPrettyPrinter needs to adapt that for local enums and local interfaces
• if the number prefix is removed, the code handling that in DefaultJavaPrettyPrinter can be removed

(I guess that should be tested too, independent of what we decide on)

Edit: The CommentTest#testDocumentationContract() fails now, should I add an example there?

[slarse]

Not sure if this makes it better or worse, but the constant pool name only differs if the compliance level is set to <=4. Are those lower levels relevant for spoon?

I don't think so, @monperrus?

On a side note, the names discussed here are the binary names as specified in JLS 13.1.

The CommentTest#testDocumentationContract() fails now, should I add an example there?

Any type that is a statement or expression must have a code snippet showing its use. I think the reason for only requiring this for statements/expressions is that Spoon supports on-the-fly compilation of these, so it's easy to test. Anyway, to fix the test error, you need to add an example interface to the doc and increase the compliance level in the test to 16. For example:

/**
 * This element defines an interface declaration.
 *
 * <pre>
 *     // an interface definition
 *     interface Foo {
 *        void bar();
 *     }
 * </pre>
 */

[SirYwell]

The printer now removes leading digits from local enums and local interfaces too, I also added a test case for that.

[monperrus]

if the compliance level is set to <=4. Are those lower levels relevant for spoon?
I don't think so, @monperrus?

I'd say it is relevant. AFAIK, Spoon is also used for analyzing old legacy code, incl. in very old Java versions.

[SirYwell]

I wasn't able to find any proper specification for local class names for pre Java 5 versions, it's calculated here in JDT. I tried to look into a few commits around that code but that wasn't very revealing for me.

Would it make sense to change the Javadocs to state that the simple name is the binary name in that case? Or should we strip out the last $ if below Java 5? WDYT?

[slarse]

I'd say it is relevant. AFAIK, Spoon is also used for analyzing old legacy code, incl. in very old Java versions.

@monperrus It would be good to add at least one such project to the smoke tests on Jenkins. Know of any?

I wasn't able to find any proper specification for local class names for pre Java 5 versions.

@SirYwell Java 5 and before is kind of a "water under the bridge" era. I've not found any specs for that either.

Would it make sense to change the Javadocs to state that the simple name is the binary name in that case?

I think it does. Isn't that actually the current behavior for local classes?

[SirYwell]

Isn't that actually the current behavior for local classes?

It is, but the current comment explicitely states

	 * Following the compilation convention, if the type is a local type,
	 * the name starts with a numeric prefix (e.g. local class Foo has simple name 1Foo).

which is only correct for Java >=5 as it seems.

[monperrus]

@monperrus It would be good to add at least one such project to the smoke tests on Jenkins. Know of any?

Great idea. Any old commit of old projects, such as an old Java 1.4 commit of Apache Commons Lang or Apache Commons Math.

[slarse]

Isn't that actually the current behavior for local classes?

It is, but the current comment explicitely states

	 * Following the compilation convention, if the type is a local type,
	 * the name starts with a numeric prefix (e.g. local class Foo has simple name 1Foo).

which is only correct for Java >=5 as it seems.

Alright, so we keep that behavior and just update the javadoc to say something like "binary name, which typically ". Is that what's currently implemented in your PR?

@monperrus Opening a separate issue on that old commit business.

[SirYwell]

Alright, so we keep that behavior and just update the javadoc to say something like "binary name, which typically ". Is that what's currently implemented in your PR?

Yes, the implementation of my tests currently expects a leading number for the simple name of local types.

[slarse]

@SirYwell Great, let's stick to that. I'm a little bit swamped at the moment but I hope to have a chance to review this during the weekend.

@monperrus feel free to beat me to that review.

[monperrus]

Thanks a lot @SirYwell, overall LGTM.

we're happy to wait for @slarse's eagle eye to proceed.

[slarse]

I just have one pretty minor thing to discuss, rest LGTM.

[slarse]

I just have one thought, and that's if we should simplify this like so:

Suggested change

	if (ctEnum.isLocalType()) {
	printer.writeIdentifier(stripLeadingDigits(ctEnum.getSimpleName()));
	} else {
	printer.writeIdentifier(ctEnum.getSimpleName());
	}
	printer.writeIdentifier(stripLeadingDigits(ctEnum.getSimpleName()));

This should effectively provide the same behavior, as non-local types don't have leading digits. By that logic, we might even want to push down the stripping of leading digits into DefaultTokenWriter.writeIdentifier. We never want to print an identifier with leading digits, as that's not a valid Java identifier.

What do you think? The solution in here is not bad by any means, and is the preexisting one, but the repetition across methods just got me thinking.

[SirYwell]

but the repetition across methods just got me thinking

Yeah I thought about that too, especially as it's currently done in #4134 too - having it consistent in one place would make more sense. I'll look into it.

[SirYwell]

calling the method in the DefaultTokenWriter fails due to

spoon/src/main/java/spoon/reflect/visitor/DefaultJavaPrettyPrinter.java

Line 1896 in a03c436

printer.writeIdentifier(ref.getSimpleName());

writing the numbers of anonymous classes. As an example, the following test fails (the error is thrown in the stripLeadingDigits method, but the test would fail with an empty string too) and it's also expected to have a number there:

spoon/src/test/java/spoon/test/reference/TypeReferenceTest.java

Line 491 in 3a8d04d

assertTrue(referencedTypeNames.contains("A.1"));

If that's the only case, I could just return the string itself instead of throwing the exception. This would make writing such stuff a bit slower but as it tends to be only a few chars (as long as you don't have hundreds of anonymous classes in one class) it shouldn't really matter.

---

Edit

When just returning, the custom TokenPrinter in PrinterTest writes it wrongly - obviously. So I'm not sure if I would expect the TokenWriter to deal with it or if it should be in the DefaultJavaPrettyPrinter instead

[slarse]

Test case you linked there is a pretty strange one. The method is called testAnonymousClassesHaveAnEmptyStringForItsNameInNoClasspath, the contract probably by accident states that all references should be empty strings, and the test then goes on to verify that the simple name of an anonymous class is a digit. And a bunch of other fully qualified names. So, the test IMO is overspecified and hardly does what it says, but the question still remains: what should the fully qualified name of an anonymous class be, if it does not contain that digit?

Let's be pragmatic: the beautiful solution caused complications, let's go with the less beautiful one of doing printer.writeIdentifier(stripLeadingDigits(ctEnum.getSimpleName()); all over the place :)

[SirYwell]

what should the fully qualified name of an anonymous class be, if it does not contain that digit?

As long as I didn't miss anything, there should be no case where printing the name of an anonymous type would be correct. So yeah, that's a good question.

I simplified the code now and it works with the tests that failed before, but it does not throw an exception anymore when writing anonymous type names as it seems to cause more headache than necessary I think.

[slarse]

Hi @SirYwell,

Implementation looks great. Now on my final pass review, I just have a little bit of a concern with the LocalTypesTest test class. I just feel like it might not be found when the next person comes in and has a problem with local, say, classes. Or enums.

I'd personally go looking in their dedicated test classes, CtClassTest, EnumsTest and InterfaceTest. What do you think about that? Do you agree that the tests would be more discoverable if they were in those, perhaps more expected test classes? You could put the helper methods in SpoonTestHelpers.

[SirYwell]

Do you agree that the tests would be more discoverable if they were in those, perhaps more expected test classes?

Yes, I agree with you. That said, those tests are currently written with JUnit 4. I already opened #4138 independently, I can probably also update the other tests too. Would that be preferred over mixing JUnit 4 and JUnit 5 (or writing the test for local types in JUnit 4)?

[slarse]

Adding JUnit4 tests to an existing JUnit4 test class is fine, we don't want to make it too complicated to contribute stuff that isn't strictly test-related :). I'd say add the tests here in whichever version the test classes use, convert to JUnit5 later if you've time left over.

[slarse]

LGTM, thanks @SirYwell!

[monperrus]

Super cool, thanks a lot.