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

[java] Deprecate rule NullAssignment #3714

Open
oowekyala opened this issue Jan 4, 2022 · 2 comments
Open

[java] Deprecate rule NullAssignment #3714

oowekyala opened this issue Jan 4, 2022 · 2 comments
Labels
is:deprecation The main focus is deprecating public APIs or rules, eg to make them internal, or removing them

Comments

@oowekyala
Copy link
Member

I think NullAssignment should be deprecated. Its justification is really bad:

Sometimes, this type of assignment is an indication that the programmer doesn't completely understand what is going on in the code.

This is completely unverifiable. The message is even worse:

Assigning an Object to null is a code smell. Consider refactoring.

null is not more dangerous when assigned into a variable than in other places, so the rule really has no reason to exist.
Using data flow analysis we should try to detect possible NPEs in a useful manner, not just blindly flag all null literals in the program. I think this kind of very noisy and badly justified rules make PMD look bad and they should be hunted down.

Originally posted by @oowekyala in #3649 (comment)

@adangel
Copy link
Member

adangel commented Jan 8, 2022

I'm neither for nor against deprecating this rule. I agree with your points (bad justification, too simple implementation by just looking at null literals).
I want to throw in this however: Maybe the initial reason (it's from PMD 1.02!!) for this rule is long gone, but it reminds me of the Null Object Pattern. Maybe the rule should suggest to use Null Objects instead of null? At least, the relationship of this rule and the Null object pattern should be considered in more detail before making a final decision.

@oowekyala
Copy link
Member Author

I think we can't implement a strategy to recommend the null object pattern that wouldn't be wrong most of the time... The best solution we can write would boil down to flagging every null literal and say "maybe this should be be a null object instead?". I think the problem is that PMD can't be in the head of the programmer and recommend design decisions with so little data as a null assignment. My stance is that we shouldn't even try to make such recommendations, but accept that they're out of scope and concentrate our work on more useful rules.

Maybe another part of the reason this rule exists is to avoid manually cleaning up variables by setting them to null at the end of a function, for instance. This is mostly caught by UnusedAssignment now.

@adangel adangel added the is:deprecation The main focus is deprecating public APIs or rules, eg to make them internal, or removing them label Jan 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
is:deprecation The main focus is deprecating public APIs or rules, eg to make them internal, or removing them
Projects
None yet
Development

No branches or pull requests

2 participants