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

Implement field shorthands in struct literal expressions. #11994

Merged
merged 1 commit into from
Oct 27, 2016

Conversation

eddyb
Copy link
Member

@eddyb eddyb commented Feb 2, 2014

Implements #37340 in a straight-forward way: Foo { x, y: f() } parses as Foo { x: x, y: f() }.
Because of the added is_shorthand to ast::Field, this is [syntax-breaking] (cc @Manishearth).

  • Mark the fields as being a shorthand (the exact same way we do it in patterns), for pretty-printing.
  • Gate the shorthand syntax with #![feature(field_init_shorthand)].
  • Don't parse numeric field as identifiers.
  • Arbitrary field order tests.

@huonw
Copy link
Member

huonw commented Feb 2, 2014

This is a (nice) syntax change, and so should get some discussion of some sort.

@alexcrichton
Copy link
Member

I'm surprised how easy that was to add!

I've added this to the meeting agenda (I'll put my vote in favor of it though).

@alexcrichton
Copy link
Member

One possible ambiguity issue that we've come up with on IRC:

for x in Foo { a } { b }

@huonw
Copy link
Member

huonw commented Feb 2, 2014

Maybe we could just allow omitting the field name, but not the colon:

let (some_field, other_field) = (1, 2);

for x in Foo { : some_field, : other_field } { ... }

... but that looks rather peculiar.

@eddyb
Copy link
Member Author

eddyb commented Feb 3, 2014

@huonw the ambiguity only shows up with one-field structures. How often have you seen Foo {a: a}?
In that case, adding another comma (as with one-tuples) is not that big of a problem if you really want to use the shorthand - Foo {a,} is still shorter.

@huonw
Copy link
Member

huonw commented Feb 4, 2014

We'll need a few tests for the comma thing too.

@SimonSapin
Copy link
Contributor

Even though it leaves a distasteful inconsistency in the language, for x in Foo { a } { b } seems rare enough that I don’t mind resolving the ambiguity by having the struct-expression syntaxic sugar not apply here.

@alexcrichton
Copy link
Member

We decided in today's meeting that due to ambiguities and some hygiene related concerns that we don't want this for now. Perhaps in rust 2.0!

@SimonSapin
Copy link
Contributor

Sad panda. Why as late as 2.0, isn’t it backward-compatible?

@alexcrichton
Copy link
Member

Ah I should clarify by 2.0 I mean "after 1.0" perhaps. We didn't concretely decide for a time for something like this to come in, we decided that now isn't the time for it though.

@huonw
Copy link
Member

huonw commented Feb 11, 2014

(Sorry for the spam: phone touchscreen leads to "pocket commenting" :( )

@ben0x539
Copy link
Contributor

Wouldn't be an ambiguity if we had ( ) in our for and if etc expressions like some other curly braces languages. :)

@SimonSapin
Copy link
Contributor

Too bad. I find myself often writing code ending like this:

SchemeRelativeUrl { userinfo: userinfo, host: host, port: port, path: path }

Where each field has be computed earlier. This sugar would make it much nicer.

As to the ambiguities, they only occur in cases where (IMO) it’s rare to have a literal struct expression.

(I wanted to write up some arguments that didn’t seem to appear in the meeting notes, but I’ll now stop beating this dead horse.)

@pnkfelix
Copy link
Member

@SimonSapin This seems trivial to work around for now via macro_rules! :

#[feature(macro_rules)];

macro_rules! struct_fields {
    ($Struct:ident $($field:ident),+ )
        => { $Struct { $($field: $field),+ } } ;
}

struct SchemeRelativeUrl {
    userinfo: ~str,
    host: [u8, ..4],
    port: u32,
    path: ~str
}

fn main() {
    let userinfo = ~"hi";
    let host = [1,2,3,4];
    let port = 56;
    let path = ~"/root/";
    let s = struct_fields!( SchemeRelativeUrl userinfo, host, port, path );
    println!("s: {:?}", s);
}

@SimonSapin
Copy link
Contributor

@pnkfelix This’ll help, thanks. I didn’t think of using a macro. However this doesn’t as easily cover cases where only some fields are based on a variable of the same name:

            Ok(Url { scheme: scheme, scheme_data: OtherSchemeData(scheme_data),
                     query: query, fragment: fragment })

Still, sugar baked into the language is always nicer than a macro you have to define :)

@huonw
Copy link
Member

huonw commented Feb 13, 2014

macro_rules! struct_field {
    ($Struct:ident $($short:ident),*; $($long:ident: $long_e: expr)*) => {
        $Struct { $($short: $short, )* $($long: $long_e),* }
    }
}

// ...
struct_fields!(
    Url scheme, query, fragment; scheme_data: OtherSchemeData(scheme_data)
)

@SimonSapin
Copy link
Contributor

Nice! Thanks @huonw. Is the ; necessary to remove ambiguity? Does the macro matcher not backtrack?

@huonw
Copy link
Member

huonw commented Feb 13, 2014

It doesn't backtrack (I haven't compiled that so there may actually need to be more disambiguation necessary :( ).

@eddyb
Copy link
Member Author

eddyb commented Oct 22, 2016

I'm genuinely curious to know how little needs to be updated here for it to work for #37340.

@eddyb eddyb reopened this Oct 22, 2016
@eddyb eddyb force-pushed the struct-literal-field-shorthand branch 3 times, most recently from a2fc2b0 to 4b26bde Compare October 22, 2016 08:14
@KalitaAlexey
Copy link
Contributor

@eddyb I see that in tests you follow the order of fields in a struct. Could you, please, some tests for case like:

struct Foo {
  x: i32,
  y: i32,
}

let x = 5;
let y = 6;
let foo = Foo { y, x };

@petrochenkov
Copy link
Contributor

Ha, this seems to parse shortcuts for numeric fields:

struct S(u8, u8, u8);
let s = S { 0, 1, 2 }; // Parses, 0, 1 and 2 are valid fields, but unresolved identifier expressions

Not sure if this is bad, but this is avoided in pattern parsing by calling parse_field_name only if we detected : ahead.

@petrochenkov
Copy link
Contributor

A feature gate is also probably needed now, it's not 2014 anymore :(

@eddyb eddyb force-pushed the struct-literal-field-shorthand branch from 4b26bde to 6e4ab6a Compare October 26, 2016 22:50
@eddyb eddyb force-pushed the struct-literal-field-shorthand branch from 6e4ab6a to 237bd2d Compare October 26, 2016 22:58
@eddyb eddyb force-pushed the struct-literal-field-shorthand branch from 237bd2d to 9908711 Compare October 27, 2016 00:15
@nrc
Copy link
Member

nrc commented Oct 27, 2016

@bors: r+

@bors
Copy link
Contributor

bors commented Oct 27, 2016

📌 Commit 9908711 has been approved by nrc

@bors
Copy link
Contributor

bors commented Oct 27, 2016

⌛ Testing commit 9908711 with merge bc283c9...

bors added a commit that referenced this pull request Oct 27, 2016
Implement field shorthands in struct literal expressions.

Implements #37340 in a straight-forward way: `Foo { x, y: f() }` parses as `Foo { x: x, y: f() }`.
Because of the added `is_shorthand` to `ast::Field`, this is `[syntax-breaking]` (cc @Manishearth).

* [x] Mark the fields as being a shorthand (the exact same way we do it in patterns), for pretty-printing.
* [x] Gate the shorthand syntax with `#![feature(field_init_shorthand)]`.
* [x] Don't parse numeric field as identifiers.
* [x] Arbitrary field order tests.
@bors bors merged commit 9908711 into rust-lang:master Oct 27, 2016
@eddyb eddyb deleted the struct-literal-field-shorthand branch March 9, 2017 13:40
flip1995 pushed a commit to flip1995/rust that referenced this pull request Dec 28, 2023
[`question_mark`]: also trigger on `return` statements

This fixes the false negative mentioned in rust-lang#11993: the lint only used to check for `return` expressions, and not a statement containing a `return` expression (doesn't close the issue tho since there's still a useful suggestion that we could make, which is to suggest `.ok_or()?`/`.ok_or_else()?` for `else { return Err(..) }`)

changelog: [`question_mark`]: also trigger on `return` statements
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants