sdk icon indicating copy to clipboard operation
sdk copied to clipboard

Analyzer should pass experiment flags through to dart_style

Open munificent opened this issue 1 year ago • 5 comments

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.

munificent avatar Mar 06 '24 18:03 munificent

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.

scheglov avatar Mar 06 '24 19:03 scheglov

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.

bwilkerson avatar Mar 06 '24 19:03 bwilkerson

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 avatar Mar 08 '24 01:03 munificent

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.

munificent avatar Oct 10 '24 20:10 munificent

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.

scheglov avatar Oct 10 '24 21:10 scheglov