auto-apply dartfmt fixes

[jakemac53]

Closes #551

I think this is pretty reasonable, especially since you can override the behavior.

@munificent are there any cases you know about where a fix would break code or all they all super safe?

[natebosch]

My main concern is the potential performance impact of running fixes on all code when the vast majority won't need it. I'm slightly concerned about papering over stale behavior in generators, but I think probably the benefits outweigh that concern.

[jakemac53]

Some of this is stale generators, but some of it is also just that its a lot easier to write a generator that way. For instance one of the fixes removes cascades where there is only a single one. But a generator might generate multiple cascades sometimes and not others and its pretty reasonable for it to just always generate some code using cascades, and then also pretty reasonable to have a tool come along and clean those up later?

[jakemac53]

maybe @munificent could speak to any potential perf impacts, but we are already ok with the perf impacts of formatting generally here and non of these rules stand out to me as non-local (ie: slow) fixes.

[jakemac53]

Fwiw this is the list of fixes:

  static const docComments = StyleFix._(
      'doc-comments', 'Use triple slash for documentation comments.');

  static const functionTypedefs = StyleFix._(
      'function-typedefs', 'Use new syntax for function type typedefs.');

  static const namedDefaultSeparator = StyleFix._('named-default-separator',
      'Use "=" as the separator before named parameter default values.');

  static const optionalConst = StyleFix._(
      'optional-const', 'Remove "const" keyword inside constant context.');

  static const optionalNew =
      StyleFix._('optional-new', 'Remove "new" keyword.');

  static const singleCascadeStatements = StyleFix._('single-cascade-statements',
      'Remove unnecessary single cascades from expression statements.');

[munificent]

I don't think there's anything wrong with this in general, but, for what it's worth, I have always designed the fixes with the idea in mind that a human would likely be around to vet the output. They don't promise to be as fire-and-forget as formatting changes are. It should be safe, but I can't promise that there won't be future fixes where you might not want to auto-apply them.

[jakemac53]

It should be safe, but I can't promise that there won't be future fixes where you might not want to auto-apply them.

I think if that does come up, I would be willing to update this with a specific list instead of just applying all fixes.

Another alternative though could be to only apply the cascade fix. All the other ones really should be fixed at the generator level, in an ideal world?

wdyt @natebosch ?

[natebosch]

Another alternative though could be to only apply the cascade fix. All the other ones really should be fixed at the generator level, in an ideal world?

I have a slight preference for doing that, but I would be fine with either choice.

[kevmoo]

I'm leaning against this. Maybe add it as something folks could opt-in to?

[jakemac53]

Maybe add it as something folks could opt-in to?

Folks could already opt into this today, but the builder has to do it unfortunately.

They could add their own config to make it user configurable (a list of fixes to opt into, etc), but we don't have an easy way of exposing something uniform as far as user settings go really.

[jakemac53]

I'm leaning against this.

Are you opposed to doing even just the cascade fix? All the others are things that builder authors really could fix on their own I think.

[natebosch]

@kevmoo - can you expand on your concerns?

[kevmoo]

@kevmoo - can you expand on your concerns?

Whitespace is a no-brainer. Other fixes seem like more of a stretch. I've had cases where autofixes mess up source code. 🤷

It seems a bit too magic. I'mambivalentt, though. If ya'll think it's the right thing – run w/ it!

[jakemac53]

@natebosch should I revive this? It only applies the cascade fix now.

[natebosch]

Yeah, I think applying the cascade fix makes sense.