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

suggest ? over try! #1361

Closed
oli-obk opened this issue Nov 24, 2016 · 15 comments · Fixed by rust-lang/rust#62672
Closed

suggest ? over try! #1361

oli-obk opened this issue Nov 24, 2016 · 15 comments · Fixed by rust-lang/rust#62672
Labels
A-lint Area: New lints good-first-issue These issues are a good way to get started with Clippy L-style Lint: Belongs in the style lint group

Comments

@oli-obk
Copy link
Contributor

oli-obk commented Nov 24, 2016

The ? operator has hit stable. We should lint all uses of the try macro and suggest the new operator instead.

@oli-obk oli-obk added L-style Lint: Belongs in the style lint group good-first-issue These issues are a good way to get started with Clippy A-lint Area: New lints labels Nov 24, 2016
@llogiq
Copy link
Contributor

llogiq commented Nov 24, 2016

Some people still prefer try!(_) for various reasons. Not sure a warning would be universally appreciated.

@oli-obk
Copy link
Contributor Author

oli-obk commented Nov 24, 2016

I know :( but even reading the rfc and tracking issue, the only complaint I could find is visibility, and even the complainers agree that it is just a syntax hilighting issue.

All in all, it is the recommended new way of doing error propagation, so I think we should suggest it.

Alternatively rustfmt could silently rewrite all code that doesn't have a macro_rules! try anywhere xD /s

@oli-obk
Copy link
Contributor Author

oli-obk commented Nov 24, 2016

cc @mbr opinions welcome :)

@mbr
Copy link

mbr commented Nov 24, 2016

Some people still prefer try!(_) for various reasons. Not sure a warning would be universally appreciated.

I agree, but I feel that even these are split in two camps. Some people will want to keep writing try! while others (like me) will probably deem it not important enough to not go with the "standard", even though they might have preferred try!. A clippy warning would certainly reinforce the notion of ? being the "modern" try!.

This seems like a bikeshedding issue to me at this point. The worst outcome would be having both variants common and valid, preventing me to get thoroughly used to one way or another, so for this reason please pick one and stick to it!

Alternatively rustfmt could silently rewrite all code that doesn't have a macro_rules! try anywhere xD /s

I'm still recovering from impl TODO. Please don't =).

Though adding a flag to do this automatically might be a good idea.

@bluss
Copy link
Member

bluss commented Nov 29, 2016

The Rust book doesn't mention ? yet.

@bluss
Copy link
Member

bluss commented Nov 29, 2016

On the day after Rust 1.13 was released, I was all about teaching ? but it quickly ran into trouble with the book not mentioning it lol 😄 Made me rethink, It's going to be rolled out slowly at this pace.

@est31
Copy link
Member

est31 commented Dec 9, 2016

As someone who doesn't like ? in my codebases, I'd like an opposite check: warn every time ? is found, so that I don't have to check contributions for its usage.

@leshow
Copy link

leshow commented Dec 11, 2016

@est31 The feature was merged into rust stable. I doubt rust's main linter is going to discourage it's use, that would be counterproductive.

@llogiq
Copy link
Contributor

llogiq commented Dec 11, 2016

Despite it's recent stability, it's still a young feature (for example codegen doesn't always return the same bytecode as with try!IIIC), so its benefits have yet to be proven.

@Manishearth
Copy link
Member

Manishearth commented Dec 11, 2016

(for example codegen doesn't always return the same bytecode as with try!IIIC)

try! is now implemented as ?, so codegen should return the same bytecode.

edit: nope, my bad

@sinkuu
Copy link
Contributor

sinkuu commented Dec 11, 2016

try has not been ported to use ? operator yet. https://github.com/rust-lang/rust/blob/master/src/libcore/macros.rs#L309

@bluss
Copy link
Member

bluss commented Dec 11, 2016

try!() is Result specific; ? gives less type information, so rust-lang/rust/issues/36244 is an example of type inference related regression.

@bluss
Copy link
Member

bluss commented Dec 11, 2016

It doesn't serve Rust that people hesitate about adopting features(*) (when they are this kind of direct replacement). You could say I voted -1 on having ? but now when it is part of Rust, I'm all for working with it.

There is a codegen issue: rust-lang/rust#37939 (comment) . This missing simplification/optimizations also applies to the code Ok(try!(x)) itself (pointed out by eddyb), so regardless of old try or ? it benefits Rust to resolve it (not to mention other instances when these no-op match blocks appear).

(*) Edit: So yes, that means that Rust needs to do the work so that it's worth switching to ?.

@In-line
Copy link

In-line commented Feb 13, 2019

It's still interesting to implement cargo lint for this : )

@ghost
Copy link

ghost commented May 20, 2019

https://github.com/rust-lang/rfcs/blob/master/text/2388-try-expr.md#breakage-of-the-try-macro lists good points about ? vs try!.
Wouldn't it be the best/easiest to just mark try! deprecated?
It wouldn't break anything, no lint would be required and more people would switch to ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: New lints good-first-issue These issues are a good way to get started with Clippy L-style Lint: Belongs in the style lint group
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants