Handling for new warning?

[Joe Darcy]

Hi Mike,

Sharing some experience from developing new lint warnings and clearing 
the JDK code base of them, even seemingly simple warnings often need 
some refinement before being ready for production use. This was the case 
for the equals-hashCode warning (first JDK-6563143, then JDK-8008436, 
and JDK-8009138), handling many subtleties not discussed in the 
corresponding "Effective Java" item. More recently, when starting work 
on JDK-8071961: "Add javac lint warning when a default constructor is 
created," the earliest iteration was trivial since there is easy-to-find 
code in javac which generates a default constructor. However, I quick 
had to start adding conditions to when the warning would be printed to 
avoid spurious messages.

I believe default constructors and the exception warning you're working 
on have a property in common that the warning is most sensible on 
serious/formal classes but not wanted on informal ones or ones that are 
not part of a public API.

For clearing a warning of the JDK in particular, it can be helpful to 
modify the build options, say, to exclude the warning by default 
"-Xlint:all,-new-warning" .. and only enable it on selected modules. The 
files in question are:

     jdk/make/common/SetupJavaCompilers.gmk
     jdk/make/CompileJavaModules.gmk

HTH,

-Joe

On 9/24/2019 6:37 PM, Maurizio Cimadamore wrote:
> My (very humble) 0.02$ on this; I can see where Mike is coming from 
> _and_ where Jon and Mark are coming from.
>
> The problem is that, when we are inside an _implementation_ class 
> (think of a non-exported package in a module), we probably don't want 
> to be too formal about the set of unchecked annotations being thrown.
>
> Conversely, when we are in a public-facing API, it is very likely that 
> unchecked exceptions carry value (IllegalArgumentExecption, 
> IllegalStateException to name a few) and should therefore be 
> documented. To be more concrete, on my recent work on the memory 
> access API, while developing the API and writing the docs I wish I had 
> an automatic way to detect all unchecked exception I missed in the 
> 'throws' clause, as they were likely (a) some documentation missing 
> somewhere or (b) a missing test for that particular failure mode.
>
> The problem is how to reconcile these two aspects - you want a 
> warnings that's useful, but not one that's excessively punitive so 
> that apparently good code gets flagged (I am reminded of when I 
> introduced the warning for raw types and later on relaxed that not to 
> be triggered on stuff like 'instanceof List', which is pretty 
> widespread - and for which adding an extra '<?>' doesn't really add a 
> lot of value).
>
> And, in this case, it can be tricky to come up with a way to 
> distinguish between API and internals. Perhaps we could use the module 
> system to our advantage, and only enable the Lint check for public 
> methods in public classes/interfaces in exported packages. I'm pretty 
> sure something like this should reduce the 'noise' of such a check 
> quite significantly (and the remaining places where the warning is 
> still issued are likely to be informative for us).
>
> Thoughts?
>
> Maurizio
>
> On 25/09/2019 01:59, Jonathan Gibbons wrote:
>>
>> On 9/24/19 5:46 PM, Mike Duigou wrote:
>>>
>>>
>>>
>>>
>>> On 2019-09-24 16:25, mark.reinhold at oracle.com wrote:
>>>> 2019/9/24 15:56:37 -0700, openjdk at duigou.org:
>>>>> The proposed change is to add a lint level warning, ie.
>>>>> -XLint:runtime-undeclared , suggestion to add the unrequired but 
>>>>> missing
>>>>> throws declarations for RuntimeException et al. The advantage to
>>>>> providing the complete throws including RuntimeExceptions is to make
>>>>> them more visible and encourage handling of them. We have one good
>>>>> example of a library which does declare the RuntimeException that are
>>>>> thrown, the JDK.
>>>>
>>>> I’m confused.  I thought you said earlier that there are thousands of
>>>> missing cases in the JDK.
>>>
>>> JDK seems to be in pretty good shape though -Werror stops 
>>> compilation well before completion. The observed problems with large 
>>> numbers of missing declarations have been in javac where there are 
>>> hundreds (perhaps many more) missing throws declarations primarily, 
>>> so far, for IllegalArgumentException and CompletionFailure.
>>
>> I don't think it would be acceptable to change javac so that 
>> CompletionFailure is specified all over the place.   Story: a long 
>> time ago, back when I joined the javac team, I tried doing something 
>> similar to what you're proposing now, except that I tried to do it by 
>> changing CompletionFailure to a checked exception. As you might 
>> guess, the size of the change became unreasonable and I abandoned the 
>> experiment.
>>
>> I'd also be concerned about IllegalArgumentException and friends like 
>> NullPointerException becoming too pervasive across the codebase.
>>
>> -- Jon
>>
>>
>>>
>>> There's no easy way to know the total number of potential throws 
>>> "fixes" needed because adding missing declarations has a tendency to 
>>> produce new warnings in the methods which call the fixed method. I 
>>> added a couple hundred across a dozen or so files but didn't feel I 
>>> was any near complete based on the number of files I hadn't yet 
>>> covered in the javac source.
>>>
>>>>
>>>>> ...
>>>>>
>>>>> The check will apply to all instances of RuntimeException and all
>>>>> subclasses.
>>>>
>>>> I agree with Jon: This proposal is likely to have broad impact and 
>>>> thus
>>>> needs wider discussion.  May I suggest the jdk-dev list?
>>>
>>>
>>> For JDK this would seem to be a separate problem--the missing 
>>> "throws" could in some cases mean that the method signature and 
>>> javadoc are an incomplete specification of the API and a 
>>> potential/probable JCK issue.
>>>
>>> For the moment I am going to working on completing the minimal patch 
>>> which provides the new lint warning and allows compilation by 
>>> disabling the warning.
>>>
>>>>
>>>> - Mark
>>>
>>>
>>> Cheers,
>>>
>>> Mike