dart-lang / dart_style

An opinionated formatter/linter for Dart code
https://pub.dev/packages/dart_style
BSD 3-Clause "New" or "Revised" License
645 stars 117 forks source link

Rework how function types, type annotations, and typedefs are handled. #1493

Closed munificent closed 3 months ago

munificent commented 3 months ago

Rework how function types, type annotations, and typedefs are handled.

In the process of trying to simplify the number of Piece classes, I noticed that FunctionPiece basically exists to split between the return type annotation and the rest of a function. That's pretty similar to how VariablePiece handles splitting between a variable's type annotation and name.

I unified those, but then it made typedefs format funny. Looking into it, it's because typedefs have = but weren't using AssignPiece. Instead, they just never allowed splitting at the =. So I made that uniform with the rest of the style and used AssignPiece here.

That led to some weird looking code in cases like:

Function(int someParameter) someVariable;

If that line doesn't fit, the formatter has to decide whether to split inside the type annotation or between the type and variable. There were different heuristics for return types followed by function names versus type annotations followed by variable names. Unifying those led to some weird output like:

Function(
  int someParameter,
) Function(
  int someParameter,
) someVariable;

This is a variable whose type is a function that returns another function. Admittedly, no one writes code like this. Ultimately, I felt like the weirdness was from allowing the variable name to hang off the end of a split annotation. In most places in the style, if an inner construct splits, the outer one does too.

So I changed that. If a type annotation splits, then we also split after the type annotation too. That means after a return type before a function name, or after a variable type before a variable. So instead of allowing:

SomeGenericClass<
  LongTypeArgument,
  AnotherLongTypeArgument
> variable;

The split in the type argument list forces the variable name to split too:

SomeGenericClass<
  LongTypeArgument,
  AnotherLongTypeArgument
>
variable;

I think I like how this looks a little more but I'm not sure. In practice, it doesn't matter much because it's rare for a type annotation to be long enough to split, but it does happen. For what it's worth, it's consistent with metadata on variables. If the metadata splits, we also split before the variable too:

@SomeMetadata(
  'annotation argument',
  'another argument',
)
variable;

Thoughts?