re-write flutter analyze (the single-shot and --flutter-repo) to use the analysis server by devoncarew · Pull Request #16281 · flutter/flutter

devoncarew · 2018-04-05T17:47:09Z

Re-write flutter analyze (the single-shot mode and --flutter-repo) to use the analysis server.

not yet ready for review -

…the analysis server

devoncarew · 2018-04-10T18:54:21Z

OK, this is now ready for review. Previously, we had:

flutter analyze implemented using the dartanalyzer command-line tool
flutter analyze --flutter-repo implemented using the package:analyzer APIs
flutter analyze --watch implemented using the analysis server

This was a lot of different code paths and implementations for doing flutter analysis. Additionally, the specific package:analyzer APIs that flutter analyze was using are deprecated, and won't support the full fidelity of the changes for the Dart 2.0 type system; it's important to move flutter analyze off of them.

This PR then:

re-writes flutter analyze and flutter analyze --flutter-repo to be based on the analysis server
for flutter analyze --flutter-repo, changes from passing 29 different directories into the analysis server to 3 top level ones; we see a substantial performance benefit from this (initial analysis of the repo using the analysis server drops from 90 seconds to 40)
some clean up to the 3 different analysis options files in this repo to reduce duplication. We now have one top level analysis_options.yaml file for the repo (analysis_options_repo.yaml is deleted); one small delta to that in /packages/ to turn on public_member_api_docs, and one in /packages/flutter_tools to reference the top level options file, and not enable public_member_api_docs
removed the feature of flutter analyzeing individual files (just directories are supported now)

Locally, I see a perf speedup in the initial (cold) repo analysis of 25% (50s to 40s), and additional (warm) analysis speedup of 3x (50s to 16s).

Since we're using the analysis server, flutter analyze is analyzing the user's pubspec.yaml file, and will now catch issues in the flutter: section (we have some lints for the assets section).

tvolkert · 2018-04-10T22:28:45Z

dev/bots/analyze-sample-code.dart

    final Process process = await Process.start(
      _flutter,
-      <String>['analyze', '--no-preamble', mainDart.path],
+      <String>['analyze', '--no-preamble', '--no-congratulate', mainDart.parent.path],


Can you not analyze just one file anymore?

No, that is a use case that this PR removes.

(and, it is one that I want to remove, for implementation simplicity, and to reduce the overall use case complexity here)

FWIW, as a user, I find that not being able to analyze a single file (or rather, a single entry point, plus any files it might import directly or indirectly in the same package) is really annoying. I will often, for example, copy a file as a backup and then make backwards-incompatible changes which cause the analyzer to complain about the backup file. I also often will have a dummy package where I have multiple different "main.dart" entry points (named various things), and I only care about the one I'm editing, even though many of the others have tons of problems because I was just copying and pasting other people's code into them.

tvolkert · 2018-04-10T22:35:13Z

dev/devicelab/bin/tasks/dartdocs.dart

-      if (entry == 'Building flutter tool...') {
+    await for (String entry in analysis.stderr.transform(utf8.decoder).transform(const LineSplitter())) {
+      print('analyzer stderr: $entry');
+      if (entry.startsWith('[lint] ')) {


Lints show up in stderr, but info, warning, and error show up in stdout?? wat?

How come this uses the [...] syntax but above you have the bullet point syntax?

The [...] message is specifically generated elsewhere ([lint] $undocumentedMembers public members lack...); I'm just preserving current behavior as there's already a fair amount going on in this PR.

This single message shows in stderr (not infos, warnings, or errors as they're part of the regular tool output). It's part of the exit message if there were undoc'ed members found.

We could choose to change this is this PR - to clarify that this is a marker message generated elsewhere - or add some comments to that effect.

tvolkert · 2018-04-10T22:51:50Z

packages/analysis_options.yaml

@@ -0,0 +1,8 @@
+# Take our settings from the repo's main analysis_options.yaml file, but include
+# an additional rule to validate that public members are documented.


Awesome (🎉 for not having two almost-duplicated options files)!

Yeah, all the copies have comments saying to please make changes to all files, and all the files have separate diffs...

tvolkert · 2018-04-10T23:29:00Z

packages/flutter_tools/lib/src/commands/analyze.dart

-    argParser.addOption('write', valueHelp: 'file', help: 'Also output the results to a file. This is useful with --watch if you want a file to always contain the latest results.');
-    argParser.addOption('dart-sdk', valueHelp: 'path-to-sdk', help: 'The path to the Dart SDK.', hide: !verboseHelp);
+  AnalyzeCommand({bool verboseHelp: false, this.workingDirectory}) {
+    argParser.addFlag('flutter-repo',


This should probably be hidden

tvolkert · 2018-04-10T23:30:04Z

packages/flutter_tools/lib/src/commands/analyze.dart

+        help: 'Include all the examples and tests from the Flutter repository.',
+        defaultsTo: false);
+    argParser.addFlag('current-package',
+        help: 'Analyze the current project, if applicable.', defaultsTo: true);


Versus analyzing what? Does it even work to pass --no-current-package?

It does work, however I would be happy to remove the option; leaving it in preserves current behavior.

tvolkert · 2018-04-10T23:50:58Z

packages/flutter_tools/lib/src/commands/analyze.dart

+        defaultsTo: false);
+    argParser.addFlag('current-package',
+        help: 'Analyze the current project, if applicable.', defaultsTo: true);
+    argParser.addFlag('dartdocs',


Does this do anything anymore?

Yup, it should have the same behavior as before. For the flutter repo, without it, the cli tool will report the number of undocumented members. With it, it'll report and list them individually (and fail if there are any undoc'd members).

Hixie · 2018-04-11T04:39:49Z

analysis_options.yaml

 #   - analysis_options.yaml (this file)
-#   - analysis_options_repo.yaml
 #   - packages/flutter/lib/analysis_options_user.yaml
 #   - https://github.com/flutter/plugins/blob/master/analysis_options.yaml


we should probably change this back to four and point to https://github.com/flutter/engine/blob/master/analysis_options.yaml while we're at it...

Hixie · 2018-04-11T04:44:10Z

packages/flutter_tools/lib/src/commands/analyze_continuously.dart

-
  void _handleAnalysisErrors(FileAnalysisErrors fileErrors) {
-    fileErrors.errors.removeWhere(_filterError);
+    fileErrors.errors.removeWhere((AnalysisError error) => error.type == 'TODO');


can't we turn these off in the options file?

We now have the same options file for IDE and cli analysis. You do want to see these in the IDE (or use IDE level settings to hide them). They're not valuable on the cli; we filter them out from the dartanalyzer cli tool as well.

Hixie · 2018-04-11T04:45:12Z

packages/flutter_tools/lib/src/commands/analyze_once.dart

+    if (argResults['flutter-repo']) {
+      // check for conflicting dependencies
+      final PackageDependencyTracker dependencies =
+      new PackageDependencyTracker();


wrapping is off here

devoncarew

Thanks for taking a pass through! PR updated

devoncarew · 2018-04-11T14:29:03Z

analysis_options.yaml

 #   - analysis_options.yaml (this file)
-#   - analysis_options_repo.yaml
 #   - packages/flutter/lib/analysis_options_user.yaml
 #   - https://github.com/flutter/plugins/blob/master/analysis_options.yaml


devoncarew · 2018-04-11T14:37:39Z

dev/bots/analyze-sample-code.dart

    final Process process = await Process.start(
      _flutter,
-      <String>['analyze', '--no-preamble', mainDart.path],
+      <String>['analyze', '--no-preamble', '--no-congratulate', mainDart.parent.path],


No, that is a use case that this PR removes.

devoncarew · 2018-04-11T14:40:30Z

dev/devicelab/bin/tasks/dartdocs.dart

-      if (entry == 'Building flutter tool...') {
+    await for (String entry in analysis.stderr.transform(utf8.decoder).transform(const LineSplitter())) {
+      print('analyzer stderr: $entry');
+      if (entry.startsWith('[lint] ')) {


The [...] message is specifically generated elsewhere ([lint] $undocumentedMembers public members lack...); I'm just preserving current behavior as there's already a fair amount going on in this PR.

This single message shows in stderr (not infos, warnings, or errors as they're part of the regular tool output). It's part of the exit message if there were undoc'ed members found.

devoncarew · 2018-04-11T14:41:11Z

packages/analysis_options.yaml

@@ -0,0 +1,8 @@
+# Take our settings from the repo's main analysis_options.yaml file, but include
+# an additional rule to validate that public members are documented.


Yeah, all the copies have comments saying to please make changes to all files, and all the files have separate diffs...

devoncarew · 2018-04-11T14:41:37Z

packages/flutter_tools/lib/src/commands/analyze.dart

-    argParser.addOption('write', valueHelp: 'file', help: 'Also output the results to a file. This is useful with --watch if you want a file to always contain the latest results.');
-    argParser.addOption('dart-sdk', valueHelp: 'path-to-sdk', help: 'The path to the Dart SDK.', hide: !verboseHelp);
+  AnalyzeCommand({bool verboseHelp: false, this.workingDirectory}) {
+    argParser.addFlag('flutter-repo',


devoncarew · 2018-04-11T14:42:43Z

dev/bots/analyze-sample-code.dart

    final Process process = await Process.start(
      _flutter,
-      <String>['analyze', '--no-preamble', mainDart.path],
+      <String>['analyze', '--no-preamble', '--no-congratulate', mainDart.parent.path],


(and, it is one that I want to remove, for implementation simplicity, and to reduce the overall use case complexity here)

devoncarew · 2018-04-11T14:43:42Z

packages/flutter_tools/lib/src/commands/analyze.dart

+        help: 'Include all the examples and tests from the Flutter repository.',
+        defaultsTo: false);
+    argParser.addFlag('current-package',
+        help: 'Analyze the current project, if applicable.', defaultsTo: true);


It does work, however I would be happy to remove the option; leaving it in preserves current behavior.

devoncarew · 2018-04-11T14:45:45Z

packages/flutter_tools/lib/src/commands/analyze.dart

+        defaultsTo: false);
+    argParser.addFlag('current-package',
+        help: 'Analyze the current project, if applicable.', defaultsTo: true);
+    argParser.addFlag('dartdocs',


Yup, it should have the same behavior as before. For the flutter repo, without it, the cli tool will report the number of undocumented members. With it, it'll report and list them individually (and fail if there are any undoc'd members).

devoncarew · 2018-04-11T14:46:48Z

packages/flutter_tools/lib/src/commands/analyze_continuously.dart

-
  void _handleAnalysisErrors(FileAnalysisErrors fileErrors) {
-    fileErrors.errors.removeWhere(_filterError);
+    fileErrors.errors.removeWhere((AnalysisError error) => error.type == 'TODO');


We now have the same options file for IDE and cli analysis. You do want to see these in the IDE (or use IDE level settings to hide them). They're not valuable on the cli; we filter them out from the dartanalyzer cli tool as well.

devoncarew · 2018-04-11T14:47:20Z

packages/flutter_tools/lib/src/commands/analyze_once.dart

+    if (argResults['flutter-repo']) {
+      // check for conflicting dependencies
+      final PackageDependencyTracker dependencies =
+      new PackageDependencyTracker();


tvolkert · 2018-04-11T15:06:30Z

packages/flutter_tools/lib/src/commands/analyze.dart

+    argParser.addOption('write',
+        valueHelp: 'file',
+        help:
+            'Also output the results to a file. This is useful with --watch if you want a file to always contain the latest results.');


It looks like you're going for dartfmt style in this file, in which case let's wrap these long strings:

help: 'Also output the results to a file. This is useful with --watch if ' 'you want a file to always contain the latest results.');

tvolkert · 2018-04-11T15:10:16Z

packages/flutter_tools/lib/src/commands/analyze_continuously.dart

      final PackageDependencyTracker dependencies = new PackageDependencyTracker();
      dependencies.checkForConflictingDependencies(repoPackages, dependencies);
-      directories = repoPackages.map((Directory dir) => dir.path).toList();
+      directories = repoRoots;


Just remove the local variable and refer to repoRoots directly?

I cleaned up the logic here a bit -

tvolkert

LGTM

danrubel

1 to simplifying the "flutter analyze" implementation and reducing the number of analysis options files.
Looks like good progress to me.

… to use the analysis server (#16281)" This reverts commit 2f41ea5.

… to use the analysis server (#16281)" (#16482) This reverts commit 2f41ea5.

…the analysis server (flutter#16281) re-write flutter analyze (the single-shot and --flutter-repo) to use the analysis server

… to use the analysis server (flutter#16281)" (flutter#16482) This reverts commit 2f41ea5.

re-write flutter analyze (the single-shot and --flutter-repo) to use …

8599a0a

…the analysis server

googlebot added the cla: yes label Apr 5, 2018

devoncarew added 2 commits April 5, 2018 11:06

update the analyze-sample-code.dart script

bbd2d8d

make sure flutter analyze --watch doesn't fail on missing docs

254610f

devoncarew mentioned this pull request Apr 5, 2018

move flutter analyze off of the task model dart-lang/sdk#32478

Closed

devoncarew added 5 commits April 9, 2018 19:21

Merge branch 'master' into flutter_analyze

d06dda6

update to pass in fewer directories to analyze

f64cfc9

fix error display on windows

31975b5

unskip a now working test

209a592

make windows tests more robust

16e745b

devoncarew requested review from Hixie, danrubel and tvolkert April 10, 2018 18:54

tvolkert reviewed Apr 10, 2018

View reviewed changes

Hixie reviewed Apr 11, 2018

View reviewed changes

devoncarew added 2 commits April 11, 2018 07:29

Merge branch 'master' into flutter_analyze

4d4daa1

review comments

f529bff

devoncarew commented Apr 11, 2018

View reviewed changes

tvolkert reviewed Apr 11, 2018

View reviewed changes

tvolkert approved these changes Apr 11, 2018

View reviewed changes

review comments; wrap lines

9b21d10

danrubel approved these changes Apr 11, 2018

View reviewed changes

devoncarew merged commit 2f41ea5 into flutter:master Apr 11, 2018

devoncarew added a commit that referenced this pull request Apr 11, 2018

Revert "re-write flutter analyze (the single-shot and --flutter-repo)…

11e65c7

… to use the analysis server (#16281)" This reverts commit 2f41ea5.

devoncarew mentioned this pull request Apr 11, 2018

Revert "re-write flutter analyze (the single-shot and --flutter-repo) to use the analysis server" #16482

Merged

devoncarew added a commit that referenced this pull request Apr 11, 2018

Revert "re-write flutter analyze (the single-shot and --flutter-repo)…

0714852

… to use the analysis server (#16281)" (#16482) This reverts commit 2f41ea5.

devoncarew mentioned this pull request Apr 25, 2018

re-write flutter analyze to use the analysis server #16979

Merged

DaveShuckerow pushed a commit to DaveShuckerow/flutter that referenced this pull request May 14, 2018

Revert "re-write flutter analyze (the single-shot and --flutter-repo)…

2c67208

… to use the analysis server (flutter#16281)" (flutter#16482) This reverts commit 2f41ea5.

github-actions bot locked as resolved and limited conversation to collaborators Aug 11, 2021

		@@ -0,0 +1,8 @@
		# Take our settings from the repo's main analysis_options.yaml file, but include
		# an additional rule to validate that public members are documented.

Conversation

devoncarew commented Apr 5, 2018

Uh oh!

devoncarew commented Apr 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devoncarew left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!