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.

Sign up for GitHub

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

Jump to bottom

Add detector for catching NullPointerExceptions #1740

Merged
merged 7 commits into from
Oct 22, 2021
Merged

Add detector for catching NullPointerExceptions #1740

merged 7 commits into from
Oct 22, 2021

Conversation

gamesh411
Copy link
Contributor

@gamesh411 gamesh411 commented Oct 5, 2021 •
edited

Make sure these boxes are checked before submitting your PR -- thank you!

  • Added an entry into CHANGELOG.md if you have changed SpotBugs code

@gamesh411
Copy link
Contributor Author

gamesh411 commented Oct 5, 2021 •
edited

Hi,
This detector detects the catch of NullPointerException as a null checking deemed more elegant (hence the style tag).
I plan on making this detector handle the ancestors of NullPointerException as well (as per the SEICert guideline), but in my prototype, where I handle those as well, there are a lot of false positives thanks to try-with-resources language structures as those are compiled to exception handling in byte code catching Throwable instances.
In order to avoid those false positives I would need a minor refactoring regarding the DominatorsAnalysis API (see PR #1741). Once I have that in place, I can extend this detector with the ancestor handling.

ThrawnCA
ThrawnCA requested changes Oct 12, 2021
View reviewed changes
spotbugs/etc/messages.xml Outdated
<Detector class="edu.umd.cs.findbugs.detect.DontCatchNullPointerException">
<Details>
<![CDATA[
<p> Nullponter exceptions or its ancestors should not be caught.</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

Misspelled "pointer"

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<p> Nullponter exceptions or its ancestors should not be caught.</p>
<p>Nullpointer exceptions or its ancestors should not be caught.</p>

Copy link
Member

Choose a reason for hiding this comment

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

Current implementation checks NullPointerException only, so it's better to remove or its ancestors.
Note that NullPointerException has three throwable ancestors:

  1. java.lang.Throwable
  2. java.lang.Exception
  3. java.lang.RuntimeException

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, fixed.

KengoTODA
KengoTODA requested changes Oct 12, 2021
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
spotbugs/etc/messages.xml Outdated
<Detector class="edu.umd.cs.findbugs.detect.DontCatchNullPointerException">
<Details>
<![CDATA[
<p> Nullponter exceptions or its ancestors should not be caught.</p>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<p> Nullponter exceptions or its ancestors should not be caught.</p>
<p>Nullpointer exceptions or its ancestors should not be caught.</p>

spotbugs/src/main/java/edu/umd/cs/findbugs/detect/DontCatchNullPointerException.java Outdated Show resolved Hide resolved
spotbugs/src/main/java/edu/umd/cs/findbugs/detect/DontCatchNullPointerException.java Outdated Show resolved Hide resolved
spotbugs/src/main/java/edu/umd/cs/findbugs/detect/DontCatchNullPointerException.java Outdated Show resolved Hide resolved
spotbugs/src/main/java/edu/umd/cs/findbugs/detect/DontCatchNullPointerException.java Outdated
import edu.umd.cs.findbugs.visitclass.PreorderVisitor;

public class DontCatchNullPointerException
extends PreorderVisitor implements Detector {
Copy link
Member

Choose a reason for hiding this comment

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

PreorderVisitor has already implemented Detector so no need to implement it again.

Suggested change
extends PreorderVisitor implements Detector {
extends PreorderVisitor {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

spotbugs/etc/messages.xml Outdated
<Detector class="edu.umd.cs.findbugs.detect.DontCatchNullPointerException">
<Details>
<![CDATA[
<p> Nullponter exceptions or its ancestors should not be caught.</p>
Copy link
Member

Choose a reason for hiding this comment

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

Current implementation checks NullPointerException only, so it's better to remove or its ancestors.
Note that NullPointerException has three throwable ancestors:

  1. java.lang.Throwable
  2. java.lang.Exception
  3. java.lang.RuntimeException

@gamesh411
Copy link
Contributor Author

Thanks for the quick response! I have applied the suggestions as best I could. I will ask it here as well, do you prefer the approach where I add every amendment in separate commits (and squash at the end), or should I just rebase every time when it is necessary?

ThrawnCA
ThrawnCA previously approved these changes Oct 13, 2021
View reviewed changes
KengoTODA
KengoTODA requested changes Oct 13, 2021
View reviewed changes
Copy link
Member

@KengoTODA KengoTODA left a comment

Choose a reason for hiding this comment

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

Mostly LGTM, but please fix a minor problem in the CHANGELOG.

CHANGELOG.md Outdated Show resolved Hide resolved
spotbugs/src/main/java/edu/umd/cs/findbugs/detect/DontCatchNullPointerException.java
.constantToString(getConstantPool()
.getConstant(type));

if (name.equals(NULLPOINTER_EXCEPTION_FQCN)) {
Copy link
Member

Choose a reason for hiding this comment

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

memo: Confirmed that name is not null-able. So it's safe to invoke instance method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks a lot! I have also just quickly checked the source ( https://commons.apache.org/proper/commons-bcel/apidocs/src-html/org/apache/bcel/classfile/ConstantPool.html#line.96 ), and every path seems fine.

KengoTODA
KengoTODA previously approved these changes Oct 14, 2021
View reviewed changes
Copy link
Member

@KengoTODA KengoTODA left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for your contribution!
I will keep this PR open for days to ask other teammates to review.

ThrawnCA
ThrawnCA previously approved these changes Oct 14, 2021
View reviewed changes
@KengoTODA KengoTODA dismissed stale reviews from ThrawnCA and themself via da1cb16 October 22, 2021 11:05
@KengoTODA KengoTODA merged commit 422e744 into spotbugs:master Oct 22, 2021
@KengoTODA KengoTODA mentioned this pull request Mar 2, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KengoTODA KengoTODA KengoTODA approved these changes

@ThrawnCA ThrawnCA ThrawnCA left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@gamesh411 @KengoTODA @ThrawnCA