I think we'll probably also want to add some other test scenarios as well, including with this enabled alongside some other config opts. For example, a test that also has indent_style set to Visual with a function signature that has to be formatted across multiple lines to validate the visual alignment of the params and prevent any regressions

I'll add more tests once the implementation is finalized. I already have some that use these options with max_width.

I'll add more tests once the implementation is finalized. I already have some that use these options with max_width.

Awesome thank you. I went ahead and did a quick spot check that the spacing in the signature didn't impact the parameter alignment with "Visual" indent style so will give the updated implementation another pass shortly

Thanks @calebcartwright. Were you able to make the other pass and see if the implementation is ready for more tests?

Thanks @calebcartwright. Were you able to make the other pass and see if the implementation is ready for more tests?

Not yet unfortunately. We've got a high priority broken toolstate issue that's occupying my rustfmt bandwidth ATM

Ok, take your time

Apologies for the delay @dylni. I feel like this is heading in a good direction and have left some inline feedback for your review.

I am ok with adding fn_no_generics_space, provided we rename it to something more general. For fn_generic_spaces, I prefer to remove it from this PR.

Are you thinking we should have the option apply only spacing to fn sigs that have no generics, and generics will still be foo<T>(bar: T)?

the benefit of having these options does not worth the complexity of interface and implementation.

I can see why you're saying this, but almost all of the complexity is coming from fn_generics_spacing = OnlyBefore. It's unusual to have a space between an ident and generics, so there was no existing support.

I don't think fn_generics_spacing needs to be removed to fix the complexity, but OnlyBefore and BeforeAndAfter can be removed initially.

@dangerbarber and @bugeats Are you still interested in having a space before generics too?

I don't think fn_generics_spacing needs to be removed to fix the complexity, but OnlyBefore and BeforeAndAfter can be removed initially.

IMO it'd be good to get clarity on what spacing style variants you think we should allow @topecongiro

Within the original issue there were several different spacing options requested and discussed.

We obviously have to support the following as default no spacing is the (by far) most commonly used style that's required in the style guide:

fn foo() {}
fn foo<T>(bar: T) {}

Shortened snippets from new spacing options requested in #3564 (comment)

fn foo () {}
fn foo <T> (bar: T) {}

Additional ask from lower in that thread at #3564 (comment)

fn foo<T> (bar: T) {}

I still believe all of these style variants to be rather niche in Rust, but if we're going to support one of them then I personally struggle to justify not supporting the others; it's a "why this but not that" question I can't really answer 🤷‍♂️

I prefer to have a single configuration option, which, when is set to true, add spaces after the fn identifier. If the fn happens to have generics, then add spaces after generics instead of the identifier.

Note the last example in true: if the generic clause is multi-lined, then we should not add a space.

false (default)

fn foo() {
    // ...
}
fn foo_with_multi_lined(
    // mutli-lines parameters
    a: u32,
    b: u32,
    c: u32,
) {
    // ...
}
fn foo<T>(bar: T) {
    // ...
}
fn foo<T>(
    // mutli-lines parameters
    a: T,
    b: T,
    c: T,
) {
    // ...
}

true

fn foo () {
    // ...
}
fn foo_with_multi_lined (
    // mutli-lines parameters
    a: u32,
    b: u32,
    c: u32,
) {
    // ...
}
fn foo<T> (bar: T) {
    // ...
}
fn foo<T> (
    // mutli-lines parameters
    a: T,
    b: u32,
    c: u32,
) {
    // ...
}
fn foo<
    // One should use a where clause for large generics, really
    T: Foo + Bar,
    F: FooBar,
>(
    // mutli-lines parameters
    a: T,
    b: u32,
    c: u32,
) {
    // ...
}

I prefer to have a single configuration option, which, when is set to true, add spaces after the fn identifier. If the fn happens to have generics, then add spaces after generics instead of the identifier.

That SGTM, and if I'm not mistaken that's the style you originally preferred as well @dylni. This approach would ensure consistency between generics in fns with other generic usage, which is one way we could justify this change without the granular lhs/rhs spacing on the generics.

That SGTM, and if I'm not mistaken that's the style you originally preferred as well

Yes @calebcartwright. I wanted to wait for you to comment, because I'm biased. The PR now uses the simpler interface

@topecongiro Why not add a space when the function's multilined? I would expect it to make implementation more complicated and a little inconsistent

@dylni My first thought was that it would be more consistent with rustfmt other styles if we don't add a space between the closing > and the opening (. On second thought, I am more feeling like there is no problem with either, so let's keep the current implementation, which simplifies the code.

I don't have any objections to this. I still think there could be value in having something along the lines of sig in the option name, but I don't feel strongly enough that we should hold this up.

I suppose this new config option will be released alongside the new fn_call_width option so perhaps from a naming convention going forward we can assume options that have call in the name are applied on function calls, and those that don't are applied on signatures.

I suppose this new config option will be released alongside the new fn_call_width option so perhaps from a naming convention going forward we can assume options that have call in the name are applied on function calls, and those that don't are applied on signatures.

This is actually a good point.

@dylni Would you mind renaming the configuration option to space_before_fn_sig_paren? We are sorry for requesting many updates on this PR, this should be the last one.

For example, a test that also has indent_style set to Visual with a function signature that has to be formatted across multiple lines to validate the visual alignment of the params and prevent any regressions

There's now a test for indent_style

Would you mind renaming the configuration option to space_before_fn_sig_paren?

Fixed

We are sorry for requesting many updates on this PR, this should be the last one.

No problem. Thank you and @calebcartwright for the reviews

Awesome, thank you @dylni 🎉

Thanks 🎉

Also want to note that this new option will of course be available if you build rustfmt from source, though do want to note it'll probably be a few release cycles yet before it's available in rustfmt obtained via rustup

Thanks everybody for working on this! I'll go ahead and close the tracking issue now.

Was this feature removed? I can't find references in the codebase and trying to use the option gives an "Unknown configuration option" in the latest nightly

Was this feature removed? I can't find references in the codebase and trying to use the option gives an "Unknown configuration option" in the latest nightly

You should always look at the config site (https://rust-lang.github.io/rustfmt/?version=v1.4.38&search=) and select the version of rustfmt that you are using in order to see the available configuration options in your version. Unless you're compiling rustfmt from source I'd advise against looking at what's in source control.

This particular option hasn't been ported to a released version of rustfmt yet, which as an fyi, is what the backport-triage and/or 1x-backport:pending labels are used to indicate

Modify VSCode's code highlighting regardless of function definition

async patch_rust () {
    const fp_rust = 'C:/Program Files/Microsoft VS Code/resources/app/extensions/rust/syntaxes/rust.tmLanguage.json'
    
    let config = await fread_json(fp_rust)
    
    let funcdef = config.repository.functions.patterns[1]
    assert(funcdef.comment === 'function definition')
    
    // original: \\b(fn)\\s+((?:r#(?!crate|[Ss]elf|super))?[A-Za-z0-9_]+)((\\()|(<))
    // add ' ?' after function name
    funcdef.begin = '\\b(fn)\\s+((?:r#(?!crate|[Ss]elf|super))?[A-Za-z0-9_]+) ?((\\()|(<))'
    
    await fwrite(fp_rust, config)
}

Add function name in function definition with black bold and highlight

"editor.tokenColorCustomizations": {
    "textMateRules": [
        {"scope": "entity.name.function", "settings": {"foreground": "#000000", "fontStyle": "bold"}},
        {"scope": "support.function", "settings": {"foreground": "#000000", "fontStyle": "bold"}} ,
    ]
},

Restart VSCode and you will get

For https://doc.rust-lang.org/book/

span.hljs-function::after
    content: ' '

if (location.hostname.includes('doc.rust-lang.org')) 
    $$('code.language-rust span.hljs-function').forEach($fn => {
        let sibling = $fn.nextSibling
        if (sibling.nodeType === Node.TEXT_NODE)
            sibling.textContent = sibling.textContent.replace(/<(.*)>\((.*)\) {/, '<$1> ($2) {')
    })

function $$ <TElement extends Element> (selector: string) {
    return document.querySelectorAll<TElement>(selector)
}