Preserve brackets around if-lets and skip while-lets

[dingxiangfei2009]

r? @jieyouxu

Tracked by #124085

Fresh out of #129466, we have discovered 9 crates that the lint did not successfully migrate because the span of if let includes the surrounding brackets (..) like the following, which surprised me a bit.

if (if let .. { .. } else { .. }) {
// ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
// the span somehow includes the surrounding brackets
}

There is one crate that failed the migration because some suggestion spans cross the macro expansion boundaries. Surely there is no way to patch them with match rewrite. To handle this case, we will instead require all spans to be tested for admissibility as suggestion spans.

Besides, there are 4 false negative cases discovered with desugared-while let. We don't need to lint them, because the else branch surely contains exactly one statement because the drop order is not changed whatsoever in this case.

while let Some(value) = droppy().get() {
..
}
// is desugared into
loop {
    if let Some(value) = droppy().get() {
        ..
    } else {
        break;
        // here can be nothing observable in this block
    } 
}

I believe this is the one and only false positive that I have found. I think we have finally nailed all the corner cases this time.

[dingxiangfei2009]

On a side note, I think the migration suggestions on unsafe attributes are broken. If we run edition migration, the unsafe and the surrounding brackets are misplaced, leading to code rejection. To reproduce the issue, check out the crate hictor @ 0.1.6 with hash ccf408f4326a858c00dd845a64a86b16f360a801.

[dingxiangfei2009]

@rustbot labels +A-edition-2024

[jieyouxu]

There is one crate that failed the migration because some suggestion spans cross the macro expansion boundaries. Surely there is no way to patch them with match rewrite. To handle this case, we will instead require all spans to be tested for admissibility as suggestion spans.

Yes, I believe in general we want to check all spans (incl. their subspans) for Span::from_expansion to be false (modulo cases where proc-macros produce spans that have no distinguishing syntax context) before making a suggestion.

[jieyouxu]

Thanks, this LGTM in general, I have some cases that we may want to add a test or two for (trying really hard to come up with something that might break this logic).

[jieyouxu]

@rustbot author

[dingxiangfei2009]

@rustbot ready

• applied suggestions with additional UI test

[dingxiangfei2009]

There is one crate that failed the migration because some suggestion spans cross the macro expansion boundaries. Surely there is no way to patch them with match rewrite. To handle this case, we will instead require all spans to be tested for admissibility as suggestion spans.

Yes, I believe in general we want to check all spans (incl. their subspans) for Span::from_expansion to be false (modulo cases where proc-macros produce spans that have no distinguishing syntax context) before making a suggestion.

Now we are checking all the span. We bail out of emitting a suggestion because I think the fix will not be successful if any of them is not a part of a continuous user source span.

[jieyouxu]

Thanks, you can r=me after PR CI is green.

[jieyouxu]

@bors delegate+

[bors]

✌️ @dingxiangfei2009, you can now approve this pull request!

If @jieyouxu told you to "r=me" after making some further change, please make that change, then do @bors r=@jieyouxu

[dingxiangfei2009]

@bors r=@jieyouxu

Onto the merge train we go.

[bors]

📌 Commit ed5443f has been approved by jieyouxu

It is now in the queue for this repository.