Fix semicolon_outside_block suggests wrongly when inside macros

[profetia]

Closes #14926

changelog: [semicolon_outside_block] fix wrong suggestions when inside macros

[rustbot]

r? @samueltardieu

rustbot has assigned @samueltardieu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[samueltardieu]

I have a hard-time convincing myself that all this is necessary:

• !block.span.from_expansion() is already checked in SemicolonBlock::check_stmt()
• It looks like adding && stmt.span.contains(block.span) into the guard in SemicolonBlock::check_stmt() passes all the tests you add

Can you think of other test cases that would pass with your changes and fail with the proposed guard?

[profetia]

You are right about the first point. But for the second one, I think in SemicolonBlock::check_stmt() is not doing the same thing. What I'm doing here is checking the surrounding of the block to determine if it is safe to move the ; out, while that method is checking inside the block.

mac!(
    { <- block
        another_mac!(); // <- stmt
    }
);

[samueltardieu]

I think in SemicolonBlock::check_stmt() is not doing the same thing.

I'm not sure I understand: your code is called by check_stmt().

What I'm doing here is checking the surrounding of the block to determine if it is safe to move the ; out, while that method is checking inside the block.

This is why I was asking of a test case where my proposed addition to the guard in check_stmt() is not enough, in order to convince me that this does not cover every case we are interested in not linting.

[samueltardieu]

This is why I was asking of a test case where my proposed addition to the guard in check_stmt() is not enough, in order to convince me that this does not cover every case we are interested in not linting.

To make sure I am perfectly clear, what I am saying is that if I take the original code from master and changes it to:

    fn check_stmt(&mut self, cx: &LateContext<'_>, stmt: &Stmt<'_>) {
        match stmt.kind {
            StmtKind::Expr(Expr {
                kind: ExprKind::Block(block, _),
                ..
            }) if !block.span.from_expansion() && stmt.span.contains(block.span) => {

(note the added && stmt.span.contains(block.span))

then all the uitests pass, including the ones you have added in the current PR. So I am wondering where the behavior difference might be.

[profetia]

Oh I see, I've added the guard to the wrong place. In this case, I think your idea is right. I'm updating this PR to this better version. Thank you!