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

implement the `?` operator #31954

Merged
merged 2 commits into from Mar 8, 2016

Conversation

Projects
None yet
10 participants
@japaric
Copy link
Member

japaric commented Feb 28, 2016

The ? postfix operator is sugar equivalent to the try! macro, but is more amenable to chaining:
File::open("foo")?.metadata()?.is_dir().

? is accepted on any expression that can return a Result, e.g. x()?, y!()?, {z}?,
(w)?, etc. And binds more tightly than unary operators, e.g. !x? is parsed as !(x?).

cc #31436


cc @aturon @eddyb

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Feb 28, 2016

r? @arielb1

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

expr_call(lctx, e.span, err_ctor, hir_vec![from_expr], None)
};
let err_pat = pat_err(lctx, e.span, pat_ident(lctx, e.span, err_ident));
let ret_expr = expr(lctx, e.span, hir::Expr_::ExprRet(Some(err_expr)), None);

This comment has been minimized.

@oli-obk

oli-obk Feb 29, 2016

Contributor

this line is too long for make tidy

@durka

This comment has been minimized.

Copy link
Contributor

durka commented Feb 29, 2016

Needs a feature gate, right?

@japaric

This comment has been minimized.

Copy link
Member

japaric commented Feb 29, 2016

? is now behind a feature gate.

@brson

This comment has been minimized.

Copy link
Contributor

brson commented Feb 29, 2016

🎉

@brson

This comment has been minimized.

Copy link
Contributor

brson commented Feb 29, 2016

It might be useful to have tests of what happens when the try expansion fails to resolve. Is it nice? Hideous?

@brson

This comment has been minimized.

Copy link
Contributor

brson commented Feb 29, 2016

Here's the try test case. I think it looks nice. The ? really gets out of the way so you don't notice the error handling.

@seanmonstar

This comment has been minimized.

Copy link
Contributor

seanmonstar commented Mar 1, 2016

The RFC claims this feature flag should be question_mark. https://github.com/rust-lang/rfcs/blob/master/text/0243-trait-based-exception-handling.md

@japaric

This comment has been minimized.

Copy link
Member

japaric commented Mar 1, 2016

It might be useful to have tests of what happens when the try expansion fails to resolve. Is it nice? Hideous?

I don't have a build at hand, but the error message should be similar to the one for unresolved for loops. The one for for loops looks like this:

#![feature(no_core)]
#![no_core]

fn main() {
    for x in xs {}
}
$ rustc for.rs
for.rs:5:14: 5:16 help: run `rustc --explain E0425` to see a detailed explanation
for.rs:5:5: 5:9 error: failed to resolve. Maybe a missing `extern crate iter`? [E0433]
for.rs:5     for x in xs {}
             ^~~~
for.rs:5:5: 5:9 help: run `rustc --explain E0433` to see a detailed explanation
for.rs:5:5: 5:19 error: unresolved name `iter::IntoIterator::into_iter` [E0425]
for.rs:5     for x in xs {}
             ^~~~~~~~~~~~~~
for.rs:5:5: 5:19 help: run `rustc --explain E0425` to see a detailed explanation
for.rs:5:5: 5:9 error: failed to resolve. Maybe a missing `extern crate iter`? [E0433]
for.rs:5     for x in xs {}
             ^~~~
for.rs:5:5: 5:9 help: run `rustc --explain E0433` to see a detailed explanation
for.rs:5:5: 5:19 error: unresolved name `iter::Iterator::next` [E0425]
for.rs:5     for x in xs {}
             ^~~~~~~~~~~~~~
for.rs:5:5: 5:19 help: run `rustc --explain E0425` to see a detailed explanation
for.rs:5:5: 5:19 error: unresolved enum variant, struct or const `Some` [E0419]
for.rs:5     for x in xs {}
             ^~~~~~~~~~~~~~
for.rs:5:5: 5:19 help: run `rustc --explain E0419` to see a detailed explanation
for.rs:5:5: 5:19 error: unresolved enum variant, struct or const `None` [E0419]
for.rs:5     for x in xs {}
             ^~~~~~~~~~~~~~

@seanmonstar will push a commit renaming the feature gate in a bit.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Mar 4, 2016

r? @nikomatsakis

(stealing review)

@rust-highfive rust-highfive assigned nikomatsakis and unassigned arielb1 Mar 4, 2016

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Mar 4, 2016

So first review done, comments:

  • Move parsing code into parse_dot_or_call_expr_with_ instead
  • Move feature-gate code into PostExpansionVisitor
  • Feels like there could/should be some negative tests, but I'm not sure precisely what :) maybe not needed

(I'll take another look afterwards.)

@japaric

This comment has been minimized.

Copy link
Member

japaric commented Mar 6, 2016

Move parsing code into parse_dot_or_call_expr_with_ instead
Move feature-gate code into PostExpansionVisitor

Done. The impl looks simpler now. :-)

Feels like there could/should be some negative tests, but I'm not sure precisely what :) maybe not needed

You mean like checking that ExprKind::Try(ExprKind::Binary) is never created?

This seems to be roughly what we do elsewhere -- but do we need to do anything to make hygiene work out for us? Can we make a test where the context defines const val: u32 = 0 or something, just to make sure that the name val here is always a binding and does not conflict with this in-scope constant?

I've added a test for this.

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Mar 6, 2016

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

@japaric

This comment has been minimized.

Copy link
Member

japaric commented Mar 6, 2016

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

Will rebase after @nikomatsakis takes a second look :-).

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Mar 7, 2016

@japaric

The changes look great. r=me, feel free to rebase. :)

You mean like checking that ExprKind::Try(ExprKind::Binary) is never created?

No, I meant some kind of test that a + b? parses as expected, but it doesn't seem necessary now with the newer code.

implement the `?` operator
The `?` postfix operator is sugar equivalent to the try! macro, but is more amenable to chaining:
`File::open("foo")?.metadata()?.is_dir()`.

`?` is accepted on any *expression* that can return a `Result`, e.g. `x()?`, `y!()?`, `{z}?`,
`(w)?`, etc. And binds more tightly than unary operators, e.g. `!x?` is parsed as `!(x?)`.

cc #31436

@japaric japaric force-pushed the japaric:rfc243 branch from d6a9621 to 210dd61 Mar 7, 2016

@japaric

This comment has been minimized.

Copy link
Member

japaric commented Mar 7, 2016

@bors: r=nikomatsakis

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Mar 7, 2016

📌 Commit 210dd61 has been approved by nikomatsakis

bors added a commit that referenced this pull request Mar 7, 2016

Auto merge of #31954 - japaric:rfc243, r=nikomatsakis
The `?` postfix operator is sugar equivalent to the try! macro, but is more amenable to chaining:
`File::open("foo")?.metadata()?.is_dir()`.

`?` is accepted on any *expression* that can return a `Result`, e.g. `x()?`, `y!()?`, `{z}?`,
`(w)?`, etc. And binds more tightly than unary operators, e.g. `!x?` is parsed as `!(x?)`.

cc #31436

---

cc @aturon @eddyb
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Mar 7, 2016

⌛️ Testing commit 210dd61 with merge fc751a0...

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Mar 7, 2016

💔 Test failed - auto-linux-64-opt

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Mar 8, 2016

@japaric

This comment has been minimized.

Copy link
Member

japaric commented Mar 8, 2016

@bors: r=nikomatsakis

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Mar 8, 2016

📌 Commit 2de4932 has been approved by nikomatsakis

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Mar 8, 2016

⌛️ Testing commit 2de4932 with merge 3af60f8...

bors added a commit that referenced this pull request Mar 8, 2016

Auto merge of #31954 - japaric:rfc243, r=nikomatsakis
implement the `?` operator

The `?` postfix operator is sugar equivalent to the try! macro, but is more amenable to chaining:
`File::open("foo")?.metadata()?.is_dir()`.

`?` is accepted on any *expression* that can return a `Result`, e.g. `x()?`, `y!()?`, `{z}?`,
`(w)?`, etc. And binds more tightly than unary operators, e.g. `!x?` is parsed as `!(x?)`.

cc #31436

---

cc @aturon @eddyb

@bors bors merged commit 2de4932 into rust-lang:master Mar 8, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@japaric japaric deleted the japaric:rfc243 branch Mar 10, 2016

@perlun

This comment has been minimized.

Copy link
Contributor

perlun commented Aug 11, 2017

For the record, this was stabilized as part of 1.13.0, i.e no longer behind a feature gate. I hadn't seen it until now, when I happened to see it being referred to in the docs. 😉 Nice with some more syntactic sugar!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment