False positive CLASS_NOW_FINAL making a public class final when it has a single private 0-argument constructor

[garydgregory]

Over at Apache Commons Compress PR 712, using The Maven plugin version 0.23.1, we are seeing a CLASS_NOW_FINAL when we change a public class to public final class but this class has a private 0-argument constructor:

    /** No instances needed. */
    private TarUtils() {
    }

According to the JLS' binary compatibility specification for Java 21:

If a class that was not declared final is changed to be declared final, then an IncompatibleClassChangeError is thrown if a binary of a pre-existing subclass of this class is loaded, because final classes can have no subclasses; such a change is not recommended for widely distributed classes.

Because our class cannot be subclassed due to its private 0-argument constructor, changing it to final is binary compatible, which is why we think there is a bug when JApiCmp signals CLASS_NOW_FINAL.

I have tested this use case using Java 8, 11, 17, 21, and 25 and I can indeed run such a configuration.

[ppkarwasz]

For additional context: this edge case was also discussed and addressed in bndtools/bnd#5808. That thread has a longer discussion of the ramifications.

Summarizing: a single private constructor does not absolutely prevent subclasses from existing. However, any possible subclasses are tightly constrained:

• Java 11 and later: subclasses must belong to the same nest group, which is fixed at compile time for the class with the private constructor.
• Java 8: the compiler may generate a package-private synthetic constructor at compile time, which again limits subclassing to code in the same package.

In either case, the only potential subclasses are confined to the same package. This means that marking such a class final does not break binary compatibility from the perspective of external clients, since any affected subclasses cannot exist outside the package boundary.

[siom79]

Thanks for reporting.

The requested solution is to introduce an new change type like CLASS_NOW_FINAL_PRIVATE_CONSTRUCTOR that is binary compatible?

[ppkarwasz]

Hi @siom79 ,

This edge case is a little bit wider. If a class is not subclassable, many checks no longer apply:

• You can remove protected fields and methods.
• You can lower the access level of protected fields and methods.
• You can add a final modifier to the class.

Therefore, instead of checking if a class is final, I would propose to check if a class is subclassable, using this recursive definition:

• The class is public, not sealed and has at least one public or protected constructor,
• Or the class has at least one subclassable subclass (in the same package).

What do you think?

Note: Our main concern is that we can not disable the CLASS_NOW_FINAL check without either disabling all the remaining checks on TarUtils or disabling this specific check for all the remaining classes.

[siom79]

The point here is that japicmp is also a change tracking tool. So we still need CLASS_NOW_FINAL to track changes of the final flag.
Currently the information if a change is incompatible is bound to the change type. The concept you propose seems to be an abstract layer above the mere change tracking.

[garydgregory]

Hi @siom79

My main concern is not so much change tracking but binary compatibility and the ability to break a build when a binary break occurs.

IMO this is the greatest value japicmp provides to all of Apache Commons and the Java ecosystem at large, and I'm grateful for it 😄 That said, if there's anything we can do to reduce the casting of a wider net for this use case, it would be much appreciated.

[siom79]

What about the solution mentioned in this issue?

I understand that they solved it by evaluating the following table (F: class is final, P all constructors are private):

From to:	_ _	_ F'	P' F'	P'_
_ _	🆗	MAJOR	MAJOR	MAJOR
_ F	MINOR	🆗	MAJOR	MAJOR
P F	MINOR	MINOR	🆗	🆗
P _	MINOR	MINOR	🆗	🆗

So at first glance this means introducing a new type CONSTRUCTORS_NOW_ALL_PRIVATE in addition to CLASS_NOW_FINAL and evaluating compatibility based on the combination of both.

What do you think?

[garydgregory]

Nice table 👌

Our use case would be (I think): P_ x P'F = OK. Right?

[siom79]

Yes, your initial case would be P_ to P'F': OK

In the meantime I think that removing CLASS_NOW_FINAL and introducing CLASS_NOW_NOT_SUBCLASSABLE on the base of the table might be a more consistent solution.

From to:	__	_F'	P'_	P'F'
__	OK	CLASS_NOW_NOT_SUBCLASSABLE	CLASS_NOW_NOT_SUBCLASSABLE	CLASS_NOW_NOT_SUBCLASSABLE
_F	OK	OK	OK	OK
P_	OK	OK	OK	OK
PF	OK	OK	OK	OK

[ppkarwasz]

Hi @siom79,

The point here is that japicmp is also a change tracking tool. So we still need CLASS_NOW_FINAL to track changes of the final flag. Currently the information if a change is incompatible is bound to the change type. The concept you propose seems to be an abstract layer above the mere change tracking.

I don’t think the notion of “subclassability” (or maybe it is better to call is “extensibility”) requires changing how change types are bound to incompatibility. Instead, it should affect which members are considered relevant during comparison.

From what I understand, japicmp works roughly like this:

• It discovers all classes (public and package-private) in the old and new versions.
• For each class, it builds a JApiClass object that holds changes for its modifiers, fields and methods.
• Depending on configuration, JApiClass filters out certain members. By default, only protected and public members are tracked.

What I propose is refining the filtering step based on whether the class is “subclassable”:

• If the class is “subclassable”:

  • Changes to public and protected members are relevant.
  • Changes to the final modifier are also relevant and generate CLASS_NOW_FINAL and CLASS_NO_LONGER_FINAL changes.

• If the class is not “subclassable”:

  • Only changes to public members are relevant, because protected members are effectively equivalent to package-private in this case.
  • The addition or removal of the final modifier is not relevant.

There are some subtle cases to consider. For example:

public class A {
  private A() {}

  protected void doFoo() {}

  public static class B extends A {
    public B() {}
  }
}

In this scenario, removing doFoo is not a relevant change for A itself, since A cannot be subclassed externally.
However, it is a relevant change for B, because B is subclassable by users and inherits doFoo.

[ppkarwasz]

Our use case would be (I think): P_ x P'F = OK. Right?

Our case is a bit broader. In my PR I:

• add the final modifier to TarUtils,
• remove several protected methods, and
• change some methods from protected to package-private.

The goal of these changes is to reduce cognitive load for developers: it should be immediately clear that protected methods are not part of the public API. This makes it easier to change their contracts and signatures, since only a few internal call sites need to be updated.

[siom79]

One thing to keep in mind is that whether a class can be extended or not may change between versions.

That’s why I started with a commit that introduces CLASS_NOW_NOT_EXTENDABLE instead of CLASS_NOW_FINAL, since it covers both cases: final classes and classes with only private constructors (and possibly sealed classes in the future).

What still needs to be done is skipping protected fields and methods when the new class is not extendable.

[garydgregory]

TY @ppkarwasz , that's a brilliant explainer! 😄

[garydgregory]

One item that's not 100% clear to me is whether the introduction of CLASS_NOW_NOT_EXTENDABLE and the new logic means that our use case will be handled with or without a change in our JApiCmp configuration.