JDK 16 RFR of JDK-8071961: Add javac lint warning when a default constructor is created

[Jan Lahoda]

Hi Joe,

Thanks for this addition! A few comments for your consideration:
-the logic in checkDefaultConstructor could probably be simplified - 
going from "c" the symbol, and as long as the symbol being check is a 
TYP and PUBLIC, go to its owner. E.g. like:
         if (lint.isEnabled(Lint.LintCategory.DEFAULT_CTOR) &&
             Feature.MODULES.allowedInSource(source)) {
             Symbol test = c;
             while (test.kind == TYP && (test.flags() & PUBLIC) != 0) {
                 test = test.owner;
             }
             if (test.kind != PCK) {
                 return ;
             }

-not sure if the check for unnamed package is needed, but it would be 
nice to have a test verifying proper behavior for unnamed packages, 
unnamed module and in non-module mode (i.e. JDK 8 mode).

-should the warning be suppressable? We would need to defer the warning 
using deferredLintHandler and check if it was already suppressed.

-should protected classes (classes with effectively protected access) be 
covered as well?

-for tests like this, I would suggest to consider using TestRunner, 
ModuleTestBase or another framework we use for programmatic tests, to 
avoid adding many small files for testcases, but I don't mind those too 
much. See e.g.:
test/langtools/tools/javac/options/BCPOrSystemNotSpecified.java

Jan

On 29. 07. 20 22:10, Joe Darcy wrote:
> Hello,
> 
> Please review the code changes and CSR for
> 
>      JDK-8071961: Add javac lint warning when a default constructor is 
> created
>      webrev: http://cr.openjdk.java.net/~darcy/8071961.5/
>      CSR: https://bugs.openjdk.java.net/browse/JDK-8250795
> 
> Some background on the design of the warning and broader usage context, 
> while default constructors can be convenient for informal code, they can 
> be a bit troublesome for more formal APIs, such as the public classes of 
> packages in the JDK. One issue is that default constructors do not have 
> javadoc. Another is that a class that semantically should not be 
> instantiated, say it is a solely a holder for  static constants and 
> methods, can get instantiated and subclassed. (Subclasssing such a class 
> is an anti-pattern to allow use short names for the static members, 
> which is no longer necessary since static imports as of Java SE 5.0.)
> 
> Until they were recently fixed, there were about one hundred remaining 
> default constructors in the JDK API outside of the client libraries ( 
> JDK-8250212 Address reliance on default constructors in the JDK 
> (umbrella) ). Other occurrences of this issue were sporadically fixed 
> over the years ( JDK-8236695, JDK-8177153, JDK-8071959, etc.).
> 
> In terms of detailed criteria to issue the new warnings, there was the 
> usual tension in warnings between reducing false positives and false 
> negatives. For example, warning for *any* default constructor, even in a 
> throw-away class, would be more annoying than helpful. With some 
> guidance from the JDK code base, criteria in the current patch are a 
> default constructor merits a warning if:
> 
> * The class is in a named package and the packaged has an unqualified 
> export from its module AND
> * The class is public and, if it is a nested class, all of its lexically 
> enclosing types are public too.
> 
> An unqualified export, where the package is available to use to any 
> module and not just named ones, was taken to indicate classes in the 
> package can comprise a "formal API". It would be simple to change this 
> to an unqualified export, but I wanted to avoid unwanted instances of a 
> new warning. If a public nested class is a non-public enclosing class, 
> the nested class is not directly part of the exported API. These 
> combinations of kinds of exports and nesting are tested in the tests in 
> the DefaultCtor directory.
> 
> The text of the warning message implies what criteria are being used:
> 
>   # 0: symbol, 1: symbol, 2: symbol
>   compiler.warn.default-ctor=\
>       class {0} in exported package {1} of module {2} relies on a 
> default (implicit) constructor
> 
> Once the javac-related changes are reviewed, I'll send a RFR out to 
> build-dev for the make system updates. Basically modules whose use of 
> default constructors has not been addressed (java.desktop, 
> jdk.accessibility, jdk.unsupported.desktop, etc.) have the new warning 
> disabled.
> 
> Thanks,
> 
> -Joe
>