Flutter style mode for dartfmt

[Hixie]

There's a number of places where dartfmt formats Flutter code in ways that we discourage in our style guide.

• The dart formatter puts if statement bodies on the same line as if statement expressions (dartfmt doesn't like 'if' flow control statements without blocks (FIXED) #448)

  It's easy, when reading large blocks of code, to miss an early return when it looks like this:

  bool operator ==(dynamic other) {
    if (other is! ValueKey<T>) return false;
    final ValueKey<T> typedOther = other;
    return value == typedOther.value;
  }

  For this reason, we ask, in the Flutter style guide, that all "if" statements have their bodies on a separate line than the expression, as in:

  bool operator ==(dynamic other) {
    if (other is! ValueKey<T>)
      return false;
    final ValueKey<T> typedOther = other;
    return value == typedOther.value;
  }

  The dart formatter prefers putting early returns on the same line as the if statement.

• The dart formatter does not implement our "one-line blocks don't get {}, multiline blocks do" rule. (Convert between "if () ..." and "if () { ... }" based on line length (WONTFIX) #453) (won't fix)

• The dart formatter does not implement our "symmetric punctuation" rule. In Flutter code, the punctuation that follows an open bracket ("(", "{", "[", "<") must be the same as the punctuation that precedes the matching close bracket (")", "}", "]", ">").

  This is especially bad around Widget build functions, where the nesting is critical to readability, and where dartfmt makes them unreadable. (Constructor call style (FIXED) #446)

• The dart formatter is too eager to split lines early to fit lines into 80 characters. (allow lines to go over 80 chars when the alternative is worse (WONTFIX) #450) (won't fix)

  For example, it formats code like this:

  message +=
     'string constant that exceeds 80 characters anyway';

  In the Flutter style, we prefer not to break after assignment operators, even if it makes the line wider. In this instance, breaking early doesn't help anyway since the line will be wider than 80 characters regardless unless we break the string constant (which is what we would probably prefer):

  message += 'string constant that exceeds '
             '80 characters anyway';

  Note also the way we prefer to align the literals here rather than indenting the second line by 4.

• The dart formatter loses alignment that matters for showing code symmetry. (Vertical alignment #508)

  For example:

   bool _canUpdate(Widget oldWidget, Widget newWidget) {
    return oldWidget.runtimeType == newWidget.runtimeType &&
           oldWidget.key == newWidget.key;
  }

  Becomes, with the formatter:

  bool _canUpdate(Widget oldWidget, Widget newWidget) {
    return oldWidget.runtimeType == newWidget.runtimeType &&
        oldWidget.key == newWidget.key;
  }

• The dart formatter is willing to wrap lines that use =>. Flutter style says that a line that the arguments and the expression of a => declaration must all be on the same line. (Convert between => and { return ...; } based on line length (WONTFIX) #452) (won't fix)

• The dart formatter uses {} for empty blocks rather than { }. We use {} to mean "empty Map". (Put a space inside empty blocks { } #445) (won't fix)

• Sometimes, the dart formatter just wraps the code arbitrarily in ugly ways.

  For instance, this code (dartfmt doesn't like 'for' flow control statements without blocks (FIXED) #449):

           for (GlobalKeyRemoveListener listener in localListeners)
             listener(key);

  ...becomes formatted to this:

           for (GlobalKeyRemoveListener listener
               in localListeners) listener(key);

  ...which is unreadably confusing.

  Another example (the original was just one line; splitting it is probably fine, but not like this!) (allow lines to go over 80 chars when the alternative is worse (WONTFIX) #450):

         assert(heroes[tag]
                 .keys
                 .where((Key key) => mostValuableKeys.contains(key))
                 .length <=
             1);

• Constructor named arguments aren't separated from their {} punctuation by spaces.

  Flutter style uses the following:

  const OneChildRenderObjectWidget({ Key key, this.child }) : super(key: key);

  dartfmt uses:

  const OneChildRenderObjectWidget({Key key, this.child}) : super(key: key);

• Wrapped lines are indented 4 by dartfmt, 2 by Flutter style. (fixed for argument lists with trailing commas, won't fix for other cases)

Combining some of the above, you end up with things like this code (#447):

  const _HeroManifest({
    this.key,
    this.config,
    this.sourceStates,
    this.currentRect,
    this.currentTurns
  });

...being formatted to:

  const _HeroManifest(
      {this.key,
      this.config,
      this.sourceStates,
      this.currentRect,
      this.currentTurns});

Notice the 4 space indents and the asymmetric whitespace around brackets.

Style guide: https://github.com/flutter/engine/blob/master/sky/specs/style-guide.md

[sethladd]

I wonder how extensible dartfmt is. Is there something we can do, without forking it and doing major re-engineering?

[munificent]

For this reason, we ask, in the Flutter style guide, that all "if" statements have their bodies on a separate line than the expression, as in:

Our style guide prohibits multi-line if statements without curly bodies, which is why the formatter doesn't format them the way you'd like.

This follows Google's Java style guide which doesn't allow ifs without curlies at all. We loosen that to allow single-line ones, which are really handy for things like parameter validation.

I've seen a number of style guides for various languages that prohibit multi-line if statements without curlies because they are a notorious source of bugs. Most recently, Apple's gotofail security vulnerability was caused by exactly this and a bad merge.

The dart formatter does not implement our "one-line blocks don't get {}, multiline blocks do" rule.

Do you mean this for all blocks, or just ones used in if, etc?

The dart formatter does not implement our "symmetric punctuation" rule. In Flutter code, the punctuation that follows an open bracket ("(", "{", "[", "<") must be the same as the punctuation that precedes the matching close bracket (")", "}", "]", ">").

I didn't follow this one. Can you give me some examples?

The dart formatter is too eager to split lines early to fit lines into 80 characters.

In your example here, the formatter will do what you want (minus the alignment) if you split the string yourself. It's not that it's too eager to fit into 80 characters, it's that you gave it something it couldn't fit into 80 in any nicer way.

The formatter can't split string literals for lots of reasons that I can get into if you're curious. The main one is that it isn't easily reversible.

The dart formatter loses alignment that matters for showing code symmetry.

In general, I don't think most Google style guides encourage alignment. I don't dislike it, but it is quite hard to implement well, I think. I wouldn't rule out the formatter supporting it, but it's an open question as to if it's actually feasible to implement in an automated, efficient way. I've never had the time to give it a try.

The dart formatter is willing to wrap lines that use =>. Flutter style says that a line that the arguments and the expression of a => declaration must all be on the same line.

If your code follows the Flutter style and does fit on one line, then why would the formatter add any splits? Can you give an example?

The dart formatter uses {} for empty blocks rather than { }. We use {} to mean "empty Map".

Interesting. Empty blocks are not very common in Dart and when they do occur, they generally are in places where I don't think it's that easy to mistake it for a map. We probably could change this, though I don't know what our users would think. Is this a big deal for you folks?

It feels edge casey to me. If I were dropped on to a team that had one style or the other for this, I would probably just go with the flow and adopt the prevailing style.

Sometimes, the dart formatter just wraps the code arbitrarily in ugly ways.

         for (GlobalKeyRemoveListener listener in localListeners)
           listener(key);

This is ugly because multi-line for loops without {} bodies violate the style guide. It's not arbitrary it's just that you're feeding an AST to it that grammatically violates the style rules. In that case, I haven't implemented rules to try to handle that well because it's not a supported form in the first place.

(Whether multi-line for loops without curlies should be supported is a different argument. If it were allowed, it would be trivial to format them better.)

Another example (the original was just one line; splitting it is probably fine, but not like this!):

       assert(heroes[tag]
               .keys
               .where((Key key) => mostValuableKeys.contains(key))
               .length <=
           1);

Yeah, this is a nasty edge case. Again, the behavior isn't arbitrary. (Despite the temptation, dartfmt does not use Random anywhere.)

The general rules in play here are:

1. If a subexpression gets a line break, the surrounding expressions are usually forced to split too. This helps avoid code like:

some(deeply(nested(function(argument,
    another))), waitWhichFunctionDoesThisBelongTo?)

There are lots of other cases where this general rule avoids weird ambiguous splits and indentation.

1. Indentation reflects expression nesting. Shallower indentation gets assigned to outer expressions. Hopefully it's self-evident that this is a good thing.

In some cases, these two rules interact in the above way where you get more indentation and line breaks than you expect. You probably expect:

The output is helpful in some ways: it makes it visually clear that the <= 1 applies to the result of that entire method chain. We could do this:

        assert(heroes[tag]
            .keys
            .where((Key key) => mostValuableKeys.contains(key))
            .length <= 1);

In this case it would be OK, but there are other cases where allowing that would push the <= 1 way over to the right and make it really easy to not notice. Imagine:

        assert(heroes[tag]
            .keys
            .where((Key key) => mostValuableKeys.contains(key))
            .someMuchLongerMethod(with, a, whole, bunch, of, arguments) <= 1);

Constructor named arguments aren't separated from their {} punctuation by spaces.

Do you do the same thing for optional positional parameters?

This, to me, is in the bucket of things where either style is fine but being consistent matters most. Do you think it's a compelling advantage to have spaces here? (Personally, I think the Dart syntax for optional parameters is so hopelessly ugly that it's impossible to format it gracefully, but dartfmt does the best it can.)

Wrapped lines are indented 4 by dartfmt, 2 by Flutter style.

You do that for all wrapped lines? That's different from Google's C++, Java, and JavaScript style guides. Don't you worry about confusing code like:

someFunction(very, long,
  parameter, list) {
  body(mixes, in, with, the, wrapped, parameters);
}

or:

if (some(long(condition(
  that, wraps) {
  body(mixes, in, with, the, wrapped, parameters);
}

Or, I suppose, you have the even worse:

if (some(long(condition(
  that, wraps)
  body(mixes, in, with, the, wrapped, parameters);

Combining some of the above, you end up with things like this code:

  const _HeroManifest(
      {this.key,
      this.config,
      this.sourceStates,
      this.currentRect,
      this.currentTurns});

Yeah, I agree that's not great. For what it's worth, we could change this in dartfmt without changing either of the rules you mention. We would just switch to formatting optional parameter lists as if they were collections.

[Hixie]

The formatter would avoid the 'goto fail' problem, because it would make it obvious that the second 'goto' wasn't in the "if" block. I'd much rather risk that, than risk the much more dangerous (IMHO) case of early returns being essentially invisible because they're hidden on the right end of a line in a dense block (in much the same way that you describe the <= 1 as being pushed "way over to the right and" made "really easy not to notice" later on).

The "one-line blocks don't get {}, multiline blocks do" rule is, specifically:

If a flow control structure's statement is one line long, then don't use braces around it, unless it's part of an "if" chain and any of the other blocks have more than one line. (Keeping the code free of boilerplate or redundant punctuation keeps it concise and readable.)

Where I wrote "symmetric punctuation" I meant "symmetric whitespace", my apologies. Whatever spaces (whether single spaces, newlines, etc) appear after an open bracket (of any kind), the same sequence of whitespace must appear before the matching close bracket (relative to the current indent level). Thus this:

 foo(bar(
   baz()
 ))

...or:

 foo(
   bar(baz())
 )

...or:

 foo(
   bar(
     baz()
   )
 )

...as opposed to:

 foo(bar(
   baz()))

I hope that clears that up.

In the example regarding "too eager", it was actually derived from code where what I wanted was a single 81 character line, rather than two lines. The formatter was more worried about minimising the average line length than about avoiding a line break after the assignment operator. (But I don't want to set the line limit to 81, because generally it should be 80.)

Regarding aligning expressions, I agree that it might be hard. I just don't want the formatter to make the alignment worse; if it's already aligned, then it should be left alone. I agree that it's hard to know whether it's "already aligned" or not.

For the => splitting case, if the code doesn't fit on one line, then the => should be replaced by { }.

The { } vs {} thing isn't a big deal, I was just listing all the things I noticed. (We developed our style before we knew of the Dart style guide.) I do think it's prettier, though. Specifically, I think it conveys "block of code" better than without the space. FWIW, we have a two or three dozen of these in Flutter's codebase so far, they look like this:

void detach() { }
void acceptGesture(int pointer) { }
void test() { setState(() { }); }

...etc. Typically either empty blocks in virtual methods that don't do anything in the class that introduces them, or calls to a method that takes a callback but where the call is really only wanted for the side-effect (so the callback is empty). If this was the only issue, we'd just change our style.

Re loops, I think it's telling that what we call a "one-line for loop", you call a "multiline for loop", and what you call a "one-line for loop", we call a style guide violation. :-P The control flow condition should never be on the same line as the control flow body, according to our style guide. It's just too easy to miss the control flow when you're scanning the code.

Regarding spaces around positional arguments, we don't currently do this but we have so few of them that it's hard to say if that's a conscious decision. We have named arguments all over the place and not having the space around the first and last but having the space after the comma separating them looks really weird to me. I don't think it's that ugly once you style it like we do. I do agree it's ugly if you style it the way that dartfmt does. ;-)

Your three examples would wrap like this in our style, which avoids the problems you mention:

someFunction(
  very,
  long,
  parameter,
  list
) {
  body(mixes, in, with, the, wrapped, parameters);                                                                             
}

if (some(long(condition(
  that, wraps
)))) {
  body(mixes, in, with, the, wrapped, parameters);
}

There are many examples of this in our code, because we have lots and lots of really long argument lists and lots and lots of nested calls in bodies.

Anyway. My goal here wasn't to argue about style. I just wanted to let you know why we're not currently using dartfmt.

[munificent]

The formatter would avoid the 'goto fail' problem, because it would make it obvious that the second 'goto' wasn't in the "if" block.

Good point.

I hope that clears that up.

Yes, it does, thanks. I think applying that rule even for ) looks nice, but it would be so disruptive to our existing code that it's probably not feasible to change.

For the => splitting case, if the code doesn't fit on one line, then the => should be replaced by { }.

The formatter only makes whitespace changes, for lots of reasons, so it can't do this fix-up. (There are lots of other simple AST changes like this it could make if it did.) So for stuff like this, it produces the best output it can and relies on the human to realize that the output doesn't match the style guide and adjust the actual code in a way that let's the formatter do its job.

A similar example is really long identifiers or string literals. The formatter can't force those to fit inside the line length, so it relies on you to help it out.

The { } vs {} thing isn't a big deal, I was just listing all the things I noticed. (We developed our style before we knew of the Dart style guide.) I do think it's prettier, though. Specifically, I think it conveys "block of code" better than without the space.

I do like that having a space makes code like this clearer:

takesTwoCallbacks(() {}, () => {}); // {} mean very different things here!

I don't know how the userbase would think about this change, but I filed a separate bug to track it: #445. I need to see what kind of impact it has but I think it's a reasonable change.

The control flow condition should never be on the same line as the control flow body, according to our style guide. It's just too easy to miss the control flow when you're scanning the code.

I personally use blank lines to make that clear.

Thanks for the details!

[Hixie]

I'm curious about the "no code transformation" rule, but I've filed #452 and #453 about that.

I personally use blank lines to make that clear.

We have a lot of functions that have a lot of early returns dotted around, but are otherwise relatively short and closely related to nearby code (e.g. a field, getter, and setter, where the setter has two early returns). Since they're related to each other we group them together without blank lines between them. If the function had blank lines, it would be really weird, a bit like having blank lines in the middle of a paragraph.

[kasperpeulen]

Yes, it does, thanks. I think applying that rule even for ) looks nice, but it would be so disruptive to our existing code that it's probably not feasible to change.

@munificent What do you mean with this? Do you mean the code in the dart_style package? Or existing code formatted with dartfmt? I would personally love symmetric punctuation.

[munificent]

Or existing code formatted with dartfmt?

This one. It would affect millions of lines of code. Worse, it would change them to be different from hundreds of millions of other lines of code written in C++, Java, and JS using style guides that don't line up ( and ).

[munificent]

Closing this out since I don't think I'll be taking action on the remaining couple of tasks. The others are all done now.