Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix NPE with Path.getFileName() #3

Closed
wants to merge 2 commits into from

Conversation

@modmuss50
Copy link
Contributor

modmuss50 commented Nov 7, 2019

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

This comment has been minimized.

Copy link
Member

jamierocks commented Nov 7, 2019

Interesting, do you know what circumstances getFileName returns null?

@Minecrell

This comment has been minimized.

Copy link
Member

Minecrell commented Nov 7, 2019

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

@jamierocks

This comment has been minimized.

Copy link
Member

jamierocks commented Nov 7, 2019

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

This comment has been minimized.

Copy link
Member

jamierocks commented Nov 7, 2019

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.

jamierocks added a commit that referenced this pull request Nov 8, 2019
Merges GH-3
@jamierocks jamierocks closed this Nov 8, 2019
@modmuss50

This comment has been minimized.

Copy link
Contributor Author

modmuss50 commented Nov 8, 2019

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.

@Minecrell

This comment has been minimized.

Copy link
Member

Minecrell commented Nov 8, 2019

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

@jamierocks

This comment has been minimized.

Copy link
Member

jamierocks commented Nov 8, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.