-
Notifications
You must be signed in to change notification settings - Fork 898
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
allow optional programmer discretion on chain length/function arguments per line #4306
Comments
So, rustfmt doesn't really have the concept of "programmer discretion"; it's mostly intended to produce the same output given the same parse tree, regardless of whitespace. There are exceptions to that; for instance, rustfmt has the concept of "paragraph breaks", where a blank line between blocks of code will be preserved, on the assumption that it was intended to break up code into logical chunks. I can see how this would be similar. On the other hand, what happens if one of those .x().y().z() blocks becomes longer than the configured line width? Would you expect that one to get split into one-call-per-line, with the others remaining untouched? What happens if the arguments to a function are long? I do think rustfmt could do a much better job with chains, in a variety of ways. For example, I'd love to see rustfmt consider putting simple calls on the same line of the foo.bar(
big_arg1,
big_arg2,
})?.build()?;
foo.bar(BigStruct {
...
}).await? And for the case you're describing, it looks like there is a common pattern as well, and I'm wondering if it could be easily expressed in a way rustfmt could learn to handle:
Suppose that you put something like those attributes on the methods, and then rustfmt automatically implemented the paired-calls pattern you're using? That would probably be easier to implement in rustfmt than attempting to preserve semantics implied by line breaks in the original code. And it'd mean that you get a uniform style across your codebase in the manner you want. Thoughts? |
+1, to me this is the most desired change pending for
I was about to ask this, if
In my opinion, the second option is less chaotic when line splitting is needed, it's easier to regroup things in a semantic way. |
@joshtriplett FWIW I'll just throw in my 2c that annotations like you describe can really grow to encumber code bases with more "noise" than code. Once a formatter (or tool) starts to go down this path, it's a slippery slope. |
@ericsampson I realize that it would add complexity to the formatter. But having ways to automatically format codebases to match a more nuanced formatting style would help greatly, especially if such annotations can occur in crates that provide such libraries. Consider clap, for instance, the configuration of which often involves a massive nested set of chained calls. It'd be nice to improve the formatting of that, without people having to hand-tweak it. |
@joshtriplett Thanks for the thoughtful response, and more generally for everything you've done for Rust! I think one of my more general frustrations is that the Rust coding style itself actually defers quite a bit to programmer discretion (frequently using language like "prefer" rather than "must"), but -- as you point out -- that discretion is not as reflected in We should also remember that, ultimately, the programmer does have discretion: when and where |
@joshtriplett of course it's the maintainer's choice 110% so do whatever you think works best, I just wanted to give a little food for thought re the annotations path based on some experience from other languages. Cheers!! |
Given how it's accomplished in some other languages/formatters, the simplest solution would probably be to bless a rustfmt configuration or annotation which results in this format remaining stable unless a line is too long. (In which case, only that line would get broken up.) fn func() {
p.RCC.pllcfgr.modify(|_, w| {
w.pll1vcosel().wide_vco() //
.pll1rge().range8() //
.divp1en().enabled() //
.divq1en().enabled() //
.divr1en().enabled()
});
p.RCC.d1cfgr.write(|w| {
w.d1cpre().div1() //
.d1ppre().div1() //
.hpre().div1()
});
} |
Sorry, have been meaning to comment on this one for a while. Though implicitly indicated previously by the inclusion of this issue in a milestone, I want to state explicitly that we're going to try to support this; just a matter of figuring how best to do so. |
This is spot on, and I want to emphasize it to provide context since I suspect this thread is likely to see some increased traffic due to the reference from your post (which was a very enjoyable read by the way!) rustfmt is indeed the AST-driven/pretty-printer type of code reformatter. This means that rustfmt has to process and write something for each and every AST node from the parsed program, and there has to be something that defines how rustfmt rewrites each node. The Style Guide is what provides those definitions, and those definitions are what rustfmt uses by default when reformatting. Yes, the Style Guide has language that varies between "must", "should", etc., but rustfmt by design uses and applies all the Style Guide prescriptions regardless of the strength of wording so that it has a consistent target formatting to work with. I think this is a powerful and useful feature, as it allows users to easily ensure their code is fully compliant with all of the Style Guide prescriptions (regardless of "should" vs "must"), with zero configuration. I don't think we'd want to remove that feature, just as similarly there are places where the style guide uses "must", but rustfmt also has configuration options that will let users do the exact opposite, and I wouldn't want to remove those either (e.g. derives guidelines and merge_derives config). I've personally come to prefer these types of code formatters. While I don't absolutely love every aspect of the emitted formatting either, I do love being able to defer to an automated tool that provides predictable and deterministic styling and eliminates back and forth debates on subjective topics that in my experience typically prove to be rather unproductive. There's definitely still occasions where I'll give rustfmt's output a cross look, but overall the benefits of automated reformatting, and doing so according the Style Guide, outweigh the instances of formatting that I dislike. I think there's a lot of users and contexts where that value holds true as well. However, I know those tradeoffs don't weigh out the same for everyone, and I 100% agree there's cases where the emitted formatting is so painful that users are forced to consider polluting their code with myriad skip attributes or not using rustfmt altogether. The snippets in the description are good examples of this in my opinion. While I do think reformatting according to the Style Guide should continue to be rustfmt's default modus operandi, I also think we should strive to provide options that at a minimum allow users to avoid these types of specific cases that a sizeable portion of the community indeed finds particularly painful. Close observers of recent rustfmt releases may have noticed early hints of this, such as configuration options that have a The challenge is that it's not always easy to incorporate this within rustfmt. We just don't know at this point whether we could support this via some new config options (e.g. Prettier is also an AST/pretty-printer type of formatter, and it is able to support preserving existing formatting in some cases, so I have to think we've got opportunities to do so as well. At some point in the near future I'm going to be doing some refactoring of the chain handling code to address an unrelated long-standing issue. While I'm in there, I'll also be thinking through if/how we could do this and what questions we'd need to get answered before attempting to implement some type of |
I'm not keen on this, primarily for two reasons. First, I know this was offered as an alternative, but I feel like the ask was for something non-attribute based, at least implicitly if not explicitly. Users can already toss Secondly, and somewhat selfishly 😄, I fear this could snowball pretty quickly and we'll end up with dozens and dozens of attributes. I worry this would add a lot of burden and complexity within the rustfmt codebase, but I also think it could make code noisy and messy for end users. |
On Mon, Oct 12, 2020 at 07:43:00PM -0700, Caleb Cartwright wrote:
I'm not keen on this, primarily for two reasons. First, I feel like the ask was for something non-attribute based (at least implicitly if not explicitly). Users can already toss `#[rustfmt::skip]` in there but I think that's proving too ineffective especially in these types of cases.
...
Secondly, and somewhat selfishly 😄, I fear this could snowball pretty quickly and we'll end up with dozens and dozens of attributes. I worry this would add a lot of burden and complexity within the rustfmt codebase, but I also think it could make code noisy and messy for end users.
The difference is that these attributes would appear on method
definitions, and would then improve formatting for all users of those
methods. One method definition, many many callers. That seems worth a
few annotations for cases where rustfmt will never be able to
understand otherwise.
|
Would would mean decisions about the formatting of a codebase could be deferred to a dependency's author, rather than in the control of the developers of the actual code in question? That doesn't seem like a good shift in responsibility. |
@calebcartwright Thank you for writing up a great summary.
This means rustfmt needs to be able to resolve names, which is currently (and will be, sadly) outside the scope of rustfmt. |
I sometimes think one formating rule or the other would better be different, but what i really love about rustfmt, is that the same code always looks the same. If you allow programmer discretion in formating every third programmer would do much worse. |
While I agree on this point as a general trend, I have to admit that it does fail in the aforementioned examples. In those cases, strickly sticking to it is valuating standardization over readability, which I think is a bad idea as well. I think in this very cases, human flexibility is better than blind procedural formatting. I also think that examples like the ones given above are both rare enough that special-casing them is the right solution, and frequent enough that falling back on |
Fully agreed! rustfmt overall does a great job, but there are some cases where some programmers like to put more nuance into their formatting to convey additional information or to increase readability by symmetry, and then rustfmt can sometimes remove that information and likewise remove the symmetry. This issue is an example of this; my own pet peeve here is rustfmt's handling of |
On the extreme other end, you might be able to solve the original example with language:
I'll admit this is probably overkill (and may be impossible in some cases), but there may be situations where this might be useful for cases beyond simply formatting, and a nice format would just be a bonus. |
Thinking about a solution that works 9 out of 10 cases, can't there be some setting that when splitting, would allow chains to be not 1 per line, but some other specified number? I expect the typical code would be quite symmetric, but in other parts of the code regular 1 per line call makes more sense. @joshtriplett I think the chain start/end annotations, even if nice for rustfmt, are going to be a "crate service" provided to the users which might not happen or work nice in practice. And when the chain formatting doesn't happen for the user, the source of the issue is going to be very obscure, unless you're familiar with this particular formatting choice. Also, will these chain attributes take precedence over the defaults? Won't that mean that depending on the crates you draw from, different parts of the same code would look different? I've hit this problem in a code doing parsing with pest and have a similar problem to @bcantrill, but in my case the end of chain is a local choice that makes sense from the perspective of the operations I'm doing. In the test code I have these: parse.unwrap()
.next().unwrap().into_inner() // inside typedef
.next().unwrap().into_inner() // inside rhs_typedef_complex
.next().unwrap().into_inner() // inside complex_type_def
.next().unwrap().into_inner() // inside rhs_typedef_simple But in pair
.into_inner().next().unwrap()
.into_inner().next().unwrap() |
Visual Indent based on first and second item of first line (as described above in #4306 (comment)) fn f() {
looooooooooooooooooonnnnggg.foooo.baaarr()
.bazzzzzzzzz()
.quxxxxxxxx()
} |
Visual Indent based on last and second to last item of first line (as described above in #4306 (comment)) fn f() {
looooooooooooooooooonnnnggg.foooo.baaarr()
.bazzzzzzzzz()
.quxxxxxxxx()
} |
Any updates on this feature or anything I can do to help get it over the line? |
I share many of the sentiments discussed so far. What's the status of this? |
The last comments from me on this thread posed questions to @bcantrill as the original author as well as the community at large, both of which have largely gone unanswered. Unfortunately, as has typically been the case historically, we receive a lot of complaints about chain handling but comparatively little feedback when it comes to the specifics of the actionable, concrete elements we can support. There's no updates from the rustfmt team on this, and while I appreciate the offers to help, what we really need is more responses/feedback on the questions already posed above. |
My apologies; I didn't realize that this was blocked on me! I would say that this variant looks great: One follow up question: would this apply to function arguments as well? (We have much the same challenge with, e.g., Thank you again for your work on this important piece of the Rust ecosystem, and my apologies again for not realizing that further work had become blocked on my feedback! |
I would like to provide another perspective on this issue: Due to disability I am coding on a very specific setup; an ancient thinkpad with a small screen and a large font size. Effectively this gives me a 26 row/105 column setup to work with. I often use split screen. I very actively decides which code should go onto my main view and which does not. I tend to avoid lines with just a closing parenthesis. Sometimes I deliberately split arguments into multiple lines to have them all on the screen, sometimes I deliberately use very long lines just because the precise content of an error message is not that important most of the time. Right now, rustfmt style makes effective coding very hard for me sometimes. I just deactivated it as part of the CI setup for a project because my loss in productivity is just not worth it. This feature will help a lot in that regard! |
|
I'm a fan of the first formatting style you mentioned (#4306 (comment)) but would add that I'd expect it to still split at line length - just pick the For the |
Could rustfmt avoid "rechaining" lines with a trailing comment. Reasoning:
acount.set_owner("name")
.debit(cash).credit(cash) // ensure debits and credits match
.set_transaction_date(today)
.set_id(id)
.begin_tran().write_entries().commit_tran()?; // write within a transation
|
It doesn't solve the desire for manual control over groups in the chain. A comment is not always warranted or desired. A single "leave this alone" attribute at the start of the expression feels better than a magic or load bearing comment. Even if a comment is warranted it may well be long, and requiring line width excursions to get the behaviour is also not ideal. |
I agree. It doesn't really solve the desire for manual control, and there are better ways to help achieve that objective, as you state. But I still feel that the presence of line comments is relevant to chaining of functions, and somehow needs to be incorprated and work alongside any explicit control. One more item. I think the rule "dont unchain/chain lines with an end of line comment" applies to other structures too. And reformatting code without paying attention to line comments effectively "breaks" the comment or renders it meaningless. Rust supports "//" style line comments as a language feature. And they are tied to lines, and thus the line breaks around them become meaningful. Its probably one of the few cases where whitespace like this carries meaning in the language syntax. So rustfmt should not ignore this aspect of the language. For example arrays. let tokens : [&str: 9] = [ "-", // hyphenation
"a", "e", "i, "o", "u", // vowels
",", ".", "?" // punctuation
]; and also to conditional booleans in an if-then expression (obviously this example can be written more idiomatically) if balance > 0 &&
name.len() > 0 && name[0].is_digit() // check length before accessing by index
&&
''' |
@calebcartwright are you thinking about moving one of your proposals forward, need help, ? |
@calebcartwright if you're still looking for feedback, especially to help get these changes across the line, then I'd say that your "variant" proposal looks pretty good to me, and would be much better than the status quo, which is far more destructive. Could you introduce your variant behind a config switch, and then we (the end users) can start using it and give you better feedback? Right now, I wouldn't know how to try your variant mentioned here - it looks good, but does an implementation exist so I can try it with my own day-to-day code? |
Sorry for the excessive delay in providing an update/response, and thanks to all for the discussion and feedback. In short, we're going to introduce a new config option that will support controlling the line break behavior between chain elements that will, more or less, look like what I outlined in prior comments above. There's a few reasons why we're going to do this, with the primary two being:
I can't and won't attempt to provide an estimate for when this new option might land on nightly. It's likely going to have to be accompanied by some other internal chain-related refactoring, and we've got various other priorities as well. However, portions of the changes needed to achieve this have actually already been implemented and merged (as they were independently beneficial), and I anticipate we'll be able to spend some bandwidth on this before the end of the year. Finally, while I certainly appreciate the offers to help, I don't think the development of this new option is something that others can readily assist with; it's a complex part of the codebase that has a number of outstanding code-level design questions which also need to be considered alongside other needed changes. As such I don't think it would be practical for us to delegate nor mentor the work. The best way users could assist would be helping with rustfmt more generally (please PM me on Zulip if interested), and/or testing the new config option once available. My apologies, I did not intend to imply this was blocked on you, I just wanted to know if the proposal seemed reasonable to you given that you'd taken the time to make the case for this feature in the OP of this issue. We'd made some good progress on this, but then hit a wall trying to determine how it should behave with other options. It subsequently fell off our radar due to other priorities popping up and a lack of community feedback (at the time).
Not now, as we'll need to focus on chains first. However, if we're able to find success with chains then I think we'd be able to look at call site args next. Thanks for sharing , I'm glad to hear you think this will likely be beneficial for you. Though not necessarily related to the specific feature request in this issue, I'd suggest reviewing the Rust Style Guide (which defines the style rustfmt has to provide by default), and if there are other cases where you feel those prescriptions could be better given those constraints you're working within, then consider sharing with the Style Team in https://github.com/rust-lang/style-team/ (similarly with us on the Rustfmt Team here in this repo if you think a new config option could be helpful to provide some non-default behavior) Thanks for the feedback
Understand why users would like that, but this was (somewhat) discussed previously and that's not going to be in scope for what we'll do from the rustfmt side. It's a bit too much of "having one's cake, and eating it too", with too many edge cases and competing user preferences for me to justify the rustfmt team spending cycles on it. Others will be able to take a crack at implementing additional variants for the new option once it lands if they'd really like this type of conditional preservation. |
I would assume that the AST tree contains name information and we would be more than happy with having something like: chain.always_inline. I feel like most of us would be happy with such a solution. I personally do not have any issues with every element being placed on a new line. However, in the case of unwrap() or await it gets a little bit annoying. I would personally just run with something similar to always_inline = ["await", "unwrap", "iter", "to_*", "into"]. Ideally it would be great if there was an option to not reformat chains at all as it currently lacks the feature due to the fact that it rebuilds code instead of changing it from the AST. While these stop-gap measures might not be the ideal solution long-term it would still be quite useful for the ability to create simple configurations without dipping into the documentation on creating more complex patterns as most of the time. Just my 2 cents as this is still far better than whatever jetbrains gives you for formatting especially around (some) macros. |
Thanks again for all the input, but at this point I'm going to lock this issue as resolved (n.b. this is not a reaction to any particular comment, I'd meant to do this when I made my prior comment) This issue was a request for a specific enhancement to be made available in two specific positional contexts, which we've already responded to and announced our plan of action. |
When lines hit
max_width
(or its derivatives), rustfmt will break chains and function calls with one call or argument per line. We have found this to result in code that is less clear, and no more consistent with Rust's style than the code that it replaces. We would like to see an option whereby these chains and function calls would be left to programmer discretion as long as they observe the constraints of indentation andmax_width
.To make this more concrete, we have experienced this most recently in the embedded Rust world, where hardware-abstracting crates make extensive use of types to constrain context (viz. svd2rust's API).
The upshot of this is that we end up with long chains that are asymmetric in that different positions in the chain actually denote different states. Often, the programmer will use the line break to help clarify what's going on; some (real) examples:
In this case, the programmer has clearly grouped the chain to indicate the type states -- and to the reader, both uses are clear. rustfmt, however, doesn't like either of these expressions; they get reformatted as:
In both cases, meaning has been lost -- even though the code as written is consistent with Rust's style. We can of course wrap these constructs in
#[rustfmt skip]
-- but they come up often enough that we would very much rather fix it in rustfmt. And to be clear, we don't want these to be compressed either (as raised in #4146 and #2010); we seek to have them be optionally left to the programmer's discretion, within the bounds of the configured style.Thank you in advance for your consideration and your work on this essential (if underappreciated!) part of the Rust ecosystem!
cc: @cbiffle @steveklabnik @davepacheco @ahl
The text was updated successfully, but these errors were encountered: