[analysis_server] Need to plumb through experiment flags to DartFormatter

[parlough]

After dart-lang/dart_style#1256, dart_style through the DartFormatter that the server uses no longer enables available experiments automatically, so format requests in the IDE crash when trying out language experiments.

The analysis server should pass the experiments specified in the analysis_options.yaml file to the formatter.

[parlough]

\cc @DanTup Since I'm not sure if LSP or the VS Code extension accesses the formatter differently.

[DanTup]

For LSP we use the formatter by referencing dart_style and using it's API:

sdk/pkg/analysis_server/lib/src/lsp/source_edits.dart

Line 20 in efe3d00

DartFormatter _formatter = DartFormatter();

sdk/pkg/analysis_server/lib/src/lsp/source_edits.dart

Line 94 in efe3d00

formattedResult = _formatter.formatSource(code);

The analysis server should pass the experiments specified in the analysis_options.yaml file to the formatter.

It's possible we have multiple contexts with their own analysis_options.yaml, so we may need a new formatter per-context. I see we also create formatters in a few different places in the server (LSP, legacy protocol, src/g3, analyzer_plugin so each one might need to be updated.

FYI @bwilkerson @pq I dont' know if this might also be impacted by changes to contexts. I guess we need to be able to get from a path/parsed result to a DartFormatter that is configured for the analysis_options/context for that file (and ensure that formatter is updated/replaced if the analysis_options change).

[bwilkerson]

It's a little simpler than that. We don't cache the formatter, we re-create it every time it's needed. (It's a lightweight object, so instantiating one is inexpensive.) We will need to use the new APIs to determine which experiments are enabled, but those APIs already exist, so that should be doable now.

[DanTup]

It's a little simpler than that. We don't cache the formatter

Oh, that's much simpler. In LSP we currently do cache it and I thought I had copied that from legacy (and there was a reason for it), but perhaps it was just an optimisation I invented and can be removed :-)

@parlough do you have an example of how to trigger the crash you see? (and were you planning to look at this, or shall I?)

[bwilkerson]

... I thought I had copied that from legacy ...

It's possible that we're caching it and I didn't realize it, but I don't think we should need to.

[DanTup]

No I think you're right - I couldn't see any caching except in LSP, so I think that was my doing and it's unnecessary. Looking through https://github.com/dart-lang/dart_style/blob/main/lib/src/dart_formatter.dart I also don't see any state that seems like it would benefit from this.

[parlough]

@parlough do you have an example of how to trigger the crash you see?

Sorry, I wasn't fully clear, the analysis server doesn't crash, just fails to format. In IntelliJ this is surfaced as "Failed to format code", and in VS Code it doesn't seem any warning is shown to the user by default, the formatter just silently fails. (I've seen other people raise that as a point of confusion before).

For reproducing:

1. Enable inline-class experiment on a recent main SDK build that has the updated dart_style.
2. Add any extension type to your file. If I'm lazy, I just copy one from a test or the spec and make some changes that need formatting.
3. Format using the server

(and were you planning to look at this, or shall I?)

I'm not going to have time for a few days due to the US holiday, so if you want to take a look, that'd be appreciated! I also am not sure the best/desired way to get access to the analysis context from the source_edits file anyway, so I'll perhaps leave that to you :)

[parlough]

As for caching, yeah I agree it's not necessary from looking at the formatter.

[bwilkerson]

I'll also note that this isn't a high priority as the inline-class experiment has been enabled by default on main at this point. To the best of my knowledge the formatter doesn't yet support any of the not-yet-enabled experimental language features.

[parlough]

Unfortunately I don't think that will work this time, since dart_style currently defaults to 3.0 as its language version that it passes in to the analyzer:

https://github.com/dart-lang/dart_style/blob/8df008e51a1acdaeb7c0c077956760e2226e6254/lib/src/dart_formatter.dart#L222-L229

Maybe that needs to change its logic as well (otherwise dart format won't work by default either), or is it supposed to be manually updated once a feature is supported? \cc @munificent

Edit: Updating dart_style to 3.3: dart-lang/dart_style#1330

[DanTup]

I can reproduce the issue with a simple LSP test that uses extension type.

@bwilkerson I have a ParsedUnitResult and can get .unit.featureSet (and the same via .session.analysisContext.getAnalysisOptionsForFile(unit.path)) but the only way I can see to check experiments is by checking known features and I'm not sure if that's what we really want here (presumably, we want to pass just the raw set of experiments defined by the user in analysis_options - although I do wonder whether it would be better if we could just pass the whole FeatureSet to the formatter?).

[bwilkerson]

Unfortunately I don't think that will work this time, since dart_style currently defaults to 3.0 as its language version that it passes in to the analyzer ...

You're right. The default language version has to be increased before extension types will be parsed correctly.

Technically the formatter ought to be passing in the real default language version as specified by the minimum SDK constraints. That's something that the analyzer can probably compute for it, so it shouldn't require too much work.

But I have to wonder whether we might want the formatter to just always use the version of the language that's supported by the version of the analyzer package that it's running against. What I don't know is whether the formatter would break if it encountered an unhandled class of AST node or whether it guards against that in some reasonable fashion (such as refusing to format the file, or just leaving the unrecognized portion as-is without attempting to format it).

presumably, we want to pass just the raw set of experiments defined by the user in analysis_options ...

It looks like the ExperimentStatus has the information you're referring to, but it doesn't make it publicly available. We'd need to plumb it through to expose it from AnalysisOptions.

... although I do wonder whether it would be better if we could just pass the whole FeatureSet to the formatter?

I've actually suggested a few times that the formatter might want an API for the server to use that would allow us to pass in the already parsed AST. That would be slightly more efficient, though parsing is really fast, so not a huge performance gain. But assuming that the formatter wouldn't crash given ASTs containing nodes it doesn't yet know about, it would eliminate the need to pass this information back into the formatter.

I suspect, though, that we need @munificent's input before we can go much farther in deciding how best to communicate this data to the formatter.

[munificent]

Maybe that needs to change its logic as well (otherwise dart format won't work by default either), or is it supposed to be manually updated once a feature is supported? \cc @munificent

Edit: Updating dart_style to 3.3: dart-lang/dart_style#1330

Yes, it should be updated. Sorry I missed that one. Thank you for the fix!

[parlough]

Closing in favor of #55125 which has a bit more focused discussion on the remaining tasks here. Thanks all!