dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.24k stars 1.58k forks source link

Analyzer should pass experiment flags through to dart_style #55125

Open munificent opened 8 months ago

munificent commented 8 months ago

The dart_style library API now accepts experiment flags. I added support for that when I made the formatter not hardcode all known experiments. It passes those flags onto the parser when it parses code to format it.

My understanding is that analyzer doesn't currently pass the enabled experiments set in analysis_options.yaml through to the formatter when it formats. It probably should so that the formatter parses the code using the same experiments that analyzer itself is using.

scheglov commented 8 months ago

We don't have experimental flags easily when we format. We convert experiments into FeatureSet, and this is what parseString() wants. So, I propose that DartFormatter accepts FeatureSet instead.

bwilkerson commented 8 months ago

Or, as an alternative, consider accepting a ParsedUnitResult, this would negate the need for knowing the experiment flags because it would

munificent commented 8 months ago

We don't have experimental flags easily when we format. We convert experiments into FeatureSet, and this is what parseString() wants. So, I propose that DartFormatter accepts FeatureSet instead.

I'm hesitant to have the formatter's library API depend on a type from the analyzer package. I know, obviously, that the formatter's implementation is heavily dependent on analyzer. But from the user's perspective, that's an implementation detail.

Or, as an alternative, consider accepting a ParsedUnitResult, this would negate the need for knowing the experiment flags because it would

  • contain a CompilationUnit that was parsed with all of the correct experiments enabled
  • provide access to the FeatureSet
  • be more efficient because most of the time we will already have the result computed before we call the formatter's API.

Ooh, that latter bullet point is definitely appealing. I see that ParsedUnitResult also exposes a LineInfo which the formatter needs. The formatter's API doesn't accept a bare AST node because we also need that line info in places where the input newlines are significant. But it looks like ParsedUnitResult would work.

Let me think about it some more. I'm still on the fence about having analyzer types in the public API, but maybe it's worth doing.

munificent commented 1 month ago

I took a little look at this today. Adding support for formatting a given ParsedUnitResult is pretty easy. However, I'm not sure how to test it. As far as I can tell, there's no public API in analyzer to create a ParsedUnitResult for a given piece of code. And I'm not allowed to implement ParsedUnitResult with my own mock implementation either.

Is there something I'm missing, or a better way to go about this? I certainly like the idea of the formatter not having to reparse code when invoked by the analyzer.

scheglov commented 1 month ago

Something like this

import 'package:analyzer/dart/analysis/analysis_context_collection.dart';
import 'package:analyzer/dart/analysis/results.dart';
import 'package:analyzer/file_system/overlay_file_system.dart';
import 'package:analyzer/file_system/physical_file_system.dart';

Future<void> main() async {
  var resourceProvider = OverlayResourceProvider(
    PhysicalResourceProvider.INSTANCE,
  );

  var workspacePath = '/workspace';
  var testPackageRootPath = '$workspacePath/test';
  var testFilePath = '$testPackageRootPath/lib/test.dart';

  resourceProvider.setOverlay(
    testFilePath,
    content: r'''
class A {}
class B {}
''',
    modificationStamp: -1,
  );

  var collection = AnalysisContextCollection(
    resourceProvider: resourceProvider,
    includedPaths: [
      workspacePath,
    ],
  );

  var analysisContext = collection.contextFor(testFilePath);
  var analysisSession = analysisContext.currentSession;
  var parsedUnit = analysisSession.getParsedUnit(testFilePath);
  parsedUnit as ParsedUnitResult;
  print(parsedUnit.unit);
}

We could almost use MemoryResourceProvider, but AnalysisContextCollection needs Dart SDK, so to do this you would need to use non-API createMockSdk from package:analyzer/src/test_utilities/mock_sdk.dart, and then pass its sdkPath.