Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't trigger semicolon_if_nothing_returned in expanded code #7789

Merged
merged 1 commit into from
Oct 8, 2021

Conversation

flip1995
Copy link
Member

@flip1995 flip1995 commented Oct 8, 2021

Fixes #7768

Before, this lint didn't trigger on macros. With rust-lang/rust#88175
this isn't enough anymore. In this PR a WhileLoop desugaring kind was
introduced. This overrides the span of expanded expressions when
lowering the while loop. So if a while loop is in a macro, the
expressions that it expands to are no longer marked with
ExpnKind::Macro, but with ExpnKind::Desugaring. In general, this is
the correct behavior and the same that is done for ForLoops. It just
tripped up this lint.

r? @camsteffen

changelog: [semicolon_if_nothing_returned]: Fix regression on macros containing while loops

Before this lint didn't trigger on macros. With rust-lang/rust#88175
this isn't enough anymore. In this PR a `WhileLoop` desugaring kind was
introduced. This overrides the span of expanded expressions when
lowering the while loop. So if a while loop is in a macro, the
expressions that it expands to are no longer marked with
`ExpnKind::Macro`, but with `ExpnKind::Desugaring`. In general, this is
the correct behavior and the same that is done for `ForLoop`s. It just
tripped up this lint.
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Oct 8, 2021
@camsteffen
Copy link
Contributor

@bors r+

I wonder if in_macro should just be replaced with from_expansion.

@bors
Copy link
Collaborator

bors commented Oct 8, 2021

📌 Commit b423b85 has been approved by camsteffen

@bors
Copy link
Collaborator

bors commented Oct 8, 2021

⌛ Testing commit b423b85 with merge 22144c0...

@bors
Copy link
Collaborator

bors commented Oct 8, 2021

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: camsteffen
Pushing 22144c0 to master...

@bors bors merged commit 22144c0 into rust-lang:master Oct 8, 2021
@flip1995 flip1995 deleted the double_semi_if_nothing_returned branch October 9, 2021 10:30
@flip1995
Copy link
Member Author

flip1995 commented Oct 9, 2021

I wonder if in_macro should just be replaced with from_expansion.

Doing this only makes 4 tests fail:

diff --git a/tests/ui/blocks_in_if_conditions.stderr b/tests/ui/blocks_in_if_conditions.stderr
index 079f2feb5..6be6bc494 100644
--- a/tests/ui/blocks_in_if_conditions.stderr
+++ b/tests/ui/blocks_in_if_conditions.stderr
@@ -22,13 +22,5 @@ error: omit braces around single expression condition
 LL |     if { true } { 6 } else { 10 }
    |        ^^^^^^^^ help: try: `true`
 
-error: this boolean expression can be simplified
-  --> $DIR/blocks_in_if_conditions.rs:40:8
-   |
-LL |     if true && x == 3 { 6 } else { 10 }
-   |        ^^^^^^^^^^^^^^ help: try: `x == 3`
-   |
-   = note: `-D clippy::nonminimal-bool` implied by `-D warnings`
-
-error: aborting due to 3 previous errors
+error: aborting due to 2 previous errors
 
diff --git a/tests/ui/from_iter_instead_of_collect.stderr b/tests/ui/from_iter_instead_of_collect.stderr
index 8f08ac8c3..5686e8c7a 100644
--- a/tests/ui/from_iter_instead_of_collect.stderr
+++ b/tests/ui/from_iter_instead_of_collect.stderr
@@ -78,17 +78,5 @@ error: usage of `FromIterator::from_iter`
 LL |         let _ = collections::BTreeSet::<u32>::from_iter(0..3);
    |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `.collect()` instead of `::from_iter()`: `(0..3).collect::<collections::BTreeSet<u32>>()`
 
-error: usage of `FromIterator::from_iter`
-  --> $DIR/from_iter_instead_of_collect.rs:60:15
-   |
-LL |     for _i in Vec::from_iter([1, 2, 3].iter()) {}
-   |               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `.collect()` instead of `::from_iter()`: `[1, 2, 3].iter().collect::<Vec<_>>()`
-
-error: usage of `FromIterator::from_iter`
-  --> $DIR/from_iter_instead_of_collect.rs:61:15
-   |
-LL |     for _i in Vec::<&i32>::from_iter([1, 2, 3].iter()) {}
-   |               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `.collect()` instead of `::from_iter()`: `[1, 2, 3].iter().collect::<Vec<&i32>>()`
-
-error: aborting due to 15 previous errors
+error: aborting due to 13 previous errors
 
diff --git a/tests/ui/nonminimal_bool_methods.stderr b/tests/ui/nonminimal_bool_methods.stderr
index a2df889d6..65d912982 100644
--- a/tests/ui/nonminimal_bool_methods.stderr
+++ b/tests/ui/nonminimal_bool_methods.stderr
@@ -54,29 +54,5 @@ error: this boolean expression can be simplified
 LL |     let _ = !c ^ c || !a.is_some();
    |                       ^^^^^^^^^^^^ help: try: `a.is_none()`
 
-error: this boolean expression can be simplified
-  --> $DIR/nonminimal_bool_methods.rs:92:8
-   |
-LL |     if !res.is_ok() {}
-   |        ^^^^^^^^^^^^ help: try: `res.is_err()`
-
-error: this boolean expression can be simplified
-  --> $DIR/nonminimal_bool_methods.rs:93:8
-   |
-LL |     if !res.is_err() {}
-   |        ^^^^^^^^^^^^^ help: try: `res.is_ok()`
-
-error: this boolean expression can be simplified
-  --> $DIR/nonminimal_bool_methods.rs:96:8
-   |
-LL |     if !res.is_some() {}
-   |        ^^^^^^^^^^^^^^ help: try: `res.is_none()`
-
-error: this boolean expression can be simplified
-  --> $DIR/nonminimal_bool_methods.rs:97:8
-   |
-LL |     if !res.is_none() {}
-   |        ^^^^^^^^^^^^^^ help: try: `res.is_some()`
-
-error: aborting due to 13 previous errors
+error: aborting due to 9 previous errors
 
diff --git a/tests/ui/use_self.stderr b/tests/ui/use_self.stderr
index e14368a11..73bdad0aa 100644
--- a/tests/ui/use_self.stderr
+++ b/tests/ui/use_self.stderr
@@ -120,12 +120,6 @@ error: unnecessary structure name repetition
 LL |             TestStruct::from_something()
    |             ^^^^^^^^^^ help: use the applicable keyword: `Self`
 
-error: unnecessary structure name repetition
-  --> $DIR/use_self.rs:258:25
-   |
-LL |         async fn g() -> S {
-   |                         ^ help: use the applicable keyword: `Self`
-
 error: unnecessary structure name repetition
   --> $DIR/use_self.rs:259:13
    |
@@ -168,5 +162,5 @@ error: unnecessary structure name repetition
 LL |             S2::new()
    |             ^^ help: use the applicable keyword: `Self`
 
-error: aborting due to 28 previous errors
+error: aborting due to 27 previous errors

Some of those might be bugs / inaccuracies in the lowering process of spans in rustc.

JohnTitor added a commit to JohnTitor/rust that referenced this pull request Oct 22, 2021
…vidtwco

Don't mark for loop iter expression as desugared

We typically don't mark spans of lowered things as desugared. This helps Clippy rightly discern when code is (not) from expansion. This was discovered by `@flip1995` at rust-lang/rust-clippy#7789 (comment).
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Oct 22, 2021
…vidtwco

Don't mark for loop iter expression as desugared

We typically don't mark spans of lowered things as desugared. This helps Clippy rightly discern when code is (not) from expansion. This was discovered by ``@flip1995`` at rust-lang/rust-clippy#7789 (comment).
flip1995 pushed a commit to flip1995/rust-clippy that referenced this pull request Nov 4, 2021
Don't mark for loop iter expression as desugared

We typically don't mark spans of lowered things as desugared. This helps Clippy rightly discern when code is (not) from expansion. This was discovered by ``@flip1995`` at rust-lang#7789 (comment).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regression in semicolon_if_nothing_returned involving macros
4 participants