Skip to content

Commit

Permalink
Fixes #2184: NPE on package-info.java without package declaration.
Browse files Browse the repository at this point in the history
  • Loading branch information
rspilker committed Jul 24, 2019
1 parent 1f58c92 commit 867c3ca
Show file tree
Hide file tree
Showing 2 changed files with 7 additions and 1 deletion.
1 change: 1 addition & 0 deletions doc/changelog.markdown
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ Lombok Changelog
* BUGFIX: Delombok would turn something like `List<byte[]>...` in a method parameter to `List<byte...>...` [Issue #2140](https://github.com/rzwitserloot/lombok/issues/2140)
* BUGFIX: Javac would generate the wrong equals and hashCode if a type-use annotation was put on an array type field [Issue #2165](https://github.com/rzwitserloot/lombok/issues/2165)
* BUGFIX: Eclipse 2019-06 + JDK-12 compatibility + an `@Singular` builder entry would produce a cascade of error dialogs. [Issue #2169](https://github.com/rzwitserloot/lombok/issues/2169)
* BUGFIX: Javac would throw a NullPointerException if the package-info.java did not contain a package declaration. [Issue #2184](https://github.com/rzwitserloot/lombok/issues/2184)
* IMPROBABLE BREAKING CHANGE: Stricter validation of configuration keys dealing with identifiers and types (`lombok.log.fieldName`, `lombok.fieldNameConstants.innerTypeName`, `lombok.copyableAnnotations`).
* IMPROBABLE BREAKING CHANGE: The fields generated inside builders for fields with defaults (with `@Builder` on a class with fields marked `@Default`) now have `$value` as the name; direct manipulation of these fields is not advised because there is an associated `$set` variable that also needs to be taken into account. [Issue #2115](https://github.com/rzwitserloot/lombok/issues/2115)

Expand Down
7 changes: 6 additions & 1 deletion src/core/lombok/javac/apt/LombokProcessor.java
Original file line number Diff line number Diff line change
Expand Up @@ -389,7 +389,12 @@ static String getModuleName(Element element) {
}

private JCCompilationUnit toUnit(Element element) {
TreePath path = trees == null ? null : trees.getPath(element);
TreePath path = null;
if (trees != null) {
try {
path = trees.getPath(element);
} catch (NullPointerException ignore) {}
}
if (path == null) return null;

return (JCCompilationUnit) path.getCompilationUnit();
Expand Down

6 comments on commit 867c3ca

@Masterxilo
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldnt there be a comment why the catch is needed and that this is actually a bug in javac and this is a workaround? shouldnt the fix involve making sure at least in the future the correct file is reported when javac generates weird exceptions in places? me and the other guy we lost more time in finding this bug than it took to hackfix this. the debugging and reporting experience in the future could be aided if a more extensive resolution is made to make the error case more easily there might be more such errors lurking in the future, so this would be a worthy investment, paying off better than fighting just a symptom

@rspilker
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really appreciate you took the time to chase down the cause of the problem, and that you created a git repo for me to clone.

Occasionally we add comments for certain workarounds. In this case, the git annotation should help to find the reason for the catch.

We also considered an alternative fix: skip package-info.java and module-info.java

If you want to spend time on improving the code or error reporting, you could create a pull request. Before you start working on it, please check with us if we would agree with your chosen solution.

@rspilker
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was a good idea, I've added the some comments in ff1c01d

@pzygielo
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Masterxilo - did you manage to report it to Oracle (probably possible) or OpenJDK (probably impossible)? I can see only the old NPE ... in TreeInfo.declarationFor method (and corresponding OpenJDK) but for other case.

@amseager
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@psygielo I've also reproduced it with Zulu JDK btw

@pzygielo
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amseager

I've also reproduced it with Zulu JDK btw

No surprise as it's OpenJDK based.

On the other hand - ECJ works fine.

Please sign in to comment.