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

Only treat plain literal patterns as short #135251

Merged
merged 1 commit into from
Jan 16, 2025

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Jan 8, 2025

See #134228 (comment) and #134228 (comment) for context. We never wanted to treat const blocks and paths as short, only plain literals.

I don't know how to write a test for this, it.s not clear to me how the short pattern check actually affects the formatting

@rustbot

This comment was marked as resolved.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 8, 2025
@rustbot

This comment was marked as resolved.

@oli-obk
Copy link
Contributor Author

oli-obk commented Jan 8, 2025

r? @ytmimi

@rustbot rustbot assigned ytmimi and unassigned Mark-Simulacrum Jan 8, 2025
Comment on lines 50 to 51
ast::ExprKind::ConstBlock(_) => false,
ast::ExprKind::Path(..) => false,
Copy link
Contributor

@ytmimi ytmimi Jan 8, 2025

Choose a reason for hiding this comment

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

As you mentioned in #134228 (comment), ConstBlock and Path patterns snuck into rustfmt unexpectedly. Let me know if I'm misunderstanding anything, but assuming that's the case then we'll need to gate these changes on the style edition:

Accessing the style_edition requires us to add some additional context to is_short_pattern and is_short_pattern_inner. We can either pass the entire &RewriteContext<'_>, or just the &Config (context.config). The &RewriteContext<'_> is available at the call site so this shouldn't be too difficult.

 ///     - `[small, ntp]`
 ///     - unary tuple constructor `([small, ntp])`
 ///     - `&[small]`
-pub(crate) fn is_short_pattern(pat: &ast::Pat, pat_str: &str) -> bool {
+pub(crate) fn is_short_pattern(context: &RewriteContext<'_>, pat: &ast::Pat, pat_str: &str) -> bool {
     // We also require that the pattern is reasonably 'small' with its literal width.
-    pat_str.len() <= 20 && !pat_str.contains('\n') && is_short_pattern_inner(pat)
+    pat_str.len() <= 20 && !pat_str.contains('\n') && is_short_pattern_inner(context, pat)
 }
 
-fn is_short_pattern_inner(pat: &ast::Pat) -> bool {
+fn is_short_pattern_inner(context: &RewriteContext<'_>, pat: &ast::Pat) -> bool {

Then we can gate the change:

Suggested change
ast::ExprKind::ConstBlock(_) => false,
ast::ExprKind::Path(..) => false,
ast::ExprKind::ConstBlock(_) | ast::ExprKind::Path(..) => {
// Let's leave a comment explaining why this needs to be gated
context.config.style_edition() <= StyleEdition::Edition2024
}

@ytmimi
Copy link
Contributor

ytmimi commented Jan 8, 2025

I don't know how to write a test for this, it.s not clear to me how the short pattern check actually affects the formatting

Yeah, I think this will be tougher to test. Especially because I think we need to gate this change on the style edition as to not change any behavior with stable formatting. Assuming we don't change any behavior for style editions <= 2024, then I think we should be good.

@ytmimi
Copy link
Contributor

ytmimi commented Jan 9, 2025

Thanks for gating the code. As one last sanity check I copied these changes over to https://github.com/ytmimi/rustfmt/tree/rl_rust_135251 and I'm running rustfmt's Diff-Check Job to make sure these changes don't change behavior when running on some larger rust code bases. Will follow up once that's done.

@ytmimi
Copy link
Contributor

ytmimi commented Jan 9, 2025

@oli-obk Diff-Check passed. You can r=me when you see this. I don't have the permissions to r+ this myself, though I should probably change that soon so I can approve rustfmt PRs in-tree when needed.

@oli-obk
Copy link
Contributor Author

oli-obk commented Jan 9, 2025

@bors r=ytmimi

@bors
Copy link
Contributor

bors commented Jan 9, 2025

📌 Commit be92ac3 has been approved by ytmimi

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Jan 9, 2025

🌲 The tree is currently closed for pull requests below priority 1000. This pull request will be tested once the tree is reopened.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 9, 2025
jhpratt added a commit to jhpratt/rust that referenced this pull request Jan 10, 2025
Only treat plain literal patterns as short

See rust-lang#134228 (comment) and rust-lang#134228 (comment) for context. We never wanted to treat const blocks and paths as short, only plain literals.

I don't know how to write a test for this, it.s not clear to me how the short pattern check actually affects the formatting
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 10, 2025
Rollup of 7 pull requests

Successful merges:

 - rust-lang#132607 (Used pthread name functions returning result for FreeBSD and DragonFly)
 - rust-lang#134693 (proc_macro: Use `ToTokens` trait in `quote` macro)
 - rust-lang#134732 (Unify conditional-const error reporting with non-const error reporting)
 - rust-lang#135083 (Do not ICE when encountering predicates from other items in method error reporting)
 - rust-lang#135251 (Only treat plain literal patterns as short)
 - rust-lang#135320 (Fix typo in `#[coroutine]` gating error)
 - rust-lang#135321 (remove more redundant into() conversions)

r? `@ghost`
`@rustbot` modify labels: rollup
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jan 10, 2025
Rollup merge of rust-lang#135251 - oli-obk:push-lmpyvvyrtplk, r=ytmimi

Only treat plain literal patterns as short

See rust-lang#134228 (comment) and rust-lang#134228 (comment) for context. We never wanted to treat const blocks and paths as short, only plain literals.

I don't know how to write a test for this, it.s not clear to me how the short pattern check actually affects the formatting
@oli-obk
Copy link
Contributor Author

oli-obk commented Jan 16, 2025

@bors r=ytmimi

@bors
Copy link
Contributor

bors commented Jan 16, 2025

📌 Commit c994637 has been approved by ytmimi

It is now in the queue for this repository.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 16, 2025
Only treat plain literal patterns as short

See rust-lang#134228 (comment) and rust-lang#134228 (comment) for context. We never wanted to treat const blocks and paths as short, only plain literals.

I don't know how to write a test for this, it.s not clear to me how the short pattern check actually affects the formatting
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 16, 2025
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#133720 ([cfg_match] Adjust syntax)
 - rust-lang#134496 (Update documentation for Arc::from_raw, Arc::increment_strong_count, and Arc::decrement_strong_count to clarify allocator requirement)
 - rust-lang#134754 (Implement `use` associated items of traits)
 - rust-lang#135249 (Fix overflows in the implementation of `overflowing_literals` lint's help)
 - rust-lang#135251 (Only treat plain literal patterns as short)
 - rust-lang#135556 (Clarify note in `std::sync::LazyLock` example)
 - rust-lang#135560 (Update `compiler-builtins` to 0.1.144)

r? `@ghost`
`@rustbot` modify labels: rollup
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 16, 2025
Only treat plain literal patterns as short

See rust-lang#134228 (comment) and rust-lang#134228 (comment) for context. We never wanted to treat const blocks and paths as short, only plain literals.

I don't know how to write a test for this, it.s not clear to me how the short pattern check actually affects the formatting
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 16, 2025
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#133720 ([cfg_match] Adjust syntax)
 - rust-lang#134496 (Update documentation for Arc::from_raw, Arc::increment_strong_count, and Arc::decrement_strong_count to clarify allocator requirement)
 - rust-lang#135249 (Fix overflows in the implementation of `overflowing_literals` lint's help)
 - rust-lang#135251 (Only treat plain literal patterns as short)
 - rust-lang#135556 (Clarify note in `std::sync::LazyLock` example)
 - rust-lang#135560 (Update `compiler-builtins` to 0.1.144)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 16, 2025
…iaskrgr

Rollup of 5 pull requests

Successful merges:

 - rust-lang#133720 ([cfg_match] Adjust syntax)
 - rust-lang#134496 (Update documentation for Arc::from_raw, Arc::increment_strong_count, and Arc::decrement_strong_count to clarify allocator requirement)
 - rust-lang#135249 (Fix overflows in the implementation of `overflowing_literals` lint's help)
 - rust-lang#135251 (Only treat plain literal patterns as short)
 - rust-lang#135556 (Clarify note in `std::sync::LazyLock` example)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 10d22fc into rust-lang:master Jan 16, 2025
6 checks passed
@rustbot rustbot added this to the 1.86.0 milestone Jan 16, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jan 16, 2025
Rollup merge of rust-lang#135251 - oli-obk:push-lmpyvvyrtplk, r=ytmimi

Only treat plain literal patterns as short

See rust-lang#134228 (comment) and rust-lang#134228 (comment) for context. We never wanted to treat const blocks and paths as short, only plain literals.

I don't know how to write a test for this, it.s not clear to me how the short pattern check actually affects the formatting
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants