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

Removing complex cases from the warning related to null checks #532

Merged
merged 4 commits into from
Nov 20, 2020

Conversation

orchestr7
Copy link
Member

What's done:

Null check will not trigger on the following code:

  1. if (myVar == null || otherValue == 5 && isValid) {}
  2. if (myVar != null) {
    println("not null")
    } else if (true) {
    println("Other condition")
    }

### What's done:
Null check will not trigger on the following code:
1) if (myVar == null || otherValue == 5 && isValid) {}
2) if (myVar != null) {
       println("not null")
   } else if (true) {
       println("Other condition")
   }
@codecov
Copy link

codecov bot commented Nov 16, 2020

Codecov Report

Merging #532 (97f0b32) into master (bbb494b) will decrease coverage by 0.04%.
The diff coverage is 55.55%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #532      +/-   ##
============================================
- Coverage     81.93%   81.88%   -0.05%     
- Complexity     1637     1641       +4     
============================================
  Files            78       78              
  Lines          4123     4129       +6     
  Branches       1302     1306       +4     
============================================
+ Hits           3378     3381       +3     
  Misses          219      219              
- Partials        526      529       +3     
Flag Coverage Δ Complexity Δ
unittests 81.88% <55.55%> (-0.05%) 0.00 <5.00> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ Complexity Δ
...in/org/cqfn/diktat/ruleset/rules/NullChecksRule.kt 77.77% <55.55%> (-5.56%) 18.00 <5.00> (+4.00) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bbb494b...49b9159. Read the comment docs.

@orchestr7 orchestr7 linked an issue Nov 17, 2020 that may be closed by this pull request
Comment on lines +57 to +58
val parentIfPsi = parentIf.psi
require(parentIfPsi is KtIfExpression)
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
val parentIfPsi = parentIf.psi
require(parentIfPsi is KtIfExpression)
val parentIfPsi = parentIf.psi as KtIfExpression

Is require here only for smart cast? You already have element type check above.

Copy link
Member Author

Choose a reason for hiding this comment

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

no, I also wanted to prohibit other KtElements in this method

* checks if condition is a complex expression. For example:
* (a == 5) - is not a complex condition, but (a == 5 && b != 6) is a complex condition
*/
private fun KtBinaryExpression.isComplexCondition(): Boolean {
Copy link
Member

Choose a reason for hiding this comment

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

If this function is called for every BINARY_EXPRESSION in a CONDITION, then you could encounter situation like if (foo.bar(x == null)) which is a complex statement but won't be detected by your code.

Copy link
Member Author

Choose a reason for hiding this comment

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

as we discussed - we don't like to raise warnings on such complex cases. I even should remove part of it

info/guide/guide-chapter-8.md Outdated Show resolved Hide resolved
@petertrr
Copy link
Member

petertrr commented Nov 18, 2020

I discovered that this rule currently reports simple comparisons as errors too:

val isDefault = configuration == null

yields

[AVOID_NULL_CHECKS] Try to avoid explicit null-checks. Use '.let/.also/?:/e.t.c' instead of: configuration == null

Create #550 for this

Co-authored-by: Peter Trifanov <peter.trifanov@mail.ru>
@orchestr7 orchestr7 merged commit 205dbfc into master Nov 20, 2020
@petertrr petertrr deleted the feature/null-check-complex-cases branch November 27, 2020 11:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rule 4.3.3: null-checks: cases for checking
2 participants