Add semicolon-outside/inside-block lints

[Veykril]

changelog: Add semicolon_outside_block and semicolon_inside_block lints

Fixes #7322

An earlier attempt at this can be found here #7564. This PR still implements two separate lints but I am open to merging them into a single one that's configurable.

[rust-highfive]

r? @Alexendoo

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

[Alexendoo]

👋

[bors]

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

[Veykril]

Needed to rebase, changes are in 16b016e

[Alexendoo]

Thanks! I think this is the last thing:

I believe this lint would apply to expressions returning any type rather than just ()

[Veykril]

Ah, I forgot to change that good catch. Unsure what wording to take here though, let me know if this works.

[Alexendoo]

It's tricky, maybe something like "Suggests moving the semicolon from a block's final expression outside of the block" and the opposite, to me the current reading suggests it wouldn't work on things like { a; b; }

[Alexendoo]

You can rebase on master also to fix the remark CI issue

[Alexendoo]

Suggested change

	/// Suggests moving the semicolon from a block inside of the block to its kast expression.
	/// Suggests moving the semicolon after a block to the inside of the block, after its last expression.

[Veykril]

Sorry, I was a bit hasty on that commit I think, fixed it

[oxalica]

If I read correctly, this lint doesn't trigger if both semicolons are used, yes?
unsafe { foo(); }; from #3305

[Veykril]

It does not trigger in that case no (I added it as to the tests though, thanks for bringing that up)

[Alexendoo]

Great, thanks @Veykril

@bors r+

[bors]

📌 Commit f62eab4 has been approved by Alexendoo

It is now in the queue for this repository.

[flip1995]

@bors r=Alexendoo

[bors]

📌 Commit 20ec2ce has been approved by Alexendoo

It is now in the queue for this repository.

[bors]

⌛ Testing commit 20ec2ce with merge d4cd91c...

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: Alexendoo
Pushing d4cd91c to master...

[oxalica]

@Veykril

It does not trigger in that case no (I added it as to the tests though, thanks for bringing that up)

I think the double semicolon case perfectly fits the lint name semicolon_outside_block, and it's surprising that it fires only when there's no semi inside.

Also I'd expect the following cases to be warn-by-default.

unsafe { foo(); }; // Double semicolon.
match foo() { _ => foo() }; // The arm returns `()`.
match foo() { _ => foo(), _ => { foo() } }; // All arms return `()`.
for _ in [()] { unit_fn_block()/*semi or no semi*/ }; // Always `()`.
if foo() { unit_fn_block()/*semi or no semi*/ }; // Always `()`.
loop { }; // `!`

If we want to warn these cases, while still make it configurable for unsafe { f() }; v.s. unsafe { f(); }. Should we create a separated lint for the these warning cases? I cannot come up with another name besides semicolon_outside_block.

[oxalica]

🤔 If the current semicolon-outside/inside-block behaviors rely purely on the syntax, and unsafe { foo() }; is semantically always the same as unsafe { foo(); }, could we move these into rustfmt instead?

[Jarcho]

Changing { foo() }; to { foo(); } is only sementically equivalent when there aren't any bindings inside the block. rustfmt also has the restriction of working with an untyped ast, so it's very limited on what transformations it could do with semicolon placement.

[oxalica]

Changing { foo() }; to { foo(); } is only sementically equivalent when there aren't any bindings inside the block.

Oh, I forget the drop order stuff. ;-outside-the-block will delay the drop of the last Expr. So yeah it's more complicated than rustfmt typically does.