RFR (S) 8073550 : java* tools: replace obj.getClass hacks with Assert.checkNonNull or Objects.requireNonNull

Wed Feb 25 23:48:47 UTC 2015

There are a number of minor issues here. I'm reviewing the 
langtools-v2.patch file.

1. Perhaps because of the churn, a number of files now have a new 
unnecessary import.

2. A call of either Assert.checkNonNull(x); or 
Objects.requireNonNull(x); does not need an explanatory comment of "// 
null check" and such comments should be removed.  I can understand why 
such comments might have been left by automated scripts; I don't 
understand why in a few places, such comments were newly added.

3. In general, we should not depend on the javac internal Assert 
mechanism outside of javac.

4. In some places where there are multiple checks, the old code used to 
do all the checks before using any of the checked values. That has been 
changed in a few places, and in one place could leave an open stream 
lying around. It would be better to preserve the original execution order.

Here is the full list of comments:

DocTreePath
     redundant import of Assert
102,103 redundant comments

TreePath
     redundant import of Assert
62,63   redundant comments

com.sun.tools.classfile.ClassReader
     com.sun.tools.classfile.* should not really depend on anything in
         com.sun.tools.javac.*
     In this case, I think Objects.requireNonNull is preferable
     better to do the check before opening input stream

com.sun.tools.classfile.ClassFileDependencies
     com.sun.tools.classfile.* should not really depend on anything in
         com.sun.tools.javac.*
     In this case, I think Objects.requireNonNull is preferable

ClientCodeWrapper
     NPE would be better than an assert failure, so Objects.checkNonNull
     would be better

JavacScope
     redundant import, redundant comment
     in this instance AssertionError would be better; this is an
     internal method, and javac would be wrong to provide null here

JavacTaskImpl
     redundant comment

JavacTool, JavacFileManager, Locations, RegularFileObject
     redundant import, redundant comments

ZipArchive
     redundant import

PathFileObject
     redundant import, redundant comments

BaseFileManager
434-436 remove (inline) unnecessary method

JavahTask
     redundant imports

AttributeWriter
     Previously, the code did two checks, and only if both are OK
     did the assigments happen. With the proposed check, one
     assignment might happen, but not the other.

JavapTask
     redundant imports, redundant comment

Content
     doclet internal class should not use javac internal class
     better to use Objects.requireNonNull, or remove (inline)
     the method

Start
     doclet internal class should not use javac internal class
     better to use Objects.requireNonNull

JavadocTool
     duplicate imports

-----------------------------

-- Jon

On 02/23/2015 06:52 AM, Maurizio Cimadamore wrote:
> The attached path passes all tests. I did a scan over the original 
> patch and used Objects.requireNonNull in all API-sensitive 
> implementations. I still have some doubts, so I'd appreciated if 
> somebody else could glance over the changes...
>
> Maurizio
>
> On 23/02/15 13:29, Maurizio Cimadamore wrote:
>> Hi Aleksey,
>> I did a build/test run on your patch and we have 5 langtools 
>> regression failures - they seem to be caused by the fact that there 
>> is some infrastructure in either the tests or in the file manager 
>> hierarchy itself which assign special semantics to NPEs, so when you 
>> replace them with something else (AssertionError), those tests stop 
>> working.
>>
>> Here are the failing tests:
>>
>> Execution failed: `main' threw exception: java.lang.AssertionError
>>
>>     tools/javac/T6351767.java: 
>> javax.tools.JavaCompilerTool.getStandardFileManager().list() treats 
>> directories as package
>>     tools/javac/api/6410643/T6410643.java: JSR 199: The method 
>> JavaCompilerTool.run fails to handle null arguments
>>     tools/javac/api/T6400205.java: getClassLoader(location) returns 
>> null if getLocation(location) returns null
>>     tools/javac/api/T6400207.java: JSR 199: JavaFileManager.list and 
>> unset location
>>
>> Execution failed: `main' threw exception: 
>> java.lang.reflect.InvocationTargetException
>>
>>     tools/javadoc/api/basic/GetTask_OptionsTest.java: javadoc should 
>> have a javax.tools.Tool service provider
>>
>>
>> Most of the failing tests show the following stack trace:
>>
>> java.lang.AssertionError
>> 	at com.sun.tools.javac.util.Assert.error(Assert.java:125)
>> 	at com.sun.tools.javac.util.Assert.checkNonNull(Assert.java:60)
>> *	at com.sun.tools.javac.util.BaseFileManager.nullCheck(BaseFileManager.java:434)*
>> 	at com.sun.tools.javac.file.JavacFileManager.list(JavacFileManager.java:670)
>> 	at T6351767.main(T6351767.java:45)
>> 	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
>> 	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
>> 	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
>> 	at java.lang.reflect.Method.invoke(Method.java:498)
>> 	at com.sun.javatest.regtest.MainAction$SameVMRunnable.run(MainAction.java:776)
>> 	at java.lang.Thread.run(Thread.java:745)
>>
>> I'm deferring this to Jon as his expertize on this area is much 
>> greater than mine. My feeling is that those are public facing 
>> methods, whose spec say that an NPE should occur in certain cases, so 
>> either we leave the code as is, or we replace it with 
>> Objects.requireNonNull (which correctly throws an NPE). Jon, what do 
>> you think?
>>
>> Maurizio
>>
>> On 22/02/15 21:22, Maurizio Cimadamore wrote:
>>> Looks great - thanks, I will push it for you.
>>>
>>> Maurizio
>>>
>>> On 20/02/15 14:40, Aleksey Shipilev wrote:
>>>> Hi,
>>>>
>>>> Please review and sponsor the cleanup in langtools code:
>>>> https://bugs.openjdk.java.net/browse/JDK-8073550
>>>> http://cr.openjdk.java.net/~shade/8073550/webrev.01/
>>>>
>>>> It replaces the getClass "hacks" with either proper Asserts, or
>>>> Objects.requireNonNull if Asserts are not available over the module
>>>> boundaries.
>>>>
>>>> Thanks,
>>>> -Aleksey.
>>>>
>>>
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/compiler-dev/attachments/20150225/775a697d/attachment-0001.html>