Comments wrapped at 99 chars instead of 101.

[jnqnfe]

With the following config:

brace_style = "SameLineWhere"
comment_width = 100
control_brace_style = "ClosingNextLine"
edition = "2018"
fn_args_layout = "Compressed"
hard_tabs = false
match_block_trailing_comma = true
max_width = 100
merge_derives = false
newline_style = "Unix"
normalize_doc_attributes = true
overflow_delimited_expr = true
reorder_imports = false
group_imports = "StdExternalCrate"
reorder_modules = true
#struct_field_align_threshold = 20
tab_spaces = 4
trailing_comma = "Never"
use_small_heuristics = "Max"
use_try_shorthand = true
#where_single_line = true
wrap_comments = true

The following change was made:

 /* Note, we cannot simply use the object defined in the ‘sys’ crate, since either the type or the
- * trait need to be defined locally in order to link them. Thus, we create the below type (an empty
- * one, just used as an opaque pointer), and transmute to the ‘sys’ crate one.
+ * trait need to be defined locally in order to link them. Thus, we create the below type (an
+ * empty one, just used as an opaque pointer), and transmute to the ‘sys’ crate one.
  */

The longest line was 99 chars, so wrapping should not have occurred here.

This is also a problem for other styles of comments:

     /// Maximum length of the buffer in bytes.
     ///
-    /// Setting this to [`std::u32::MAX`] will initialize this to the maximum value supported by the
-    /// server, which is recommended. In strict low-latency playback scenarios you might want to set
-    /// this to a lower value, likely together with the [`stream::FlagSet::ADJUST_LATENCY`] flag. If
-    /// you do so, you ensure that the latency doesn’t grow beyond what is acceptable for the use
-    /// case, at the cost of getting more underruns if the latency is lower than what the server can
-    /// reliably handle.
+    /// Setting this to [`std::u32::MAX`] will initialize this to the maximum value supported by
+    /// the server, which is recommended. In strict low-latency playback scenarios you might
+    /// want to set this to a lower value, likely together with the
+    /// [`stream::FlagSet::ADJUST_LATENCY`] flag. If you do so, you ensure that the latency
+    /// doesn’t grow beyond what is acceptable for the use case, at the cost of getting more
+    /// underruns if the latency is lower than what the server can reliably handle.
     ///
     /// [`stream::FlagSet::ADJUST_LATENCY`]: crate::stream::FlagSet::ADJUST_LATENCY
     pub maxlength: u32,

With the longest lines here being exactly 100 chars, no wrapping should have occurred.

Version string: rustfmt 1.4.37-stable (a178d03 2021-07-26)
Project: https://github.com/jnqnfe/pulse-binding-rust
Ran against the master branch after adding the quoted config.

The Specific projects files are https://github.com/jnqnfe/pulse-binding-rust/blob/master/pulse-binding-mainloop-glib/src/lib.rs for the first instance and https://github.com/jnqnfe/pulse-binding-rust/blob/master/pulse-binding/src/def.rs for the second.

[calebcartwright]

As with other issues it would be helpful to provide the version of rustfmt you are using, and also the plain input and formatted output in separate blocks for easier reproduction.

That being said, I think there's some confusion around what wrap_comments does, and admittedly the documentation is very light. You should look at enabling the option as giving rustfmt permission to rewrite your comments in a way it sees fit so that no lines exceed the width. That does not mean that it will emit characters iteratively up to the max width boundary and only then moving to the next line.

[jnqnfe]

@calebcartwright I do not believe that there is any confusion here in my expectations of what the feature does. Please look carefully at the snippets I gave. Every line already has a length that is less than or equal to 100 chars, so rustfmt should not have made any changes to them. rustfmt incorrectly chose to re-wrap them based upon some slightly shorter length. Specifically it seems as though, with a configured width of 100, it's wrapping anything that's 99 chars or longer, when it should only be wrapping things that are 101 chars or longer.

Please see my reply in #4928 regarding rustfmt version and the project I was playing with applying rustfmt to.

[calebcartwright]

Please look carefully at the snippets I gave.

I did 😉

Every line already has a length that is less than or equal to 100 chars, so rustfmt should not have made any changes to them. rustfmt incorrectly chose to re-wrap them based upon some slightly shorter length. Specifically it seems as though, with a configured width of 100, it's wrapping anything that's 99 chars or longer, when it should only be wrapping things that are 101 chars or longer.

I acknowledge you disagree, but I'm going to reiterate my previous comment because you're looking at this with the wrong expectation. First and foremost, there are cases where rustfmt splits these comment lines well before the ultimate width limit in an attempt to find the most appropriate boundaries for several reasons, such as trying to avoid odd splitting of words or failing to preserve semantics (breaking urls, etc.).

Secondly, you're assumption/expectation is that the comment_width is based solely on the count of characters. That's incorrect; whitespace matters. It's about the total columnar width of the lines in formatted construct, and the formatted version of the longest line in your snippet would have come in at 101 columns, exceeding your width setting and thus needing to be wrapped. You can observe the variance in how your comment would get wrapped differently when the comment is placed at different indentation levels.

Whether or not one would prefer character counts over total width is orthogonal to whether rustfmt, and those config options, are operating as intended and expected.

This is operating correctly, and the comment wrapping behavior is expected. Thank you for reaching out with the report, but I'm going to go ahead and close this given the above. Please let me know if you have any follow up questions!

[jnqnfe]

I've updated it with a version string.

My understanding from your original reply was that you thought that I was under the misunderstanding that rustfmt should be applying word-splitting to make maximum use of the configured width, which was not the case.

First and foremost, there are cases where rustfmt splits these comment lines well before the ultimate width limit in an attempt to find the most appropriate boundaries for several reasons, such as trying to avoid odd splitting of words or failing to preserve semantics (breaking urls, etc.).

Of course. I expect that.

Secondly, you're assumption/expectation is that the comment_width is based solely on the count of characters. That's incorrect; whitespace matters. It's about the total columnar width of the lines in formatted construct, and the formatted version of the longest line in your snippet would have come in at 101 columns, exceeding your width setting and thus needing to be wrapped. You can observe the variance in how your comment would get wrapped differently when the comment is placed at different indentation levels.

You've lost me. I can appreciate that rustfmt is going to apply various possible formatting changes before applying the wrapping logic, and I appreciate that whitespace counts, but since there are no pre-wrapping formatting changes to be made, I do not understand at all why on earth it would/should see these lines as being longer than the number of columns they are literally occupying.

I'm wondering, without meaning to insult, whether you've perhaps not actually counted the line length in my examples, you're just assuming that I'm counting from the start of the word "trait" or similar rather than the space in column 0? I'm counting from column 0...

|<-- column 0                                                                         column 99 -->|
/* Note, we cannot simply use the object defined in the ‘sys’ crate, since either the type or the
 * trait need to be defined locally in order to link them. Thus, we create the below type (an empty
 * one, just used as an opaque pointer), and transmute to the ‘sys’ crate one.

As demonstrated with the column number indications here, my text is already within the configured limit and thus conforming to line length policy.

With the limit at 100, the only thing accomplished per the diff here:

 |<-- column 0                                                                         column 99 -->|
 /* Note, we cannot simply use the object defined in the ‘sys’ crate, since either the type or the
- * trait need to be defined locally in order to link them. Thus, we create the below type (an empty
- * one, just used as an opaque pointer), and transmute to the ‘sys’ crate one.
+ * trait need to be defined locally in order to link them. Thus, we create the below type (an
+ * empty one, just used as an opaque pointer), and transmute to the ‘sys’ crate one.
  */

...is to move the word "empty" from a 99 column line down to the next line for no reason.

I have to bump up the limit to 102 to get it not to, upon which it chooses to make absolutely no changes to the line, which demonstrates that the wrapping is the only change being made - it's not applying any whitespace changes. I cannot comprehend why on earth it would/should think that that this line occupies 102 columns when it's actually 99. Where exactly is this additional count of 3 coming from?

I see no legitimate reason for the word "empty" to require being moved. The number of columns used for that line was within the configured column-count limit.

Similarly all of the lines here were just within the configured limit, as shown by the column number indicators:

 |<-- column 0                                                                         column 99 -->|
     /// Maximum length of the buffer in bytes.
     ///
-    /// Setting this to [`std::u32::MAX`] will initialize this to the maximum value supported by the
-    /// server, which is recommended. In strict low-latency playback scenarios you might want to set
-    /// this to a lower value, likely together with the [`stream::FlagSet::ADJUST_LATENCY`] flag. If
-    /// you do so, you ensure that the latency doesn’t grow beyond what is acceptable for the use
-    /// case, at the cost of getting more underruns if the latency is lower than what the server can
-    /// reliably handle.
+    /// Setting this to [`std::u32::MAX`] will initialize this to the maximum value supported by
+    /// the server, which is recommended. In strict low-latency playback scenarios you might
+    /// want to set this to a lower value, likely together with the
+    /// [`stream::FlagSet::ADJUST_LATENCY`] flag. If you do so, you ensure that the latency
+    /// doesn’t grow beyond what is acceptable for the use case, at the cost of getting more
+    /// underruns if the latency is lower than what the server can reliably handle.
     ///
     /// [`stream::FlagSet::ADJUST_LATENCY`]: crate::stream::FlagSet::ADJUST_LATENCY
     pub maxlength: u32,

I have to bump the limit up to 101 before it stops applying this change. These lines are occupying 100 columns, so again there is no reason to wrap them when the configured width is 100. Why on earth is it thinking that they were using 101 columns??

[jnqnfe]

Playing around a little just right now I've made some interesting additional observations...

If I change the type of comment in the first example as follows (to use // style rather than /**/), then it correctly determines that no wrapping is needed here:

|<-- column 0                                                                         column 99 -->|
// Note, we cannot simply use the object defined in the ‘sys’ crate, since either the type or the
// trait need to be defined locally in order to link them. Thus, we create the below type (an empty
// one, just used as an opaque pointer), and transmute to the ‘sys’ crate one.

It correctly does not re-wrap with the length when configured at 100 nor at 99, it only does so at 98 which is correct. This suggests that something is wrong specifically with the handling of the /**/ style.

With the other example, if I change it per the following diff to try // rather than /// whilst keeping the same length (note the a's added to the start of the text on each line):

 |<-- column 0                                                                         column 99 -->|
-    /// Setting this to [`std::u32::MAX`] will initialize this to the maximum value supported by the
-    /// server, which is recommended. In strict low-latency playback scenarios you might want to set
-    /// this to a lower value, likely together with the [`stream::FlagSet::ADJUST_LATENCY`] flag. If
-    /// you do so, you ensure that the latency doesn’t grow beyond what is acceptable for the use
-    /// case, at the cost of getting more underruns if the latency is lower than what the server can
-    /// reliably handle.
+    // aSetting this to [`std::u32::MAX`] will initialize this to the maximum value supported by the
+    // aserver, which is recommended. In strict low-latency playback scenarios you might want to set
+    // athis to a lower value, likely together with the [`stream::FlagSet::ADJUST_LATENCY`] flag. If
+    // ayou do so, you ensure that the latency doesn’t grow beyond what is acceptable for the use
+    // acase, at the cost of getting more underruns if the latency is lower than what the server can
+    // areliably handle.

...then it still exhibits the same off by one issue of being re-wrapped shorter when configured to 100 but not 101.

However when changed like this:

 |<-- column 0                                                                         column 99 -->|
-    /// Maximum length of the buffer in bytes.
-    ///
-    /// Setting this to [`std::u32::MAX`] will initialize this to the maximum value supported by the
-    /// server, which is recommended. In strict low-latency playback scenarios you might want to set
-    /// this to a lower value, likely together with the [`stream::FlagSet::ADJUST_LATENCY`] flag. If
-    /// you do so, you ensure that the latency doesn’t grow beyond what is acceptable for the use
-    /// case, at the cost of getting more underruns if the latency is lower than what the server can
-    /// reliably handle.
-    ///
-    /// [`stream::FlagSet::ADJUST_LATENCY`]: crate::stream::FlagSet::ADJUST_LATENCY
+    /* Maximum length of the buffer in bytes.
+     *
+     * aSetting this to [`std::u32::MAX`] will initialize this to the maximum value supported by the
+     * aserver, which is recommended. In strict low-latency playback scenarios you might want to set
+     * athis to a lower value, likely together with the [`stream::FlagSet::ADJUST_LATENCY`] flag. If
+     * ayou do so, you ensure that the latency doesn’t grow beyond what is acceptable for the use
+     * acase, at the cost of getting more underruns if the latency is lower than what the server can
+     * areliably handle.
+     *
+     * [`stream::FlagSet::ADJUST_LATENCY`]: crate::stream::FlagSet::ADJUST_LATENCY
+     */

Then no wrapping seems to occur at all. At first I just noted that it correctly did not adjust wrapping when configured to 100, however it also did not do so when configured to 99, or 98, or 90, or 50. It seems to be completely skipping application of wrapping in this instance, and that's both with comment_width=50 and max_width=50.

I cannot accept that there is nothing wrong with the wrapping code.

Edit: You can see in these screenshots that I have setup a right margin line in my editor to show me when I go over 100 columns and that I have not done so with these comment blocks. Furthermore I have placed the cursor on the end of the longest lines and you can see per the column number at the bottom that in the first screenshot the column to the right would be the 101st, and in the second would be the 100th (when the cursor is at the very beginning of a line it says 1), and thus both conform to a width=100 policy already, and so rustfmt should not be reducing their width.

[calebcartwright]

I'm wondering, without meaning to insult, whether you've perhaps not actually counted the line length in my examples, you're just assuming that I'm counting from the start of the word "trait" or similar rather than the space in column 0? I'm counting from column 0...

As a moderation note, let's refrain from continuing to make these types of insinuations. They set the wrong tone and don't help advance the dialog in a constructive manner.

As to the various suggestions, not going to address them individually but it's a "no" across the board. I feel like the underlying sentiment here is really more along the lines of "I'm getting a count of 100 cols, are you sure you're getting 101 and if so how?" which would certainly have been a fair question!

I'd taken the snippet from the description a few times, but suspect on the last col count I forgot to trim the leading diff chars since that leading - would indeed account for the extra col. That's my mistake. As a side note, this is part of why it's easier when the provided snippets are directly usable/reproducible so folks don't have to first manually edit every time they need to work with it.

I see no legitimate reason for the word "empty" to require being moved. The number of columns used for that line was within the configured column-count limit.

I want to circle back to what I said originally because this comment makes me believe there's still an important distinction being overlooked and a mismatch between your expected behavior and the current expected/allowed behavior of rustfmt.

When you enable comment wrapping in the current state, that means you're giving rustfmt permission to rewrite your comments as and when it sees fit, and the only "guarantee" is that the resultant comment won't have any lines that exceed the value defined in comment_width (unless it's impossible in which case rustfmt will just re-insert the associated comment contents from the source file)

However, it does not mean that rustfmt won't touch a line that's shorter than comment_width to begin with. That may be a somewhat subtle distinction, but it's a really important one that clarifies why the behavior you're seeing is permitted. Put differently, rustfmt definitely will split up a line that's too long, but it could potentially split lines that are close too, even if they don't exceed the limit.

Ultimately the width goals are more about readability, reducing rightward drift/horizontal scrolling, etc. as opposed to trying to utilize every possible column. That's part of the motivation (i.e. "make sure none of my comments are too long" vs. "don't touch any of my comment lines unless they exceed X"), and the other part is that there is history with the brittleness of this particular option when it was implemented with a more aggressive, but simplistic, model that only looked at the col widths.

Part of the enhancements/resolutions to the splitting heuristics to address some of those issues included taking a slightly less aggressive approach when it came to maximizing the col counts.

Finally, the option is still unstable, and is subject to change. We've had users request the default comment width value be changed and also for the option to support collapsing/merging multiple short lines into one. As such it's possible those heuristics could shift back to a more aggressive tact too, I'm just skeptical that'll happen.

Then no wrapping seems to occur at all. At first I just noted that it correctly did not adjust wrapping when configured to 100, however it also did not do so when configured to 99, or 98, or 90, or 50. It seems to be completely skipping application of wrapping in this instance, and that's both with comment_width=50 and max_width=50.

I can't reproduce the behavior you're describing, as the wrapping occurs for me, with the exception of course of when the limit set to 50 because rustfmt can't find a good spot to try to split the markdown link and bails (if you enable error_on_unformatted you should see more info on that one). You may want to make sure you aren't trying to use the stable formatter, as wrap_comments is an unstable options that has to have the default value changed, but unstable options cannot be used on stable.