Add option space_before_fn_sig_paren #4302

dylni · 2020-07-01T18:20:39Z

This option adds very little code, so it would be great to have it supported.

It's an enum, because the tracking issue originally asked for a space before generics too, which is not part of this PR. This option can be extended later to add BeforeGenerics or AroundGenerics as other values. #4302 (comment)

Tracking issue: #3564

calebcartwright

Thank you for the PR @dylni! Really appreciate the willingness to jump in and propose an implementation.

Some modifications would be required for merging this however, and I've left details inline below.

calebcartwright · 2020-07-01T18:41:18Z

Configurations.md

+Leave a space after the function name.
+
+- **Default value**: `"Never"`
+- **Possible values**: `"AfterGenerics"`, `"Never"`


I'm not convinced yet that a single config option with many variants would be the best way to handle all the corresponding scenarios.

Opening this door creates several spacing options that I see:

no spaces period between the ident and opening paren

space between the ident and opening paren w/no generics

space between ident and generics and generics and paren

lhs space on the generics but none on rhs

rhs space on the generics but none on the lhs

Users would then also be able to argue that they should be able to have various combinations of these stylings, for example no spacing between the ident and paren in the absence of generics, but lhs, rhs, or both spacing in the presence of generics.

I understand you may prefer to use only one of those personally, but if we're opening up control of spacing between the ident and parens at all then we'll be forced to support the other minor spacing variants/combinations as well.

If you still feel that this is best done as a single, multi-variant config option then please update this as described above to account for all such variants and combinations to see if that approach is really viable. Otherwise please change this to be a boolean that only impacts rhs spacing in the presence of generics because space_after_function_name = AfterGenerics is really unintuitive for the non-generic scenarios

Using a boolean would make it easier to support those other styles later, except for "space between the ident and opening paren w/no generics". Should I make space_before_fn_params an enum of "Always", "Generics", and "Never" to support that style? Since it wasn't mentioned in the tracking issue, I think it's enough to keep the option as a boolean.

Should I make space_before_fn_params an enum of "Always", "Generics", and "Never" to support that style? Since it wasn't mentioned in the tracking issue, I think it's enough to keep the option as a boolean.

I probably need to think on this one a bit, but my initial reaction is that a boolean is probably fine. The no-generics, just a space between the name and paren scenario really is binary in nature so a boolean feels natural. It's the generics scenario that has the Never/OnlyBefore/OnlyAfter/BeforeAndAfter mix that needs the extra control.

Think one boolean option for the fn foo() vs fn foo () scenario and an enum option for fn foo<T>() vs fn foo<T> () vs etc. should cover everything, including the two explicitly requested spacing styles

All styles are covered now. Most of the code is needed to support adding a space before generics, since that style is inconsistent with other formatting for generics.

calebcartwright · 2020-07-02T02:15:56Z

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

This reverts commit 6350a15.

dylni · 2020-07-02T19:42:26Z

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

calebcartwright · 2020-07-06T01:58:55Z

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

dylni · 2020-07-10T18:29:02Z

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

calebcartwright · 2020-07-10T21:34:40Z

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

dylni · 2020-07-11T14:04:57Z

Ok, take your time

calebcartwright · 2020-07-14T01:09:59Z

Configurations.md

@@ -803,6 +803,140 @@ By default this option is set as a percentage of [`max_width`](#max_width) provi

 See also [`max_width`](#max_width) and [`width_heuristics`](#width_heuristics)

+## `fn_generics_space`


Just a little bikeshedding on the names, but I'm wondering if something like fn_sig_generics_spacing or fn_generics_spacing would be more explicit given we're not talking about just one space. What do you think?

I agree that "spacing" is better, but "sig" feels redundant since generics are only in the signature. I'm okay with whichever you prefer

Configurations.md

calebcartwright · 2020-07-14T01:13:17Z

Configurations.md

+
+See also [`fn_no_generics_space`](#fn_no_generics_space).
+
+## `fn_no_generics_space`


Same bikeshed here. What would you think about fn_sig_name_paren_space or fn_name_paren_space and then in the descriptions for the two config options (and with inline comments in example) we can re-emphasize that this one's only used in signatures that have no generics and vice versa?

I added comments inline, but I prefer fn_no_generics_spacing. I wouldn't expect fn_name_paren_space to only apply without generics. It's also not very clear that it's the complement of fn_generics_spacing

I wouldn't expect fn_name_paren_space to only apply without generics. It's also not very clear that it's the complement of fn_generics_spacing

I always try to think about folks using config options they aren't terribly familiar with and doing so either via cli override or from their editor in a rustfmt config file vs. always being on the config documentation page, and that's why I generally prefer the names to be as explicit and self-explaining as possible.

My original thought was that by itself, the name just hints at some spacing for a function signature that lacks generics vs. explicitly whether it'd inject a space between the ident of the function name and the opening paren. I was trying to think of something would more clearly indicate what the config option did and not just when it would do something.

I do take your other point though, so let's see how the broader discussion in this PR plays out and then maybe we can iterate a bit more on the name to see if we can address both concerns 👍

that's why I generally prefer the names to be as explicit and self-explaining as possible.

I agree. This is very helpful

let's see how the broader discussion in this PR plays out and then maybe we can iterate a bit more on the name to see if we can address both concerns

👍

Configurations.md

src/config.rs

calebcartwright · 2020-07-14T01:29:50Z

src/formatting/items.rs

@@ -916,7 +916,7 @@ fn format_impl_ref_and_type(
        result.push_str(format_unsafety(unsafety));

        let shape = Shape::indented(offset + last_line_width(&result), context.config);
-        let generics_str = rewrite_generics(context, "impl", generics, shape)?;
+        let generics_str = rewrite_generics(context, "impl", generics, shape, false)?;


I feel like adding a boolean to the signature for rewrite_generics is shifting some of the aspects of formatting generics to the callers 🤔

What if instead the new param added to rewrite_generics was an ItemKind and then internally rewrite_generics could deal with enabling/disabling the spacing based on the combination of the ItemKind and config options.

IIRC not all of these functions that internally call rewrite_generics have the original AST Item node, but they could either pass the corresponding ItemKind variant to rewrite_generics or could themselves take an extra param

calebcartwright · 2020-07-14T01:31:06Z

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

topecongiro

Sorry for the late review, and thank you both @dylni and @calebcartwright for taking to time to work and review this PR. However, I have to say that I am not in favor of the direction this PR is heading.

I don't think it's a good idea to have many configuration variants for controlling spaces around function names; the benefit of having these options does not worth the complexity of interface and implementation.

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.

calebcartwright · 2020-07-14T04:53:50Z

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)?

dylni · 2020-07-14T18:46:18Z

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?

calebcartwright · 2020-07-14T20:28:43Z

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 🤷‍♂️

topecongiro · 2020-07-16T01:41:16Z

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,
) {
    // ...
}

calebcartwright · 2020-07-17T00:10:32Z

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.

dylni · 2020-07-17T14:10:17Z

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

topecongiro · 2020-07-21T02:33:22Z

@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.

calebcartwright · 2020-07-21T02:55:24Z

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.

topecongiro · 2020-07-22T03:24:49Z

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.

dylni · 2020-07-23T01:50:58Z

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

calebcartwright · 2020-07-24T03:52:36Z

Awesome, thank you @dylni 🎉

dylni · 2020-07-24T14:14:37Z

Thanks 🎉

calebcartwright · 2020-07-24T14:23:18Z

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

RaidoWolf · 2020-07-24T20:58:32Z

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

m-hilgendorf · 2021-12-06T04:40:10Z

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

calebcartwright · 2021-12-13T02:52:55Z

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

ShenHongFei · 2023-09-08T12:42:32Z

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)
}

Add option space_after_function_name

a626b1e

dylni mentioned this pull request Jul 1, 2020

Add option to put a space after function name and generic parameters #3564

Closed

calebcartwright requested changes Jul 1, 2020

View reviewed changes

Rename option and change type to bool

6350a15

dylni changed the title ~~Add option space_after_function_name~~ Add option space_before_fn_params Jul 1, 2020

dylni added 2 commits July 2, 2020 15:33

Revert "Rename option and change type to bool"

75ccb9e

This reverts commit 6350a15.

Cover other styles

fff36d2