goal: switch analyzer to use --use-fasta-parser by default

[devoncarew]

No description provided.

[devoncarew]

CL in progress here: https://dart-review.googlesource.com/c/sdk/+/67090

[devoncarew]

@danrubel, it looks like we're not able to build the SDK with this flag on by default (I assume mostly the change to the AnalysisOptions.useFastaParser flag default).

https://logs.chromium.org/logs/dart/buildbucket/cr-buildbucket.appspot.com/8939840433045031248/+/steps/build_dart/0/stdout

The build is failing when DDC tries to build the summary of the Dart SDK libraries. It doesn't proceed if it finds any compilation errors (warnings are OK). W/ this flag flip, we're now reporting lots of errors in places we hadn't before. I assume these were things that are special cased by the analyzer's parser? This must be handled by DDC already in some fashion, in order to build DDK.

[devoncarew]

and, /cc'ing @jmesserly, for the DDC / DDK question

[devoncarew]

Actually, to see the repro, you need to switch the default for --use-fasta-parser in the command line analyzer's options.dart file, delete the sdk's build cache, and then try and re-build the sdk.

[jmesserly]

I can take a look, thanks for letting me know!

Those errors look like the imports/part-of are not working in the SDK. In particular:

[error] Target of URI doesn't exist: 'classes.dart'. (dart:_runtime, line 33, col 6)

this means we fail to resolve part "classes.dart" from dart:_runtime ... that should look for a URI dart:_runtime/classes.dart and find it via the DartUriResolver. At least that's how it works in Analyzer right now. In dartdevk DDC sets up CFE with libraries.json, that's how SDK URIs are resolved.

How does URI resolution work when Analyzer is configured to useFastaParser? I can try to debug it and see what's going on.

Another question: does Analyzer support @patch when run with the CFE parser?

[jmesserly]

fyi -- I will be out for 3 weeks starting Monday. So I'll see if I can figure this out today. :)

[devoncarew]

How does URI resolution work when Analyzer is configured to useFastaParser? I can try to debug it and see what's going on.

I don't know for sure, but would suspect that the uri resolution is the same (that switching the parser doesn't change how we do resolution).

Another question: does Analyzer support @patch when run with the CFE parser?

@danrubel, do you know?

[jmesserly]

I haven't been able to reproduce this by setting the flag on AnalysisOptionsImpl (and cleaning my build output) ... is it possible I need to patch in the full CL?

Is there somewhere in Analyzer that I can add a print statement to see if the CFE parser is kicking in?

[jmesserly]

spoke too soon ... I was able to repro by pulling in part of that CL:

  static const bool useFasta =
      const bool.fromEnvironment("useFastaParser", defaultValue: true);

[devoncarew]

I was able to repro by pulling in part of that CL:

Awesome! I did find that I also had to delete my cached build of the SDK as well.

[jmesserly]

yeah I'd cleaned my output already. It needs clean output plus that static const bool useFasta change.

It looks like the patch_sdk script is broken. It's failing to copy some part files, that's why you need clean output. If you have those in your output directory it will appear to work. Also the static flag is required because patch_sdk uses the raw Analyzer APIs for parsing (unlike DDC proper, which uses resolved trees and AnalysisOptions).

I'll see if I can figure out why some part files are not getting copied.

[jmesserly]

parseDirectives fails to parse any directives for https://github.com/dart-lang/sdk/blob/master/pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/runtime.dart ... does that give y'all enough info to investigate?

[devoncarew]

Woohoo, thanks! A parser smoking gun :) I'll open a separate issue for that; thanks for the investigation.

[jmesserly]

this appears to be the fix to CFE:

diff --git a/pkg/front_end/lib/src/fasta/parser/parser.dart b/pkg/front_end/lib/src/fasta/parser/parser.dart
index 93c34fc709..499e652899 100644
--- a/pkg/front_end/lib/src/fasta/parser/parser.dart
+++ b/pkg/front_end/lib/src/fasta/parser/parser.dart
@@ -381,6 +381,7 @@ class Parser {
         token = parseScript(token);
       } else {
         token = parseMetadataStar(token);
+        final String value = token.next.stringValue;
         if (identical(value, 'import')) {
           directiveState?.checkImport(this, token);
           token = parseImport(token);

I'll leave this to someone more familiar with CFE to land. Basically, even though it parsed the metadata, it was still using the value variable that was still pointing at the @ metadata token.

[jmesserly]

I'm guessing library metadata isn't all that common, so this hasn't been hit. Also IDK if the parseDirectives is used by CFE normal parsing, it looks like it might be a special method (because it calls listener.endCompilationUnit(...)) so this code path might not be used much. Anyways YAY, I'm excited about having a single parser! 👍

[danrubel]

@jmesserly Yes, that was the problem. Thank you!
This should fix it... https://dart-review.googlesource.com/c/sdk/+/67440

[devoncarew]

Current known issues here:

• several unit test failures in pkg_tested/dart_style/test/formatter_test when the analyzer is switched to --use-fasta-parser #34040
• front_end/tool/perf_test fails when the analyzer is switched to --use-fasta-parser #34039

[devoncarew]

@jcollins-g just landed a CL to do this.