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

[Maurizio Cimadamore]

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/20150223/e011a99e/attachment.html>