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

Function body blocks #3629

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

joshtriplett
Copy link
Member

@joshtriplett joshtriplett commented May 7, 2024

Co-authored-by: Eric Holk

Rendered

Co-authored-by: Eric Holk <eric.holk@gmail.com>
@joshtriplett joshtriplett added the T-lang Relevant to the language team, which will review and decide on the RFC. label May 7, 2024
@kennytm
Copy link
Member

kennytm commented May 7, 2024

is this supposed to be read together with #3628?

@joshtriplett
Copy link
Member Author

Both have value independently, and one could be approved without the other, but yes, the two proposals both make each other better.

@davidbarsky
Copy link

davidbarsky commented May 7, 2024

A downside of this proposal that I don't see mentioned is that it would make error recovery more difficult in IDEs like rust-analyzer or RustRover. Delimiters like parentheses/curly brackets make it trivial for IDEs to assume boundaries between snippets of incomplete code, but this RFC would throw a wrench into such approaches. I don't think the aesthetic wins outweigh the tooling downsides, especially for new Rust programmers.

@VitWW
Copy link

VitWW commented May 7, 2024

It will be ok for full "block" expressions:

  • unsafe
  • loop
  • while and while let
  • for
  • async
  • match
  • if and if let, if and only if there's no else.
  • try (once it exists)
  • gen (once it exists)
  • async gen (once it exists)

@kennytm
Copy link
Member

kennytm commented May 7, 2024

i think this will cause some longer-than-expected sequences before disambiguation can be resolved:

fn g<T>() where T: for

the for here may be the looping expression or the HRTB quantifier

fn g<T>() where T: for _ in [0] {}
fn g<T>() where T: for<'a> Fn(&'a u8) {}

(with rust-lang/rust#86935 you have to consume the < as well before determining whether it is HRTB or expression.)

I suspect that with #3628 that async T being a type would cause some parsing issue in the where clause against async {} block too.

@joshtriplett
Copy link
Member Author

@davidbarsky wrote:

A downside of this proposal that I don't see mentioned is that it would make error recovery more difficult in IDEs like rust-analyzer or RustRover. Delimiters like parentheses/curly brackets make it trivial for IDEs to assume boundaries between snippets of incomplete code, but this RFC would throw a wrench into such approaches.

This (for both humans and tools) is exactly why I'm proposing to only allow this for constructs that accept a single block. Allowing an arbitrary expression would have the problem you're describing. Allowing e.g. try { ... } or gen { ... } doesn't seem like it would be substantially more complex than the existing { ... }.

@joshtriplett
Copy link
Member Author

@kennytm wrote:

i think this will cause some longer-than-expected sequences before disambiguation can be resolved:

fn g<T>() where T: for

the for here may be the looping expression or the HRTB quantifier

fn g<T>() where T: for _ in [0] {}
fn g<T>() where T: for<'a> Fn(&'a u8) {}

(with rust-lang/rust#86935 you have to consume the < as well before determining whether it is HRTB or expression.)

In this case, the parser should know that it's expecting a type rather than a function body, right?

And even if not, as you said, after a couple of tokens you know what you have.

I suspect that with #3628 that async T being a type would cause some parsing issue in the where clause against async {} block too.

AFAICT you would know one symbol after async, the moment you see {. Also, I would think that the parser state should know whether it's expecting a type or the possible start of a function, but perhaps there's an ambiguous case.

@kennytm
Copy link
Member

kennytm commented May 7, 2024

@joshtriplett

In this case, the parser should know that it's expecting a type rather than a function body, right?

No, T: is a perfectly fine WhereClauseItem (it just asserts T is well-formed). The code below runs fine today.

fn g<T>() where T: {
    for _ in [0] {}
}

Similar issue with empty where clause:

fn g<T>() where   for<'a> fn(&'a T): Sized {}
fn g<T>() where { for _ in [0] {} }

And even if not, as you said, after a couple of tokens you know what you have.

Well #3629 (comment) explained it quite clearly.

@kennytm
Copy link
Member

kennytm commented May 7, 2024

yeah I know the inner attribute is bad style but will the following apply the attribute to the function item itself?

fn X()
unsafe {
    #![allow(non_snake_case)]
}

and if we want to add an attribute to the block is it still like this?

fn X()
#[allow(unsafe_code)]
unsafe {
}

@traviscross traviscross added the T-style Relevant to the style team, which will review and decide on the RFC. label May 7, 2024
@traviscross
Copy link
Contributor

cc @rust-lang/style

@davidbarsky
Copy link

@davidbarsky wrote:

A downside of this proposal that I don't see mentioned is that it would make error recovery more difficult in IDEs like rust-analyzer or RustRover. Delimiters like parentheses/curly brackets make it trivial for IDEs to assume boundaries between snippets of incomplete code, but this RFC would throw a wrench into such approaches.

This (for both humans and tools) is exactly why I'm proposing to only allow this for constructs that accept a single block. Allowing an arbitrary expression would have the problem you're describing. Allowing e.g. try { ... } or gen { ... } doesn't seem like it would be substantially more complex than the existing { ... }.

What Kenny said in #3629 (comment). Malformed/incomplete code—even along those lines—is really common. It's not necessarily more difficult to change a parser, but the code as-it-exists (or rather, as it is written by people) would mean that they'd have a small, but perceptible degradation in their IDE.

If you're to consider a syntactic change like this, I think something like the following:

fn foo(x: i32) -> i32 = gen {
    todo!()
};

...would sidestep most of the incremental parsing issues.

@kennytm
Copy link
Member

kennytm commented May 9, 2024

fn foo(x: i32) -> i32 = gen {
    todo!()
};

this would be #3369

@clarfonthey
Copy link
Contributor

I really don't like this solution for, most of the reasons folks have mentioned. It feels too close to the braceless C statements that Rust tried so hard to avoid, and it just looks weird to me.

I get all the motivation for this, but I don't think that the solution is satisfactory. I would much prefer just indenting my functions one more level and dealing with the consequences of that than adding in new syntax, unless we do find that the equals-expression syntax doesn't have the issues that were brought up in previous discussions.

I'm basically assuming that the indenting is the main issue here, since typing braces is relatively trivial. The language is a balancing act of tradeoffs and I think that this one is extremely minor and not worth introducing entirely new syntax for.

@belkadan
Copy link

belkadan commented May 9, 2024

I personally think it particularly doesn't read very well when the function header wraps onto multiple lines:

fn example(
  x: AVeryLongTypeNameThatPushesTheSignatureOntoMultipleLines,
  y: u32
) -> u32
match x {
  _ => y
}

@tmccombs
Copy link

tmccombs commented May 9, 2024

Another prior art is scala , where the = is actually required, and the RHS can be any expression (in fact a body with braces is just a specific case of that, since such a body is itself an expression).

I don't have a strong opinion on it if there is a delimeter such as =, but without it, I think the body feels disjointed from the signature, and it isn't visually apparent the two are part of the same item.

@SOF3
Copy link

SOF3 commented May 9, 2024

could I argue that this is ultimately a rustfmt issue, that it should prefer styles that squeeze short block wrappers on the same line or indent level?

in other words: why do we need to introduce a separate syntax rather than simply changing the way code gets formatted? saving a pair of curly braces has no practical benefit other than instructing the formatter not to increase the indent level.

@N4tus
Copy link

N4tus commented May 9, 2024

Putting the body block as well as the closing '}' on the same line avoids the indentation, while only beeing a style change:

fn example(
  x: AVeryLongTypeNameThatPushesTheSignatureOntoMultipleLines,
  y: u32
) -> impl Iterator<Item = u32> { gen {
  yield x.0;
  yield y;
}}

You could even put the gen block declaration on the next line:

fn example(
  x: AVeryLongTypeNameThatPushesTheSignatureOntoMultipleLines,
  y: u32
) -> impl Iterator<Item = u32> { 
gen {
  yield x.0;
  yield y;
}}

Also in this motivating example:

fn foo() -> NamedFutureType
async {
    ...
}

how would you convert the async block to the user-specified NamedFutureType?

@kennytm
Copy link
Member

kennytm commented May 9, 2024

Also in this motivating example:

fn foo() -> NamedFutureType
async {
    ...
}

how would you convert the async block to the user-specified NamedFutureType?

that's how TAIT works...

#![feature(type_alias_impl_trait)]

use std::future::Future;

type NamedFutureType = impl Future<Output = ()>;

fn foo() -> NamedFutureType {
    async {
    }
}

@slanterns
Copy link
Contributor

slanterns commented May 10, 2024

I think at least the alternative with a = looks much more better than the current proposal: https://theincredibleholk.org/blog/2023/12/15/rethinking-rusts-function-declaration-syntax/.

@fbenkstein
Copy link

How does if / if let without else work? Is the function only allowed to return unit ()?

@clarfonthey
Copy link
Contributor

How does if / if let without else work? Is the function only allowed to return unit ()?

The combined ifs and elses are all considered a single block, so, those are fine. All of these functions can return a type, as mentioned in the examples.

@fbenkstein
Copy link

This is from the RFC text:

The full list of block constructs permitted at the top level of a function:
...

  • if and if let, if and only if there's no else.

The combined ifs and elses are all considered a single block, so, those are fine.

I don't understand how these two statements fit together. Maybe it should say "if and only if there is an else"?

@Yokinman
Copy link

Even if it's possible to have an expression follow the signature unambiguously I would still prefer some kind of separator. It seems more future-proof, and it would definitely make it easier to read simpler single-line functions.

Human Visual Parsing

If formatted poorly, the block construct could "disappear" into the function signature. We recommend that the default Rust style use a newline to separate the type from the block, making this visually straightforward to parse.

This seems to be the only problem with using a separator - Rust style prefers line breaks before operators and with extra indentation, which would render the purpose of this moot.

fn countup(limit: usize) -> impl Iterator<Item = usize>
    = gen {
        for i in 0..limit {
            yield i;
        }
    };
    
// vs
fn countup(limit: usize) -> impl Iterator<Item = usize> {
    gen {
        for i in 0..limit {
            yield i;
        }
    }
}

I think we can all agree that eliding a layer of indentation inside a block is never gonna be an accepted style change, since it breaks the intuitive reading that the beginning and ending of any scope will have corresponding indentation. Now you'd have to match brackets yourself, or maybe your IDE will highlight them for you:

fn countup(limit: usize) -> impl Iterator<Item = usize> {
gen {
    for i in 0..limit {
        yield i;
    }
}}

Maybe it could be styled like match arms, where the line break occurs after the arrow?

fn countup(limit: usize) -> impl Iterator<Item = usize> =>
gen {
    for i in 0..limit {
        yield i;
    }
}

On the other hand, obviously putting the keyword before fn solves this by definition, but is it really important for the keyword to be immediately visible if the return type is now explicit?

@petrochenkov
Copy link
Contributor

I would prefer the

fn f() = EXPR;

alternative, if we are doing this at all.

It can work for both block-like bodies

fn f() = match {
    // arms
};

and for very short bodies

fn f(x: u8) = x + 1;

I'm pretty sure there was an old closed RFC about this (by Centril?), but I cannot find it now.

@joshtriplett
Copy link
Member Author

@davidbarsky made a compelling case that parsing would be substantially easier with the = separator. Given that, I'll switch the RFC over to propose that instead.

@compiler-errors
Copy link
Member

@joshtriplett: Given the amount of feedback this RFC this has received, I would prefer if that were opened as a separate RFC PR. It's a totally separate proposal, so it would be nice to give people a separate, clean slate to react to the new proposal.

@kennytm
Copy link
Member

kennytm commented May 15, 2024

please address the arguments from #3369 in that new RFC

@SOF3
Copy link

SOF3 commented May 16, 2024

would also be worth considering if anyone prefers a rustfmt-based approach, which involves no compiler change and only has the disadvantage of double closing brace (which is not worse than nested blocks in function calls ({ \n...\n })). This is to add a nightly rustfmt option that allows collapsing the outermost nesting block into the first line if it is short and/or without where bounds:

fn short_fn() { async {
    // ...
}}

fn a_long_line(of: Arguments) -> And<Return, Types> {
    async {
        // ...
    }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-lang Relevant to the language team, which will review and decide on the RFC. T-style Relevant to the style team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet