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

[munificent]

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?

[kallentu]

I guess I'm interested in both your thoughts on this, but I think I find the variable being on a new line harder to skim through the parameter lists. Not sure if it's just something to get used to.

[munificent]

Yeah, I agree it can get gnarly in a big parameter list. And this one is particularly bad because I think it's a huge pile of generated code.

The old formatter has a style where if a return type or type annotations splits, it indents the thing after it, like:

main() {
  SomeReallySuperLongTypeAnnotation<EvenWith, SomeTypeArguments>
      aLongLocalVariable;
}

SomeReallySuperLongTypeAnnotation<EvenWith, SomeTypeArguments>
    aLongFunctionName() {}

class C {
  SomeReallySuperLongTypeAnnotation<EvenWith, SomeTypeArguments>
      aLongFieldDeclaration;
}

I've always felt that was kind of weird. It doesn't do that for metadata:

main() {
  @SomeReallySuperLongMetadataAnnotation<EvenWith, SomeTypeArguments>()
  var aLongLocalVariable;
}

@SomeReallySuperLongMetadataAnnotation<EvenWith, SomeTypeArguments>()
aLongFunctionName() {}

class C {
  @SomeReallySuperLongMetadataAnnotation<EvenWith, SomeTypeArguments>()
  var aLongFieldDeclaration;
}

That feels kind of strange and inconsistent to me. Both the metadata and the type are "annotations" applied to some member, but a type annotation causes the member to be indented but the metadata doesn't.

In the new style, I got rid of the indentation when return types and type annotations split:

main() {
  SomeReallySuperLongTypeAnnotation<EvenWith, SomeTypeArguments>
  aLongLocalVariable;
}

SomeReallySuperLongTypeAnnotation<EvenWith, SomeTypeArguments>
aLongFunctionName() {}

class C {
  SomeReallySuperLongTypeAnnotation<EvenWith, SomeTypeArguments>
  aLongFieldDeclaration;
}

I think that generally looks nicer. The idea is that the actual declaration—function, parameter, variable, etc.— should be on the left. If there is stuff before it that doesn't fit on the same line, it gets split, but the declaration itself is still left-aligned.

But when there is a split inside the return type itself, we have to decide whether that forces a split after the return type too. Before the PR, the answer was no, so you could get:

main() {
  SomeReallySuperLongTypeAnnotation<
    EvenWith,
    SomeTypeArguments,
    AThirdTypeArgument
  > aLongLocalVariable;
}

This PR changes it to:

main() {
  SomeReallySuperLongTypeAnnotation<
    EvenWith,
    SomeTypeArguments,
    AThirdTypeArgument
  >
  aLongLocalVariable;
}

That agrees with the previous principle that the actual declaration should always be at the beginning of the line. I like that idea. It does mean that a bunch of declarations one after the other can get a little confusing:

  TResult when<TResult extends Object?>({
    required TResult Function() initialize,
    required TResult Function(AppState state) receiveUpdatedAppState,
    required TResult Function() completedInitialFlow,
    required TResult Function(
      RouteId<CommonPageRoutePaywall> routeId,
    )
    dismissPaywall,
    required TResult Function(
      NotificationPermission status,
    )
    receiveNotificationPermissionStatus,
  }) {}

For what it's worth, I don't think that's really any worse than the current output:

  TResult when<TResult extends Object?>({
    required TResult Function() initialize,
    required TResult Function(AppState state) receiveUpdatedAppState,
    required TResult Function() completedInitialFlow,
    required TResult Function(
      RouteId<CommonPageRoutePaywall> routeId,
    ) dismissPaywall,
    required TResult Function(
      NotificationPermission status,
    ) receiveNotificationPermissionStatus,
  }) {}

I think the best answer is for the author to put some blank lines in:

  TResult when<TResult extends Object?>({
    required TResult Function() initialize,
    required TResult Function(AppState state) receiveUpdatedAppState,
    required TResult Function() completedInitialFlow,

    required TResult Function(
      RouteId<CommonPageRoutePaywall> routeId,
    )
    dismissPaywall,

    required TResult Function(
      NotificationPermission status,
    )
    receiveNotificationPermissionStatus,
  }) {}

Or use some typedefs so they don't get splits inside their type annotations. In practice, code like this is very rare. There are a ton of tests affected by this PR, but that's because I used local variable declarations to write all of the tests for type annotation formatting and splitting. :)

[natebosch]

My gut reaction is that the new style is harder to read, but I'm hoping that's more a factor of familiarity than innate readability. I don't think I have a problem with treating metadata annotations differently from type annotations, but in general I agree with your reasoning here. I think we should run with this left-aligned style.

[kallentu]

I'm hoping that's more a factor of familiarity than innate readability

I also hope so.

For the previous style for that specific example, every initial character down the column is an alphabetic character (required in this case) or ).

My brain has been trained by the previous indenting style to say, okay if it's an alphabetical character (assuming no annotations), it's the start of a new parameter. That won't be the case with the new left-aligned style.

The short style parameter list of this test looked like the following. And I really liked how "skimmable" this was.

  TResult when<TResult extends Object?>({
    required TResult Function() initialize,
    required TResult Function(AppState state) receiveUpdatedAppState,
    required TResult Function() completedInitialFlow,
    required TResult Function(RouteId<CommonPageRoutePaywall> routeId)
        dismissPaywall,
    required TResult Function(NotificationPermission status)
        receiveNotificationPermissionStatus,
    required TResult Function(Object error, StackTrace stackTrace) receiveError,
 // ...
  }) {
    return projectOptions(routeId, action);
  }

[munificent]

Yeah, that's a good point. It is easier to the see the beginning of each parameter like that. Some of that has to do with each starting with required. I still think it's very hard to find the parameter names in that example, though. What I wouldn't give to be able to do:

  when<TResult extends Object?>({
    initialize: () -> TResult,
    receiveUpdatedAppState: (state: AppState) -> TResult,
    completedInitialFlow: () -> TResult,
    dismissPaywall: (routeId: RouteId<CommonPageRoutePaywall>) -> TResult,
    receiveNotificationPermissionStatus:
        (NotificationPermission status) -> TResult,
  }): TResult {}

The real issue is that the natural way to format something is to have the most important thing left-most and indent the subordinate stuff. But with types on the left, the subordinate stuff comes first. :(

If it's OK with you, let's try the style here because it simplifies the formatter implementation somewhat. In practice, it's very rare for type annotations to split anyway, so I suspect it won't matter much. We can always iterate on it in the future if users don't like it, and having a simpler set of Piece classes now makes it easier to improve the performance.

[kallentu]

I'm okay with the changes.
I think Bob's reasoning works well for members or top-level declarations, but I'm a little wary for complex parameter lists when you have so many one after another.