search

CadixDev / Mercury

A source transformation and -remapping framework for Java.
Eclipse Public License 2.0
54 stars 23 forks source link

Fix NPE with Path.getFileName() #3

Closed modmuss50 closed 4 years ago

modmuss50 commented 4 years ago

Path.getFileName() can be null, thus casuing a NPE when Mercury is walking the files.

I discoverved this when trying to use an NIO zip file system as the source input.

jamierocks commented 4 years ago

Interesting, do you know what circumstances getFileName returns null?

stephan-gh commented 4 years ago

Does it work for the ZIP file system otherwise? That's pretty nice actually!

jamierocks commented 4 years ago

Does it work for the ZIP file system otherwise? That's pretty nice actually!

It should do, you can grab the root directory of a zip as a Path, and all of the typical operations you can do would work.

jamierocks commented 4 years ago

To elaborate the issue here, is that directories are being tested - in the specific case of zip file systems, getFileName() returns null for the / path.

I have always handled this with a .filter(p -> !Files.isDirectory(p) check, however this may be even more safe - so I'm happy to merge this, given @Minecrell's approval.

modmuss50 commented 4 years ago

Thanks for merging this.

Sorry for wasting your time as I should have tested my PR against the actaull code I had. Turns out the AST turns the Paths back into Files

CompilationUnitResolver.java#L1013

This PR wont do any harm, but was sadly a waste of time, sorry again for the inconvenience I caused.

stephan-gh commented 4 years ago

Yeah... Sorry about that! The JDT API is quite limited in that regard...

jamierocks commented 4 years ago

That sucks, never-the-less, there is still some worth to this PR in certain edge-cases. Going forward I might change it to .filter(p -> !Files.isDirectory(p) to be explicit about which cases we prevent, but for now its fine.