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

2024 edition migration seems overly eager to rewrite every if let into match #133554

Closed
djc opened this issue Nov 27, 2024 · 3 comments
Closed

2024 edition migration seems overly eager to rewrite every if let into match #133554

djc opened this issue Nov 27, 2024 · 3 comments
Labels
A-edition-2024 Area: The 2024 edition A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` C-bug Category: This is a bug. L-if_let_rescope Lint: if_let_rescope T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@djc
Copy link
Contributor

djc commented Nov 27, 2024

I tried running cargo +nightly fix --edition against Quinn. It seems to rewrite a lot of instances of if let to match, without seemingly much explanation or discrimination. This makes the code harder to read/less concise in many cases. Is this really necessary for all these cases? Could it somehow be made to be more precise/discriminate?

@@ -1254,8 +1284,11 @@ impl fmt::Debug for State {
 }
 
 fn wake_stream(stream_id: StreamId, wakers: &mut FxHashMap<StreamId, Waker>) {
-    if let Some(waker) = wakers.remove(&stream_id) {
-        waker.wake();
+    match wakers.remove(&stream_id) {
+        Some(waker) => {
+            waker.wake();
+        }
+        _ => {}
     }
 }
 
@@ -95,8 +95,11 @@ impl Incoming {
 impl Drop for Incoming {
     fn drop(&mut self) {
         // Implicit reject, similar to Connection's implicit close
-        if let Some(state) = self.0.take() {
-            state.endpoint.refuse(state.inner);
+        match self.0.take() {
+            Some(state) => {
+                state.endpoint.refuse(state.inner);
+            }
+            _ => {}
         }
     }
 }
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Nov 27, 2024
@jieyouxu jieyouxu added A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. T-lang Relevant to the language team, which will review and decide on the PR/issue. A-edition-2024 Area: The 2024 edition L-if_let_rescope Lint: if_let_rescope T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` C-bug Category: This is a bug. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Nov 27, 2024
@jieyouxu
Copy link
Member

cc @dingxiangfei2009 @traviscross (FYI)

@zh-jq-b
Copy link

zh-jq-b commented Nov 28, 2024

As you can see in #133529, cargo fix --edition --workspace changed 282 files (1438 insertions, 1451 deletions), most of them are unneeded if let -> match, while manual fix only need 19 files changed, 48 insertions(+), 37 deletions(-).

@traviscross
Copy link
Contributor

Thanks for the report. The essence of this probably duplicates:

So let's track this there.

@traviscross traviscross closed this as not planned Won't fix, can't repro, duplicate, stale Nov 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-edition-2024 Area: The 2024 edition A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` C-bug Category: This is a bug. L-if_let_rescope Lint: if_let_rescope T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants