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

[Joe Darcy]

Hello,

Following the guidance of Jon and Jan, next iteration of the webrev is at:

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

In this version, the checkDefaultConstructor method uses the 
deferredLintHandler so that the annotation suppression can be done 
properly, which is checked for by the tests.

The implementation checks 
lint.isEnabled(LintCategory.MISSING_DECLARED_CTOR) at the top-level as 
well as passing that condition into the lambda for the 
deferredLintHandler. The first call checks if the warning is enabled at 
all and the second if it is enabled AND hasn't been suppressed an an 
annotation. (The annotation information is not available during the 
compile phase when the first check is run.) It would be functionally 
correct to just do the deferred check in the lambda, but I estimated the 
outer check would be inexpensive screening.

Following the model from the existing test file, I wrote 
DefaultCtorWarningToolBox.java to generate the seven small files 
previously stored separately. Since I started by copying the existing 
file, I kept its copyright date as the starting date.

Thanks,

-Joe

On 7/30/2020 10:21 PM, Joe Darcy wrote:
> 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
>
>