Fix collapsible_else_if FP on conditionally compiled stmt

[profetia]

Closes #14799

changelog: [collapsible_else_if] fix FP on conditionally compiled stmt

[rustbot]

r? @llogiq

rustbot has assigned @llogiq.
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]

r? samueltardieu

[samueltardieu]

After more thinking about it, I think we can do much better, fix more problems, and create a useful utility function that can be used by other lints in similar situations.

I noticed that we parse the beginning of the block two times: once to look for comments, and once to look for auto-removed code. This is not efficient, and we could have a function in clippy_utils that checks for both at the same time, or even one that allows comments but detects significant content:

/// Checks whether a given span has any significant token. A significant token is a non-whitespace
/// token, including comments unless `skip_comments` is set.
/// This is useful to determine if there are any actual code tokens in the span that are omitted in
/// the late pass, such as platform-specific code.
pub fn span_contains_non_whitespace(cx: &impl source::HasSession, span: Span, skip_comments: bool) -> bool {
    matches!(span.get_source_text(cx), Some(snippet) if tokenize_with_text(&snippet).any(|(token, _, _)|
        match token {
            TokenKind::Whitespace => false,
            TokenKind::BlockComment { .. } | TokenKind::LineComment { .. } => !skip_comments,
            _ => true,
        }
    ))
}

(that would supersede span_contains_no_comment() which can be removed)

This will be particularly useful in collapsible_if.rs as:

• collapsible_if_if suffers the same problem about conditionally compiled statements
• collapsible_if_if has an option lint_commented_code which triggers the lint even in the presence of commented code
• collapsible_else_if would benefit from using the same option

If you create this method on CollapsibleIf:

    // Check that nothing significant can be found but whitespaces between the initial `{` of `block`
    // and the beginning of `stop_at`.
    fn block_starts_with_significant_tokens(
        &self,
        cx: &LateContext<'_>,
        block: &Block<'_>,
        stop_at: &Expr<'_>,
    ) -> bool {
        let span = block.span.split_at(1).1.until(stop_at.span);
        span_contains_non_whitespace(cx, span, self.lint_commented_code)
    }

then you can add a &self parameter to check_collapsible_else_if(), and call && !self.block_starts_with_significant_tokens(cx, else_block, else_). check_collapsible_if_if() can get a similar && !self.block_starts_with_significant_tokens(cx, then, inner). This way, the block_starts_with_comment function can be removed as it is no longer useful.

Also, you would need to:

• register lint_commented_code as being an option of collapsible_else_if as well
• create test code for collapsible_else_if in ui-toml with this option enabled – you might be able to share code with collapsible_if_if, which already tests for this
• add test code for collapsible_if_if to ensure that #[cfg(…)] blocks prevent the lint from triggering as expected

All this started from your initial PR, which made me think more about it.

What do you think?