IdentityObject & abstract superclasses

[Dan Smith]

> On Aug 17, 2020, at 5:44 PM, Dan Smith <daniel.smith at oracle.com> wrote:
> 
> Alternative story:
> 
> An inline class must not extend IdentityObject through any of its superclasses. (That's it.)
> 
> A non-inline class implicitly implements IdentityObject if it 1) is concrete, 2) declares an instance field, or 3) has a non-empty <init> method. Abstract classes can also explicitly implement IdentityObject.
> 
> Changing a class so that it implements IdentityObject is a binary incompatible change.
> 
> Now we have a highly-visible concept (IdentityObject) that programmers should generally be aware of anyway, and they should readily understand a difference between abstract classes that implement IdentityObject and those that don't.

Refining some of these details. I spent some time looking at abstract classes in the JDK to figure out what the best strategy is for deciding which implement IdentityObject.

Something I want to be careful about: adding or removing 'IdentityObject' is a binary incompatible change. (Adding will break inline subclasses, removing will break clients depending on subtyping—although, FWIW, the verifier doesn't check superinterface types.) So we don't want to make it too easy to accidentally change whether a class implements IdentityObject.

---

First, the low-hanging fruit: concrete non-inline classes (except Object) always implement IdentityObject. An abstract class indirectly implements IdentityObject when it extends a concrete class.

In java.base, there are 479 abstract classes, and 455 that don't extend a concrete class.

In java.desktop (and dependencies other than java.base; built on macOS), there are 528 abstract classes, and 491 that don't extend a concrete class.

In jdk.compiler (and dependencies other than java.base), there are 479 abstract classes, and 459 that don't extend a concrete class.

---

Second: instance fields. An inline class can't have a superclass with instance fields, which makes sense because there's no mechanism for mutating superclass instance fields, during instance initialization or later in an instance method. (It would be possible to design a factory-based initialization process, compartmentalized across multiple superclasses, but we haven't done so and don't plan to.)

Inner classes have an implicit "enclosing instance" field.

java.base:
455 candidates
125 extend another class with instance fields
17 more are inner classes
164 more declare their own instance fields
= 149 remaining

java.desktop:
491 candidates
118 extend another class with instance fields
16 more are inner classes
173 more declare their own instance fields
= 184 remaining

jdk.compiler:
459 candidates
210 extend another class with instance fields
21 more are inner classes
124 more declare their own instance fields
= 104 remaining

Most abstract classes—at least ⅔ in these code bases—declare or inherit instance fields.

I think it makes sense that an abstract class that declares instance fields, or an inner abstract class, automatically implements IdentityObject. We can infer this both at compile time and class load time.

There's a little risk here that refactorings involving private fields could cause surprises. Traditionally, factoring something out into a private field (maybe for performance?) is an implementation detail. But I think we can get programmers to buy into the mindset that declaring a field is an implicit opt-out from inline subclasses. It's something new that requires some attention as you're programming.

---

Third: constructors. There is no delegated instance initialization process for inline classes, so a superclass of an inline class must not have any instance initialization logic. Just where we draw the line on "any instance initialization logic" is unclear.

java.base:
149 candidates
9 with a non-empty constructor (perform SecurityManager checks)
16 with a non-empty super constructor
1 with an empty constructor with a parameter (java.security.cert.CertStoreSpi)
1 with multiple constructors, all empty (java.security.SecureRandomSpi)
6 with an empty constructor with restricted access*
58 with an unrestricted empty no-arg constructor
58 with no constructor

java.desktop:
184 candidates
2 with a non-empty constructor
2 with an empty constructor with restricted access*
46 with an unrestricted empty no-arg constructor
134 with no constructor

jdk.compiler:
104 candidates
4 with a non-empty constructor
1 with an empty constructor with restricted access*
20 with an unrestricted empty no-arg constructor
79 with no constructor

(*Restricted access means a package-access or private constructor in a public or protected class, and a private constructor in a package-access class.)

As a baseline, all the "no constructor" cases should be inline superclass candidates, so should not implement IdentityObject.

However, it's quite common to redundantly declare an empty constructor, e.g. for documentation purposes. (In fact, javac just added an optional lint warning supporting a coding convention in which constructors should always be explicit—see JDK-8071961.) Those classes probably shouldn't be IdentityObjects, either.

Restricted access doesn't necessarily need to prevent inline subclasses, and doesn't have much to do with identity. We could leave it to the compiler to check super constructor access. But how to enforce that at run time? (There is no 'invokespecial' in the bytecode....)

Constructor parameters are a borderline signal that you expect something to happen on instance initialization, even if you don't actually do anything with the parameter right now. If you have *multiple* constructors, maybe we only care about the no-args one.

And then there are constructors with something more than 'super()' in their body. (Instance initializers, too, although I found no examples of them in any of the abstract classes I looked at.) These classes clearly need to implement IdentityObject in order to guarantee the traditional superclass initialization behavior.

An approach I like, though it may be too disruptive: it's a compile-time error to declare a non-empty constructor in an abstract class unless that class implements IdentityObject (explicitly, indirectly, or by virtue of having fields). That would trigger ~10 compiler errors in java.base, 2 in java.desktop, and 4 in jdk.compiler. The advantage is that there's no ambiguity or surprises—for an abstract class without fields, you either opt in to IdentityObject explicitly, or you implicitly assert that you don't intend to, and then the compiler catches any problems with that assertion.

Alternatively, we can once again infer 'implements IdentityObject' if a constructor has parameters or a body consisting of anything other than 'super()'. It's fairly easy to change the implicit 'implements' by doing something like adding a 'println' statement—but in practice, those sorts of changes may not be all that likely.

In the JVM:

- We've discussed a flag on <init> methods to indicate that they are "empty". It seems a shame to do that when the compiler can also generate 'implements IdentityObject'. But this is a "wrong default" situation—abstract classes need to be presumed IdentityObjects unless they explicitly choose not to be. (I worry, e.g., about bytecode generators that didn't get the memo and continue to assume checks in their <init> methods will always be executed.) So I think we'll need a rule that says you implicitly implement IdentityObject at class load time unless you have a special "empty" <init> method.

- Encoding "empty": current proposal is that an empty <init> is ACC_ABSTRACT and has no Code attribute. We could revisit if we like, maybe with a new flag, or a class-level attribute, but this still seems pretty reasonable.

- How to handle access control? I do think it's worth supporting limited access somehow (totally reasonable to allow inline subclasses in the current package, but reject subclasses elsewhere). I don't love any of the options, but maybe the best approach is to add a preparation- or initialization-time check that the superclass has a "<init>()V" method with appropriate access? Or maybe we could have some kind of class-level attribute that controls access for subclassing (this is sort of like sealing)?

Hmm, the more I think about this, the less enthusiastic I am about encoding important class properties in an "<init>" method that's never going to be referenced...

---

Finally: synchronized methods. These strike me as a pretty subtle implementation detail. But they're also *very* closely tied to the semantics of IdentityObject. Among our remaining candidate classes, they're pretty rare:

java.base:
123 candidates
7 with synchronized methods
= 116 can safely allow inline subclasses (24% of all abstracts)

java.desktop:
182 candidates
7 with synchronized methods
= 175 can safely allow inline subclasses (33% of all abstracts)

jdk.compiler:
100 candidates
0 with synchronized methods
= 100 can safely allow inline subclasses (21% of all abstracts)

So synchronized methods on these field-free, initialization-free classes are pretty rare, but not unheard of. (Examples: java.net.URLStreamHandler, java.io.InputStream, java.lang.invoke.MethodHandleImpl.)

Again, I think we can go one of two ways:

- It's a compiler error to declare a 'synchronized' method if you don't already implement IdentityObject.

or

- Classes with synchronized methods implicitly implement IdentityObject.

The first strategy would produce 7 new compiler errors in java.base and 7 more in java.desktop... but fixing those errors might prompt some reflection about the tradeoff between synchronizing on 'this' and allowing inline subclasses. The second strategy is frictionless to adopt, but may lead to surprises when code is refactored (note that 'synchronized' isn't documented by javadoc).

In the JVM: I'm much less concerned about surprising inline subclasses in this case. The bytecode generator can explicitly implement IdentityObject if they want, and if they didn't/forgot/weren't aware of it, invoking the method on an inline class instance will throw an exception, just as it would if they'd used 'monitorenter' in the method body instead.