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

[Jan Lahoda]

Hi,

This looks good to me. The login in Check.checkDefaultConstructor could 
still be simplified, but doesn't have to.

Jan

On 12. 08. 20 18:43, Joe Darcy wrote:
> 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
>>
>>