Format-on-save not picking up language version of surrounding package

[munificent]

I've got a work-in-progress CL to roll the new formatter into the SDK. This morning, I built the SDK and poked around in Visual Studio Code (which I have pointed at my just-built SDK). As most do, I have it set to auto-format when I save a file.

To my surprise, it applied the new formatting style to the code I was working on even though the surrounding package's language version is 3.0.

I tracked the problem down to analysis_server/lib/src/lsp/source_edits.dart. If I swap that out with DartFormatter.latestShortStyleLanguageVersion and rebuild the SDK, I get the old style. That means that result.unit.declaredElement?.library.languageVersion.effective must be returning null.

Do you have any idea why it isn't picking up the language version of the surrounding package?

I checked, and I didn't see any lingering .packages files, and the package does have a .dart_tool/package_config.json file with an appropriate language version set:

    {
      "name": "dart_style",
      "rootUri": "../",
      "packageUri": "lib/",
      "languageVersion": "3.0"
    }

I tried with two unrelated packages and got the same behavior in both.

[srawlins]

CC @scheglov @bwilkerson

[munificent]

Also cc @DanTup.

[DanTup]

result.unit.declaredElement?.library.languageVersion.effective must be returning null.

For formatting we currently only use a parsed (not resolved) AST, so declaredElement is always null.

@bwilkerson is there an accurate way to get the effective version without resolving the file (we can access languageVersionToken if it's in the source, but that won't handle the case where it's not)? Or would it be better to just resolve the file? (generally in the IDE case we're going to be resolving the file for other requests while typing anyway, so likely either it'll be cached, or we would cache it for a subsequent request).

[bwilkerson]

I'd be concerned about the performance of requiring every file to be resolved before it could be formatted.

For formatting we currently only use a parsed (not resolved) AST, so declaredElement is always null.

I would have assumed that we would have used the cached resolved AST when it's available. Seems like we could be more performant if we did. Not that it really matters here because we also need to be able to format without resolution data, so anything that depends on resolution data sometimes being there won't work.

There might be an easier way that I'm not aware of, but I believe we can get the information we need by checking the languageVersionToken, and using ExperimentStatus.currentVersion if it's null.

[DanTup]

I would have assumed that we would have used the cached resolved AST when it's available.

I'm not sure I understand. The code currently calls getParsedUnit - are you saying that this should return a resolved AST if one is cached? (Although even if it does, I don't think we could rely on there being one cached, so I think if we need a resolved AST, we need to ask for one?).

I believe we can get the information we need by checking the languageVersionToken, and using ExperimentStatus.currentVersion if it's null.

If there's no token, isn't the current version influenced by what's in the pubspec.yaml (the environment constraint)?

[bwilkerson]

... are you saying that this should return a resolved AST if one is cached?

Yes. There's no (significant) difference in the AST after it's been resolved. Using getParsedUnit is essentially just a promise that the caller won't try to access any resolution data (such as elements or types).

... isn't the current version influenced by what's in the pubspec.yaml ...

Yes, I'd missed that. For that we'd need to ask the AnalysisContext, but I can't see an API that allows access to that information. (We can ask for the analysis options, and ask the options for a FeatureSet, but it doesn't appear to have a way to access the _sdkLanguageVersion information. We'll probably need to add a getter somewhere to return that information.

[DanTup]

@munificent this should be fixed - let me know if you still see any issues with the change above.

Btw, for convenience, you can run the analysis server from source using the dart.analyzerPath setting in VS Code. You just need to make sure you're also using a compatible SDK (I periodically grab bleeding edge builds from here but if you've recently built one and are only changing the analysis server, that should work too):

(ofc, you'll need to run gclient sync if changing DEPS first)

{
"dart.analyzerPath": "/Users/dantup/Dev/dart-sdk/sdk/pkg/analysis_server/bin/server.dart",
"dart.sdkPath": "~/Dev/Dart/bleeding-edge" // Or recently built local SDK
}

[munificent]

@munificent this should be fixed - let me know if you still see any issues with the change above.

Tried it out this morning and it's working great now. Thank you!

It feels pretty magical to change the SDK version in my pubspec to "^3.7.0-edge", hit save, watch pub get run, and the next time I save a Dart file in VS Code, it gets reformatted with the new style.