How should the formatter handle missing package_config.json files?

[munificent]

@stuartmorgan ran into pain when the new formatter in the Dart SDK broke a Flutter CI. The problem was that the format CI check wasn't running dart pub get before running dart format. That meant the package didn't have a package config, so the formatter defaulted to the latest language version (and thus the new style) even though the code was actually on an older version and should have used the older style.

When I made the formatter language-version aware, I had it look for a .dart_tools/package_config.json file. This is consistent with how dart analyze, dart run, etc. behave.

For most users editing code on their local machine, that's probably fine. They will have already run dart pub get and have a valid package config. But the formatter is often run on CI and possibly other contexts where that has happened. Right now, in those places, the behavior you get is silently applying the new style (and then failing CI). What should we do instead?

Some ideas:

1. Like other dart tools, dart format could automatically run dart pub get on the user's behalf if it thinks it's necessary. I'm not sure exactly what "thinks it's necessary" means or how the other tools detect that they need to run dart pub get. Probably if it finds a pubspec but no corresponding package config, or the package config's mod time is older than the pubspec's?

   This does the right thing, but it means running the formatter might mutate the file system outside of the formatted files. I'm not sure how I feel about that.

2. If we don't find a package config, look for a pubspec. If found, print an error and tell the user to run dart pub get. This is more explicit than (1), but also probably more annoying. If the tool knows what the problem is, why does it yell at the user to solve it?

3. Read the language version directly from the pubspec.yaml file. The formatter could parse the pubspec, look for the SDK constraint, and infer the language version from that the same way that pub does. No need to run pub or mutate the file system.

   We'd have to decide how it behaves if there is both a package config and a pubspec. Do we only read the pubspec if there's no package config, or do we always prefer the pubspec?

   This would mean that the formatter is on the hook to consistently and correctly implement the same logic for inferring a language version from the pubspec that pub uses. If that logic changes (for example, by allowing different constraint syntax in the SDK constraint), then the formatter has to be updated too.

4. Do nothing. It's an annoying UX if users hit it but we can mostly avoid it with good messaging. Once the world is on the new style, it won't affect many users.

@dart-lang/language-team, any thoughts on the right approach? I lean towards 3, but I'm on the fence.

[stuartmorgan]

That meant the package didn't have a package config, so the formatter defaulted to the latest language version (and thus the new style) even though the code was actually on an older version and should have used the older style.

Importantly for why it was painful: at first I could not figure out why this was happening, because when I ran exactly the same command locally I got the old formatter. It was only when after a while of being confused when I ran it over my entire local repo instead of one package I hit—via pure luck—a package that I happened not to have done anything else with locally since the last time I cleaned my entire local repo. And from there I was able to trial-and-error my way into determining what the trigger was.

Even then I remained pretty confused, because it's non-obvious to people who don't work on the internals of these tools that "You typically control the language version by setting a min SDK constraint in your package's pubspec." (from the WIP changelog) actually means "You typically control the language version by setting a min SDK constraint in your package's pubspec and then running pub get".

[stuartmorgan]

4. Do nothing.

If we don't want a complex solution, 2 would be dramatically better than 4 IMO. It is hard to overstate how much worse mysterious failures are than explicit ones.

[leafpetersen]

Here is an issue about running pub get automatically. I don't have a lot of context with where this is, other than remembering being very confused when this first rolled out and I tried to analyze something offline and it didn't work (hopefully the UX has gotten better for that case). It does feel weird that formatting would run pub get.

[jakemac53]

The pubspec is the better source of truth (if present), so if we are going to look there I would trust that over the package config (this will also handle the case of editing the SDK constraint but not running a pub get).

I don't love the idea of the formatter having to look for both files, but it also feels wrong for a pub get to be required as a part of formatting. I could see either way being ok.

Afaik the other commands just always run a dart pub get, and rely on the fact that pub will not actually do anything if a lockfile is present and still valid. cc @bkonyi can confirm I believe.

[bkonyi]

Afaik the other commands just always run a dart pub get, and rely on the fact that pub will not actually do anything if a lockfile is present and still valid. cc @bkonyi can confirm I believe.

I don't know exactly which commands run dart pub get before executing, but I don't think all commands perform a dart pub get. dart test and dart run both do and I'd assume dart compile would as well.

[munificent]

It's worth noting that the reason that other tools run pub get and then use the resulting package_config.json file is because that specifies the language version of not just the current package but all of the (transitive) packages it depends on. You need that information if you're going to statically analyze, compile, or run a Dart program.

But dart format doesn't need that. It only needs the language version of the current package. Given that, it should be sufficient to read the pubspec and infer the language version from there. Having the formatter treat the pubspec as the source of truth instead of also comparing it to the package_config.json file and trying to determine if the config is out of date is probably simpler.

And the formatter already does YAML parsing to read the page width configuration (alas, from a different file), so it's not taking on another dependency to read the pubspec.

The formatter probably should still support the package_config file too, for rare cases where a user has a config but isn't using pub (like the Dart SDK itself).

[eernstg]

Drive-by comment: It sounds like option 3 (rely on pubspec.yaml, use the latest language version if there is none) would have a couple of very nice properties:

• pubspec.yaml is highly visible to users as they need to create and maintain it. Hence, it's a meaningful place to go to if a question comes up like "which language version is dart format actually using?". It is in any case the place where changes would need to be made. This means that it naturally fits the role as the 'source of truth'.
• .dart_tools/package_config.json is somewhat invisible. It is possible to create it (and align it with pubspec.yaml) by running dart pub get, but it is not itself the source of truth.
• It makes sense that other tools depend on .dart_tools/package_config.json and insist that it should be in a fully updated state when analysis/compilation/etc. proceeds (which might be achieved by running dart pub get implicitly). Those tools need all the information in .dart_tools/package_config.json. However, it's tempting to say that running dart pub get is overkill when we only want to know the language version, and if we don't want to run dart pub get implicitly then pubspec.yaml should be used rather than a (potentially out-of-date or missing .dart_tools/package_config.json).

This would mean that the formatter is on the hook to consistently and correctly implement the same logic for inferring a language version from the pubspec that pub uses. If that logic changes (for example, by allowing different constraint syntax in the SDK constraint), then the formatter has to be updated too.

Could the implementation be shared?

The formatter probably should still support the package_config file too, for rare cases where a user has a config but isn't using pub (like the Dart SDK itself).

OK, so there could be a reason to be able to use .dart_tools/package_config.json when pubspec.yaml doesn't exist (or perhaps it would be possible to add a suitable pubspec.yaml to the SDK and similar special locations?).

[lrhn]

Either:

• dart-cli automatically runs pub get for you, if you have a pubspec.yaml and no package_config.json or one that is out-of-date. That's consistent with every other tool that depends on language version, and requires no extra code, just opting in to being one of those tools.
• Or have dart format itself look for pubspec.yaml and then ./dart_tool/package_config.json itself, in that order, repeating in parent directories, and using the first one it finds.

The error mode of using it in a Pub package directory with no package_config.json and defaulting to no language version is not a good user experience.
The goal shoud be that dart format works and does what it is intended to do. And to be able to format a file that has no pubspec.yaml or package_config.json (as the latest language version, or the version of its language version marker).

A distant second would be erroring out and saying that you have to run dart pub get, but if it can see that, it might as well do it for you.

Even then, looking only for .dart_tool/package_config.json might skip past a pubspec.yaml file which haven't had dart pub get run yet, which is still potentially incorrect.
A package repo can contain more than one pubspec.yaml file, fx in the example directory. If you run dart pub get in the root, it won't run it in the example directory, and then finding the root package-config might be wrong.

• Like other dart tools, dart format could automatically run dart pub get on the user's behalf if it thinks it's necessary.

That is the canonical way to bring the package directory into a useful state where Dart tools can work on the Dart source (and correctly determine whether it's even valid Dart source at all.)

There is no tool which depends on Dart language versions which can be trusted to work, correctly or at all, on code without an associated language version, which usually means a .dart-tool/package_config.json file. The formatter has now joined that club, so it should get the same (consistent!) treatment as every other such tool.
The formatter used to be able to pretend that language versions didn't exist. It had a good run.

Or it can look for the answer itself, and make sure it still works for a file that has no configuration file at all.
(It should still work with a .dart_tool/package_config.json file if there is no pubspec.yaml file.)

That means: Starting with the current directory of the Dart file it wants to find a version for:

• If pubspec.yaml exists in the directory, read it.
  * If it declares a min-SDK constraint, use that.
  * If it doesn't, scream bloody murder and refuse to do anything.
  (I don't think we default to current-language-version in that case.)
• If not, if .dart_tool/package_config.json exists relative to the directory, read that.
  * If it assignes the current directory a package with a language version, use that.
  * Otherwise use the current language version.
• Otherwise if a parent directory exists, go to that and repeat from two steps above.
• Otherwise use the current language version.

That's something that can be computed once and cached for each directory.

It prefers pubspec.yaml if there is one. If package_config.json hasn't been generated yet,
or is out of date, then we assume that that is precisley the case, and the formatter is just ahead
of the rest of the tools in using the correct language version, without spending the time on regenerating the package config file.

A Pub package (as defined by a pubspec.yaml file) should have a package_config.json, and the only thing the formatter does differently is that it doesn't force that file to be created, it just assumes what Pub would generate for a standard Pub package (a package entry with "rootUri": ".", "packageUri": "lib/").

Otherwise if .dart_tool/package_config.json exists, use that over anything else.
That is what all other tools will use (and have Pub generate if they need to).

Even if that package config doesn't actually assign a package to the current directory, still don't look further.
(A hand-crafter package_config.json could choose to not assign a package to the directory of the .dart_tool, just to some sub-directories. Is that what workspaces already do?)
Even so, the placement of the .dart_tool/package_config.json means that it is the canonical source of packages and versioning for that entire directory, and all subdirectories that don't have a closer config. It's possible for a directory inside that to have no associated package and therefore no associated version.

(And if all files have a language version marker, you might never need to find the default language version, if you are willing to do a little pre-parsing to check for that. It has to occur before any Dart declaration in the file.)

[jakemac53]

A package repo can contain more than one pubspec.yaml file, fx in the example directory. If you run dart pub get in the root, it won't run it in the example directory, and then finding the root package-config might be wrong.

This is a very compelling argument imo for always looking for a pubspec.yaml as well and preferring those

[munificent]

OK, we talked about this in the language meeting this morning. We decided that the formatter will do Lasse's second suggestion. It will walk surrounding directories looking for either a pubspec.yaml or .dart_tools/package_config.json. As soon as it finds either, it will use that file to infer the language version. If it finds both in the same directory, then the pubspec.yaml wins. (If they disagree, it is almost certainly because the package_config.json file is out of date.)

I'll also look into making this logic available as a library from the dart_style package so that tools using the formatter as a library have access to that behavior if they want it.

[munificent]

OK, here is an annoying wrinkle. Let's say you have a pair of packages like:

shared_stuff/
  lib/
    analysis_options.yaml containing:
      | formatter:
      |   page_width: 100

my_app/
  pubspec.yaml containing:
    | environment:
    |   sdk: >=3.6.0
    | dependencies:
    |   shared_stuff: any

  analysis_options.yaml containing:
    | include: package:shared_stuff/analysis_options.yaml

  bin/
    main.dart

So you've got a package my_app that configures its page width by including an analysis_options.yaml file from another package. This pattern is very common for lints, and I expect it will be pretty common for organization-wide page width configuration.

Let's say you haven't run pub get on my_app. So you have a pubspec but no package config. Can you format this package correctly?

No. The formatter will be able to figure out the language version from the pubspec.yaml file. But it won't be able to figure out the page width from the analysis_options.yaml because it can't process the package: include without having a package config.

(In retrospect, if I knew the formatter might read the pubspec.yaml file directly, I may have said the page width should be configured in there instead of in analysis_options.yaml. That would sidestep the issue around handling package: includes, at the expense that there would be no way to define page widths shared across multiple projects.)

Since you will still need to run pub get to get reliably accurate formatting, maybe reading the pubspec.yaml file to get the language version isn't worth it.

[lrhn]

Can you format this package correctly?

Obviously not. If your formatting configuration can depend on data from another package, and no file from that other package can be resolved without running pub get, then you can't know the intended formatting configuration.

This assumes that there is a correct formatting, and it's just not known yet. There isn't necessarily one correct formatting if the line length can depend on the version-solve. Changing one constraint can lead to including a different version of the imported analysis_options.yaml file, and a completely different formatting of the entire repo. Fun times!
That means, IMO, that if you have a package_config.json, you should probably use that, even if pubspec.yaml disagrees. The data in package_config.json is also (potentially) used to resolve the analysis options import. Using the language version from pubspec.yaml and the line length impled by package_config.json may be inconsistent.
If someone has edited pubspec.yaml, that doesn't do anything until they run pub get.

I do feel that analysis_options is a better fit for a "style configuration" than pubspec.yaml, so even if you could change your mind and put the formatter configuration into pubspec.yaml, it might not be the best place.

How is analysis_options.yaml discovered? Is it "look up the directory chain" independently of looking for package resolution, or does it stop at a pubspec.yaml and/or package_config.json?
Does an imported analysis_options.yaml do its own search for .dart_tool/package_config.json to resolve a package: URI import, or does it use the configuration of the file that imported it? (I hope the latter, anything else gets weird pretty fast. I'll assume that.)

Consider a package structure of:

  analysis_options.yaml -- contains `import "package:ourstyle/style.yaml"` which has a line length.
  pubspec.yaml
  example/
     pubspec.yaml
     file.dart

Here trying to format file.dart will find pubspec.yaml in the same directory. Even if it runs pub get on that,
that doesn't run pub get in the parent directory, so the analysis_options.yaml in the parent directory still
cannot resolve the package:ourstyle.
The analyzer does analysis-options and package-config discovery for every directory being analyzed. Or inside the analysis root at least. Here it would find the analysis_options.yaml even with an analysis root of example/, but it would then use the package config from example/ to understand it. It also never runs pub get.

Maybe the only way to win is not to play :)

If every configuration is inside the same Pub package root directory (pubspec.yaml, .dart_tool/package_config.json and analysis_options.yaml) then we will do something smart.

Otherwise you'll likely have to have run pub get in the other directories. (Does Pub have some way to link pubspecs, so that doing pub get on one will also do pub get on another, say so that you can do dart pub get in the root of a package and have example/pubspec.yaml pub-getted too. @jonasfj)

We're already stretching things by treating the file as an entry point (so its package configuration is located relative to the file), but that makes sense for Pub packages.

Maybe we need to:

• Find the closes enclosing analysis_options.yaml for the file to format. If found, look for a line length,
  or else an import, following imports until reaching a package:-URI.

  • If not line length is found before a package: URI, then find the closest enclosing
    .dart_tool/package_config.json/pubspec.yaml for the original analysis_options.yaml file.
  • If that's a pubspec.yaml, run pub get on the pubspec.yaml file, and continue with the
    generated package_config.json.
  • Use the package_config.json, resolve the URI and keep following imports, using the same
    package configuration, until a line length is found.
  • (If at any time a file doesn't exist or has an invalid format, bail out with an error.)

• If no analysis options file is found, or no declaration of line length is in the found file or its imports,
  use default line length.

• Then find the closest enclosing .dart_tool/package_config.json/pubspec.yaml for the file to format. Prefer
  package_config.json if it's there. (If that happens to be the same package root as for the analysis options,
  we may have already run pub get). Get the default language version for the file from that.
  (If the file is not inside any package of the enclosing .dart_tool/package_config.json, use current language
  version.)

• If no enclosing file is found, use current language version.

(Which can be interleaved into a single loop, which will most often end at the same point anyway.)

That is, you only need to do a pub get if there is an analysis options file with no line-length until a (transitive) import with a package: URI, and there is a pubspec.yaml file with no corresponding .dart_tool/package_config.json.

In that case, it'd probably also be OK to just say "import URI can't be found, please run pub get".
After all, the configuration that they are asking the formatter to use depends on running pub get.
(Or it can do it itself, which would be the nicer feature.)

If formatting a directory, find the configurations on the way down, rather then trying to find them for each .dart file.
You always know whether you've seen anything in the parent directories anyway.

[munificent]

Changing one constraint can lead to including a different version of the imported analysis_options.yaml file, and a completely different formatting of the entire repo. Fun times!

I did think through that edge case when I first added support for page width configuration. As far as I can tell, it's not a problem. The page width used to format code inside a package is always based on that package's own dependency solve, not the solve of other packages that may be using it transitively.

For example, say my_app depends on foo and bar which both depend on shared_config. Both foo and bar have analysis_options.yaml files containing include: 'package:shared_config/analysis_options.yaml'. For perverse example reasons, every version of shared_config's analysis_options.yaml file sets a page width equivalent to that minor version + 80. So version 1.0.0 of shared_config sets a page width of 80, version 1.1.0 is 81, etc.

Now lets say that my_app, foo, and bar are all on your local machine in sibling directories and my_app has path dependencies on foo and bar. You've run pub get on all three packages and:

• my_app selected shared_config 1.0.0
• foo selected shared_config 1.1.0
• bar selected shared_config 1.2.0

If you then run dart format on the parent directory that contains all three of those package folders:

• The files inside my_app/ will use the package_config.json for my_app and be formatted at page width 80.
• The files inside foo/ will use the package_config.json for foo and be formatted at page width 81.
• The files inside bar/ will use the package_config.json for bar and be formatted at page width 82.

Unlike compiling and analyzing, the entrypoint package's dependency graph never affects how a file is formatted. It's always formatted according to its own surrounding package's version solve. We don't start at the files in my_app, traverse their imports to find stuff in foo and then start formatting foo according to that original context. Each file is formatted alone according to its own surrounding context.

So as far as I can tell, it all works out fine.

Using the language version from pubspec.yaml and the line length impled by package_config.json may be inconsistent. If someone has edited pubspec.yaml, that doesn't do anything until they run pub get.

Right. That's why I no longer think it's a good idea to read the pubspec.yaml for the language version. The best we can probably do is look for a pubspec.yaml and report a warning if we find one and don't find a package config or the package config is out of date.

I do feel that analysis_options is a better fit for a "style configuration" than pubspec.yaml, so even if you could change your mind and put the formatter configuration into pubspec.yaml, it might not be the best place.

Me too. In particular, since there is a lint that checks page width, it seems reasonable to configure the page width in the same place that lints are configured.

How is analysis_options.yaml discovered? Is it "look up the directory chain" independently of looking for package resolution, or does it stop at a pubspec.yaml and/or package_config.json?

It walks the directory chain just looking for analysis_options.yaml files. It ignores pubspecs and package configs. You can have an analysis_options.yaml file in a subdirectory of a package and it will only affect files in that subdirectory. Or you can have an analysis_options.yaml file in a directory that contains a bunch of package directories and all of those packages will share those analysis options. (I don't know whether those are particularly great semantics, but they are how analyzer behaves as I understand it, so the formatter works the same way.)

Does an imported analysis_options.yaml do its own search for .dart_tool/package_config.json to resolve a package: URI import, or does it use the configuration of the file that imported it? (I hope the latter, anything else gets weird pretty fast. I'll assume that.)

It uses the original package configuration of the first analysis_options.yaml file. The latter, which is definitely what we want.

Consider a package structure of:

  analysis_options.yaml -- contains `import "package:ourstyle/style.yaml"` which has a line length.
  pubspec.yaml
  example/
     pubspec.yaml
     file.dart

Here trying to format file.dart will find pubspec.yaml in the same directory. Even if it runs pub get on that, that doesn't run pub get in the parent directory, so the analysis_options.yaml in the parent directory still cannot resolve the package:ourstyle. The analyzer does analysis-options and package-config discovery for every directory being analyzed. Or inside the analysis root at least. Here it would find the analysis_options.yaml even with an analysis root of example/, but it would then use the package config from example/ to understand it. It also never runs pub get.

Maybe the only way to win is not to play :)

Yeah, you can definitely craft weird situations if you try, but I suspect most users simply don't do that. If a user has an analysis_options.yaml file that isn't logically inside a single package but does contain a "package:" include... who knows how they expected it to behave, but odds are good it won't do anything particularly useful.

If every configuration is inside the same Pub package root directory (pubspec.yaml, .dart_tool/package_config.json and analysis_options.yaml) then we will do something smart.

Otherwise you'll likely have to have run pub get in the other directories. (Does Pub have some way to link pubspecs, so that doing pub get on one will also do pub get on another, say so that you can do dart pub get in the root of a package and have example/pubspec.yaml pub-getted too. @jonasfj)

We're already stretching things by treating the file as an entry point (so its package configuration is located relative to the file), but that makes sense for Pub packages.

Maybe we need to:

• Find the closes enclosing analysis_options.yaml for the file to format. If found, look for a line length,
  or else an import, following imports until reaching a package:-URI.

  • If not line length is found before a package: URI, then find the closest enclosing
    .dart_tool/package_config.json/pubspec.yaml for the original analysis_options.yaml file.
  • If that's a pubspec.yaml, run pub get on the pubspec.yaml file, and continue with the
    generated package_config.json.
  • Use the package_config.json, resolve the URI and keep following imports, using the same
    package configuration, until a line length is found.
  • (If at any time a file doesn't exist or has an invalid format, bail out with an error.)

• If no analysis options file is found, or no declaration of line length is in the found file or its imports,
  use default line length.

• Then find the closest enclosing .dart_tool/package_config.json/pubspec.yaml for the file to format. Prefer
  package_config.json if it's there. (If that happens to be the same package root as for the analysis options,
  we may have already run pub get). Get the default language version for the file from that.
  (If the file is not inside any package of the enclosing .dart_tool/package_config.json, use current language
  version.)

• If no enclosing file is found, use current language version.

That is, uh, a lot of complexity. The only reason the formatter has been successful so far is because (for capacity reasons), I've been ruthless about restricting complexity. I'm trying to dial that up a little bit because users really do want project-wide page width configuration. But I don't have the capacity to add a lot of smarts in how to pokes around in the file system. That would just be an unending bug farm. I don't want to spend the rest of my days dealing with bugs like "I have an analysis_options.yaml file that is actually a symlink to one in another package's directory that is nested inside another directory that isn't a pub package but has an explicit package_config.json file...".

In that case, it'd probably also be OK to just say "import URI can't be found, please run pub get". After all, the configuration that they are asking the formatter to use depends on running pub get. (Or it can do it itself, which would be the nicer feature.)

Currently, if it doesn't have a package config to resolve "package:" includes, or the package config doesn't specify the named package, it simply silently fails. I don't love silent failures, but it does keep things simpler.

If formatting a directory, find the configurations on the way down, rather then trying to find them for each .dart file. You always know whether you've seen anything in the parent directories anyway.

There are likely some annoying failure modes or at least performance issues with that since the user can pass multiple potentially overlapping directory paths on the command line. I just have it find configurations for each file but then cache them on a per-directory basis.

[lrhn]

Don't have to try to be smart.
This was a very-best effort attempt to not run pub get unless it's absolutely needed.

The non-smart approach is:

• find nearest enclosing analysis options file. Resolve imports, and if any depends on a package configuration, find the nearest enclosing package configuration for the analysis option file.
• find nearest enclosing package configuration for the file to format.

You find an enclosing package configuration by finding the nearest enclosing pubspec.yaml/.dart_tool/package_config.json. If it's not a JSON file, run pub get on the pubspec to generate it.
(Probably does have to be workspace aware.)

The code for the latter is general. It can probably be written to also do pub get if the package_config.json is out of date, but the formatter doesn't need that.
What means it should be able to share whatever code other dart commands use to trigger running pub get.
(Just possibly, although unlikely, twice. Once for the analysis options file and once for the file to format.)

Then cache line length and language version per directory.

(We can always change what happens when the cache is empty for a directory.)

[jonasfj]

Having dartdev give the formatter a way to invoke ensurePubspecResolved (passing a callback) might be okay. Then the formatter can invoke ensurePubspecResolved:

• before it reads package_config.json for language version, and,
• before it interprets analyzer_options.yaml.

The only downside is that the formatter might have issues in places where package_config.json and pubspec.yaml exists, but the package_config.json is hand written. The only place we know this to happen is the SDK.

We've contemplated introducing resolution: external to pubspec.yaml to alleviate such issues.

cc @sigurdm

---

In general: We aim for ensurePubspecResolved to extremely fast! We want dartdev to run it implicitly. The side effects of running pub get are generally better than throwing an error (because the error almost always says to run pub get).