Add semicolon inside/outside block lints

[1c3t3a]

This PR adds two new lints to clippy according to #7322. The two lints offer two flavors of the same thing: they check if for () returning blocks the semicolon is inside or outside the block and correct it if possible. At the moment the semicolon_outside_block lint fires in the dogfood test.

• Followed [lint naming conventions][lint_naming]
• Added passing UI tests (including committed .stderr file)
• cargo test passes locally
• Executed cargo dev update_lints
• Added lint documentation
• Run cargo dev fmt

changelog:
Added two new lints according to #7322.
[`semicolon_inside_block`]: Checks for a block with a () returning expression if the semicolon is inside the block.
[`semicolon_outside_block`]: Checks for a block with a () returning expression if the semicolon is outside the block.

[rust-highfive]

r? @flip1995

(rust-highfive has picked a reviewer for you, use r? to override)

[1c3t3a]

I don't really understand why the semicolon_outside_block test is failing. On my machine the test runs through and the suite errors on the dogfood tests... The same happened with the tests at my fork.

[xFrednet]

I believe that the pipeline rebases your change on the current master that might be the reason for the failing test. Have you tried rebasing your changes? 🙃

[1c3t3a]

I refactored the semicolon_outside_block lint to only use check_fn instead of the parent_iter and HirId comparison hocus pocus. It still fires inside the clippy codebase in situations where I personnally would not apply the lint. Example:

error: consider moving the `;` outside the block for consistent formatting
1072
   --> src/driver.rs:99:69
1073
    |
1074
99  |           config.parse_sess_created = Some(Box::new(move |parse_sess| {
1075
    |  _____________________________________________________________________^
1076
100 | |             track_clippy_args(parse_sess, &clippy_args_var);
1077
101 | |         }));
1078
    | |____________^
1079
    |
1080
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#semicolon_outside_block
1081
help: put the `;` outside the block
1082
    |
1083
99  ~         config.parse_sess_created = Some(Box::new(move |parse_sess| {
1084
100 +             track_clippy_args(parse_sess, &clippy_args_var)
1085
101 +         }));
1086
    |

I think it might be a good idea to move the lint into the restriction category or #[allow()]ing it.

[flip1995]

I think it might be a good idea to move the lint into the restriction category or #[allow()]ing it.

If it's 2 lints that contradict eachother, then they have to go into restriction, since projects (like Clippy), that enable the whole pedantic group will get contradicting or over-eager suggestions.

Another possibility would be to merge those 2 lints into one lint with a configuration option. But in any case, I see this as a restriction lint.

I'm not sure what the better approach is here. I'm fine with continuing with 2 separate lints for now and make the final decision right before the PR is merged.

---

Another thing I noticed when skimming this PR is that those 2 lints are in separate files / lint passes. I would expect that almost all of the code for those 2 lints could be shared, except for the final decision what lint should be triggered. So what's the motivation in splitting this up?

[flip1995]

Why do we have to check a whole function here? Couldn't you just use check_block and then get the parent stmt and check if it is a StmtKind::Semi?

[1c3t3a]

Sure, but that way I would also suggest a semicolon after the block of an fn, or a match arm. I tried finding out if the block belongs to such things via parent_iter, but that didn't really work the way expected. I thought check_fn makes a cleaner approach, as it only iterates over blocks that we're interested in.

[flip1995]

The problem with this approach is, that you will only be able to allow this lint at the function scope and not directly on the block.

[1c3t3a]

Another possibility would be to merge those 2 lints into one lint with a configuration option. But in any case, I see this as a restriction lint.

Alright, I think the configuration sounds good. Also restriction sounds fair then.

Another thing I noticed when skimming this PR is that those 2 lints are in separate files / lint passes. I would expect that almost all of the code for those 2 lints could be shared, except for the final decision what lint should be triggered. So what's the motivation in splitting this up?

Actually I didn't split them up because of a real reason, adding a configuration would also merge them as well. I initially wrote the semicolon_inside_block lint to just suggest a change for the span of the expression rather the whole block that's been detected. In the semicolon_outside_block that doesn't help as the suggestion always targets the end of the block as well as the last statement. That's currently the reason why the contents of the then part of the if_chain! differ.

[flip1995]

Actually I didn't split them up because of a real reason, adding a configuration would also merge them as well

I would like to see those two lints implemented in the same file and LintPass then.

[bors]

☔ The latest upstream changes (presumably #7604) made this pull request unmergeable. Please resolve the merge conflicts.

[giraffate]

ping from triage @1c3t3a. Can you have any updates on this? If you have any questions on this, feel free to ask us.

[1c3t3a]

@giraffate Hei there, I am still planning to implement this, but I probably won't have time to do so until October. Is that fine with you? Otherwise I am ok with someone taking over.

[giraffate]

@1c3t3a

Hei there, I am still planning to implement this, but I probably won't have time to do so until October. Is that fine with you?

Okay!

[giraffate]

ping from triage @1c3t3a. Can you have time to work on this?

[flip1995]

Also if you have any questions blocking you from continue working on this, don't hesitate to ask!

[giraffate]

@1c3t3a ping from triage. According the triage procedure, I'm closing this because 2 weeks have passed with no activity. If you have more time to work on this, feel free to reopen this.

[Veykril]

@1c3t3a Would you mind if I try to pick this PR up again bringing it over the finish line?

[1c3t3a]

Sure go ahead! I don't plan to work on this in the next time :)

[jonboh]

@rustbot label -S-inactive-closed