update flutter analyze to call dartanalyzer #8808

danrubel · 2017-03-15T20:58:20Z

Now that dartanalyzer uses the default Flutter analysis options (package:flutter/analysis_options_user.yaml) to analyze a flutter project,
and a Dart SDK containing this functionality has rolled into Flutter...

Update flutter analyze to use dartanalyzer for everything except analyzing the flutter repository.

Contains many but not all of the changes in the original PR
Fixes Consider upping severity of missing required hints to warnings in analysis options. #8087 which appears to have regressed, and adds a test
Incrementally improves flutter analyze
One step closer to having flutter analyze always delegate to dartanalyzer
so that flutter tools might someday not be tightly coupled to analyzer

@Hixie

Hixie · 2017-03-15T21:01:25Z

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

+    // Use dartanalyzer directly when possible
+    if (!flutterRepo) {
+      final String dartanalyzer = fs.path.join(
+          Cache.flutterRoot, 'bin', 'cache', 'dart-sdk', 'bin', 'dartanalyzer');


nit: newline before ) since there was a newline after the (

Hixie · 2017-03-15T21:01:40Z

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

+
+      if (cmd.length < 2 || currentPackage) {
+        final Directory projDir = await projectDirFor(fs.currentDirectory.absolute);
+        if (projDir != null) {


nit: no braces around one-line statements

Hixie · 2017-03-15T21:01:54Z

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

+      if (cmd.length < 2 || currentPackage) {
+        final Directory projDir = await projectDirFor(fs.currentDirectory.absolute);
+        if (projDir != null) {
+          cmd.add((projDir).path);


nit: i think the inner parentheses are redundant here?

Artifact of refactoring tools. Removed.

Hixie · 2017-03-15T21:07:05Z

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

+    if (!flutterRepo) {
+      final String dartanalyzer = fs.path.join(
+          Cache.flutterRoot, 'bin', 'cache', 'dart-sdk', 'bin', 'dartanalyzer');
+      final List<String> cmd = <String>[dartanalyzer];


nit: please don't use abbreviations for variable names.

Renamed cmd to arguments. Hopefully that's more clear.

Hixie · 2017-03-15T21:09:50Z

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

+      final List<String> cmd = <String>[dartanalyzer];
+      cmd.addAll(dartFiles.map((FileSystemEntity f) => f.path));
+
+      if (cmd.length < 2 || currentPackage) {


I got a bit confused reading this, in part because I was reading "cmd" as "our command line" as opposed to "the command line we are building", and in part because it took me a while to realise that "< 2" was equivalent to "nothing got added in the previous step, and so there's one entry, the analyzer itself".

I suggest adding the path to the analyzer after all this work of adding the files to be analyzed (inserting at index 0). That would let you change this from length < 2 to isEmpty which I think would be clearer.

I like that approach. Done.

Hixie · 2017-03-15T21:09:57Z

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

+      cmd.addAll(dartFiles.map((FileSystemEntity f) => f.path));
+
+      if (cmd.length < 2 || currentPackage) {
+        final Directory projDir = await projectDirFor(fs.currentDirectory.absolute);


nit: no abbreviations

Hixie · 2017-03-15T21:11:48Z

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

    }

    final bool currentDirectory = argResults['current-directory'] && (argResults.wasParsed('current-directory') || dartFiles.isEmpty);
    final bool currentPackage = argResults['current-package'] && (argResults.wasParsed('current-package') || dartFiles.isEmpty);


I think we should remove --current-directory in this patch. This would simplify the logic here and is consistent with the long term plan in #7274.

Makes sense. Done.

Hixie · 2017-03-15T21:14:20Z

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

+        if (projDir != null) {
+          cmd.add((projDir).path);
+        }
+      } else if (currentDirectory) {


imho we should remove this here and just let people use . if they want this.

Hixie · 2017-03-15T21:15:25Z

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

+        }
+      } else if (currentDirectory) {
+        cmd.add('.');
+      } else if (!cmd[1].startsWith(fs.currentDirectory.path)) {


I don't understand what this is doing. Why does the path of the first command line argument matter?

Also is path guaranteed to end in a separator?

If the files being analyzed are outside of the current directory hierarchy then dartanalyzer does not yet know how to find the .packages file. I'll add a comment to that effect.

Hmm... not sure. Modified the code to ensure that it does.

Hixie · 2017-03-15T21:16:34Z

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

+      stopwatch.stop();
+      final String elapsed = (stopwatch.elapsedMilliseconds / 1000.0).toStringAsFixed(1);
+      if (exitCode != 0)
+        throwToolExit('(Ran in ${elapsed}s)');


propagate the exit code?

Good idea. Done.

Hixie · 2017-03-15T21:17:28Z

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

-        if (isDartFile(entry)) {
-          dartFiles.add(entry);
-          foundOne = true;
+    // Use dartanalyzer directly when possible


this comment should elaborate on why it's not possible if flutterRepo is true.

Good point. Done.

Hixie · 2017-03-15T21:18:48Z

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

    }
  }

+  Future<Directory> projectDirFor(FileSystemEntity entity) async {


nit: no abbreviations (several cases in this method including the method name)

Renamed to projectDirectoryContaining.

Hixie · 2017-03-15T21:19:55Z

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

      else
        throwToolExit('(Ran in ${elapsed}s)');
    }
    if (argResults['congratulate']) {


Update the docs to mention that congratulate will be ignored if you're not using either --watch or --flutter-repo?

Help text updated.

Hixie · 2017-03-16T03:44:27Z

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

+    // Use dartanalyzer directly except when analyzing the Flutter repository.
+    // Analyzing the repository requires a more complex report than dartanalyzer
+    // currently supports (e.g. missing member dartdoc summary).
+    // TODO(danrubel) enhance dartanalyzer to provide this type of summary


nit: missing color after TODO(danrubel)

Hixie · 2017-03-16T03:45:29Z

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

+      final List<String> arguments = <String>[];
+      arguments.addAll(dartFiles.map((FileSystemEntity f) => f.path));
+
+      if (arguments.length < 1 || currentPackage) {


arguments.length < 1 is better written arguments.isEmpty
Amusingly this is a creative-enough way to write isEmpty that the lint that looks for this kind of thing didn't catch it. :-)

Hixie · 2017-03-16T03:47:02Z

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

+          final Directory projDir = await projectDirectoryContaining(target);
+          if (projDir != null) {
+            final String packagesPath = fs.path.join(projDir.path, '.packages');
+            if (packagesPath != null) {


how can it be null?

Heh. Left over from refactoring. Removed null check.

Hixie · 2017-03-16T03:47:23Z

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

+          final String targetPath = arguments[0];
+          final FileSystemEntity target = await fs.isDirectory(targetPath)
+              ? fs.directory(targetPath) : fs.file(targetPath);
+          final Directory projDir = await projectDirectoryContaining(target);


nit: projDir is two abbreviations

Missed that one when renaming. Fixed.

Hixie · 2017-03-16T03:49:31Z

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

+        // In this situation, use the first file as a starting point
+        // to search for a "pubspec.yaml" and ".packages" file.
+        // TODO(danrubel): fix dartanalyzer to find the .packages file
+        if (!arguments[0].startsWith(currentDirectoryPath)) {


I think we should explicitly check that all the files would find the same .packages (or all would find no .packages?), and exit with a message saying that we don't yet support checking multiple packages at once if you're not also checking the flutter repo. I've definitely run into the case where I've tried to use flutter analyze to analyze two packages at once (an app I was working on and the support library I was writing for the app), and it would be massively confusing if that did something different depending on the order in which the paths were listed.

Good point. I simplified the code to always pass in the .packages file path and report an error as you suggested.

Hixie · 2017-03-16T03:51:33Z

packages/flutter_tools/test/analyze_once_test.dart

+      toolExit: false,
+    );
+  });
+}


Hixie · 2017-03-16T03:52:51Z

packages/flutter_tools/test/analyze_once_test.dart

+        '[lint] Only throw instances of classes extending either Exception or Error',
+        '1 warning and 1 lint found.',
+      ],
+      // TODO(danrubel) fix dartanalyzer to have non-zero exit code


we should make sure to fix this asap because anyone depending on flutter analyze in their build bots will be skewered by this regression.

I updated this functionality to work around this issue so that flutter analyze will return a non-zero exit code even though dartanalyzer never does.

Hixie · 2017-03-16T03:54:02Z

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

    // Not used by analyze --watch
-    argParser.addFlag('congratulate', help: 'Show output even when there are no errors, warnings, hints, or lints.', defaultsTo: true);
+    argParser.addFlag('congratulate', help: 'When analyzing the flutter repository, show output even when there are no errors, warnings, hints, or lints.', defaultsTo: true);
    argParser.addFlag('preamble', help: 'Display the number of files that will be analyzed.', defaultsTo: true);


looks like this also only works with --flutter-repo now

Good point. Help text updated.

Hixie · 2017-03-16T19:46:06Z

packages/flutter_tools/test/analyze_once_test.dart

    await createTestCommandRunner(command).run(arguments);
    expect(toolExit, isFalse, reason: 'Expected ToolExit exception');
-  } on ToolExit catch (e) {
+  } on ToolExit catch (_) {


You can just remove the "catch" clause entirely (just } on ToolExit {)

Hixie · 2017-03-16T19:46:44Z

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

 class AnalyzeCommand extends FlutterCommand {
-  AnalyzeCommand({bool verboseHelp: false}) {
+  /// The working directory for testing analysis using dartanalyzer.
+  final Directory workingDirectory;


nit: constructor first

Hixie · 2017-03-16T19:47:06Z

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

+  /// The working directory for testing analysis using dartanalyzer
+  final Directory workingDirectory;
+
+  AnalyzeOnce(ArgResults argResults, this.repoPackages, { this.workingDirectory }) : super(argResults);


nit: constructors first please

Sorry. Fixed.

* update platform_channel package name * lowercase name

…to the same TabBarController (flutter#9015)

* adds border radius to ink widgets. sets default ink border radius for material buttons with no background colors * tidying up code * add ink test stub * remove unused import

…ivity in FlutterActivity (flutter#9036)

…gmentActivity in FlutterActivity (flutter#9036)" (flutter#9041) This reverts commit 6bf0ceb.

…e of FragmentActivity in FlutterActivity (flutter#9036)"" (flutter#9046) Increased the time limit slightly to allow the microbenchmark test time to finish.

…allow use of FragmentActivity in FlutterActivity (flutter#9036)"" (flutter#9046)" (flutter#9047) Test is still failing with increased timeout. This reverts commit 864b3c3.

We now use modern scrolling machinery and patterns. The API should also be easier to maintain over time. Fixes flutter#6166 Fixes flutter#2591 Fixes flutter#3123

…el/flutter into flutter-analyze-call-dartanalyzer

googlebot · 2017-03-28T12:07:59Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

danrubel · 2017-03-28T12:19:15Z

So.... this is what happens when I rebase and push.
Note to self... don't use rebase in this situation. :-)

danrubel · 2017-03-28T12:24:12Z

Closing this in favor of the identical PR... #9049

update flutter analyze to call dartanalyzer

cfdcde5

googlebot added the cla: yes label Mar 15, 2017

Hixie reviewed Mar 15, 2017

View reviewed changes

danrubel added 2 commits March 15, 2017 20:02

address comments

6affa60

fix analyze_duplicate_names_test

6627db9

Hixie reviewed Mar 16, 2017

View reviewed changes

packages/flutter_tools/test/analyze_once_test.dart

toolExit: false,

);

});

}

Copy link

Contributor

Hixie Mar 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice test!

Hixie reviewed Mar 16, 2017

View reviewed changes

danrubel added 2 commits March 16, 2017 10:49

address comments and rework finding ".package" file

ffb0846

fix hint and disable locking for test

d7cf38c

Hixie reviewed Mar 16, 2017

View reviewed changes

jakobr-google and others added 21 commits March 27, 2017 12:11

iOS: Make flutter doctor unhappy if CocoaPods is missing. (flutter#8979)

e93f4a5

remove platform_channel/android/buildSrc (flutter#9020)

67bd17b

update platform_channel package name (flutter#9021)

657a4c3

* update platform_channel package name * lowercase name

Fixes issue with removing and re-adding a toolbar which is connected …

e271fa3

…to the same TabBarController (flutter#9015)

Update devfs_test to use MemoryFileSystem (flutter#8811)

ff61962

Disable flaky WIndows test :( (flutter#9029)

cea7c66

Add border radius to Ink widgets (flutter#8693)

23981f5

* adds border radius to ink widgets. sets default ink border radius for material buttons with no background colors * tidying up code * add ink test stub * remove unused import

Shrine gallery demo: support for landscape layout (flutter#9025)

9192f67

Include fragment support in Android build to allow use of FragmentAct…

6bf0ceb

…ivity in FlutterActivity (flutter#9036)

Added a TabBar regression test (flutter#9038)

26c3662

Revert "Include fragment support in Android build to allow use of Fra…

c346d51

…gmentActivity in FlutterActivity (flutter#9036)" (flutter#9041) This reverts commit 6bf0ceb.

Revert "Revert "Include fragment support in Android build to allow us…

864b3c3

…e of FragmentActivity in FlutterActivity (flutter#9036)"" (flutter#9046) Increased the time limit slightly to allow the microbenchmark test time to finish.

Revert "Revert "Revert "Include fragment support in Android build to …

e2b49d6

…allow use of FragmentActivity in FlutterActivity (flutter#9036)"" (flutter#9046)" (flutter#9047) Test is still failing with increased timeout. This reverts commit 864b3c3.

Update and modernize the flutter_markdown package (flutter#9044)

bbac5dc

We now use modern scrolling machinery and patterns. The API should also be easier to maintain over time. Fixes flutter#6166 Fixes flutter#2591 Fixes flutter#3123

update flutter analyze to call dartanalyzer

8a20f9a

address comments

1dec838

fix analyze_duplicate_names_test

7c56594

address comments and rework finding ".package" file

e1e35af

fix hint and disable locking for test

3d82c3b

address comments

1544a4e

Merge branch 'flutter-analyze-call-dartanalyzer' of github.com:danrub…

d374d14

…el/flutter into flutter-analyze-call-dartanalyzer

googlebot added cla: no and removed cla: yes labels Mar 28, 2017

danrubel pushed a commit to danrubel/flutter that referenced this pull request Mar 28, 2017

rebase of flutter#8808

8b901e7

danrubel mentioned this pull request Mar 28, 2017

update flutter analyze to call dartanalyzer #9049

Merged

danrubel closed this Mar 28, 2017

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

update flutter analyze to call dartanalyzer #8808

update flutter analyze to call dartanalyzer #8808

Uh oh!

Conversation

danrubel commented Mar 15, 2017

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!

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!