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

Extend CanBeNonNullable rule to check function params #4431

Merged
merged 24 commits into from
Jan 12, 2022

Conversation

severn-everett
Copy link
Contributor

This addresses issue #4377.

val b = a!! + 2
}
""".trimIndent()
Assertions.assertThat(subject.compileAndLintWithContext(env, code)).hasSize(1)
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
Assertions.assertThat(subject.compileAndLintWithContext(env, code)).hasSize(1)
assertThat(subject.compileAndLintWithContext(env, code)).hasSize(1)

@schalkms
Copy link
Member

Thanks for reviewing this! 🙂 @BraisGabin

@codecov
Copy link

codecov bot commented Jan 1, 2022

Codecov Report

Merging #4431 (9875284) into main (ff7824d) will decrease coverage by 0.22%.
The diff coverage is 69.45%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #4431      +/-   ##
============================================
- Coverage     84.32%   84.10%   -0.23%     
- Complexity     3293     3299       +6     
============================================
  Files           472      473       +1     
  Lines         10522    10716     +194     
  Branches       1885     1962      +77     
============================================
+ Hits           8873     9013     +140     
- Misses          671      684      +13     
- Partials        978     1019      +41     
Impacted Files Coverage Δ
...ekt/rules/style/SpacingBetweenPackageAndImports.kt 75.00% <ø> (ø)
...osch/detekt/rules/bugs/UselessPostfixExpression.kt 71.73% <33.33%> (-0.99%) ⬇️
.../arturbosch/detekt/rules/style/CanBeNonNullable.kt 71.60% <70.52%> (-1.54%) ⬇️
...urbosch/detekt/rules/performance/ArrayPrimitive.kt 74.19% <100.00%> (-0.81%) ⬇️
...etekt/generator/printer/defaultconfig/Exclusion.kt 96.66% <0.00%> (-0.11%) ⬇️
...rbosch/detekt/formatting/wrappers/TrailingComma.kt 100.00% <0.00%> (ø)
...ab/arturbosch/detekt/formatting/KtLintMultiRule.kt 95.89% <0.00%> (+0.05%) ⬆️

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 ff7824d...9875284. Read the comment docs.

Copy link
Member

@BraisGabin BraisGabin left a comment

Choose a reason for hiding this comment

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

🎉 Thanks! The tests are really good!

@severn-everett
Copy link
Contributor Author

Still got a few more to appease the CodeCov gods 😁, plus that case with override functions that was uncovered in BaseContext.

@severn-everett
Copy link
Contributor Author

@BraisGabin I misread your comment about what to do when checking whether a param isn't null, so this is going to take a bit to rework; I'll let you know when it's ready to be evaluated again. On a side note, is there a way to access the PR labels? It'd be nice to just put a WIP label on this for situations like this.

@BraisGabin
Copy link
Member

You can move it back to draft

@severn-everett
Copy link
Contributor Author

@BraisGabin This should be good for review now.

Copy link
Member

@BraisGabin BraisGabin left a comment

Choose a reason for hiding this comment

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

Thanks for the hard work! I have a cople of comments more. Tell me what do you think.

@@ -65,5 +65,5 @@ class ExceptionRaisedInUnexpectedLocation(config: Config = Config.empty) : Rule(
private fun isPotentialMethod(function: KtNamedFunction) = methodNames.any { function.name == it }

private fun hasThrowExpression(declaration: KtExpression?) =
declaration?.anyDescendantOfType<KtThrowExpression>() == true
declaration?.anyDescendantOfType<KtThrowExpression>() ?: false
Copy link
Member

Choose a reason for hiding this comment

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

Is this change needed? Both do the same and I don't see why one should be promoted over the other

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is probably code from an older iteration and should be reverted.

fun startWith(value: String?): Boolean = excludes.any { value?.startsWith(it) ?: false }
fun startWith(value: String?): Boolean {
return if (value != null) excludes.any(value::startsWith) else false
}
Copy link
Member

Choose a reason for hiding this comment

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

I agree that this code is better (we don't need to loop the collection). But was the previous code flagged by CanBeNonNullable? I don't think it should.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is code from an older iteration and can probably be reverted.


fun foo(a: A?) = a?.foo
""".trimIndent()
assertThat(subject.compileAndLintWithContext(env, code)).hasSize(1)
Copy link
Member

Choose a reason for hiding this comment

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

I'm not 100% sure of this. I do agree that I would prefer a to be not null on my code but I don't know if this could be far too strict for a default rule of detekt. At the end of the day you are taking a decision about what to do return when a is null. It just turns out that it's a really "trivial" decission.

That's exactly the same as write:

fun foo(a: A?) = if (a == null) null else a.foo

And as far as I don't like it too much the writer of foo took the decision to do something with the null value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My thought was that it'd move the null-check to outside of the function so that the function call could be skipped entirely - see the code in UselessPostfixExpression for how it'd look using the rule as it now is. However, you're right that the code in your example wouldn't be flagged, so it'd be better to revert this rule aspect and keep things as symmetrical as possible.

return aObj?.foo
}
""".trimIndent()
assertThat(subject.compileAndLintWithContext(env, code)).hasSize(1)
Copy link
Member

Choose a reason for hiding this comment

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

Same here

@BraisGabin
Copy link
Member

To me this is good to go now.

@cortinico cortinico added this to the 1.20.0 milestone Jan 5, 2022
@cortinico cortinico added the rules label Jan 5, 2022
@cortinico cortinico changed the title Enable CanBeNonNullable rule to check function params Extend CanBeNonNullable rule to check function params Jan 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants