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 rules to suggest usage of check(), require() and error(). #1570

Merged
merged 8 commits into from Apr 8, 2019
Merged

Add rules to suggest usage of check(), require() and error(). #1570

merged 8 commits into from Apr 8, 2019

Conversation

marschwar
Copy link
Contributor

Closes #1502

Since this is my first rule I am glad to get feedback and am open for alternative naming suggestions.

arturbosch
arturbosch reviewed Apr 1, 2019
View reviewed changes
Copy link
Member

@arturbosch arturbosch 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 rules. They look good to me.
Have you tested them on some of our analysis projects? -> detekt/scripts/get_analysis_projects.groovy
I'm not sure if just checking for the exception thrown is sufficient here.

@arturbosch arturbosch added this to the RC15 milestone Apr 1, 2019
@marschwar
Copy link
Contributor Author

I have not done it yet, but will definitely do so. Thanks for the tip. Are you worried about false positives or false negatives? I can't really think of any circumstance where it is better to throw an IllegalArgumentException or IllegalStateException directly. But I guess I will see.

3flex
3flex approved these changes Apr 1, 2019
View reviewed changes
Copy link
Member

@3flex 3flex left a comment

Choose a reason for hiding this comment

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

There's always the chance that someone creates their own version of IllegalStateException or IllegalArgumentException which this will flag because it's not using type or symbol resolution... but I don't think it's worth it to use that for this rule. Looks good!

detekt-rules/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/UseCheckOrErrorSpec.kt Outdated
doSomething()
if (a < 0) throw IllegalStateException()
}"""
Assertions.assertThat(subject.lint(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.

nit: please import org.assertj.core.api.Assertions.assertThat so these can change to assertThat(...) instead of Assertions.assertThat(...)

schalkms
schalkms requested changes Apr 1, 2019
View reviewed changes
Copy link
Member

@schalkms schalkms left a comment •
edited

Choose a reason for hiding this comment

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

Thanks! That’s a very nice first rule.
The format of the linted code between """ looks very weird.
The plain text has a weird indentation.

@schalkms
Copy link
Member

schalkms commented Apr 1, 2019

Regarding the symbol resolution, I would say we need to go over all rules and see where we can apply it after the PR is merged.

@marschwar
Copy link
Contributor Author

I just ran a check on most of those analysis projects and there were a lot of occurrences where the rule correctly reports a violation. I did find 2 cases with possible false positives though.

Creating a lambda that throws an IllegalArgumentException I would think is an edge case. But I could try to detect this.

  fun unsafeRunSync(): A =
    unsafeRunTimed(Duration.INFINITE)
      .fold({ throw IllegalArgumentException("IO execution should yield a valid result") }, ::identity)

This I consider more relevant. Throwing an exception with a cause should never report an issue.

    private fun missing(): Nothing {
        if  (cause == null) {
            throw IllegalStateException(
                "Module with the Main dispatcher is missing. " +
                        "Add dependency providing the Main dispatcher, e.g. 'kotlinx-coroutines-android'"
            )
        } else {
            val message = "Module with the Main dispatcher had failed to initialize" + (errorHint?.let { ". $it" } ?: "")
            throw IllegalStateException(message, cause)
        }
    }

schalkms
schalkms requested changes Apr 4, 2019
View reviewed changes
Copy link
Member

@schalkms schalkms left a comment

Choose a reason for hiding this comment

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

Can we please resolve the code duplication.
Otherwise, the code and the structure looks very nice now.

detekt-rules/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/UseRequire.kt Outdated
@@ -42,6 +42,9 @@ class UseRequire(config: Config = Config.empty) : Rule(config) {
}

private fun KtThrowExpression.isIllegalArgumentException(): Boolean {
return findDescendantOfType<KtCallExpression>()?.firstChild?.text == "IllegalArgumentException"
val callExpression = findDescendantOfType<KtCallExpression>()
val argumentCount = callExpression?.valueArgumentList?.children?.size ?: 0
Copy link
Member

Choose a reason for hiding this comment

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

Code duplication here
Can we group this together and pass the type of the exception?

detekt-rules/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/UseCheckOrError.kt Outdated
private fun KtThrowExpression.isIllegalStateException(): Boolean {
return findDescendantOfType<KtCallExpression>()?.firstChild?.text == "IllegalStateException"
private fun KtThrowExpression.isIllegalStateExceptionWithoutCause(): Boolean {
val callExpression = findDescendantOfType<KtCallExpression>()
Copy link
Member

Choose a reason for hiding this comment

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

Code duplication here
Can we group this together and pass the type of the exception?

schalkms
schalkms approved these changes Apr 4, 2019
View reviewed changes
Copy link
Member

@schalkms schalkms left a comment

Choose a reason for hiding this comment

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

LGTM!

detekt-rules/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/ThrowExtensions.kt
import org.jetbrains.kotlin.psi.psiUtil.findDescendantOfType
import kotlin.reflect.KClass

internal fun KtThrowExpression.isIllegalStateException() =
Copy link
Member

Choose a reason for hiding this comment

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

Nice refactoring!

@arturbosch arturbosch merged commit 3b5a7c0 into detekt:master Apr 8, 2019
@marschwar marschwar deleted the do-not-throw branch April 8, 2019 19:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@3flex 3flex 3flex approved these changes

@arturbosch arturbosch arturbosch approved these changes

@schalkms schalkms schalkms approved these changes

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

Successfully merging this pull request may close these issues.

None yet
4 participants
@marschwar @schalkms @3flex @arturbosch