Implement a front end API for querying the dependency structure of a project.

Implement a front end API for querying the dependency structure of a project.

R=sigmund@google.com

Committed: https://github.com/dart-lang/sdk/commit/ede20b6558298a43c5757342f4d24e67366d03c4

[Paul Berry, 2016-12-15 23:17:03]

paulberry@google.com changed reviewers:
+ danrubel@google.com, sigmund@google.com

[Siggi Cherem (dart-lang), 2016-12-15 23:51:33]

lgtm, minor comments below

https://codereview.chromium.org/2575403005/diff/1/pkg/front_end/lib/dependenc...
File pkg/front_end/lib/dependency_grapher.dart (right):

https://codereview.chromium.org/2575403005/diff/1/pkg/front_end/lib/dependenc...
pkg/front_end/lib/dependency_grapher.dart:25: var packages = <String, Uri>{}; //
TODO(paulberry): support packages
should we have a "digested options" at some point that are simplified from
CompilerOptions? (e.g. .pacakgesPath converted into a packages map, etc)?

https://codereview.chromium.org/2575403005/diff/1/pkg/front_end/lib/dependenc...
pkg/front_end/lib/dependency_grapher.dart:38: class Graph {
as we start looking into integrating alternative implementations for computing
this, we might need to have it in a shared place or make the constructor public
(same thing with LibraryNode)

https://codereview.chromium.org/2575403005/diff/1/pkg/front_end/lib/dependenc...
pkg/front_end/lib/dependency_grapher.dart:51: /// A map of all the libraries in
the program, keyed by the URI of their
all libraries in the program or in the cycle?

https://codereview.chromium.org/2575403005/diff/1/pkg/front_end/lib/dependenc...
pkg/front_end/lib/dependency_grapher.dart:63: final dependencies =
<LibraryNode>[];
do we want to split imports and exports or in general it doesn't matter?

I normally simplify and treat them the same, but just wondering

https://codereview.chromium.org/2575403005/diff/1/pkg/front_end/lib/dependenc...
pkg/front_end/lib/dependency_grapher.dart:68: /// The URI of this library's
defining compilation unit.
you may be using an alphabetic order, but I rather sort the fields the opposite
way:
uri, parts, dependencies

(putting first the most defining piece of this object)

https://codereview.chromium.org/2575403005/diff/1/pkg/front_end/lib/dependenc...
pkg/front_end/lib/dependency_grapher.dart:146: var unit =
parser.parseDirectives(token);
I'm OK delaying this until later to avoid over-engineering, but I'm thinking
ahead about how to make this more pluggable as we play with different
scanners/parsers.

One idea would be to split the walker bits from the graph bits and how we
extract the dependencies from the files. For example:


class _Directives {
  final List<Uri> parts;
  final List<Uri> dependencies;
}

_Directives computeDirectives(contents); // implemented as you have here

computeDependencies() async {
  contents = ...;
  var directives = computeDirectives(contents);
  library.parts.addAll(directives.parts);
  directives.dependencies.forEach((uri) {
     var node = walker.nodeForUri(uri);
     library.dependencies.add(node.library);
  });  


again, maybe too early for this.

https://codereview.chromium.org/2575403005/diff/1/pkg/front_end/test/dependen...
File pkg/front_end/test/dependency_grapher_test.dart (right):

https://codereview.chromium.org/2575403005/diff/1/pkg/front_end/test/dependen...
pkg/front_end/test/dependency_grapher_test.dart:25:
expect(library.uri.toString(), uri);
style nit: I prefer '${library.uri}'

It's shorter and more easily recognizable that it is a string.

Same thing below:
(dep) => '${dep.uri}'
(part) => '$part'

https://codereview.chromium.org/2575403005/diff/1/pkg/front_end/test/dependen...
pkg/front_end/test/dependency_grapher_test.dart:29: library.parts.map((part) =>
part.toString()), unorderedEquals(parts));
is .toList needed here (or not needed above?)

[Paul Berry, 2016-12-16 02:46:46]

https://codereview.chromium.org/2575403005/diff/1/pkg/front_end/lib/dependenc...
File pkg/front_end/lib/dependency_grapher.dart (right):

https://codereview.chromium.org/2575403005/diff/1/pkg/front_end/lib/dependenc...
pkg/front_end/lib/dependency_grapher.dart:25: var packages = <String, Uri>{}; //
TODO(paulberry): support packages
On 2016/12/15 23:51:33, Siggi Cherem (dart-lang) wrote:
> should we have a "digested options" at some point that are simplified from
> CompilerOptions? (e.g. .pacakgesPath converted into a packages map, etc)?

Perhaps, I'm not sure yet.  I kind of want to fill out this implementation in
future CLs and then see what patterns I can extract from the resulting code. 
But that's definitely one option.  If we go with that approach, we might want to
make some of the getters in "digested options" be lazy, so that we don't pay a
price for digesting options that aren't used.

https://codereview.chromium.org/2575403005/diff/1/pkg/front_end/lib/dependenc...
pkg/front_end/lib/dependency_grapher.dart:38: class Graph {
On 2016/12/15 23:51:33, Siggi Cherem (dart-lang) wrote:
> as we start looking into integrating alternative implementations for computing
> this, we might need to have it in a shared place or make the constructor
public
> (same thing with LibraryNode)

Acknowledged.

https://codereview.chromium.org/2575403005/diff/1/pkg/front_end/lib/dependenc...
pkg/front_end/lib/dependency_grapher.dart:51: /// A map of all the libraries in
the program, keyed by the URI of their
On 2016/12/15 23:51:33, Siggi Cherem (dart-lang) wrote:
> all libraries in the program or in the cycle?

Whoops, I meant to say "in the cycle".  Fixed.

https://codereview.chromium.org/2575403005/diff/1/pkg/front_end/lib/dependenc...
pkg/front_end/lib/dependency_grapher.dart:63: final dependencies =
<LibraryNode>[];
On 2016/12/15 23:51:33, Siggi Cherem (dart-lang) wrote:
> do we want to split imports and exports or in general it doesn't matter?
> 
> I normally simplify and treat them the same, but just wondering

My intuition is no--any client that needs to distinguish imports from exports
probably needs a lot of other information that this interface doesn't expose. 
But it would be easy to add import and export lists should they become useful.

https://codereview.chromium.org/2575403005/diff/1/pkg/front_end/lib/dependenc...
pkg/front_end/lib/dependency_grapher.dart:68: /// The URI of this library's
defining compilation unit.
On 2016/12/15 23:51:33, Siggi Cherem (dart-lang) wrote:
> you may be using an alphabetic order, but I rather sort the fields the
opposite
> way:
> uri, parts, dependencies
> 
> (putting first the most defining piece of this object)

Done.

https://codereview.chromium.org/2575403005/diff/1/pkg/front_end/lib/dependenc...
pkg/front_end/lib/dependency_grapher.dart:146: var unit =
parser.parseDirectives(token);
On 2016/12/15 23:51:33, Siggi Cherem (dart-lang) wrote:
> I'm OK delaying this until later to avoid over-engineering, but I'm thinking
> ahead about how to make this more pluggable as we play with different
> scanners/parsers.
> 
> One idea would be to split the walker bits from the graph bits and how we
> extract the dependencies from the files. For example:
> 
> 
> class _Directives {
>   final List<Uri> parts;
>   final List<Uri> dependencies;
> }
> 
> _Directives computeDirectives(contents); // implemented as you have here
> 
> computeDependencies() async {
>   contents = ...;
>   var directives = computeDirectives(contents);
>   library.parts.addAll(directives.parts);
>   directives.dependencies.forEach((uri) {
>      var node = walker.nodeForUri(uri);
>      library.dependencies.add(node.library);
>   });  
> 
> 
> again, maybe too early for this.

Acknowledged.

https://codereview.chromium.org/2575403005/diff/1/pkg/front_end/test/dependen...
File pkg/front_end/test/dependency_grapher_test.dart (right):

https://codereview.chromium.org/2575403005/diff/1/pkg/front_end/test/dependen...
pkg/front_end/test/dependency_grapher_test.dart:25:
expect(library.uri.toString(), uri);
On 2016/12/15 23:51:33, Siggi Cherem (dart-lang) wrote:
> style nit: I prefer '${library.uri}'
> 
> It's shorter and more easily recognizable that it is a string.
> 
> Same thing below:
> (dep) => '${dep.uri}'
> (part) => '$part'

Done.

https://codereview.chromium.org/2575403005/diff/1/pkg/front_end/test/dependen...
pkg/front_end/test/dependency_grapher_test.dart:29: library.parts.map((part) =>
part.toString()), unorderedEquals(parts));
On 2016/12/15 23:51:33, Siggi Cherem (dart-lang) wrote:
> is .toList needed here (or not needed above?) 

Not needed above.  Good catch.

[Paul Berry, 2016-12-16 02:47:25]

Description was changed from

==========
Implement a front end API for querying the dependency structure of a project.
==========

to

==========
Implement a front end API for querying the dependency structure of a project.

R=sigmund@google.com

Committed:
https://github.com/dart-lang/sdk/commit/ede20b6558298a43c5757342f4d24e67366d03c4
==========

[Paul Berry, 2016-12-16 02:47:25]

Committed patchset #1 (id:1) manually as
ede20b6558298a43c5757342f4d24e67366d03c4 (presubmit successful).