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

RFC: Reserve f(a = b) in Rust 2018 #2443

Closed
wants to merge 1 commit into from

Conversation

kennytm
Copy link
Member

@kennytm kennytm commented May 18, 2018

@Centril Centril added the T-lang Relevant to the language team, which will review and decide on the RFC. label May 18, 2018
@burdges
Copy link

burdges commented May 19, 2018

Isn't the type ascription issue necessarily already solved for struct creation?

struct<A> Foo { a: A }
Foo { a: (bar: f64) } // Not sure if you need the parentheses
Foo { a: f64 } // Also not sure if the field pun form works

```rust
macro_rules! call { ($e:expr) => { foo($e) } }
call!(a = 1);
// allowed, expands to `foo((a = 1))`.
Copy link
Member

Choose a reason for hiding this comment

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

This gets into the "are macros tokens or not" question that I never understand. If the macro used a tt instead of an expr would it be invalid? Why did the macro expand to something with parens here?

I think the number of cases involved in the "not in these places" definition still has me inclined towards the "assignment is just a statement" alternative instead (with some things like "match arms can be any statement", perhaps).

Copy link
Member Author

Choose a reason for hiding this comment

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

@scottmcm Yes if it is expanded via :tt instead of :expr it would be invalid.

"Something with parenthesis" is already the existing behavior of :expr. Compare:

macro_rules! a {
    (expr: $e:expr) => { 3 * $e };
    (tts: $($t:tt)*) => { 3 * $($t)* };
}

fn main() {
    println!("{}", a!(expr: 7 + 13));   // 60
    println!("{}", a!(tts: 7 + 13));    // 34
}

from all editions (including 2015 and 2018), i.e. the syntax `f(a = b)` should be unreserved:

1. We decided to permanently reject named arguments, or
2. We accepted named arguments but have chosen a different syntax (e.g. `f(a: b)`).
Copy link
Member

Choose a reason for hiding this comment

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

This is a great section! Thanks for including it.

@scottmcm
Copy link
Member

@burdges

Foo { a: f64 } // Also not sure if the field pun form works

This compiles, but probably doesn't do what you were thinking: rust-lang/rust-clippy#2676

@kennytm
Copy link
Member Author

kennytm commented May 19, 2018

@burdges No you can't place an arbitrary expression in a struct literal, e.g. Foo { &a } is not valid syntax. Thus struct literal has no conflict with type ascription.

@burdges
Copy link

burdges commented May 19, 2018

I'm surprised if Foo { &a } does not work because Foo { ref mut a } works for destructuring. I suppose type ascription can give an lvalue so Foo { (a: f64) } might work?

@petrochenkov
Copy link
Contributor

@burdges

I'm surprised if Foo { &a } does not work because Foo { ref mut a } works for destructuring.

You are not alone: https://internals.rust-lang.org/t/suggestion-references-in-struct-literal/6789

@joshtriplett
Copy link
Member

I do think it's worth linting against this, if only because it looks confusing to use an assignment expression as a function argument. (And since assignment returns (), it doesn't make much sense to use it as an argument regardless.) However, I don't think we should reserve this syntax.

@est31
Copy link
Member

est31 commented May 23, 2018

I've originally expressed concerns with this very change in the internals thread but since then changed my opinion.

I still do not like named arguments, but = is the least worst syntax for this bad feature so while I would prefer continue to desire an RFC or an official descision that named arguments won't be added to the language, I 👍 this RFC.

@Ixrec
Copy link
Contributor

Ixrec commented May 23, 2018

Although this is technically off-topic, @est31 Are your objections to named arguments (regardless of syntax) written down anywhere? I'm completely unaware of what they might be and have never seen anyone else claim they're an undesirable feature (as opposed to a low priority).

@steveklabnik
Copy link
Member

@Ixrec @est31 best discussed over here: #323

@est31
Copy link
Member

est31 commented May 23, 2018

@Ixrec I'll reply to your question on the thread that @steveklabnik linked. Don't want this thread to become a discussion of the feature itself.

@ids1024
Copy link

ids1024 commented May 29, 2018

However, I don't think we should reserve this syntax.

@joshtriplett Why? Do you think named arguments are a bad idea, object to this particular syntax, or want to avoid breaking the use of assignments in function calls?

@vpzomtrrfrt
Copy link

I personally value consistency, and to disallow this syntax in one specific case goes against that. There are plenty of syntaxes we could use instead that won't require restrictions on existing constructs, so to me this is not a desirable change.

@nrc
Copy link
Member

nrc commented Jun 8, 2018

I'm strongly in favour of named arguments, but I don't think we should be speculatively reserving syntax for it. We might settle on something else or decide not to have it at all. Or we might not work on it until after the next edition and we can reserve something then.

In general, I don't think we should be using the edition to reserve keywords and syntax speculatively. If we have a firm plan and the timing works with an edition (e.g., dyn Trait), then great, but I don't think it is in good spirit to guess what might happen and hurry reservations to support those guesses (even when it is a feature which I would really like to 'encourage' to happen sooner)>

@anirudhb
Copy link

anirudhb commented Jun 8, 2018

I don't see what's wrong with this because:

  1. It is forward-compatible (for named arguments?)
  2. It is easy to get around typing () every time:
mod aliases {
    let one = 1;
    let zero = 0;
    // ...
    let nil = (); // It is nil, right? I forgot the actual name
}
fn nilly_function() {
    stuff1(()); // stuff1(aliases::nil);
    stuff2(()); // stuff2(aliases::nil);
    // use aliases::nil;
    stuff3(Ok(()), Some(()), ()); // stuff3(Ok(nil), Some(nil), nil);
}

@leoyvens
Copy link

leoyvens commented Jun 8, 2018

@nrc Speculative discussion is generally unproductive, but in this case I argue that we can't escape speculation and that reserving is the right speculation.

Speculation is unavoidable. Either the feature must speculate for a future edition, or an edition must to speculate for the syntax. Right now is we have good opportunity to discuss the second option.

Not reserving inhibits discussion. It is reasonable to plan for delegate and dyn as contextual keywords. However in f(a = b) the a = b is always interpreted as an assignment and I'm not aware of any reasonable way to resolve that without editions, so no plan for this syntax could exist without bringing along planning for an edition. For named arguments syntax likely blocks any progress in discussing the merit of the feature, so this makes discussion always feel too early, because there is no plan for an edition, or too late, because the edition scope is set. It would be really difficult to get the timing for discussing it right.

Reserving costs almost nothing. This is not the same as keyword reservation, reserving a keyword has a serious impact on the availability of identifiers. The syntax discussed here is currently virtually useless and unused, the impact is likely to be zero. The alternative of stable feature gates makes little sense for changes of virtually zero breakage.

So yes, let's speculate, and in that, let's reserve. Thanks @kennytm for writing this.

@scottmcm
Copy link
Member

scottmcm commented Jun 9, 2018

It seems to me that all three points could apply just as well to keyword reservations, @leodasvacas, which makes me skeptical that there's a major difference here. The post in the other thread says

up-front reservations are wrong and a mistake in the initial Edition proposal [because] they force up-front decisions about surface issues of features that are not yet fully proposed, let alone accepted or implemented. (#2441 (comment))

That's explicitly about avoiding speculation, regardless of kind.

For named arguments, is it reasonable to have concerns about whether another mechanism is really needed in the language? Yes. Are there multiple possible reasonable syntaxes? Yes. (The RFC even mentions some.) Could it be done with a suboptimal transitional syntax in the epoch in which it's accepted, reserving the chosen one only in a following epoch? Yes.

@hanna-kruppe
Copy link

hanna-kruppe commented Jun 9, 2018

It seems to me that all three points could apply just as well to keyword reservations, @leodasvacas, which makes me skeptical that there's a major difference here.

New keywords can be introduced as contextual keywords without involving editions (and optionally promoted to a full keyword in a later edition), because they mostly occur in syntactical positions where an extra identifier wouldn't be valid syntax anyway. When handling new keywords this way, we don't need to speculate (we can just implement the contextual keyword when the feature is designed) and there's nothing "inhibiting discussion" if it hasn't been reserved up-front.

There's no comparable way to opportunistically reinterpret f(a = b) without breaking backwards compatibility. There are a few hypothetical ways, like a "stable feature gate" opt-in, but those have no precedent in Rust and have big drawbacks for everday use that contextual keywords don't have.

@leoyvens
Copy link

leoyvens commented Jun 9, 2018

That's explicitly about avoiding speculation, regardless of kind.

@scottmcm True, the policy as written does apply here. I'm just noting that some trade-offs are different.

opportunistically reinterpret

Edit: The idea below could never work, nevermind. We can't reinterpret a = b based on the signature.

@rkruppe We could depending on the specifics of each proposal. If the caller can choose whether to pass an argument by position or by name (Python, C# 7, Scala), then we need this syntax. But if the signature mandates whether an argument is named or positional (e.g. Swift), then anything past the last positional argument would be interpreted as a named argument rather than an assignment.

Not reserving creates that complication in the design space, but depending on your preference you might care less or more about the reservation.

@Centril
Copy link
Contributor

Centril commented Jun 9, 2018

I like @scottmcm do not see any notable difference between this RFC and #2441. If that one is closed then so should this one be in my opinion.

@rkruppe

New keywords can be introduced as contextual keywords without involving editions (and optionally promoted to a full keyword in a later edition), because they mostly occur in syntactical positions where an extra identifier wouldn't be valid syntax anyway.

Well, fail and throw couldn't have been introduced as contextual because fail {} would be a perfectly valid expression, equivalent to fail (), which would conflict with struct fail {}.

@leodasvacas

Not reserving inhibits discussion. It is reasonable to plan for delegate and dyn as contextual keywords.

This applies equally to fail and throw which can't be contextual per note above.

Reserving costs almost nothing. This is not the same as keyword reservation, reserving a keyword has a serious impact on the availability of identifiers.

As explained in #2441, those reservations cost very little as well. The impact was not at all serious.

@ids1024
Copy link

ids1024 commented Jun 9, 2018

Can anyone think of a real-world disadvantage to reserving this? Can you find any crate that uses code like this, even if it's accidental and leads to bugs? Any reason someone would write code like this, or a macro would generate it? I'm trying to think even of a reason you would use it for code golf, but it's generally not helpful to pass an empty tuple to a function. Really, the main reason someone would write something like this is to test if Rust supports named arguments.

As far as advantages, it seems fairly clear that this is helpful if named arguments are added, and this syntax is used. And it does seem like the most obvious syntax.

So though I don't have strong opinions on named arguments and like that Rust avoids breakings changes and tries not to arbitrarily limit what code can compile, the advantages of reserving the syntax seem to outweigh the philosophical disadvantages.

throw/fail seems different since there is a comment there by someone who has used fail as an identifier, and I doubt they're the only one. I'd be interested to see if anyone can find an example of f(a = b).

@joshtriplett
Copy link
Member

It introduces an unusual corner case, and that corner case only makes sense if supporting named arguments with this particular syntax.

@repax
Copy link

repax commented Jun 10, 2018

This is the the weird corner case as I understand it:

foo(a = 5);

is not the same as:

foo((a = 5));

even though the second snippet only seems to have a redundant pair of parentheses added to it. You'd expect both of them to be equivalent to:

let x = (a = 5);
foo(x);

@hanna-kruppe
Copy link

hanna-kruppe commented Jun 10, 2018

@Centril

Well, fail and throw couldn't have been introduced as contextual because fail {} would be a perfectly valid expression, equivalent to fail (), which would conflict with struct fail {}.

Hm, I hadn't followed that discussion. fail { ... } doesn't seem like a big problem, though. I imagine "fail with the expression being a block" would be quite rare, so it wouldn't be a big burden to just interpret fail as identifier when followed by a bracket.

However, fail () actually has the same problem (it's currently a call). One could give fail lowest priority to remove the need for parentheses for anything (edit this falls apart when you want to "fail" with tuples, -x, and probably some other things).

The other way to make it contextual would be to involve name resolution information (it's a call if there's a function fail in scope at this point) -- similar to @leodasvacas's earlier example of interpreting f(a = b) differently depending on the signature of f. That would work, but (in both cases, fail and named arguments) it brings a lot of complexity and baggage we'd probably rather avoid. Not just for rustc, but also for people reading code and even more so for tools like rustfmt.

@Centril
Copy link
Contributor

Centril commented Jun 10, 2018

@rkruppe

However, fail () actually has the same problem (it's currently a call).

Ouch! This hadn't even occurred to me; it seems more serious.

That would work, but (in both cases, fail and named arguments) it brings a lot of complexity and baggage we'd probably rather avoid.

Agreed :)

@repax
Copy link

repax commented Jun 10, 2018

We could look for ways to make this prettier:

foo(Foo {x: 5, y: 7})

How about this?

foo({x: 5, y: 7})

Or this?

foo{x: 5, y: 7}

@Centril
Copy link
Contributor

Centril commented Jun 10, 2018

@repax I scribbled some thoughts about foo({ x: 5, y: 7 }) a while back, https://gist.github.com/Centril/879683253d2d6124143a116d6dc8d20b, the details on FRU are sadly unworkable but the basic idea seems fine to me.

@Centril
Copy link
Contributor

Centril commented Jun 10, 2018

@ids1024

And it does seem like the most obvious syntax.

Not to me. The struct initialization and pattern binding syntax is unfortunately Type { field: expr }, not Type { field = expr }. So if we are being consistent with Rust as-is, then foo(x: 5, y: 7) is what I would use.

@leoyvens
Copy link

leoyvens commented Jun 12, 2018

My previous workaround idea could never work, but one that could work is to have f(a = b) be an assignment if a is a variable in scope (in which case we lint) and be a named argument otherwise, in epochs previous to whenever we reserve this.

@joshtriplett
Copy link
Member

We discussed this a bit in the lang team meeting. Rough summary of that discussion:

  • We're open to seeing a warning lint for this, because it seems extremely unlikely that the user intended to execute a=b and then pass () to a function. We weren't sure whether this should be a clippy lint or a rustc lint, but we think it makes sense for one or the other of those to add a warn-by-default lint on this.
  • For the same reason as the recent keyword reservation discussions, we think @aturon's statement about not pre-reserving keywords prior to the definition of the feature using those keywords also applies to pre-reserving syntax like this. Based on that, we don't want to pre-reserve this syntax for named arguments; we'd want to see a full RFC for named arguments first and evaluate the proposed syntax as one component of that RFC.

Based on that:

@rfcbot fcp close

@rfcbot
Copy link
Collaborator

rfcbot commented Jun 14, 2018

Team member @joshtriplett has proposed to close this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once a majority of reviewers approve (and none object), 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.

@rfcbot rfcbot added proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. disposition-close This RFC is in PFCP or FCP with a disposition to close it. labels Jun 14, 2018
@repax
Copy link

repax commented Jun 14, 2018

I think a clippy lint is sufficient. For a=b to even type check in the argument position the fn must already expect the unit type. And for it to compile there must be a mut variable, a, that can be assigned b.

All that makes it highly unlikely to be unintentional. If anything, it's bad style.

I think it's in the domain of clippy to lint against syntactically correct, intentional but weird expressions.

@joshtriplett
Copy link
Member

@repax A function accepting a generic and resolving it to () seems like the more likely scenario.

@rfcbot rfcbot added the final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. label Jun 22, 2018
@rfcbot
Copy link
Collaborator

rfcbot commented Jun 22, 2018

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

@rfcbot rfcbot removed the proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. label Jun 22, 2018
@JelteF
Copy link

JelteF commented Jun 29, 2018

I would like to share my disagreement with closing this RFC. I do agree that this RFC is similar enough to the ones that reserve keywords, that the same reasoning that @aturon articulated applies here as well

However, I feel that (for the moment) there's a big flaw in the argument he made on the throw/fail RFC (#2441 (comment)). He states:

A key point is how confident we feel that we can add some reasonable mechanism for within-edition reservations (like I proposed earlier). I'm not aware of strong opposition to an idea along these lines, but as always it's hard to predict how things will play out.

@nikomatsakis and I felt particularly strongly that up-front reservations are wrong and a mistake in the initial Edition proposal, basically for the reasons I've already outlined in the thread: they force up-front decisions about surface issues of features that are not yet fully proposed, let alone accepted or implemented.

To me it seems that this argument contradicts itself. It's very easy to turn the reasoning around this way:
It's an up-front decision to close RFCs (that reserve syntax and keyword arguments) because of some not yet fully proposed, let alone accepted feature (the linked proposal from @aturon).

Once this proposal from @aturon is actually an accepted RFC I completely agree that we can close all these issues about syntax and keyword reservations. Right now this is (afaik) not the case. To me it would be a shame to not have this syntax available until the next Rust edition, because no solution to adding keywords and syntax changes inbetween rust editions was accepted.

So to me there's two solutions (which I'm both fine):

  1. Create and accept a proposal to do syntax and keyword changes in between editions
  2. Don't close (and possibly accept) this RFC for this reason

P.S. It would have been better if I would have brought this up in the previous RFC, where the original argument was made. However, I don't really follow the language design closely, but my eye was caught on this by a comment on reddit on the "This Week In Rust". I do hope this concern is still taken seriously though.

PPS. Is there some way that I should let rfcbot know about this concern or is commenting like this enough.

@joshtriplett
Copy link
Member

@JelteF The argument we're making is "this doesn't need to be tied to the edition". Given a concrete proposal we wanted to accept that uses this syntax for keyword arguments, we could make that transition with an explicit opt-in even without an edition, and then just get to a point where a future edition does it by default. So this wouldn't need to wait for a future edition, and doesn't need to preemptively reserve the syntax.

@JelteF
Copy link

JelteF commented Jun 29, 2018

we could make that transition with an explicit opt-in even without an edition, and then just get to a point where a future edition does it by default.

Like I said, I would agree with the argument that you're making. IF there would be an actual accepted RFC for the process of adding these changes outside of an edition. As far as I can tell there is none. So using that as part of your reasoning against this RFC is at the moment just as weak an argument as using a possible future "keyword argument RFC" as an argument in favor of this RFC.

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this RFC. and removed final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. labels Jul 2, 2018
@rfcbot
Copy link
Collaborator

rfcbot commented Jul 2, 2018

The final comment period, with a disposition to close, as per the review above, is now complete.

By the power vested in me by Rust, I hereby close this RFC.

@rfcbot rfcbot added closed This FCP has been closed (as opposed to postponed) and removed disposition-close This RFC is in PFCP or FCP with a disposition to close it. labels Jul 2, 2018
@rfcbot rfcbot closed this Jul 2, 2018
@JelteF
Copy link

JelteF commented Jul 2, 2018

Should this really be closed when there has been no response regarding the concern I posted?

@aturon
Copy link
Member

aturon commented Jul 2, 2018

@JelteF Ugh yeah, that's the pitfall of automatic closure by the bot. Sorry I didn't have a chance to reply sooner.

I understand the argument you're making, but it also supposes that the RFCs are equivalent, in terms of consensus needed etc. The language team has talked extensively about keyword reservation at this point, and while they don't have agreement about the features that may need new keywords, they do have agreement about the need for a mechanism to introduce new keywords within an Edition. That doesn't make it a sure bet, of course, but as I summarized on the other thread, it has proved untenable to try to bikeshed keyword selection for features we haven't even agreed should exist.

@JelteF
Copy link

JelteF commented Jul 2, 2018

Thanks for the response. I guess that makes sense. I at least definitely agree that it would be super nice to have a mechanism for creating new keywords and breaking language changes in between editions.

However, I do think it would be good not to wait too long with that RFC though. Because any RFCs that needs a new keyword would be blocked on it.

@aturon
Copy link
Member

aturon commented Jul 2, 2018

However, I do think it would be good not to wait too long with that RFC though. Because any RFCs that needs a new keyword would be blocked on it.

Indeed, that's a great point! Probably we should wait until after the Edition ships before opening the discussion, but should do it ASAP after that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed This FCP has been closed (as opposed to postponed) finished-final-comment-period The final comment period is finished for this RFC. T-lang Relevant to the language team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.