-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Match scrutinee need necessary parentheses for structs #113679
Conversation
r? @TaKO8Ki (rustbot has picked a reviewer for you, use r? to override) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fix looks insufficient for chained operators, for example this still triggers the lint:
struct Foo {}
fn main() {
match (&&Foo {}) {
_ => {}
}
}
I think the correct fix is to add a case for AddrOf
to contains_exterior_struct_lit
here:
rust/compiler/rustc_ast/src/util/parser.rs
Lines 388 to 396 in fe03b46
ast::ExprKind::Await(x, _) | |
| ast::ExprKind::Unary(_, x) | |
| ast::ExprKind::Cast(x, _) | |
| ast::ExprKind::Type(x, _) | |
| ast::ExprKind::Field(x, _) | |
| ast::ExprKind::Index(x, _) => { | |
// &X { y: 1 }, X { y: 1 }.y | |
contains_exterior_struct_lit(x) | |
} |
1e96933
to
a215ef4
Compare
a215ef4
to
fa9d5e7
Compare
@bors r=cjgillot |
… r=cjgillot Match scrutinee need necessary parentheses for structs Fixes rust-lang#113459
⌛ Testing commit fa9d5e7 with merge c2d4b0fd01ca8b5b1acd7e99ede9ba14d7578afe... |
This comment has been minimized.
This comment has been minimized.
💔 Test failed - checks-actions |
@matthiaskrgr |
my first thought was that perhaps since the 2 weeks the PR was opened, some other changes went in or additional tests were added which the first version of the pr did not account for, but I didn't look at it in very detail. |
fa9d5e7
to
eb10728
Compare
I rebased to master yesterday, let me rebase it again to have a try. |
The error is a SIGSEGV on one of the apple builders. Let's try again, see if it's a legitimate failure. |
📌 Commit eb10728643010ce1a95ecea2a7ac5291dc6958ed has been approved by It is now in the queue for this repository. |
⌛ Testing commit eb10728643010ce1a95ecea2a7ac5291dc6958ed with merge f933bc7a2f16deeccd550634d5744ce0a76c16aa... |
☀️ Test successful - checks-actions |
👀 Test was successful, but fast-forwarding failed: 422 Update is not a fast forward |
Finished benchmarking commit (f933bc7a2f16deeccd550634d5744ce0a76c16aa): comparison URL. Overall result: ✅ improvements - no action needed@rustbot label: -perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 650.779s -> 652.03s (0.19%) |
So, it isn't merged? |
yes, seems failed in merging. |
@matthiaskrgr Test was successful, but fast-forwarding failed: 422 Update is not a fast forward |
eb10728
to
c44b35e
Compare
@bors retry |
uhh. |
According to homu, this isn't actually approved. @bors r=cjgillot rollup=never |
☀️ Test successful - checks-actions |
Finished benchmarking commit (d7e7510): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 632.706s -> 631.44s (-0.20%) |
Fixes #113459