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

Deprecate `try!` macro #62672

Merged
merged 3 commits into from Aug 9, 2019

Conversation

@lzutao
Copy link
Contributor

commented Jul 14, 2019

Replaces #62077

Fixes rust-lang/rust-clippy#1361
Fixes #61000

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Jul 14, 2019

r? @KodrAus

(rust_highfive has picked a reviewer for you, use r? to override)

@lzutao

This comment has been minimized.

Copy link
Contributor Author

commented Jul 14, 2019

@lzutao lzutao changed the title Deprecated `try!` macro Deprecate `try!` macro Jul 14, 2019

@petrochenkov

This comment has been minimized.

Copy link
Contributor

commented Jul 15, 2019

Implementation LGTM, but this should go through a library team FCP?

@petrochenkov

This comment has been minimized.

Copy link
Contributor

commented Jul 15, 2019

@sfackler

This comment has been minimized.

Copy link
Member

commented Jul 15, 2019

@rfcbot fcp merge

@rfcbot

This comment has been minimized.

Copy link

commented Jul 15, 2019

Team member @sfackler has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@matklad

This comment has been minimized.

Copy link
Member

commented Jul 15, 2019

Just to make sure folks are aware, ?can generate measurably slower code than try! or a manual match: #37939. I don't think this needs to block the deprecation.

@dtolnay

This comment has been minimized.

Copy link
Member

commented Jul 15, 2019

I am sad about this because of the ? performance issue, but ultimately I think it's fine to deprecate. In serde_derive we still don't use ? because it measurably hurts our benchmarks. Then again, we don't use try! either because it measurably hurts our compile time; none of our code needs the From::from error conversion and cutting it out saved significantly on type checking and borrow checking time.

Not a great situation... 😞

@rfcbot

This comment has been minimized.

Copy link

commented Jul 15, 2019

🔔 This is now entering its final comment period, as per the review above. 🔔

@est31

This comment has been minimized.

Copy link
Contributor

commented Jul 15, 2019

@dtolnay your problems would be solved by providing proper customizability support to ?. The Try trait is only partial support for behavior customization and it's not even stable yet (you can't shadow it, for example, unlike the try macro). IMO it's far too early to pursue deprecation of try...

@est31

This comment has been minimized.

Copy link
Contributor

commented Jul 15, 2019

I suggest anyone who needs the advanced features of try that aren't available for ? to use my try crate: https://crates.io/crates/try

I'll have to think about a good name replacement that is edition 2018 compliant.

@BO41

This comment has been minimized.

Copy link
Contributor

commented Jul 16, 2019

RELNOTES: The try! macro will be deprecated in 1.39.0 in favor of the ? operator.

EDIT: maybe even mention what @est31 said:

For anyone who wishes to continue to use the try! macro, I recommend using this try! crate: https://crates.io/crates/try

@mark-i-m

This comment was marked as resolved.

Copy link
Contributor

commented Jul 17, 2019

@rustbot modify labels: +relnotes

@rustbot

This comment was marked as resolved.

Copy link
Collaborator

commented Jul 17, 2019

Error: Label relnotes can only be set by Rust team members

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

@bors

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2019

☔️ The latest upstream changes (presumably #63029) made this pull request unmergeable. Please resolve the merge conflicts.

@lzutao lzutao force-pushed the lzutao:deprecated-try-macro branch from af4d100 to 8c0ab27 Jul 28, 2019

@lzutao

This comment has been minimized.

Copy link
Contributor Author

commented Jul 28, 2019

Ping @sfackler. Could you review this PR?

@Centril Centril modified the milestones: 1.38, 1.39 Jul 29, 2019

src/test/ui/lint/lint-qualification.rs Outdated Show resolved Hide resolved
src/test/ui/derived-errors/issue-31997.rs Outdated Show resolved Hide resolved
src/test/ui/associated-types/cache/chrono-scan.rs Outdated Show resolved Hide resolved
@Centril

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

r? @Centril

r=me rollup when green + with requested changes.

@rust-highfive rust-highfive assigned Centril and unassigned dtolnay Aug 8, 2019

@lzutao lzutao force-pushed the lzutao:deprecated-try-macro branch from 8c0ab27 to a7dd076 Aug 8, 2019

@lzutao

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2019

CI is green now. I will push squashed commits after .

BO41 and others added some commits Jun 23, 2019

Deprecate `try!` macro
Co-Authored-By: Mazdak Farrokhzad <twingoow@gmail.com>
Co-Authored-By: Oliver Middleton <olliemail27@gmail.com>

@lzutao lzutao force-pushed the lzutao:deprecated-try-macro branch from a7dd076 to 6842316 Aug 9, 2019

@lzutao

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2019

Ping @Centril

@Centril

Centril approved these changes Aug 9, 2019

@Centril

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

@bors r+ rollup

@bors

This comment has been minimized.

Copy link
Contributor

commented Aug 9, 2019

📌 Commit 6842316 has been approved by Centril

Centril added a commit to Centril/rust that referenced this pull request Aug 9, 2019

@Centril Centril referenced this pull request Aug 9, 2019

bors added a commit that referenced this pull request Aug 9, 2019

Auto merge of #63408 - Centril:rollup-skqrez3, r=Centril
Rollup of 7 pull requests

Successful merges:

 - #62672 (Deprecate `try!` macro)
 - #62950 (Check rustbook links on all platforms when running locally)
 - #63114 (Remove gensym in format_args)
 - #63397 (Add tests for some ICEs)
 - #63403 (Improve test output)
 - #63404 (enable flt2dec tests in Miri)
 - #63407 (reduce some test sizes in Miri)

Failed merges:

r? @ghost

@bors bors merged commit 6842316 into rust-lang:master Aug 9, 2019

4 checks passed

pr Build #20190809.4 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-6.0) Linux x86_64-gnu-llvm-6.0 succeeded
Details
pr (LinuxTools) LinuxTools succeeded
Details
@@ -302,6 +302,7 @@ macro_rules! debug_assert_ne {
/// ```
#[macro_export]
#[stable(feature = "rust1", since = "1.0.0")]
#[rustc_deprecated(since = "1.39.0", reason = "use the `?` operator instead")]

This comment has been minimized.

Copy link
@RalfJung

RalfJung Aug 9, 2019

Member

Notice that this means that nightly will start warning in a week -- which is an entire cycle before the deprecation is announced on the release notes.

We usually deprecate 3 releases in the future, so that between the deprecation announcement on a stable release and nightly starting to warn, there is still a full cycle. (Well, "usually" as in, both times I was involved in a future deprecation this is what we did -- in one case after some pushback when we wanted to go faster.) Right now, that would mean deprecating with Rust 1.41.

This comment has been minimized.

Copy link
@Centril

Centril Aug 9, 2019

Member

I don't think that matters much; try! has been deprecated for a long time in the docs and since ? was stabilized -- the change here is only due to a bugfix in the language.

@lzutao lzutao deleted the lzutao:deprecated-try-macro branch Aug 9, 2019

flip1995 added a commit to flip1995/rust-clippy that referenced this pull request Aug 15, 2019

Rustup to rust-lang/rust#62672
try macro is deprecated now, so Clippy will drop the support for it also
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.