Format tuple like structs #93

marcusklaas · 2015-06-02T21:33:40Z

This adds formatting for tuple like structs, and should address https://github.com/nrc/rustfmt/issues/86.

nrc · 2015-06-02T21:43:08Z

tests/idem/structs.rs

+    (AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA, // Comment
+     BBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBB,
+     G,
+     W);


I think I would format like this:

struct Qux<'a, N: Clone + 'a, E: Clone + 'a, G: Labeller<'a, N, E> + GraphWalk<'a, N, E>, W: Write + Copy>( AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA, // Comment BBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBB, G, W, );

Note the trailing comma - we probably want three options for trailing commas in tuples - always, never, and vertical only (maybe we have these options already, actually).

That looks good, but isn't it inconsistent with the rest of the formatting rules? We usually don't have a 'dangling' closing paren for tuple-like structures.

I don't think we can be consistent with both the expression form and regular structs. I think being closer to other items is more important here (and gives less weird indenting). Struct literals also vary a bit from struct definitions, so I think its OK.

Alright, will implement.

nrc · 2015-06-02T22:00:40Z

Also, if you take this approach I wonder if you can format each field independently so we can cope with a mix of struct-like and tuple-like fields. Obvs whether you use {} or () will have to be determined by the first field, but the rest could be a mix. That should give slightly nicer code and should be more future proof too.

marcusklaas · 2015-06-02T22:05:46Z

That was my initial approach, but it didn't work out very well for me. Especially switching between horizontal mode and vertical mode is unique to the tuple variant. It also requires distinct formatting code, and then we'd probably just reimplement write_list.. I am of course open to suggestions on how to do this nicely!

nrc · 2015-06-02T22:11:49Z

I might be missing the subtleties having not written the code. But it seems like the vertical vs horizontal is just a parameter to write_list and can be deduced at the same time as whether to use {} or (). Formatting each field into a string should be independent of any formatting of the whole struct.

marcusklaas · 2015-06-02T22:16:12Z

Right, I think I understand now. So we'd make visit_field return a String instead of pushing changes directly and then use write_list to format the struct in both cases?

nrc · 2015-06-02T22:18:21Z

Yeah, I think that should work. I might be wrong because of attributes on fields, but hopefully not.

marcusklaas · 2015-06-02T22:18:47Z

I'll look into it!

marcusklaas · 2015-06-02T22:36:15Z

Should be functionally correct now, but still needs a refactor. WIP.

marcusklaas · 2015-06-05T15:38:07Z

I am a bit stuck here. What gives me the most troubles is that in a struct like
rust struct Foo(SomeType, OtherType);.
the span for the last StructField contains the closing parent.. But not when it is followed by a comment:
rust struct Foo(SomeType, OtherType /* this is a type */);.
Is this something that we should consider a bad span and fix in libsyntax, or is this something we should hack around?

nrc · 2015-06-08T00:01:10Z

@marcusklaas definitely a libsyntax bug, lets fix it there.

marcusklaas · 2015-06-16T14:41:17Z

I don't know what changed, but that bug that I mentioned earlier no longer seems to be present! Still, I have troubles finishing this PR.

Quick recap: I'm trying to format all structs using the write_list function. This works fairly well, but there's some pain points. For example, it's not clear how to deal with comments preceding fields:

struct Problematic {
    // Our first and
    pub x: String, // only field
}

This is very difficult, I think. I do have some workarounds in mind, but I thought I'd run them by you first. We could pass two associated comments for every item we give to write_list: a preceding and a trailing one. We could also "merge" the preceding with the comment with the actual item before we pass them to write_list. I like the latter option better, but it gives write_list fewer options to refactor things. Consider for example this struct:

struct Tuple(
    // Some int
    i32
);

That would ideally be formatted as

struct Tuple(i32 /* Some int */);

That can't be done (easily) if we can't distinguish between the comment and the item itself.

On a related note, I believe the comments we pass to write_list should be stripped of "comment tokens" (//, /* and */) so that'd do such transformations more easily.

This may all be going a bit far, so maybe it's best to just do something that works now (merge preceding comment and item texts) and leave improvements for later. What do you think, @nrc?

nrc · 2015-06-16T22:23:23Z

Hmm, it is a tricky issue. I think there are a few questions here. The first one is, do we want to move comments around? I had been ducking this issue previously. We could imagine having a style guideline that says "all comments on fields should happen on the previous line" or something. My intuition is that, in general, the programmer probably knows best with comments and we should do the minimum of reorganisation. We can certainly re-visit this later though.

I do think we can and should change the kind of comment - i.e., /* ... */ to //... where possible. I don't feel this is high priority, but if it works, then great!

The next question is how much we want to get right. There are already some cases we don't even try to handle, e.g., pub x /* a comment */: String, will just drop the comment on the floor. I think this is OK for now, though not ideal in the long run. I think we probably need to handle comments in both the positions you mention in your comment (i.e., pre and post).

Given that there is so much variety in how we could place comments, I believe we have to let write_list do it, rather than trying to do some merging before passing to write_list.

So, all of this suggests that having pre- and post-comments passed to write_list is a good idea. Probably we should pass one list of structs, rather than three lists. Probably need to start using Options rather than empty strings too. I think passing the comments without the // or /* */ is a good idea. (The complicated aspect here is that where to put the comments and what tokens to use depends on how write_list lays them out).

I think this shouldn't be too hard. I think the quick and dirty approach will actually cause problems in the short run too, so isn't worth it. I'd be fine with not covering all the possible situations in write_list for now. I.e., panicking in some situations and fixing them later. Only implement what you need here and to not cause regressions.

The other option if you think that is too slow would be to not handle side comments for now (i.e., only handle comments on the preceding line).

marcusklaas · 2015-06-16T22:28:01Z

Thank you for your wisdom Nicholas. I fully agree and will try to implement what was suggested.

marcusklaas · 2015-06-18T01:18:40Z

I've given this an update. It should close https://github.com/nrc/rustfmt/issues/86 and https://github.com/nrc/rustfmt/issues/76, since struct-like enum variants are formatted now as well. Some progress on https://github.com/nrc/rustfmt/issues/27 has been made as well. Formatting the comments correctly was the biggest challenge, after all.

A brief description of the approach: let write_list do most of the heavy lifting. It takes a lift of ListItems now, which is basically a String with optional pre- and post-comments, as you suggested. I've defined a pre-comment to be the first comment after an item and additionally that starts on the same line. I think that makes sense, but there is room for ambiguity. For example:

struct AmbiguousIsMe {
    pub x: 32, // this is clearly belongs to x
          // but what about this?
    // this clearly belongs to y
    y: PrivateType
}

Most expressions and items that use write_list now support pre-comments too! See the fn-simple.rs tests.

There's a million edge cases, which tend to make things messy. For example, a long post-comment will be converted into a block-style comment. The last post-comment in expression-like lists will also be in block-style, to prevent the comments from commenting out actual code:

fn test(
    arg_one: i32, // looooooooooong comment
    arg_two: &str // comment
) {

may become

fn test(arg_one: i32, // looooooooooong comment
        arg_two: &str /* comment */) {

There's many changes here and I am confident about only few of those, so I would be very grateful for any feedback!

cassiersg · 2015-06-18T10:31:52Z

src/string.rs

+                          .trim_left_matches("* ")
+                          .trim_left_matches('*')
+                          .trim_right_matches("*/")
+                          .trim_right_matches(' ')


I think there is a problem with comments like

// // /* <- yes, I want to talk about comments in my comment

Good catch!

cassiersg · 2015-06-18T13:10:16Z

Except theses details, seems very nice.
I just needed something like this to work on comment handling into expressions.

marcusklaas · 2015-06-18T17:56:51Z

Thank you very much for your excellent commentary, @cassiersg. I'll get to work!

nrc · 2015-06-18T23:00:01Z

src/string.rs

+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+// Format comments and string literals


I would break this into separate modules for stings and comments, unless there is a lot of overlap I'm not noticing

cassiersg · 2015-06-20T13:28:58Z

src/lists.rs

+                                                    offset);
+
+            result.push(' ');
+            result.push_str(&formatted_comment);
        }


The handling of comments in Mixed mode seems to be incorrect : line_len should be updated with the comments len.

Excellent observation, thanks!

marcusklaas · 2015-06-23T23:12:07Z

Should also fix https://github.com/nrc/rustfmt/issues/87 now.

nrc · 2015-06-24T00:27:45Z

src/lists.rs

@@ -41,11 +47,38 @@ pub struct ListFormatting<'a> {
    pub h_width: usize,
    // Available width if we layout vertically
    pub v_width: usize,
+    // Non-expressions, e.g. items, will have a new line at the end of the list.
+    // Important for comment styles.
+    pub is_expression: bool


Could this be named to be more about what you're doing rather than why? ends_with_newline or ends_block or something? (also needs a trailing comma)

The more I think about this, the less I like it. Could we append a newline and block_indent instead of using a block comment in the expression case? It feels a bit irregular atm.

I agree it's not pretty, but I am afraid it will be a bit irregular in any case. If I understand correctly, we have to decide between

(item1, // Comment 1 item2, // Comment 2 item3, /* Comment 3 */)

and

(item1, // Comment 1 item2, // Comment 2 item3, // Comment 3 )

Personally, I don't believe the latter is always better than the first, but I can get behind it if that's what is decided on.

I don't want to worry too much about this - it should be a total edge case exactly because there is no way to format it nicely. Another alternative might be to change all post-comments to /* */. Or to only do that if there is a final comment? Any way we do this is kind of inconsistent. I guess I prefer a little inconsistency in the punctuation rather than in the comment style (and the code should be simpler), but I don't think it is too important.

Alright. I've left it as it is for now, if you do not mind. We can improve this later.

nrc · 2015-06-24T00:29:11Z

src/lists.rs

+            result.push_str(&rewrite_comment(comment,
+                                             // Block style in non-vertical mode
+                                             tactic != ListTactic::Vertical,
+                                             1000,


I'm not a fan of these magic 1000s - could you use an Option rather than a usize?

This was actually a bug! 😨

Format tuple like structs

marcusklaas · 2015-06-25T21:19:23Z

Nice! 🎊

Thank you for your guidance and patience in this, @nrc! I learned a lot doing it!

nrc · 2015-06-25T22:19:49Z

A pleasure; thanks for the PR!

nrc reviewed Jun 2, 2015
View reviewed changes

cassiersg reviewed Jun 18, 2015
View reviewed changes

cassiersg mentioned this pull request Jun 18, 2015

Add a Rewrite trait #107

Merged

nrc reviewed Jun 18, 2015
View reviewed changes

marcusklaas mentioned this pull request Jun 19, 2015

Update import list formatting #113

Merged

cassiersg reviewed Jun 20, 2015
View reviewed changes

marcusklaas added 2 commits June 23, 2015 16:54

Format tuple-like structs

0ef5db9

Format comments in struct literals

9f3ab0b

nrc reviewed Jun 24, 2015
View reviewed changes

Fixup comment wrapping in lists

28f7bd4

nrc added a commit that referenced this pull request Jun 25, 2015

Merge pull request #93 from marcusklaas/tuple-structs

8cef678

Format tuple like structs

nrc merged commit 8cef678 into rust-lang:master Jun 25, 2015

marcusklaas deleted the tuple-structs branch September 11, 2015 09:23

scampi mentioned this pull request Nov 28, 2018

// Reformated to // even with normalize_comments = true #3209

Closed

scampi mentioned this pull request Jan 31, 2019

format_doc_comments causes adjacent comments to be made multiline #3268

Closed

Format tuple like structs #93

Format tuple like structs #93

Uh oh!

Conversation

marcusklaas commented Jun 2, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nrc commented Jun 2, 2015

Uh oh!

marcusklaas commented Jun 2, 2015

Uh oh!

nrc commented Jun 2, 2015

Uh oh!

marcusklaas commented Jun 2, 2015

Uh oh!

nrc commented Jun 2, 2015

Uh oh!

marcusklaas commented Jun 2, 2015

Uh oh!

marcusklaas commented Jun 2, 2015

Uh oh!

marcusklaas commented Jun 5, 2015

Uh oh!

nrc commented Jun 8, 2015

Uh oh!

marcusklaas commented Jun 16, 2015

Uh oh!

nrc commented Jun 16, 2015

Uh oh!

marcusklaas commented Jun 16, 2015

Uh oh!

marcusklaas commented Jun 18, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cassiersg commented Jun 18, 2015

Uh oh!

marcusklaas commented Jun 18, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcusklaas commented Jun 23, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcusklaas commented Jun 25, 2015

Uh oh!

nrc commented Jun 25, 2015

Uh oh!

Uh oh!