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: remove `do` #10815

Closed
chris-morgan opened this Issue Dec 5, 2013 · 12 comments

Comments

Projects
None yet
7 participants
@chris-morgan
Member

chris-morgan commented Dec 5, 2013

I started a mailing list thread on this topic about a week ago; in the first post I summarised the situation. I'll quote it here verbatim:

(Do I win the prize for the shortest thread name yet?)

error: last argument in do call has non-procedure type: ||

In the past three or four days I've seen at least as many enquiries in #rust about this error, and I'm sure there have been at least several others while I haven't been monitoring it. This is evidently causing quite a bit of confusion.

Here's a summary of the change. The syntax is the same as it was before:

do expr { block }
do expr |args| { block }
do expr(args) { block }
do expr(args) |args| { block }

These used to desugar to the following, respectively:

expr(|| { block })
expr(|args| { block })
expr(args, || { block })
expr(args, |args| { block })

These now desugar to the following, respectively:

expr(proc() { block })
expr(proc(args) { block })
expr(args, proc() { block })
expr(args, proc(args) { block })

The change is that it now accepts a procedure rather than a closure. No syntax change, just a semantics change which breaks a lot of code.

Closure: a stack function; used to be &fn(..) -> _, is now |..| -> _. Can be called multiple times, requires no allocations and is not Send.

Procedure: a heap function; used to be ~once fn(..) -> _, is now proc(..) -> _. Can be called once, requires heap allocation and is Send.

Procedures are good for sending cross-task; things like the task body are a good match. Still, I think there are a few problems with how things are at present (i.e. after the do semantics change):

  1. do is still using the syntax of a closure (|..| { .. }), despite it now being a procedure.
  2. All of a sudden, things using closures need to shift away from using do or use procedures; this is causing confusion and may cause bad design decisions where nice sugar triumphs over what is actually needed; often the best solution may not be clear. (I, for example, had not thought about the fact that proc was going to allocate; the ~once fn name was clearer about that. I'll speak about &once fn another time. Don't mention it now, this thread is just about do.)

I have two solutions that I think could answer these concerns. Leaving it as it is seems a bad idea to me.

(a) Kill do

I've had mixed feelings about do. Overall, it's pretty trivial syntax sugar, but it's sugar of a dubious sort, because it changes something that looks like a function call with N arguments to be a function call with N+1 arguments. That's just a matter of learning it.

Still, do is nice sugar in the way it gets rid of parentheses at the end. Overall, is it worth it? I don't know.

Once do is gone, there's no problem left: just remove the sugar everywhere it was used and everything works and will do for the foreseeable future.

(b) Make do support both closures and procedures

The syntax of do can be clearly seen to include the closure syntax. We could easily extend it to support both closures and procedures.

Here is a proposed do using closures once more, keeping the syntax it had last week:

do expr || { block }
do expr(args) || { block }
do expr |args| { block }
do expr(args) |args| { block }

Here is a proposed do using procedures as the current behaviour is, but with new syntax which is clearly a procedure:

do expr proc() { block }
do expr(args) proc() { block }
do expr proc(args) { block }
do expr(args) proc(args) { block }

This does leave these cases which are currently valid unclear:

do expr { block }
do expr(args) { block }

The options for this are (a) disallowing it; (b) making it always of the function types; and (c) inferring the type. I generally prefer the last solution but it is the most difficult. I'm not sure how it all fits into the function traits stuff at all.

Incidentally, all this leaves the possibility open of making do work for any argument type, where do expr1 expr2 simply desugars to expr1(expr2) and do expr1(args) expr2 to do expr1(args, expr2). I don't know if that would be a good thing or not; it's probably best to avoid discussion of that at present.

Summary

Leaving do in its present form seems to me a distinctly bad idea, with the syntax of one form of function while it uses another form of function. I think we need to redo do very soon. (I'd save this joke for later in the thread, but I'm afraid someone else might steal it. I expect all responses to indicate they're in favour of this by using the title "Re: do" :P.)

For myself, I have no preference to indicate; I am torn between the two options.

After some discussion, I think that for the moment, removing do is probably the better option. We can then see about adding it or something similar back later if we want to.

So then, concrete proposal: remove do from the language.

@liigo

This comment has been minimized.

Show comment
Hide comment
@liigo

liigo Dec 6, 2013

Contributor

No, don't remove do before you have better syntax. Fn({}) is too ugly to accept.

Contributor

liigo commented Dec 6, 2013

No, don't remove do before you have better syntax. Fn({}) is too ugly to accept.

@adrientetar

This comment has been minimized.

Show comment
Hide comment
@adrientetar

adrientetar Dec 8, 2013

Contributor

I've often felt like do was not useful enough to be worth-being, admittedly.

Contributor

adrientetar commented Dec 8, 2013

I've often felt like do was not useful enough to be worth-being, admittedly.

@brson

This comment has been minimized.

Show comment
Hide comment
@brson

brson Dec 10, 2013

Contributor

I think we should remove it at this point. I liked it better when it worked for stack closures.

Contributor

brson commented Dec 10, 2013

I think we should remove it at this point. I liked it better when it worked for stack closures.

@hatahet

This comment has been minimized.

Show comment
Hide comment
@hatahet

hatahet Dec 10, 2013

Contributor

@brson Is it worth making spawn a built-in function in order to keep the nice syntax we currently get with do? It seems that this is the main point of contention for keeping do.

Contributor

hatahet commented Dec 10, 2013

@brson Is it worth making spawn a built-in function in order to keep the nice syntax we currently get with do? It seems that this is the main point of contention for keeping do.

@chris-morgan

This comment has been minimized.

Show comment
Hide comment
@chris-morgan

chris-morgan Dec 10, 2013

Member

@hatahet Do you mean making it a built-in keyword with syntax of its own? As it stands, it is in the prelude which means it's a built-in function. When do is removed (which I'm working on), it will just operate as

spawn(proc() {
    ...
})

That is not a horribly ugly monstrosity. Sure, it's not quite as nice-looking as do spawn { ... }, but it's not bad and is clear. I believe there will be general agreement in the core team that spawn should not be made a separate keyword (spawn { ... }); there are other ways of spawning a task (e.g. if you manually construct a task object you have task.spawn(proc() { ... }). And any syntax which could cope with all the cases would be exactly what we're looking at removing now.

Member

chris-morgan commented Dec 10, 2013

@hatahet Do you mean making it a built-in keyword with syntax of its own? As it stands, it is in the prelude which means it's a built-in function. When do is removed (which I'm working on), it will just operate as

spawn(proc() {
    ...
})

That is not a horribly ugly monstrosity. Sure, it's not quite as nice-looking as do spawn { ... }, but it's not bad and is clear. I believe there will be general agreement in the core team that spawn should not be made a separate keyword (spawn { ... }); there are other ways of spawning a task (e.g. if you manually construct a task object you have task.spawn(proc() { ... }). And any syntax which could cope with all the cases would be exactly what we're looking at removing now.

@hatahet

This comment has been minimized.

Show comment
Hide comment
@hatahet

hatahet Dec 10, 2013

Contributor

@chris-morgan Yep, that's what I was referring to. You're right -- I guess spawn(proc() { }); doesn't look too bad.

Contributor

hatahet commented Dec 10, 2013

@chris-morgan Yep, that's what I was referring to. You're right -- I guess spawn(proc() { }); doesn't look too bad.

chris-morgan added a commit to chris-morgan/rust that referenced this issue Dec 15, 2013

Remove usage of `do` keyword, in line with #10815.
I haven't removed the keyword yet; this is just a matter of stopping
using it. Changing the `do` keyword to complaining of obsolete syntax is
the next step.

Also I have *not* removed the `do` keyword tests; they still use `do`.
These are:

- src/test/compile-fail/do-lambda-requires-braces.rs
- src/test/compile-fail/do1.rs
- src/test/compile-fail/do2.rs
- src/test/compile-fail/issue-3044.rs
- src/test/run-pass/block-arg-can-be-followed-by-binop.rs
- src/test/run-pass/block-arg-can-be-followed-by-block-arg.rs
- src/test/run-pass/block-arg-can-be-followed-by-call.rs
- src/test/run-pass/block-arg-in-parentheses.rs
- src/test/run-pass/block-arg-used-as-any.rs
- src/test/run-pass/block-arg.rs
- src/test/run-pass/do-empty-args.rs
- src/test/run-pass/do-no-args.rs
- src/test/run-pass/do1.rs
- src/test/run-pass/do2.rs
- src/test/run-pass/do3.rs
@brendanzab

This comment has been minimized.

Show comment
Hide comment
@brendanzab

brendanzab Jan 2, 2014

Member

I've been de-do-ifying most of my code, and it it's actually not as bad as I thought. It definitely would simplify the language more to remove it, which is a plus in my books.

Member

brendanzab commented Jan 2, 2014

I've been de-do-ifying most of my code, and it it's actually not as bad as I thought. It definitely would simplify the language more to remove it, which is a plus in my books.

@brson

This comment has been minimized.

Show comment
Hide comment
@brson

brson Jan 25, 2014

Contributor

Nominating.

Contributor

brson commented Jan 25, 2014

Nominating.

@bytbox

This comment has been minimized.

Show comment
Hide comment
@bytbox

bytbox Jan 27, 2014

Contributor

In light of the recent email, I've taken this up as a relatively-easy but apparently-helpful task.

Contributor

bytbox commented Jan 27, 2014

In light of the recent email, I've taken this up as a relatively-easy but apparently-helpful task.

@brson

This comment has been minimized.

Show comment
Hide comment
@brson

brson Jan 27, 2014

Contributor

@hatahet I think we're more likely to prettify spawn with a macro.

spawn! {
    ...
}

Is pretty hot, though I think the braces don't work in that position right now.

Contributor

brson commented Jan 27, 2014

@hatahet I think we're more likely to prettify spawn with a macro.

spawn! {
    ...
}

Is pretty hot, though I think the braces don't work in that position right now.

@brson

This comment has been minimized.

Show comment
Hide comment
@brson

brson Jan 27, 2014

Contributor

@bytbox Awesome! Ping me or anybody on #rust-internals if you need some guidance.

Contributor

brson commented Jan 27, 2014

@bytbox Awesome! Ping me or anybody on #rust-internals if you need some guidance.

@hatahet

This comment has been minimized.

Show comment
Hide comment
@hatahet

hatahet Jan 27, 2014

Contributor

@brson Great to hear :)

Contributor

hatahet commented Jan 27, 2014

@brson Great to hear :)

bytbox added a commit to bytbox/rust that referenced this issue Jan 28, 2014

@bytbox bytbox referenced this issue Jan 28, 2014

Merged

Remove do #11868

bytbox added a commit to bytbox/rust that referenced this issue Jan 28, 2014

bors added a commit that referenced this issue Jan 28, 2014

bytbox added a commit to bytbox/rust that referenced this issue Jan 28, 2014

@bors bors closed this in a6867e2 Jan 29, 2014

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