feature: sealed classes (JEP 409) #4337

SirYwell · 2021-12-07T19:43:16Z

This targets #4189.

With Java 17, sealed classes were finalized. This Pull Request adds support for it to spoon. For the changes to the JLS for this feature, see here.

Model

The CtSealable interface is adds methods to retrieve, remove and add permitted types. It does not handle the sealed modifier (e.g. adding it).

Shadow Model

As sealed isn't a modifier in Bytecode, it is set whenever the class has permitted types.

Questions

Currently, it is possible to set all modifiers final, asbtract, sealed and non-sealed at the same time. Would it make sense to have a similar validation here like it is done for visibility modifiers?
The methods to modify the permitted types currently return CtSealable. Other than the existing methods returning a generic type, this is quite limiting. In theory, those methods could be overridden in the subclasses, but that means 3 overridden methods in each of CtRecord, CtEnum, etc,. Is it worth that noisy overhead or is it probably better to return void instead?
It seems like the printer always prints permitted types, because before printing, the ImportAnalyzer sets implicit to false for the type references. I'm not familar with that part of the code, what am I supposed to do to avoid that?

Note regards 2.:

It seems like the ReplacementVisitorGenerator does not like overriding methods in other interfaces. It produces invalid code in such cases.

TODO:

figure out if TYPE_REF is an appropriate role for permitted types or if a new role should be added. Edit: Added PERMITTED_TYPE instead.
add javadocs
think about return types, just returning CtSealable seems limiting. Edit: See question 2.
reduce diff
call model change listener where needed
write a proper description for this PR
wait for JDT update, rebase and switch from Java 16 preview to Java 17 in tests. Edit: That was faster than expected.
make sure Java 17 only tests run in GHA
Printer tests

- clone visitor is lazy fixed, need to be regenerated - classes are printed, interfaces not yet

- generated classes may be correct now?

Co-authored-by: I-Al-Istannen <i-al-istannen@users.noreply.github.com>

MartinWitt

Very nice work. Added style and set order releated comments.
The printing looks a bit strange to me but this seems to be the JDK style.

MartinWitt · 2022-01-04T16:31:14Z

src/main/java/spoon/reflect/declaration/CtEnum.java

+
+	@Override
+	@UnsettableProperty
+	CtEnum<T> setPermittedTypes(Collection<CtTypeReference<?>> permittedTypes);


Shouldn't we use List here because we need the order for printing?
Or state in the comment the order of the collection is relevant

MartinWitt · 2022-01-04T16:33:03Z

src/main/java/spoon/reflect/declaration/CtRecord.java

+
+	@Override
+	@UnsettableProperty
+	CtRecord setPermittedTypes(Collection<CtTypeReference<?>> permittedTypes);


see other comment

MartinWitt · 2022-01-04T16:33:47Z

src/main/java/spoon/reflect/declaration/CtSealable.java

+	 * @return this.
+	 */
+	@PropertySetter(role = CtRole.PERMITTED_TYPE)
+	CtSealable setPermittedTypes(Collection<CtTypeReference<?>> permittedTypes);


see other comment

Even for printing, the order does not affect the semantics. We can use a LinkedHashSet internally and add a note to the documentation that the iteration order of the collection defines the order when printing, but I would personally prefer to sort it in the printer. Coupling the model structure that strong with the printer feels odd.

For the DJPP we can sort them, but for the sniper we more or less need the old order. Comment and LinkedHashSet is fine for me.

src/main/java/spoon/reflect/declaration/ModifierKind.java

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

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

MartinWitt · 2022-01-04T16:57:40Z

src/main/java/spoon/support/reflect/declaration/CtClassImpl.java

+	@Override
+	public CtClass<T> setPermittedTypes(Collection<CtTypeReference<?>> permittedTypes) {
+		Collection<CtTypeReference<?>> types = permittedTypes != null ? permittedTypes : CtElementImpl.emptySet();
+		getFactory().getEnvironment().getModelChangeListener().onSetDeleteAll(this, CtRole.PERMITTED_TYPE, this.permittedTypes, new HashSet<>(this.permittedTypes));


In worst case you reorder the collection with new HashSet<>(col) maybe use LinkedHashSet or something here, even if everywhere else we still do it wrong?

To be honest, I don't know the effects on the model change listener when having a different order here ;)

The default changelistener implementation ignores all but role and changed element. Other implementations I have never seen.

MartinWitt · 2022-01-04T16:58:33Z

src/main/java/spoon/support/reflect/declaration/CtClassImpl.java

+			return this;
+		}
+		if (this.permittedTypes == CtElementImpl.<CtTypeReference<?>>emptySet()) {
+			this.permittedTypes = new HashSet<>();


See other comments about ordered collections

MartinWitt · 2022-01-04T17:04:03Z

src/main/java/spoon/support/visitor/replace/ReplacementVisitor.java

@@ -1613,6 +1627,7 @@ public void visitCtCatch(final spoon.reflect.code.CtCatch catchBlock) {
 		replaceInSetIfExist(ctClass.getSuperInterfaces(), new spoon.support.visitor.replace.ReplacementVisitor.CtTypeInformationSuperInterfacesReplaceListener(ctClass));
 		replaceInListIfExist(ctClass.getFormalCtTypeParameters(), new spoon.support.visitor.replace.ReplacementVisitor.CtFormalTypeDeclarerFormalCtTypeParametersReplaceListener(ctClass));
 		replaceInListIfExist(ctClass.getTypeMembers(), new spoon.support.visitor.replace.ReplacementVisitor.CtTypeTypeMembersReplaceListener(ctClass));
+		replaceInSetIfExist(ctClass.getPermittedTypes(), new spoon.support.visitor.replace.ReplacementVisitor.CtSealablePermittedTypesReplaceListener(ctClass));


the set and order problem

algomaster99 · 2022-01-04T22:32:36Z

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

+		printer.writeln().incTab().writeKeyword("permits").writeSpace();
+		printList(sealable.getPermittedTypes(), null, false, null, false, false, ",", true, false, null, prettyPrinter::scan);


Suggested change

printer.writeln().incTab().writeKeyword("permits").writeSpace();

printList(sealable.getPermittedTypes(), null, false, null, false, false, ",", true, false, null, prettyPrinter::scan);

printList(sealable.getPermittedTypes(), "permits", true, null, false, false, ",", true, false, null, prettyPrinter::scan);

I am unsure why writeln and incTab was used. Is that necessary?

It looks like how to print these permitted types "correctly" is not really clear. I found three styles

Interface Style

https://github.com/openjdk/jdk/tree/739769c8fc4b496f08a92225a12d07414537b6c0/test/hotspot/jtreg/runtime/sealedClasses
https://github.com/openjdk/jdk/blob/739769c8fc4b496f08a92225a12d07414537b6c0/test/hotspot/jtreg/runtime/sealedClasses/planets/OuterPlanets.jcod#L26
If you look in the openjdk tests the print them like supertypes. e.g.

sealed public class OuterPlanets permits Neptune, asteroids.Pluto, asteroids.Charon

NewLine Style

If you look at the JEP(https://openjdk.java.net/jeps/409) you find them in a new line formatted.

public abstract sealed class Shape permits Circle, Rectangle, Square { ... }

Eclipse Style(?)

On example, strangely formats each permitted type in a new line. Looks as ugly as a new line for each parameter some old eclipse projects use.

public abstract sealed class Shape permits com.example.polar.Circle, com.example.quad.Rectangle, com.example.quad.simple.Square

So the best we can do is guessing what is correct. The JEP formatting could be a result of the maximum line length.
You could look at the formatters like https://github.com/google/google-java-format if you want to see how google does it. Anyone has a preferred style, any style I missed or a good explanation which style is the best?

I don't have a strong opinion on this. It just happens to be a very long line if it's all in only one (especially with generics, superinterfaces and superclasses).

See ConstantDesc for an example where the permitted types are listed vertically in the OpenJDK, but this one might look odd depending on tab size too.

In best case, the printer could determine how to format without exceeding a configurable line length, but I guess that's a whole new level of problems.

I'm open for more opinions on this.

Funfact: In current projects, we have permits formatted by IntelliJ, it's on a new line if the line would exceed 120 characters and if that would exceed the limit again, it wraps again. It then looks like

sealed class X permits A, B, C, D, E, F, G, H { }

slarse · 2022-01-29T17:00:30Z

@MartinWitt Were your concerns here fixed? Think you can bring this one in?

SirYwell · 2022-01-29T17:46:37Z

I didn't change anything about the ordering of the permitted types. I originally chose a (Hash)Set because uniqueness is more relevant semantically. Using a List does not make sense imho. I could use a LinkedHashSet internally, but doing that just for printing is odd too, especially if it becomes part of the method contracts.

Not guaranteeing any order and sorting in the printer sounds best for me, but I don't know how well the sniper printer can handle that.

I also got the questions in the PR description, I think they are still open (and also the question about the formatting style when printing permitted types)

slarse · 2022-02-02T21:35:26Z

Currently, it is possible to set all modifiers final, asbtract, sealed and non-sealed at the same time. Would it make sense to have a similar validation here like it is done for visibility modifiers?

It's not strictly necessary, as Spoon in general has little validation. But I think it's a nice effort to put validation where the effort and performance impact of it are low.

The methods to modify the permitted types currently return CtSealable. Other than the existing methods returning a generic type, this is quite limiting. In theory, those methods could be overridden in the subclasses, but that means 3 overridden methods in each of CtRecord, CtEnum, etc,. Is it worth that noisy overhead or is it probably better to return void instead?

They could be overridden, but don't need to be. It's fine to leave it to just return CtSealable imo, but I'd also be fine with void.

It seems like the printer always prints permitted types, because before printing, the ImportAnalyzer sets implicit to false for the type references. I'm not familar with that part of the code, what am I supposed to do to avoid that?

I'd have to look at this, could you point me to a test case to run where something you expect to be implicit ends up not being implicit?

SirYwell · 2022-02-04T17:00:01Z

Currently, it is possible to set all modifiers final, asbtract, sealed and non-sealed at the same time. Would it make sense to have a similar validation here like it is done for visibility modifiers?

It's not strictly necessary, as Spoon in general has little validation. But I think it's a nice effort to put validation where the effort and performance impact of it are low.

In that case, it would probably make sense to make it more consistent? Although it's currently not checked with final and abstract either...

The methods to modify the permitted types currently return CtSealable. Other than the existing methods returning a generic type, this is quite limiting. In theory, those methods could be overridden in the subclasses, but that means 3 overridden methods in each of CtRecord, CtEnum, etc,. Is it worth that noisy overhead or is it probably better to return void instead?

They could be overridden, but don't need to be. It's fine to leave it to just return CtSealable imo, but I'd also be fine with void.

I'm just not sure if it's worth to have them CtSealable. It somewhat defeats the purpose of chaining calls if you lose the actual type information. I think I'd rather go with making it void than keeping it CtSealable.

It seems like the printer always prints permitted types, because before printing, the ImportAnalyzer sets implicit to false for the type references. I'm not familar with that part of the code, what am I supposed to do to avoid that?

I'd have to look at this, could you point me to a test case to run where something you expect to be implicit ends up not being implicit?

Oh, sorry, this is actually a bit out of date. It belongs to my comment here, this is my current "fix". If you remove that part, the test here will fail for those two arguments

slarse · 2022-03-18T09:21:53Z

This one has been sitting dormant for a while, sorry about that @SirYwell. I will turn to this once I'm done with #4215

# Conflicts: # src/main/java/spoon/support/compiler/jdt/JDTTreeBuilderHelper.java

slarse · 2022-04-20T18:29:08Z

#4215 is finally done so I will try to start here over the weekend.

slarse · 2022-05-11T21:17:33Z

I got held up that weekend and then I was sick for two weeks. Back on my feet now but I'm so behind on everything I don't know when I'll get around to this.

@MartinWitt since you've already reviewed once, would you be able to wrap this one up? It's been sitting for 6 months now, we really gotta get better at bringing in these new feature PRs :s

MartinWitt · 2022-05-13T12:35:17Z

@MartinWitt since you've already reviewed once, would you be able to wrap this one up?

Sure, I will take a look at this next week.

MartinWitt · 2022-05-18T13:55:44Z

Currently, it is possible to set all modifiers final, asbtract, sealed and non-sealed at the same time. Would it make sense to have a similar validation here like it is done for visibility modifiers?

Yes, but this can be done in a future PR.

It seems like the printer always prints permitted types, because before printing, the ImportAnalyzer sets implicit to false for the type references. I'm not familar with that part of the code, what am I supposed to do to avoid that?

I'm kinda out of touch of the state of sealed types currently. Does java have implicit sealed/permitted types? Do you have an example for this?

SirYwell · 2022-05-18T16:01:09Z

I'm kinda out of touch of the state of sealed types currently. Does java have implicit sealed/permitted types? Do you have an example for this?

According to JLS 8.1.6

If C is not an enum class, then its permitted direct subclasses are those classes declared in the same compilation unit as C (§7.3) which have a canonical name (§6.7) and whose direct superclass is C.

For enums, the permitted subclasses are always implicit.

The tests contain sealed classes and interfaces with direct subtypes in the same compilation unit without explicit permits clause to cover that.

raghav-deepsource · 2022-05-20T08:00:37Z

There's one thing I'd like to point out with the changes here. with the following setup (borrowed from sonarqube):

public sealed class A permits B, C, D, E {} // Noncompliant
final class B extends A {}
final class C extends A {}
final class D extends A {}
final class E extends A {}

the permits clause seems to be interpreted as a set of fields, of the type A.permits. Also, the subclasses of A are treated as nested types of that class, despite being declared outside it.

oops, I just realised I had set the wrong Java version in my spoon env, never mind.

MartinWitt · 2022-06-16T10:10:19Z

Sorry, @SirYwell, the weather was nice and I forgot about your PR. I will complete the evaluation process in the next days. After the 'CtRecord' breaking update a short time after release, I got the idea to identify new features and code pieces as Beta or Experimental. I hope we don't need to update anything in the API, but real-world usage frequently reveals something we overlooked.
wdyt?
Edit: Btw there are merge conflicts

src/main/java/spoon/reflect/declaration/ModifierKind.java

MartinWitt · 2022-06-21T16:44:04Z

After a long time, this PR is ready2merge. Sorry, @SirYwell that this review took so long.

MartinWitt · 2022-06-27T12:55:46Z

As this is a new AST Element, any last review from @monperrus or @slarse. From my side, this PR is perfect and ready2merge.

slarse · 2022-07-05T15:34:46Z

@MartinWitt I haven\t got the time right now for diving deep into this, but I made a quick scan over the additions, in particular the new interface, and I don't have anything to add. If there's anything specific you'd like my input on then please point me in that direction. Otherwise I think you should go ahead and merge.

MartinWitt · 2022-07-05T15:56:21Z

Thanks, @SirYwell, for this great addition. Now we have support for all java features again.

slarse · 2022-07-05T16:29:12Z

Indeed, thank you for the hard work @SirYwell !

SirYwell force-pushed the feature/sealed-classes branch from 7607c00 to 45f209a Compare December 8, 2021 09:18

SirYwell force-pushed the feature/sealed-classes branch 4 times, most recently from c3cc9b3 to d4d0b2a Compare December 22, 2021 13:41

SirYwell mentioned this pull request Jan 3, 2022

review: chore: run tests with Java 17 #4393

Merged

SirYwell and others added 24 commits January 4, 2022 15:02

sealed classes

65b91d7

progress

cec1201

wip

526e28c

- clone visitor is lazy fixed, need to be regenerated - classes are printed, interfaces not yet

progress

10c934e

- generated classes may be correct now?

expand test

30b6c69

minor changes

e90d823

Fix implicit permits

c72f7b7

Co-authored-by: I-Al-Istannen <i-al-istannen@users.noreply.github.com>

use Enum for getModifiers

9f7b267

implement asserts in tests

8a268d1

Add sealed modifier to enum when applicable

4023400

don't modify enums manually

62d9b7e

correct implicitness

406d85a

reduce diff

d9311ac

fix modifier check

a2857aa

update tests

46c653b

update ModifierTargets

b6cf436

Support sealed types in shadow model

e6d7cb8

Don't build a super large shadow model

bf46ef3

fix a few checkstyle issues

6a18820

add missing UnsettableProperty annotations

010155d

introduce permitted type role

ed90231

update metamodel/generated classes

60fbda5

license...

713234d

switch from 16 + preview to 17 in tests

156e6c2

MartinWitt requested changes Jan 4, 2022

View reviewed changes

algomaster99 reviewed Jan 4, 2022

View reviewed changes

SirYwell added 3 commits January 15, 2022 09:43

use exact modifier strings for ModifierKind directly

648f923

use method reference

0031e5b

make method more descriptive

1469e2d

Merge branch 'master' into feature/sealed-classes

52f5780

# Conflicts: # src/main/java/spoon/support/compiler/jdt/JDTTreeBuilderHelper.java

use LinkedHashSet to keep order

cd0c824

resolve conflicts

54a05c6

MartinWitt reviewed Jun 21, 2022

View reviewed changes

src/main/java/spoon/reflect/declaration/ModifierKind.java Show resolved Hide resolved

MartinWitt reviewed Jun 21, 2022

View reviewed changes

src/main/java/spoon/reflect/declaration/ModifierKind.java Show resolved Hide resolved

MartinWitt changed the title ~~review: feature: sealed classes (JEP 409)~~ feature: sealed classes (JEP 409) Jul 5, 2022

MartinWitt merged commit 4bb0454 into INRIA:master Jul 5, 2022

monperrus mentioned this pull request Oct 6, 2022

release of Spoon 10.2.0 #4946

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: sealed classes (JEP 409) #4337

feature: sealed classes (JEP 409) #4337

SirYwell commented Dec 7, 2021 •

edited

Loading

MartinWitt left a comment

MartinWitt Jan 4, 2022

MartinWitt Jan 4, 2022

MartinWitt Jan 4, 2022

SirYwell Jan 5, 2022

MartinWitt Jan 5, 2022

MartinWitt Jan 4, 2022

SirYwell Jan 5, 2022

MartinWitt Jan 5, 2022

MartinWitt Jan 4, 2022

MartinWitt Jan 4, 2022

algomaster99 Jan 4, 2022

MartinWitt Jan 5, 2022

SirYwell Jan 5, 2022 •

edited

Loading

slarse commented Jan 29, 2022

SirYwell commented Jan 29, 2022

slarse commented Feb 2, 2022

SirYwell commented Feb 4, 2022

slarse commented Mar 18, 2022

slarse commented Apr 20, 2022

slarse commented May 11, 2022

MartinWitt commented May 13, 2022

MartinWitt commented May 18, 2022

SirYwell commented May 18, 2022

raghav-deepsource commented May 20, 2022 •

edited

Loading

MartinWitt commented Jun 16, 2022 •

edited

Loading

MartinWitt commented Jun 21, 2022

MartinWitt commented Jun 27, 2022

slarse commented Jul 5, 2022

MartinWitt commented Jul 5, 2022

slarse commented Jul 5, 2022

		printer.writeln().incTab().writeKeyword("permits").writeSpace();
		printList(sealable.getPermittedTypes(), null, false, null, false, false, ",", true, false, null, prettyPrinter::scan);

feature: sealed classes (JEP 409) #4337

feature: sealed classes (JEP 409) #4337

Conversation

SirYwell commented Dec 7, 2021 • edited Loading

Model

Shadow Model

Questions

Note regards 2.:

MartinWitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Interface Style

NewLine Style

Eclipse Style(?)

SirYwell Jan 5, 2022 • edited Loading

Choose a reason for hiding this comment

slarse commented Jan 29, 2022

SirYwell commented Jan 29, 2022

slarse commented Feb 2, 2022

SirYwell commented Feb 4, 2022

slarse commented Mar 18, 2022

slarse commented Apr 20, 2022

slarse commented May 11, 2022

MartinWitt commented May 13, 2022

MartinWitt commented May 18, 2022

SirYwell commented May 18, 2022

raghav-deepsource commented May 20, 2022 • edited Loading

MartinWitt commented Jun 16, 2022 • edited Loading

MartinWitt commented Jun 21, 2022

MartinWitt commented Jun 27, 2022

slarse commented Jul 5, 2022

MartinWitt commented Jul 5, 2022

slarse commented Jul 5, 2022

SirYwell commented Dec 7, 2021 •

edited

Loading

SirYwell Jan 5, 2022 •

edited

Loading

raghav-deepsource commented May 20, 2022 •

edited

Loading

MartinWitt commented Jun 16, 2022 •

edited

Loading