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

Add do yeet expressions to allow experimentation in nightly #96376

Merged
merged 5 commits into from
May 1, 2022

Conversation

scottmcm
Copy link
Member

@scottmcm scottmcm commented Apr 25, 2022

Two main goals for this:

r? @oli-obk
The lang second for doing this: rust-lang/lang-team#160 (comment)

Tracking issues

@rustbot rustbot added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Apr 25, 2022
@rust-highfive

This comment was marked as outdated.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 25, 2022
@scottmcm scottmcm added T-libs-api Relevant to the library API 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. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Apr 25, 2022
Comment on lines +228 to +231
ast::ExprKind::Yeet(None) => Some("do yeet".to_owned()),
ast::ExprKind::Yeet(Some(ref expr)) => {
rewrite_unary_prefix(context, "do yeet ", &**expr, shape)
}
Copy link
Member

@calebcartwright calebcartwright Apr 25, 2022

Choose a reason for hiding this comment

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

Typically with a new variant we'd start with rustfmt just spitting back out whatever the user wrote, but in this particular case I can't imagine a universe where we'd do anything different. As such I've no concerns with these rustfmt changes 👍

Will use this to mention an aside, and realize the style team hasn't fully come to fruition yet @joshtriplett, but I think one future question/consideration for that team should be when/how new syntax gets incorporated into the guide, including for cases like this that are obvious and noncontroversial.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, thanks @calebcartwright; I wasn't thinking about the stability details here at all. But yes, I agree that I'd be very surprised if we wanted this to format differently from return/break. Also, the syntax for this will change later, so we'll have another opportunity to rethink the formatting once that happens.

As another aside: Is there a way this match could have a catch-all arm that just does the "spit back what the user wrote", so that additions to the AST like this would do the right thing without needing a code change? I only updated the file because the PR build broke when I did nothing.

Copy link
Member

Choose a reason for hiding this comment

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

Is there a way this match could have a catch-all arm that just does the "spit back what the user wrote", so that additions to the AST like this would do the right thing without needing a code change?

Potentially. IIRC many of the big match expressions used to have a catch-all pattern arm in the earlier days when rustfmt used the AP crates, but that was intentionally changed some time back as part of an effort to ensure the rustfmt team would become aware of new syntax associated with new variants.

If/when we can implement some of the process changes previously discussed elsewhere then I'd certainly be open to adding such arms back. In the interim, for future reference, the behavior can be triggered with None or something like Some(context.snippet(<<span>>).to_owned())

@pro465

This comment was marked as off-topic.

@rustbot
Copy link
Collaborator

rustbot commented Apr 25, 2022

Error: The "Author" shortcut only works on pull requests.

Please let @rust-lang/release know if you're having trouble with this bot.

@scottmcm scottmcm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 25, 2022
);

if let Some(catch_node) = self.catch_scope {
let target_id = Ok(self.lower_node_id(catch_node));
Copy link
Contributor

Choose a reason for hiding this comment

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

preexisting, but I wish we didn't have to lower the node id twice.

| ^^^^^^^ cannot infer type
|
= note: cannot satisfy `<_ as Try>::Residual == _`
help: consider specifying the type argument in the method call
|
LL | l.iter().map(f).collect::<B>()?
| +++++
| ^ cannot infer type
Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunate that the second FromResidual impl breaks this

Comment on lines 10 to 12
= help: the following other types implement trait `FromResidual<R>`:
<Result<T, F> as FromResidual<Result<Infallible, E>>>
<Result<T, F> as FromResidual<Yeet<E>>>
Copy link
Contributor

Choose a reason for hiding this comment

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

Technically adding a second impl when there was only one before is a breaking change I think. There are some cases where inference figures something out. Considering that users can't mention FromResidual themselves, we're good here, but probably good to keep in mind.

Copy link
Member Author

Choose a reason for hiding this comment

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

Crater says we're clean, but I think the other thing that helps is the direction in which we're using it -- AsRef<T> has this problem all the time when used as a method, because with self known then if there's only one impl it's easy to know the T. But here we're going the other direction, and I know know that the inference engine allows taking advantage of "FromResidual<Concrete> is only implemented for one type", since that's fundamentally less stable, as anyone who wants could implement that themselves later if they want.

(And if there is a way that adding another impl here can be a breaking change, all the better to get a second one in early to keep anyone from finding a way to write stable code that depends on it 🙃)

@oli-obk
Copy link
Contributor

oli-obk commented Apr 27, 2022

Should we crater this? I think there are no actual problems here, but may have missed something

@scottmcm
Copy link
Member Author

Looks like the crater queue is currently empty, so might as well -- especially since check-only is fine.

@bors try

@bors
Copy link
Contributor

bors commented Apr 27, 2022

⌛ Trying commit 778944ca1ffaf781928cc846a5d41389c95da009 with merge ea2bf32540662c61547071aad6cf0851a0b02f58...

@bors
Copy link
Contributor

bors commented Apr 27, 2022

☀️ Try build successful - checks-actions
Build commit: ea2bf32540662c61547071aad6cf0851a0b02f58 (ea2bf32540662c61547071aad6cf0851a0b02f58)

@scottmcm
Copy link
Member Author

@craterbot check

@craterbot
Copy link
Collaborator

👌 Experiment pr-96376 created and queued.
🤖 Automatically detected try build ea2bf32540662c61547071aad6cf0851a0b02f58
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 27, 2022
@craterbot
Copy link
Collaborator

🚧 Experiment pr-96376 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot
Copy link
Collaborator

🎉 Experiment pr-96376 is completed!
📊 6 regressed and 4 fixed (229604 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Apr 30, 2022
@scottmcm
Copy link
Member Author

No real regressions here.

Errors:

[INFO] [stdout] error[E0583]: file not found for module `bitrust_pb`
[INFO] [stdout]  --> /opt/rustwide/target/debug/build/bitrust-a3e48e3b381fb594/out/proto_mod.rs:1:1
[INFO] [stdout]   |
[INFO] [stdout] 1 | pub mod bitrust_pb;
[INFO] [stdout]   | ^^^^^^^^^^^^^^^^^^^
[INFO] [stdout]   |
[INFO] [stdout]   = help: to create the module `bitrust_pb`, create file "/opt/rustwide/target/debug/build/bitrust-a3e48e3b381fb594/out/proto_mod/bitrust_pb.rs" or "/opt/rustwide/target/debug/build/bitrust-a3e48e3b381fb594/out/proto_mod/bitrust_pb/mod.rs"
[INFO] [stdout] error[E0583]: file not found for module `proto_mod`
[INFO] [stdout]  --> /opt/rustwide/target/debug/build/ledger-transport-zemu-9fbd9f2b9055f698/out/proto_mod.rs:2:1
[INFO] [stdout]   |
[INFO] [stdout] 2 | pub mod proto_mod;
[INFO] [stdout]   | ^^^^^^^^^^^^^^^^^^
[INFO] [stdout]   |
[INFO] [stdout]   = help: to create the module `proto_mod`, create file "/opt/rustwide/target/debug/build/ledger-transport-zemu-9fbd9f2b9055f698/out/proto_mod/proto_mod.rs" or "/opt/rustwide/target/debug/build/ledger-transport-zemu-9fbd9f2b9055f698/out/proto_mod/proto_mod/mod.rs"
[INFO] [stderr]   thread 'main' panicked at 'failed to create required symlink: Os { code: 17, kind: AlreadyExists, message: "File exists" }', /opt/rustwide/cargo-home/git/checkouts/binaryninja-api-3c86176688a74ccb/661d7c9/rust/binaryninjacore-sys/build.rs:70:14
[INFO] [stderr]   thread 'main' panicked at 'failed to create required symlink: Os { code: 17, kind: AlreadyExists, message: "File exists" }', /opt/rustwide/cargo-home/git/checkouts/binaryninja-api-3c86176688a74ccb/998d8ba/rust/binaryninjacore-sys/build.rs:70:14
[INFO] [stdout] error: expected expression, found `<eof>`
[INFO] [stdout]  --> /opt/rustwide/target/debug/build/sailfish-compiler-d2f5ef8f8c6f2180/out/templates/style-1065dfe86ae1c352:0:1
[INFO] [stdout]   |
[INFO] [stdout] 
[INFO] [stdout] 
[INFO] [stdout] error: non-statement macro in statement position: include
[INFO] [stdout]  --> src/style.rs:5:10
[INFO] [stdout]   |
[INFO] [stdout] 5 | #[derive(TemplateOnce)]
[INFO] [stdout]   |          ^^^^^^^^^^^^
[INFO] [stdout]   |
[INFO] [stdout]   = note: this error originates in the derive macro `TemplateOnce` (in Nightly builds, run with -Z macro-backtrace for more info)
[INFO] [stderr]   thread 'main' panicked at '失败:不能删除原来的 /opt/rustwide/target/assets 符号链接文件: Os { code: 20, kind: NotADirectory, message: "Not a directory" }', build.rs:72:14

But it looks like I have some conflicts to fix, so
@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 30, 2022
Using an obviously-placeholder syntax.  An RFC would still be needed before this could have any chance at stabilization, and it might be removed at any point.

But I'd really like to have it in nightly at least to ensure it works well with try_trait_v2, especially as we refactor the traits.
@scottmcm
Copy link
Member Author

scottmcm commented May 1, 2022

Conflicts were nothing interesting, just in the UI tests, so I think this is good to go, @oli-obk .

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 1, 2022
@oli-obk
Copy link
Contributor

oli-obk commented May 1, 2022

@bors r+

@bors
Copy link
Contributor

bors commented May 1, 2022

📌 Commit b317ec1 has been approved by oli-obk

@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 May 1, 2022
@bors
Copy link
Contributor

bors commented May 1, 2022

⌛ Testing commit b317ec1 with merge 508e058...

@bors
Copy link
Contributor

bors commented May 1, 2022

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 508e058 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 1, 2022
@bors bors merged commit 508e058 into rust-lang:master May 1, 2022
@rustbot rustbot added this to the 1.62.0 milestone May 1, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (508e058): comparison url.

Summary: This benchmark run did not return any relevant results.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Copy link

@isc30 isc30 left a comment

Choose a reason for hiding this comment

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

The new syntax doesn't bring anything over return Err(...)

@Cypher1
Copy link

Cypher1 commented Jan 28, 2023

@isc30 From #96373

@reza-ebrahimi yeet has an implicit .into() conversion that return Err(…) lacks.

So... kinda

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.