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

Fix checking if newline is needed before else in let-else statement #5902

Merged
merged 5 commits into from
Sep 9, 2023

Conversation

rhysd
Copy link
Contributor

@rhysd rhysd commented Sep 6, 2023

Fixes #5901

@ytmimi
Copy link
Contributor

ytmimi commented Sep 6, 2023

@rhysd I haven't had a chance to review this yet, but I wanted to see how your changes handle this case:

fn issue5901() {
    #[cfg(target_os = "linux")]
    // some comment between attr and let-else
    let Some(x) = foo() else { return; };
}

fn issue5901() {
    #[cfg(target_os = "linux")]
    /* some comment between attr and let-else */
    let Some(x) = foo() else { return; };
}

@rhysd
Copy link
Contributor Author

rhysd commented Sep 6, 2023

Thank you for the point. I'll try to add new test.

@rhysd
Copy link
Contributor Author

rhysd commented Sep 6, 2023

I confirmed that this fix didn't work for the comment between the attributes and let-else statement. I'll have a look whether it can be fixed.

 fn main() {
     #[cfg(target_os = "linux")]
     // this is test
-    let x = 42 else { todo!() };
+    let x = 42
+    else {
+        todo!()
+    };
 }

@ytmimi
Copy link
Contributor

ytmimi commented Sep 6, 2023

@rhysd thanks for taking a look 🙏🏼. Before you keep working on this I also want to point out that any change needs to be version gated since let-else support was released as stable and we can't break our formatting guarantees.

@rhysd
Copy link
Contributor Author

rhysd commented Sep 6, 2023

@ytmimi Thanks for the information. I'll check the documentation. And I may find a way to strip things before let correctly. I'll update this branch once I add more tests and the version gate.

@rhysd
Copy link
Contributor Author

rhysd commented Sep 6, 2023

@ytmimi I force-pushed the fix. I added more tests and v1 version gate as well. Is there any place to add tests for v1 formatting? Currently only v2 formatting is tested. Please let me know if I should create some test case like tests/source/let_else_v1.rs.

@ytmimi
Copy link
Contributor

ytmimi commented Sep 6, 2023

You should create a let_else_v2.rs test for this. See the Create test cases section of the Contributing guide for details on how to add configuration values to tests via comments.

src/items.rs Outdated Show resolved Hide resolved
src/items.rs Show resolved Hide resolved
tests/source/let_else.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@ytmimi ytmimi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again for taking the time to look into this. I think you're on the right track with getting the let_kw_offset early on. As mentioned, there are two other calls that use the result below for their calculations and those need to be changed as well.

@rhysd
Copy link
Contributor Author

rhysd commented Sep 7, 2023

@ytmimi Thank you for your review comments. I updated this branch to resolve them.

  • Preferred checking Version::Two to Version::One
  • Added test cases for both v1 and v2 formatting
  • Fixed calculating available space stripping things before let

src/items.rs Outdated Show resolved Hide resolved
tests/source/let_else.rs Outdated Show resolved Hide resolved
@rhysd
Copy link
Contributor Author

rhysd commented Sep 8, 2023

@ytmimi Thank you for your review comments again. I addressed them.

  • Simplified checking if the else block can be single-lined
  • Preferred todo!() expression to return; statement
  • Imported more v1 test cases into let_else_v2.rs

Copy link
Contributor

@ytmimi ytmimi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for helping to fix this issue and for your first contribution to rustfmt 🥳

@ytmimi
Copy link
Contributor

ytmimi commented Sep 8, 2023

As a last step I want to run our diff-check job, It should pass since the changes are version gated, but I still want to go through this process.

@ytmimi ytmimi merged commit 262feb3 into rust-lang:master Sep 9, 2023
27 checks passed
@ytmimi ytmimi added the release-notes Needs an associated changelog entry label Sep 9, 2023
@ytmimi ytmimi removed the release-notes Needs an associated changelog entry label Oct 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rustfmt always inserts newline before else in let-else statement with attribute
3 participants