Introduce dartanalyze into our build.

Introduce dartanalyze into our build.

This creates a separate toplevel build group ("dartcheck"), and when invoked,
it will run dartanalyze on each source file in a dart_packaged_application
template in the gn files. Currently, we disable hints, warnings, and a
few specific hard errors so we can make progress here.

Long term I'd like to have this as part of the normal build, but currently
it takes roughly three and a half seconds to check each dart file for
errors.

BUG=459376
R=zra@google.com

Committed: https://chromium.googlesource.com/external/mojo/+/2175b8834202e3e884a4e69423e899894a32dabd

[Elliot Glaysher, 2015-02-24 22:41:52]

erg@chromium.org changed reviewers:
+ jamesr@chromium.org, zra@google.com

[Elliot Glaysher, 2015-02-24 22:41:53]

This is a first pass at adding a dartanalyze step to our build. It fixes a few
nits in our bindings code, and mutes all warnings and hints for now so we can
work on the problems in the bindings layer while still offering a general tool
to engineers to find errors in their dart code.

However, it is slow and unlandable as is. It takes four seconds to run on a
single file, and I suspect that it's because of the following, where we do a
recursive directory walk because gn won't let me get_target_outputs() from a
different file.

https://codereview.chromium.org/953953003/diff/1/mojo/public/tools/dart_analy...
File mojo/public/tools/dart_analyze.py (right):

https://codereview.chromium.org/953953003/diff/1/mojo/public/tools/dart_analy...
mojo/public/tools/dart_analyze.py:17: # for each found mojom.dart in our build.
I suspect that this is what is making it slow, but I don't have any alternatives
to offer. Thoughts?

[jamesr, 2015-02-24 22:52:18]

Can you run the analyzer without telling it where the mojom.dart files are, or
is it unable to work in that mode?

[Elliot Glaysher, 2015-02-24 23:03:29]

On 2015/02/24 22:52:18, jamesr wrote:
> Can you run the analyzer without telling it where the mojom.dart files are, or
> is it unable to work in that mode?

To the best of my knowledge, it is unable to work in that mode.

So we have things like this in our dart code:

  import 'dart:mojo_bindings';
  import
'package:mojo/services/network/public/interfaces/network_service.mojom.dart';

And these custom statements need to be resolved. Our bindings layer knows what
to do with these statements, but to let the general dartanalyzer do its thing,
the dart guys added a --url-mapping command line flag to change something like
the above into an absolute path. (Relative paths don't work, hence the
construction in rules.gni.) Given that I don't have dependency information at
this point, and packages can import other packages, I have to stuff every known
.mojom.dart file into the command line.

And I can't omit dependencies because bad package imports are an error, and
deriving from an unknown class is an error.

Now that I think about it, there's another possible thing I could do, though I'm
not sure if it's cleaner or not: Given that dart .mojo files carry all their
dependencies, build the package first, then unzip it to a temporary directory.
This would contain significantly less (?) *.mojom.dart files, along with a less
complex directory structure to walk. 

Do you think that's a reasonable plan to try, or do you think the overhead of
building the package, and then immediately unzipping it would be a problem?

[zra, 2015-02-24 23:04:40]

lgtm

https://codereview.chromium.org/953953003/diff/1/mojo/public/dart/src/data_pi...
File mojo/public/dart/src/data_pipe.dart (right):

https://codereview.chromium.org/953953003/diff/1/mojo/public/dart/src/data_pi...
mojo/public/dart/src/data_pipe.dart:107: MojoResult read(ByteData data, [int
numBytes = -1, int flags = 0]) {
This should return the number of bytes read, so returning int is correct, but
the 'return status's below should become return 0.

https://codereview.chromium.org/953953003/diff/1/mojo/public/dart/src/data_pi...
mojo/public/dart/src/data_pipe.dart:153: MojoResult query() => read(null, 0,
FLAG_QUERY);
back to int with fix to read.

https://codereview.chromium.org/953953003/diff/1/mojo/public/tools/dart_analy...
File mojo/public/tools/dart_analyze.py (right):

https://codereview.chromium.org/953953003/diff/1/mojo/public/tools/dart_analy...
mojo/public/tools/dart_analyze.py:17: # for each found mojom.dart in our build.
On 2015/02/24 22:41:53, Elliot Glaysher wrote:
> I suspect that this is what is making it slow, but I don't have any
alternatives
> to offer. Thoughts?

Can you just pass main.dart to the analyzer? Does it not chase down all imports
for you? Would that not be sufficient somehow?

If I understand correctly, you can pass a --package-root argument to the
analyzer to tell it how to resolve package: imports.

https://codereview.chromium.org/953953003/diff/1/mojo/public/tools/dart_analy...
mojo/public/tools/dart_analyze.py:54: cmd.append("--url-mapping=package:%s,%s" %
(dart_name, file_path))
We should either: 1) use the --package-root flag, or 2) put the package: imports
in the packages/ subdirectory of the app, which is the default if it is not set,
and then not set the package root in the embedder.

[zra, 2015-02-24 23:04:47]

not lgtm

[zra, 2015-02-24 23:05:01]

Sorry, wrong button =(

[Elliot Glaysher, 2015-02-24 23:47:48]

ptal

https://codereview.chromium.org/953953003/diff/1/mojo/public/tools/dart_analy...
File mojo/public/tools/dart_analyze.py (right):

https://codereview.chromium.org/953953003/diff/1/mojo/public/tools/dart_analy...
mojo/public/tools/dart_analyze.py:17: # for each found mojom.dart in our build.
On 2015/02/24 23:04:39, zra wrote:
> On 2015/02/24 22:41:53, Elliot Glaysher wrote:
> > I suspect that this is what is making it slow, but I don't have any
> alternatives
> > to offer. Thoughts?
> 
> Can you just pass main.dart to the analyzer? Does it not chase down all
imports
> for you? Would that not be sufficient somehow?
> 
> If I understand correctly, you can pass a --package-root argument to the
> analyzer to tell it how to resolve package: imports.

Using --package-root works.

However, it doesn't solve the bulk of the problem. Running the analyzer goes
from four seconds to three.

[zra, 2015-02-24 23:50:36]

On 2015/02/24 23:47:48, Elliot Glaysher wrote:
> However, it doesn't solve the bulk of the problem. Running the analyzer goes
> from four seconds to three.

+johnmccutchan

John, is this consistent with analyzer performance that you have seen. Could you
update on plans for improving analyzer performance?

[Elliot Glaysher, 2015-02-25 00:37:41]

On 2015/02/24 23:51:13, zra wrote:

In case it helps, I saw that there is a --perf command line option. I also added
a timer in the python script which just measures the part where we call
dartanalyze. Here are a few runs:

../../examples/dart/wget/main.dart:
io:14
scan:97
parse:595
resolve:2168
errors:242
hints:0
other:320
total:3436
Total time running: 3.57558989525 seconds

../../examples/dart/hello_world/world/main.dart:
io:15
scan:174
parse:505
resolve:2147
errors:244
hints:0
other:322
total:3407
Total time running: 3.55623888969 seconds

../../examples/dart/console_example/main.dart:
io:16
scan:104
parse:560
resolve:1890
errors:210
hints:0
other:388
total:3168
Total time running: 3.29636311531 seconds

I don't know what resolve is, but it looks like it is the heaviest performance
counter?

[Cutch, 2015-02-25 15:20:12]

johnmccutchan@google.com changed reviewers:
+ johnmccutchan@google.com

[Cutch, 2015-02-25 15:20:13]

I don't have any specific insight into analyzer performance or the teams plan on
fixing it. That being said, I know the analyzer team is actively working on
improving performance. Someone on the analyzer team (+paul berry) could probably
provide specifics.

[jamesr, 2015-02-25 15:50:04]

If it's just going to be this slow then instead of landing this as a build step
we should add a script developers can manually run or hook into their editors.

[Cutch, 2015-02-25 16:24:16]

On 2015/02/25 15:50:04, jamesr wrote:
> If it's just going to be this slow then instead of landing this as a build
step
> we should add a script developers can manually run or hook into their editors.

This is probably a good short term strategy.

John

[Elliot Glaysher, 2015-02-25 19:11:16]

On 2015/02/25 15:50:04, jamesr wrote:
> If it's just going to be this slow then instead of landing this as a build
step
> we should add a script developers can manually run or hook into their editors.

ptal. I've added a toplevel check target which runs dartanalyze on source which
has been modified. (Along with a todo to remove it and integrate it into the
build once dartanalyzer runs faster.)

[Cutch, 2015-02-25 19:33:44]

DBC

https://codereview.chromium.org/953953003/diff/40001/examples/dart/wget/main....
File examples/dart/wget/main.dart (right):

https://codereview.chromium.org/953953003/diff/40001/examples/dart/wget/main....
examples/dart/wget/main.dart:41: 
unnecessary white space?

[Elliot Glaysher, 2015-02-25 20:19:17]

https://codereview.chromium.org/953953003/diff/40001/examples/dart/wget/main....
File examples/dart/wget/main.dart (right):

https://codereview.chromium.org/953953003/diff/40001/examples/dart/wget/main....
examples/dart/wget/main.dart:41: 
On 2015/02/25 19:33:44, Cutch wrote:
> unnecessary white space?

Done.

[zra, 2015-02-25 22:01:42]

lgtm

Until it's added to the build step, it might be useful to have instructions
somewhere explaining how to add a new Dart application, and how to invoke the
"check" target.

[Elliot Glaysher, 2015-02-26 22:32:23]

Committed patchset #5 (id:80001) manually as
2175b8834202e3e884a4e69423e899894a32dabd (presubmit successful).