search

detekt / detekt

Static code analysis for Kotlin
https://detekt.dev
Apache License 2.0
6.1k stars 758 forks source link

Remove existing rule MandatoryBracesIfStatements #2962

Closed veyndan closed 3 years ago

veyndan commented 3 years ago

The rules MandatoryBracesIfStatements and MultiLineIfElse appear to achieve the same thing. I'd nominate to remove MandatoryBracesIfStatements over MultiLineIfElse because MultiLineIfElse is maintained by ktlint (which avoids duplication in improvements/bug fixes) and MultiLineIfElse has the possibility for auto-correction.

schalkms commented 3 years ago

The suggestion is good. However, this rule needs practically 0 maintenance effort. Furthermore, there are users, who don't use the formatting ruleset and ktlint at all. I think we should keep it as it is for now. We can surely reconsider, if the maintenance effort rises. More duplicated rules exist AFAIK.

veyndan commented 3 years ago

However, this rule needs practically 0 maintenance effort.

That's not totally true, as a couple of months ago a false negative had to be fixed for this rule (https://github.com/detekt/detekt/pull/2835). It can be imagined that a future case arises again.

Furthermore, there are users, who don't use the formatting ruleset and ktlint at all.

Maybe I'm missing something, but I don't see the issue with deprecating MandatoryBracesIfStatements and telling the users to replace its usage with MultiLineIfElse.

More duplicated rules exist AFAIK.

From the talk by @Mauin, he says that all formatting rules had been stripped from detekt as ktlint does a really good job with that already. Even though MandatoryBracesIfStatements is under the style ruleset, it is still fundamentally a formatting rule.

I know this can be a tentative issue, but I'd love to have some further discussions before being closed.

schalkms commented 3 years ago

From the talk by @Mauin, he says that all formatting rules had been stripped from detekt as ktlint does a really good job with that already.

This information is a bit outdated. Previously, detekt had it’s own formatting rules that messed around with user code. The detekt team decided to wrap ktlint and remove rules that were manipulating user code. The mentioned rule belongs to the style ruleset. That’s a different story.

Please see it this way. The user has the choice, whether user code should be replaced or not.

Maybe I'm missing something, but I don't see the issue with deprecating MandatoryBracesIfStatements and telling the users to replace its usage with MultiLineIfElse.

The rules serve kind of a different use case as mentioned above. The choice is up to the user. Sure, there’s always a maintenance cost. There’s no free lunch. That’s the reason, why I said close to 0.

veyndan commented 3 years ago

Previously, detekt had it’s own formatting rules that messed around with user code. The detekt team decided to wrap ktlint and remove rules that were manipulating user code.

But the autoCorrect flag is still available in detekt which manipulates user code…

The mentioned rule belongs to the style ruleset.

As both of these rules do the exact same thing, I don't see how it matters what rulesets each of them belongs to.

The user has the choice, whether user code should be replaced or not.

I understand, but that's why there is the autoCorrect field. If they don't want the tool to automatically correct the code, they can just set this field to false in the yml.

The rules serve kind of a different use case as mentioned above. The choice is up to the user.

The way I see it is that MandatoryBracesIfStatements is equivalent to MultiLineIfElse with autoCorrect=false.

@arturbosch @BraisGabin @cortinico Maybe I'm missing something here, do you care to chime in?

schalkms commented 3 years ago

But the autoCorrect flag is still available in detekt which manipulates user code…

It’s only available in the formatting ruleset, which entirely wraps ktlint rules. autocorrect is just available for this ruleset.

As both of these rules do the exact same thing, I don't see how it matters what rulesets each of them belongs to.

The formatting ruleset consists entirely of ktlint rules. The style ruleset on the other hand consists of detekt native rules.

I understand, but that's why there is the autoCorrect field.

Because ktlint automatically formats user code according to the guidelines. Detekt doesn’t do that. That’s the difference. Ktlint is a linter, detekt a static analysis tool. Both tools serve a different purpose, but integrate nicely with each other.

schalkms commented 3 years ago

I hope that I could clear out all doubts and answer all questions @veyndan

veyndan commented 3 years ago

Hmm, I feel that you're just explaining the status quo to me (which I already understand). Let me rephrase the question:

Why would I (a detekt user) prefer MandatoryBracesIfStatements over MultiLineIfElse?

schalkms commented 3 years ago

Okay. Let me answer the following question.

Why would I (a detekt user) prefer MandatoryBracesIfStatements over MultiLineIfElse?

If a user doesn’t want to use the ktlint wrapper or has ktlint as a separate tool in use, MandatoryBracesIfStatements might be the better choice.

The same holds true for the MaxLineLength rule, for instance.

veyndan commented 3 years ago

Thanks for the answer! Just a couple of further questions on your answer.

If a user […] has ktlint as a separate tool in use, MandatoryBracesIfStatements might be the better choice.

Surely if they are using ktlint as a separate tool (and they have enabled MultiLineIfElse) why would they also enable MandatoryBracesIfStatements?

If a user doesn’t want to use the ktlint wrapper […], MandatoryBracesIfStatements might be the better choice.

Why would a user not want to use the ktlint wrapper and use detekt in isolation (i.e., without ktlint as a separate tool)?

BraisGabin commented 3 years ago

I think that, right now, we can't remove it.

My reasons:

Other duplicated rules detekt/ktlint:

@schalkms what do you think about reconsider this for 2.0?

Good thing: Now that I know that ktlin implemented this rule I'll reported them the case that I fixed in #2835 because, at least, they don't have any test to ensure that such case is ok.

veyndan commented 3 years ago

Thanks @BraisGabin! Those reasons make sense. I'd be happy with reopening this issue and adding it to the 2.0 milestone 😄

schalkms commented 3 years ago

@schalkms what do you think about reconsider this for 2.0?

If detekt is that far we can reconsider or reopen this ofc. At the moment, this is not the case due to all of the mentioned points. That's why I closed it.

schalkms commented 3 years ago

Surely if they are using ktlint as a separate tool (and they have enabled MultiLineIfElse) why would they also enable MandatoryBracesIfStatements?

A linter has a different purpose compared to a static analysis tool. A linter most likely runs only on the client, whereas a static analysis tool should run on the CI, if there's any. Detekt has a reporting functionality that ktlint has not.

Why would a user not want to use the ktlint wrapper and use detekt in isolation (i.e., without ktlint as a separate tool)?

Brais already answered this. Additionally, manipulating user code can always lead to unwanted results.

veyndan commented 3 years ago

A linter most likely runs only on the client, whereas a static analysis tool should additionally run on the CI, if there's any.

Okay, let's have an example. You run ktlint on the client with MultiLineIfElse enabled and everything passes. You then run the CI build where ktlint and detekt is enabled. Now the MultiLineIfElse rule will run from ktlint, but also MandatoryBracesIfStatements will run from detekt. Both these rules do the same thing, so running MandatoryBracesIfStatements is completely redundant.

Brais already answered this.

In this case, it is an experimental rule (even though in detekt we don't differentiate that in the documentation). What about a non-experimental rule like NewLineAtEndOfFile which is also duplicated?

Additionally, manipulating user code can always lead to unwanted results.

I've already responded to this above regarding enabling or disabling autoCorrect.

BraisGabin commented 3 years ago

I moved this idea to #2680 where we have all the ideas for 2.0

schalkms commented 3 years ago

Why did you enable ktlint as a linter on your CI? Activating both detekt rules on 1 client is redundant, yes. However, what's the issue with that? How does it hinder you when using detekt?

veyndan commented 3 years ago

Why did you enable ktlint as a linter on your CI?

You mentioned in the last comment that you enabled ktlint and detekt on the CI. Added bold for emphasis:

Activating both detekt rules on 1 client is redundant, yes.

This just furthers the question that I've been asking all this time: if activating both detekt rules is redundant, then why should we have both detekt rules (in the long run)?

How does it hinder you when using detekt?

I enabled MandatoryBracesIfStatements with auto correct in my project, hoping that it would fix all the problems. I then noticed that I was looking for MultiLineIfElse instead. It is very confusing having two rules that do the same thing, particularly when one has more advanced features than the other.

The second reason is maintainability of the project and sharing knowledge between ktlint and detekt. Some projects I work with only use ktlint, while others use detekt. If I (or anyone else) finds a bug in ktlint, I'd love to see it propagated to detekt instead of having to make another PR for the duplicate detekt rule.

schalkms commented 3 years ago

Thanks for the respons!

A linter most likely runs only on the client, whereas a static analysis tool should additionally run on the CI, if there's any.

I removed it in order to sort out further confusion. This is what I meant regarding the tool usage. Ktlint = client Detekt = client + server

Whilst both rules might do the same, the inner workings of both rules are different. The ktlint rule does only consider mandatory braces for if statements but not for loops.

https://github.com/pinterest/ktlint/blob/1fa75c63b0708bd7f4c873d610053a632450e453/ktlint-ruleset-experimental/src/main/kotlin/com/pinterest/ktlint/ruleset/experimental/MultiLineIfElseRule.kt#L19

As a side-note, I also understand that it might be confusing.

schalkms commented 3 years ago

To conclude this conversation, both mentioned rules don't work the same. MandatoryBracesIfStatements considers more cases than MultiLineIfElse. We should be careful here when removing rules, just because they seem to do the same.

veyndan commented 3 years ago

Ah must've missed that comment! @BraisGabin said in https://github.com/detekt/detekt/issues/2680#issuecomment-672816853 that he'll create an issue on ktlint to resolve this.

I think the other rules that @BraisGabin mentioned in https://github.com/detekt/detekt/issues/2680#issuecomment-672788918 still apply though, I just happened to choose an experimental rule that is doing stuff out of the scope of its name 😅 .

I agree that we should conclude the discussion though as it's getting pretty long 😂 As it's already linked in https://github.com/detekt/detekt/issues/2680, when the talks about detekt 2.0 come around, we can resume the conversation then if that's good for you.

BraisGabin commented 3 years ago

The issue: https://github.com/pinterest/ktlint/issues/828

And as @schalkms say it seems for https://github.com/pinterest/ktlint/issues/812 MandatoryBracesIfStatements and MultiLineIfElse are not going to be the same. I know that ktlint follows the kotlin coding convention really tight. And the convention doesn't talk about this case:

if (true)
  50
else 
  55

So I imagine that they will rollback that part of the rule 🤷‍♂️. They will probably maintain it for --android... I'm not a huge fan of that decission but I understand it's position.

arturbosch commented 3 years ago

It is really unfortunately for the user that there are similar/identical rules. I think we can improve the docs further to help them here. From a tool perspective it makes sense to have the original rule and the additional by the KtLint wrapper for the broadest audience:

Why you might prefer MandatoryBracesIfStatements over MultiLineIfElse? -> annotation (on file class or statement level) + baseline suppression possibility; -> that's why sonar-kotlin excludes KtLint duplicates not the detekt ones. -> for rule authors: generally detekt issues are easier to post-process due to an attached KtElement on the issue

There is often a overlap of rules between tools e.g. pmd, checkstyle and sonarqube have rules which detect classic code smells like LongMethod's and LongParameterLists. Sonarqube allows to include checkstyle and pmd as plugins. Should sonar remove it's core rules for the others? No, they may also address different users. Sonarqube moved away of just including pmd and checkstyle and support just their rulesets because they do not wanted to reparse all Java files three times -> saves time on CI/analysis.

What we can do is some kind of mapping. If a detekt rule detects something similar as a KtLint we only report once. We have the aliases feature for this.