Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tweak block argument heuristics #1548

Merged
merged 2 commits into from
Aug 28, 2024
Merged

Conversation

munificent
Copy link
Member

@munificent munificent commented Aug 28, 2024

This produces output that better matches what users expected in the associated issues. I think it also looks better in the affected regression tests. And I ran this on a large corpus and looking at the diffs, these rules seem to be much better across the board.

The new rules are basically:

  • A block formatted argument list can't end up with named arguments on multiple lines.

  • Only one trailing argument is allowed after the block argument.

The existing rules are also kept:

  • Prefer a function block argument over a collection one.

  • Only allow a single block argument.

  • Allow the first argument to be adjacent strings with a later function block argument (to handle calls like test() and group()).

Fix #1527.
Fix #1528.
Fix #1543.

To determine which argument list in an argument list should be block
formatted, if any, we need to look at all of them. We can't do it
eagerly because the heuristics are generally of the form "If only one
argument is like X, then it is block formatted."

But before this commit, DelimitedListBuilder never saw all arguments at
once. It was given them one at a time. That meant that it had to stuff
the information needed to select a block argument (BlockFormat) into
ListPiece. This is kind of dumb because once we've selected a block
argument, that data is no longer needed, but ListPiece lives all the
way through the rest of the formatting process.

Fortunately, the only expressions that support block arguments are
argument lists, and those *do* add all of the arguments to the
DelimitedListBuilder at once. So this commit adds a method to
DelimitedListBuilder to pass them all in.

There are no behavior changes in this commit. This is just rearranging
code (and saving a bit of memory) to get ready for tweaking the
heuristics.
This produces output that better matches what users expected in the associated issues. I think it also looks better in the affected regression tests. And I ran this on a large corpus and looking at the diffs, these rules seem to be much better across the board.

The new rules are basically:

* A block formatted argument list can't end up with named arguments on multiple lines.

* Only one trailing argument is allowed after the block argument.

The existing rules are also kept:

* Prefer a function block argument over a collection one.

* Only allow a single block argument.

* Allow the first argument to be adjacent strings with a later function block argument (to handle stuff like test() and group()).

Fix #1527.
Fix #1528.
Fix #1543.
Copy link
Member

@natebosch natebosch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW I'd be on board to recommend using named arguments anywhere to move named test and group arguments before the callback argument for the best formatting.

@munificent
Copy link
Member Author

munificent commented Aug 28, 2024

FWIW I'd be on board to recommend using named arguments anywhere to move named test and group arguments before the callback argument for the best formatting.

I should have mentioned this initially but I investigated this. I agree that it would be better for those named arguments to be moved before the lambda, but it turns out that it's still pretty common in existing code for those arguments to be trailing. I looked at all calls to functions named test(), group(), and testWidgets() in a corpus of pub packages:

-- Signatures (4126 total) --
   3312 ( 80.271%): ("...", () {...})
    459 ( 11.125%): ("...", () {...}, name: arg)
    173 (  4.193%): (arg)
     48 (  1.163%): (arg, arg)
     44 (  1.066%): ("...", arg)
     42 (  1.018%): (arg, () {...})
      8 (  0.194%): ()
      8 (  0.194%): ("...", name: arg, () {...})
      6 (  0.145%): (arg, () {...}, name: arg)
      5 (  0.121%): (arg, arg, arg)
      4 (  0.097%): (arg, arg, name: arg, name: arg, name: arg, name: arg, name: arg, name: arg)
      3 (  0.073%): ("...", () {...}, name: "...")
      3 (  0.073%): ("...", () {...}, name: arg, name: arg)
      3 (  0.073%): ("..\n..", () {...})
      1 (  0.024%): (arg, () {...}, name: arg, name: arg, name: arg, name: arg, name: arg)
      1 (  0.024%): (arg, arg, name: arg)
      1 (  0.024%): (arg, arg, name: arg, name: arg, name: arg, name: arg, name: arg)
      1 (  0.024%): ("...", arg, name: "...")
      1 (  0.024%): (arg, name: arg, () {...})
      1 (  0.024%): (arg, () {...}, name: arg, name: arg, name: arg, name: arg)
      1 (  0.024%): ("...", arg, name: arg, name: arg, name: arg, name: arg, name: arg, name: arg)
      1 (  0.024%): ("...", arg, name: arg, name: arg, name: arg, name: arg)

Over 10% of calls have a trailing named argument, so I figured it was worth allowing that directly to avoid disrupting code more than necessary.

@munificent munificent merged commit 05d08d9 into main Aug 28, 2024
7 checks passed
@munificent munificent deleted the tweak-block-argument-heuristics branch August 28, 2024 23:26
munificent added a commit that referenced this pull request Aug 28, 2024
This was fixed by #1548.

Fix #1533.
munificent added a commit that referenced this pull request Aug 29, 2024
@natebosch
Copy link
Member

it turns out that it's still pretty common in existing code for those arguments to be trailing.

Yeah, there are a whole bunch from before named-arguments-anywhere was an option, and I suspect they get copy/paste often enough that many authors aren't even aware of the option. Making the formatter work well for the existing corpus sounds good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants