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

[Joe Darcy]

Hi Jan,

Thanks for the review. The next iteration is at:

     http://cr.openjdk.java.net/~darcy/8071961.6

At least one more iteration will be needed on the engineering front; a 
few comments and questions below.

On 7/30/2020 3:27 AM, Jan Lahoda wrote:
> 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).

Adding

test/langtools/tools/javac/warnings/DefaultCtor/NoWarningCases.java
(http://cr.openjdk.java.net/~darcy/8071961.6/test/langtools/tools/javac/warnings/DefaultCtor/NoWarningCases.java.html)

test/langtools/tools/javac/warnings/DefaultCtor/NoWarningRecord.java
(http://cr.openjdk.java.net/~darcy/8071961.6/test/langtools/tools/javac/warnings/DefaultCtor/NoWarningRecord.java.html)

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

Yes; it should be suppressable. I changed the starting series of checks to

3835         if (lint.isEnabled(LintCategory.MISSING_DECLARED_CTOR) &&
3836 !lint.isSuppressed(LintCategory.MISSING_DECLARED_CTOR) &&

but it isn't having the desired effect; perhaps we could discuss how to 
resolve this off-line.

I also added a check for records:

3837             ((c.flags() & (ENUM  |RECORD)) == 0) &&

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

Hmm. I'm judging that as not necessary, for now.


>
> -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

Thanks for the pointer; I'll take a look. (I wrote the tests in the old 
style I was familiar with.)

Cheers,

-Joe