-
-
Notifications
You must be signed in to change notification settings - Fork 14.5k
Format heterogeneous try blocks #152293
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
Format heterogeneous try blocks #152293
Conversation
|
Some changes occurred in src/tools/rustfmt cc @rust-lang/rustfmt |
|
rustbot has assigned @Mark-Simulacrum. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
r? @ytmimi |
| // FIXME: heterogeneous try blocks, which include a type so are harder to format | ||
| ast::ExprKind::TryBlock(_, Some(_)) => Err(RewriteError::Unknown), | ||
| ast::ExprKind::TryBlock(ref block, Some(ref ty)) => { | ||
| let keyword = "try bikeshed "; |
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.
Is there going to be an issue with try /* comment */ bikeshed?
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.
Not more than with try_blocks apparently. It simply won't format. See the test case in catch.rs:
let x = try /* Invisible comment */ { foo()? };If you do:
let x = try /* Invisible comment */ { foo()?
};It won't format. I've added tests similar to the one of catch.rs.
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.
Let's add #![feature(try_blocks_heterogeneous)] to these tests so that it's easier to tell what experimental feature they're for.
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.
I've also fixed try_block.rs.
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.
Thanks I appreciate that 🙏🏼
| .and_then(|shape| shape.sub_width(2)) | ||
| .max_width_error(shape.width, expr.span)?; | ||
| let ty_str = ty.rewrite_result(context, ty_shape)?; | ||
| let prefix = format!("{keyword}{ty_str} "); |
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.
Similarly, is there going to be an issue with try bikeshed /* comment */ {ty_str}?
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.
Do we need to handle cases where the type can't fit on the current line and needs to wraps to the next line? Something like:
try bikeshed
{ty_str} {
}
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.
Good question. I gave my opinion in the PR description. I don't think this is needed given "bikeshed" is temporary so whatever we choose might not be transposable to the final syntax. But I'm happy to do it if we believe it's useful.
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 is still experimental so I think it's fine for now. T-style may want to write up some rules for this before stabilization though.
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.
Sorry, I thought I have linked it but apparently didn't. I also wrote a PR from that based on Zulip discussion: #152311.
|
@bors r+ rollup |
…mimi Format heterogeneous try blocks The tracking issue for `try_blocks_heterogeneous` is rust-lang#149488. This follows the formatting of homogeneous try blocks (feature `try_blocks`) by considering `try bikeshed <type>` to be the equivalent of `try` (in particular a single "token"). An alternative would be to permit breaking between `bikeshed` and `<type>`, but given that those 2 elements are an explicitly temporary part of the syntax, it doesn't seem worth it. There also doesn't seem to be any existing precedent breaking between a keyword and a type. It also doesn't seem to be useful in practice, given that the type itself doesn't break (which is how it works for the return type of closures) and has more chances to dominate the length in case a break is necessary. Happy to adapt anything in case this formatting is not optimal. The test is also copied from homogeneous try blocks with 2 additional test cases to demonstrate the behavior with long types. See [#t-lang > try blocks @ 💬](https://rust-lang.zulipchat.com/#narrow/channel/213817-t-lang/topic/try.20blocks/near/572387493) for context.
Rollup of 7 pull requests Successful merges: - #151152 (Add FCW for derive helper attributes that will conflict with built-in attributes) - #151954 (Add help message suggesting explicit reference cast for From/TryFrom) - #152148 (Move `impl Interner for TyCtxt` to its own submodule) - #152226 (Modernize diagnostic for indeterminate trait object lifetime bounds) - #150688 (typeck: Make it clearer that `check_pat_lit` only handles literal patterns) - #152293 (Format heterogeneous try blocks) - #152396 (Uplift `Predicate::allow_normalization` to `rustc_type_ir`)
…mimi Format heterogeneous try blocks The tracking issue for `try_blocks_heterogeneous` is rust-lang#149488. This follows the formatting of homogeneous try blocks (feature `try_blocks`) by considering `try bikeshed <type>` to be the equivalent of `try` (in particular a single "token"). An alternative would be to permit breaking between `bikeshed` and `<type>`, but given that those 2 elements are an explicitly temporary part of the syntax, it doesn't seem worth it. There also doesn't seem to be any existing precedent breaking between a keyword and a type. It also doesn't seem to be useful in practice, given that the type itself doesn't break (which is how it works for the return type of closures) and has more chances to dominate the length in case a break is necessary. Happy to adapt anything in case this formatting is not optimal. The test is also copied from homogeneous try blocks with 2 additional test cases to demonstrate the behavior with long types. See [#t-lang > try blocks @ 💬](https://rust-lang.zulipchat.com/#narrow/channel/213817-t-lang/topic/try.20blocks/near/572387493) for context.
…mimi Format heterogeneous try blocks The tracking issue for `try_blocks_heterogeneous` is rust-lang#149488. This follows the formatting of homogeneous try blocks (feature `try_blocks`) by considering `try bikeshed <type>` to be the equivalent of `try` (in particular a single "token"). An alternative would be to permit breaking between `bikeshed` and `<type>`, but given that those 2 elements are an explicitly temporary part of the syntax, it doesn't seem worth it. There also doesn't seem to be any existing precedent breaking between a keyword and a type. It also doesn't seem to be useful in practice, given that the type itself doesn't break (which is how it works for the return type of closures) and has more chances to dominate the length in case a break is necessary. Happy to adapt anything in case this formatting is not optimal. The test is also copied from homogeneous try blocks with 2 additional test cases to demonstrate the behavior with long types. See [#t-lang > try blocks @ 💬](https://rust-lang.zulipchat.com/#narrow/channel/213817-t-lang/topic/try.20blocks/near/572387493) for context.
…mimi Format heterogeneous try blocks The tracking issue for `try_blocks_heterogeneous` is rust-lang#149488. This follows the formatting of homogeneous try blocks (feature `try_blocks`) by considering `try bikeshed <type>` to be the equivalent of `try` (in particular a single "token"). An alternative would be to permit breaking between `bikeshed` and `<type>`, but given that those 2 elements are an explicitly temporary part of the syntax, it doesn't seem worth it. There also doesn't seem to be any existing precedent breaking between a keyword and a type. It also doesn't seem to be useful in practice, given that the type itself doesn't break (which is how it works for the return type of closures) and has more chances to dominate the length in case a break is necessary. Happy to adapt anything in case this formatting is not optimal. The test is also copied from homogeneous try blocks with 2 additional test cases to demonstrate the behavior with long types. See [#t-lang > try blocks @ 💬](https://rust-lang.zulipchat.com/#narrow/channel/213817-t-lang/topic/try.20blocks/near/572387493) for context.
…uwer Rollup of 11 pull requests Successful merges: - #152364 (Port a lot of attributes to the new parser) - #151954 (Add help message suggesting explicit reference cast for From/TryFrom) - #152148 (Move `impl Interner for TyCtxt` to its own submodule) - #152226 (Modernize diagnostic for indeterminate trait object lifetime bounds) - #152351 (Remove `SubdiagMessage` in favour of the identical `DiagMessage`) - #152417 (Move the needs-drop check for `arena_cache` queries out of macro code) - #150688 (typeck: Make it clearer that `check_pat_lit` only handles literal patterns) - #152293 (Format heterogeneous try blocks) - #152355 (Update documentation of rustc_macros) - #152396 (Uplift `Predicate::allow_normalization` to `rustc_type_ir`) - #152425 (Port #![test_runner] to the attribute parser)
…uwer Rollup of 10 pull requests Successful merges: - #152364 (Port a lot of attributes to the new parser) - #151954 (Add help message suggesting explicit reference cast for From/TryFrom) - #152148 (Move `impl Interner for TyCtxt` to its own submodule) - #152226 (Modernize diagnostic for indeterminate trait object lifetime bounds) - #152351 (Remove `SubdiagMessage` in favour of the identical `DiagMessage`) - #152417 (Move the needs-drop check for `arena_cache` queries out of macro code) - #150688 (typeck: Make it clearer that `check_pat_lit` only handles literal patterns) - #152293 (Format heterogeneous try blocks) - #152355 (Update documentation of rustc_macros) - #152396 (Uplift `Predicate::allow_normalization` to `rustc_type_ir`)
Rollup merge of #152293 - ia0:try_blocks_heterogeneous, r=ytmimi Format heterogeneous try blocks The tracking issue for `try_blocks_heterogeneous` is #149488. This follows the formatting of homogeneous try blocks (feature `try_blocks`) by considering `try bikeshed <type>` to be the equivalent of `try` (in particular a single "token"). An alternative would be to permit breaking between `bikeshed` and `<type>`, but given that those 2 elements are an explicitly temporary part of the syntax, it doesn't seem worth it. There also doesn't seem to be any existing precedent breaking between a keyword and a type. It also doesn't seem to be useful in practice, given that the type itself doesn't break (which is how it works for the return type of closures) and has more chances to dominate the length in case a break is necessary. Happy to adapt anything in case this formatting is not optimal. The test is also copied from homogeneous try blocks with 2 additional test cases to demonstrate the behavior with long types. See [#t-lang > try blocks @ 💬](https://rust-lang.zulipchat.com/#narrow/channel/213817-t-lang/topic/try.20blocks/near/572387493) for context.
The tracking issue for
try_blocks_heterogeneousis #149488.This follows the formatting of homogeneous try blocks (feature
try_blocks) by consideringtry bikeshed <type>to be the equivalent oftry(in particular a single "token").An alternative would be to permit breaking between
bikeshedand<type>, but given that those 2 elements are an explicitly temporary part of the syntax, it doesn't seem worth it. There also doesn't seem to be any existing precedent breaking between a keyword and a type. It also doesn't seem to be useful in practice, given that the type itself doesn't break (which is how it works for the return type of closures) and has more chances to dominate the length in case a break is necessary.Happy to adapt anything in case this formatting is not optimal.
The test is also copied from homogeneous try blocks with 2 additional test cases to demonstrate the behavior with long types.
See #t-lang > try blocks @ 💬 for context.