Issue 2417043003: Initial API for the Dart front_end package.

Paul Berry

paulberry@google.com changed reviewers: + brianwilkerson@google.com, hausner@google.com, scheglov@google.com, sigmund@google.com

4 years, 2 months ago (2016-10-13 22:38:25 UTC) #1

Paul Berry

Here is a first cut at a possible API for a Dart front end to ...

4 years, 2 months ago (2016-10-13 22:38:26 UTC) #2

Siggi Cherem (dart-lang)

Hey Paul - this looks great! Thanks for putting this together. I'm just bombarding you ...

4 years, 2 months ago (2016-10-13 23:22:16 UTC) #3

Brian Wilkerson

Some preliminary thoughts. I'd like to think about it a little more deeply and then ...

4 years, 2 months ago (2016-10-13 23:37:02 UTC) #4

Bob Nystrom

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

4 years, 2 months ago (2016-10-13 23:56:10 UTC) #5

Bob Nystrom

https://codereview.chromium.org/2417043003/diff/1/pkg/front_end/lib/compiler_api.dart File pkg/front_end/lib/compiler_api.dart (right): https://codereview.chromium.org/2417043003/diff/1/pkg/front_end/lib/compiler_api.dart#newcode5 pkg/front_end/lib/compiler_api.dart:5: /// Defines the front end API to be used ...

4 years, 2 months ago (2016-10-13 23:56:11 UTC) #6

scheglov

https://codereview.chromium.org/2417043003/diff/1/pkg/front_end/lib/compiler_api.dart File pkg/front_end/lib/compiler_api.dart (right): https://codereview.chromium.org/2417043003/diff/1/pkg/front_end/lib/compiler_api.dart#newcode13 pkg/front_end/lib/compiler_api.dart:13: typedef void ErrorHandler(CompilationError); typedef void ErrorHandler(CompilationError error); https://codereview.chromium.org/2417043003/diff/1/pkg/front_end/lib/compiler_api.dart#newcode54 pkg/front_end/lib/compiler_api.dart:54: ...

4 years, 2 months ago (2016-10-13 23:58:17 UTC) #7

Paul Berry

https://codereview.chromium.org/2417043003/diff/1/pkg/front_end/lib/compilation_error.dart File pkg/front_end/lib/compilation_error.dart (right): https://codereview.chromium.org/2417043003/diff/1/pkg/front_end/lib/compilation_error.dart#newcode9 pkg/front_end/lib/compilation_error.dart:9: import 'source_location.dart'; On 2016/10/13 23:37:01, Brian Wilkerson wrote: > ...

4 years, 2 months ago (2016-10-14 00:09:37 UTC) #8

https://codereview.chromium.org/2417043003/diff/1/pkg/front_end/lib/compilati...
File pkg/front_end/lib/compilation_error.dart (right):

https://codereview.chromium.org/2417043003/diff/1/pkg/front_end/lib/compilati...
pkg/front_end/lib/compilation_error.dart:9: import 'source_location.dart';
On 2016/10/13 23:37:01, Brian Wilkerson wrote:
> I'm not seeing this file in the CL. Did you forget to include a file, or did
you
> mean 'source_span.dart'?

Whoops, you're correct.  Thanks.

https://codereview.chromium.org/2417043003/diff/1/pkg/front_end/lib/compilati...
pkg/front_end/lib/compilation_error.dart:14: /// TODO(paulberry): add a
reference to the analyzer error code.
On 2016/10/13 23:37:01, Brian Wilkerson wrote:
> We also need the correction message and support for properties.

I'm not convinced that we need these--they sound like analyzer-centric use cases
so they shouldn't be exposed through this API.  But maybe I don't understand
what they're used for.  Let's talk about it these questions in person tomorrow.

> 
> Is there a reason why we're not using AnalysisError, which already has this
> additional support?

AnalysisError has a bunch of static methods that don't seem like they belong in
the front end API, as well as the correction message and properties mentioned
above.  Unless we're certain that all of the features of AnalysisError make
sense for back ends other than analyzer, I'd rather not presume to put them in
this API.

However, I see no problem making AnalysisError *implement* this API.

https://codereview.chromium.org/2417043003/diff/1/pkg/front_end/lib/compiler_...
File pkg/front_end/lib/compiler_api.dart (right):

https://codereview.chromium.org/2417043003/diff/1/pkg/front_end/lib/compiler_...
pkg/front_end/lib/compiler_api.dart:5: /// Defines the front end API to be used
by compiler back-ends.
On 2016/10/13 23:22:15, Siggi Cherem (dart-lang) wrote:
> nit: front end => front-end ?
> (that would match back-ends too :))

I don't have a strong preference, but in my mind they're not hyphenated, and
Wikipedia seems to agree with me:
https://en.wikipedia.org/wiki/Front_and_back_ends (although there is some
disagreement on the talk page).

https://codereview.chromium.org/2417043003/diff/1/pkg/front_end/lib/compiler_...
pkg/front_end/lib/compiler_api.dart:13: typedef void
ErrorHandler(CompilationError);
On 2016/10/13 23:58:17, scheglov wrote:
> typedef void ErrorHandler(CompilationError error);

Oops.  Thank you.

https://codereview.chromium.org/2417043003/diff/1/pkg/front_end/lib/compiler_...
pkg/front_end/lib/compiler_api.dart:17: /// Intended use: create a CompilerApi
object, set any desired options using
On 2016/10/13 23:22:15, Siggi Cherem (dart-lang) wrote:
> since these options are just set once, could we make them named arguments to
the
> constructor instead?
> 
>    new CompilerApi(
>      {String sdkPath, String packages, List<String> inputSummaries, ...});
> 

Yeah, that's a good point.  Making them named arguments to the constructor would
prevent a caller trying to change the options and then re-run (which would save
us from having to support that functionality :))

I'll rework it unless someone objects.

https://codereview.chromium.org/2417043003/diff/1/pkg/front_end/lib/compiler_...
pkg/front_end/lib/compiler_api.dart:22: /// operate on whole source files, and
do not cache intermediate results between
On 2016/10/13 23:22:16, Siggi Cherem (dart-lang) wrote:
> design discussion:
> 
> I might be jumping ahead since I'm not sure what you have in mind for caching
&
> incremental updates, but I'd like us to consider whether we can handle most of
> that outside the FrontEnd itself.
> 
> This should be possible if we make the building blocks fast and fine-grain
> enough. For example, If rebuilding an entire library is fast enough, the API
you
> have here is all we need. If we need to rebuild a single method, then maybe an
> extra API like:
> 
>    kernel.Member kernelForMethod(Uri sourceFile, String
apiSummaryOfSourceFile,
> String methodName);
> 
> could do the trick?

I don't think it's that simple, since changes to one file might affect the
kernel representation of other files.  I also have some concerns about whether
the front end would need to retain some intermediate data constructors in order
to be able to do incremental compilation efficiently, but these intermediate
data structures would increase memory pressure if not needed.

In any case, it probably is jumping ahead a little bit right now.  If you want,
I'd be glad to brainstorm about it over VC sometime.

https://codereview.chromium.org/2417043003/diff/1/pkg/front_end/lib/compiler_...
pkg/front_end/lib/compiler_api.dart:28: abstract class CompilerApi {
On 2016/10/13 23:22:16, Siggi Cherem (dart-lang) wrote:
> minor nit: what do you think about renaming this as [FrontEnd]?

I'd prefer not to, since the front end also needs APIs for use by analyzer, and
they are probably going to be different.

https://codereview.chromium.org/2417043003/diff/1/pkg/front_end/lib/compiler_...
pkg/front_end/lib/compiler_api.dart:28: abstract class CompilerApi {
On 2016/10/13 23:37:02, Brian Wilkerson wrote:
> This doesn't provide any way to set any options, such as enabling experimental
> language features. Not sure whether that's an oversight, whether that's
intended
> for a later version of the API, or whether that's expressly a non-goal.

It's intended to be added in the future.  My current thought is that
experimental language features would be supported via additional setters (or,
taking Siggi's suggestion, additional optional constructor args).

https://codereview.chromium.org/2417043003/diff/1/pkg/front_end/lib/compiler_...
pkg/front_end/lib/compiler_api.dart:49: void set packages(String path);
On 2016/10/13 23:37:02, Brian Wilkerson wrote:
> Analyzer currently supports setting a 'packages' directory as well; we might
> well need support for that in kernel.

Since the front end isn't likely to be used until Dart 2.0, I figured we
shouldn't support deprecated functionality like the "packages" directory.

https://codereview.chromium.org/2417043003/diff/1/pkg/front_end/lib/compiler_...
pkg/front_end/lib/compiler_api.dart:54: void set inputSummaries(List<String>
paths);
On 2016/10/13 23:37:02, Brian Wilkerson wrote:
> Does this include all summary files: sdk and non-sdk?

I'm not sure.  Let me think about this.

https://codereview.chromium.org/2417043003/diff/1/pkg/front_end/lib/compiler_...
pkg/front_end/lib/compiler_api.dart:54: void set inputSummaries(List<String>
paths);
On 2016/10/13 23:58:17, scheglov wrote:
> Is using summaries for pub packages an implementation detail or not allowed?
>  

I think it should be not allowed; I don't think we should bake any knowledge
about pub into the front end.

https://codereview.chromium.org/2417043003/diff/1/pkg/front_end/lib/compiler_...
pkg/front_end/lib/compiler_api.dart:61: void set uriOverride(Map<Uri, String>
mapping);
On 2016/10/13 23:22:15, Siggi Cherem (dart-lang) wrote:
> is this for dart:* libraries or for mappings like we had to do for summaries
in
> blaze?

It's for the mappings we have to do for Bazel.  It corresponds precisely to the
DDC command line option "--url-mapping".  Analyzer_cli's build mode has a
similar feature, but it's more clumsy IMHO (it requires all input files to be
specified on the command line as URI/pathname pairs).

It's not intended for redirecting dart:* URIs, and it shouldn't be necessary,
since the use cases where it will be used are for modular compilation, in which
case the dart:* URIs are coming from summaries, in which case the translation
from URI to file path will never happen for dart:* URIs.  But I can't decide
whether we should prohibit dart:* URIs or not--we might need to allow them as
part of a bootstrapping process.

https://codereview.chromium.org/2417043003/diff/1/pkg/front_end/lib/compiler_...
pkg/front_end/lib/compiler_api.dart:64: /// applied to the SDK.
On 2016/10/13 23:58:17, scheglov wrote:
> It is always a single platform, right?
> The phrase "bit mask" sounds as it might be a combination of several bits.

It's always a single platform, but it must be a power of two, because that's
necessary to allow bit masks to be used in libraries.dart to share patch files
among platforms.

I'll improve the wording to clarify.

https://codereview.chromium.org/2417043003/diff/1/pkg/front_end/lib/compiler_...
pkg/front_end/lib/compiler_api.dart:69: void set platform(int value);
On 2016/10/13 23:37:02, Brian Wilkerson wrote:
> Note that there is another definition of "platform" being proposed, so this
name
> is (or soon will be) ambiguous.

Acknowledged.

https://codereview.chromium.org/2417043003/diff/1/pkg/front_end/lib/compiler_...
pkg/front_end/lib/compiler_api.dart:75: void set permittedFiles(Iterable<String>
files);
On 2016/10/13 23:22:15, Siggi Cherem (dart-lang) wrote:
> is this mainly for validation in blaze? (to check that we don't have read both
> from sources + summaries for the same library?)

It's for Bazel workers.  Bazel workers aren't hermetically sealed like normal
Bazel actions, so they are responsible for ensuring that they only access the
files that Bazel instructs them to access.  I can't recall if analyzer_cli's
build mode implements this functionality or not, but it should, and so should
DDC, because it is necessary for reproducible builds.

https://codereview.chromium.org/2417043003/diff/1/pkg/front_end/lib/compiler_...
pkg/front_end/lib/compiler_api.dart:75: void set permittedFiles(Iterable<String>
files);
On 2016/10/13 23:37:02, Brian Wilkerson wrote:
> What use case does this support? (I have to wonder whether we need this, or
> whether we would just get an exception when trying to read a file not in the
> list.)

For the use case, see my answer to Siggi.

As to whether we should just get an exception, I believe we shouldn't, since
this situation could arise as a result of a customer attempting to import a file
from a build unit that is not listed in their Bazel BUILD file as a dependency;
as such, we should make sure we produce the same error message that would have
been produced had the compiler been invoked as a non-worker (which would be that
the file would not exist, since that's how Bazel hermetic builds work).

https://codereview.chromium.org/2417043003/diff/1/pkg/front_end/lib/compiler_...
pkg/front_end/lib/compiler_api.dart:83: /// TODO(paulberry): once Dart Kernel
has been merged into the SDK, set the
On 2016/10/13 23:22:16, Siggi Cherem (dart-lang) wrote:
> since it's already pulled into DEPS, you might be able to use
> List<kernel.Library> here (or kernel.Program?)

Oh, I didn't realize that.  I will make that change.

> 
> Related question: do we need to add a notion of a bundle/module/build-unit to
> kernel?

I don't know.  I'll add a TODO.

https://codereview.chromium.org/2417043003/diff/1/pkg/front_end/lib/compiler_...
pkg/front_end/lib/compiler_api.dart:90: /// Dart Kernel.
On 2016/10/13 23:22:16, Siggi Cherem (dart-lang) wrote:
> + and will be deperecated in the future?

Yes that's my intent.

> 
> OR, should we move this to a subclass of CompilerApi so that it is not exposed
> by default (less breaking changes when we deprecate it?)

Hmm, good question.  I don't know what would be best.

https://codereview.chromium.org/2417043003/diff/1/pkg/front_end/lib/source_sp...
File pkg/front_end/lib/source_span.dart (right):

https://codereview.chromium.org/2417043003/diff/1/pkg/front_end/lib/source_sp...
pkg/front_end/lib/source_span.dart:7: library front_end.source_location;
On 2016/10/13 23:37:02, Brian Wilkerson wrote:
> If we're going to model source information differently than analyzer currently
> does, is there a reason for not using the 'source_span' package
> (https://pub.dartlang.org/packages/source_span)

I don't know.  I will investigate that package.

https://codereview.chromium.org/2417043003/diff/1/pkg/front_end/lib/source_sp...
pkg/front_end/lib/source_span.dart:10: class SourceSpan {
On 2016/10/13 23:22:16, Siggi Cherem (dart-lang) wrote:
> it would be great if we could use SourceSpan from package:source_span
> (https://www.dartdocs.org/documentation/source_span/1.2.3/).
> 
> It has a similar API, but also a lot of helper functions that already deal
with
> reporting error messages that can highlight line segments and such.

Acknowledged.  I will investigate that package.

https://codereview.chromium.org/2417043003/diff/1/pkg/front_end/lib/source_sp...
pkg/front_end/lib/source_span.dart:12: Uri get sourceUri;
On 2016/10/13 23:37:02, Brian Wilkerson wrote:
> I would suggest 'uri'; I think it's clear that this is the URI associated with
> the source file.

Fair enough.

https://codereview.chromium.org/2417043003/diff/1/pkg/front_end/lib/source_sp...
pkg/front_end/lib/source_span.dart:25: int get startChar;
On 2016/10/13 23:37:02, Brian Wilkerson wrote:
> I would suggest 'startColumn' (and 'endColumn') as being more clear.

Sounds reasonable.

Brian Wilkerson

https://codereview.chromium.org/2417043003/diff/1/pkg/front_end/lib/compiler_api.dart File pkg/front_end/lib/compiler_api.dart (right): https://codereview.chromium.org/2417043003/diff/1/pkg/front_end/lib/compiler_api.dart#newcode17 pkg/front_end/lib/compiler_api.dart:17: /// Intended use: create a CompilerApi object, set any ...

4 years, 2 months ago (2016-10-14 14:54:45 UTC) #9

https://codereview.chromium.org/2417043003/diff/1/pkg/front_end/lib/compiler_...
File pkg/front_end/lib/compiler_api.dart (right):

https://codereview.chromium.org/2417043003/diff/1/pkg/front_end/lib/compiler_...
pkg/front_end/lib/compiler_api.dart:17: /// Intended use: create a CompilerApi
object, set any desired options using
If you intend to deprecate the sourcesToResolvedAsts methods, and if all of the
setters become named parameters to the constructor, then you're effectively down
to an abstract class with a single method. Do you anticipate adding more
methods, or should we consider defining a top-level function instead?

https://codereview.chromium.org/2417043003/diff/1/pkg/front_end/lib/compiler_...
pkg/front_end/lib/compiler_api.dart:49: void set packages(String path);
> Since the front end isn't likely to be used until Dart 2.0, I figured we
> shouldn't support deprecated functionality like the "packages" directory.

I have asked several people, and as far as I can tell there is no plan to
disallow the use of a packages directory. Pub won't create one for you, but as
far as I can tell it will be allowed. Do you have a different understanding?

https://codereview.chromium.org/2417043003/diff/1/pkg/front_end/lib/compiler_...
pkg/front_end/lib/compiler_api.dart:54: void set inputSummaries(List<String>
paths);
> > Is using summaries for pub packages an implementation detail or not allowed?
> 
> I think it should be not allowed; I don't think we should bake any knowledge
> about pub into the front end.

While it might be reasonable for the front end to not have any knowledge of pub
(although I reserve the right to ponder the implications before committing to
that :-), I don't think it's reasonable to not support the association of
summary files with packages.

If we're going to allow the specification of non-sdk summary files (see my
earlier question), then it seems like we ought to be able to do so in a way that
is general enough that a client (like analysis server) ought to be able to
specify summary files associated with packages, even if the front end doesn't
have any concept of what a package is (not sure whether "knowledge of pub"
includes knowledge of pub-style packages).

Are we going to follow the same philosophy when it comes to baking in knowledge
of Bazel?

Siggi Cherem (dart-lang)

https://codereview.chromium.org/2417043003/diff/1/pkg/front_end/lib/compiler_api.dart File pkg/front_end/lib/compiler_api.dart (right): https://codereview.chromium.org/2417043003/diff/1/pkg/front_end/lib/compiler_api.dart#newcode5 pkg/front_end/lib/compiler_api.dart:5: /// Defines the front end API to be used ...

4 years, 2 months ago (2016-10-14 16:00:49 UTC) #10

https://codereview.chromium.org/2417043003/diff/1/pkg/front_end/lib/compiler_...
File pkg/front_end/lib/compiler_api.dart (right):

https://codereview.chromium.org/2417043003/diff/1/pkg/front_end/lib/compiler_...
pkg/front_end/lib/compiler_api.dart:5: /// Defines the front end API to be used
by compiler back-ends.
On 2016/10/14 00:09:35, Paul Berry wrote:
> On 2016/10/13 23:22:15, Siggi Cherem (dart-lang) wrote:
> > nit: front end => front-end ?
> > (that would match back-ends too :))
> 
> I don't have a strong preference, but in my mind they're not hyphenated, and
> Wikipedia seems to agree with me:
> https://en.wikipedia.org/wiki/Front_and_back_ends (although there is some
> disagreement on the talk page).

To make sure all options are on the table: my leanding is towards "frontend"
(one word). We should ask Kathy, I recall when we debated "entry point" vs
"entrypoint" she leaned towards the latter in technical writing.

https://codereview.chromium.org/2417043003/diff/1/pkg/front_end/lib/compiler_...
pkg/front_end/lib/compiler_api.dart:17: /// Intended use: create a CompilerApi
object, set any desired options using
On 2016/10/14 14:54:45, Brian Wilkerson wrote:
> If you intend to deprecate the sourcesToResolvedAsts methods, and if all of
the
> setters become named parameters to the constructor, then you're effectively
down
> to an abstract class with a single method. Do you anticipate adding more
> methods, or should we consider defining a top-level function instead?

Good point. +1 on making it a single top-level function if we only have a single
function.

A related question I have: should we expose some way to build summaries as well?
Depending on whether it makes sense to do both kernel + summaries together, this
could be either by changing the return type of sourcesToKernel to include both,
or we'd have a second API.

https://codereview.chromium.org/2417043003/diff/1/pkg/front_end/lib/compiler_...
pkg/front_end/lib/compiler_api.dart:22: /// operate on whole source files, and
do not cache intermediate results between
On 2016/10/14 00:09:36, Paul Berry wrote:
> On 2016/10/13 23:22:16, Siggi Cherem (dart-lang) wrote:
> > design discussion:
> > 
> > I might be jumping ahead since I'm not sure what you have in mind for
caching
> &
> > incremental updates, but I'd like us to consider whether we can handle most
of
> > that outside the FrontEnd itself.
> > 
> > This should be possible if we make the building blocks fast and fine-grain
> > enough. For example, If rebuilding an entire library is fast enough, the API
> you
> > have here is all we need. If we need to rebuild a single method, then maybe
an
> > extra API like:
> > 
> >    kernel.Member kernelForMethod(Uri sourceFile, String
> apiSummaryOfSourceFile,
> > String methodName);
> > 
> > could do the trick?
> 
> I don't think it's that simple, since changes to one file might affect the
> kernel representation of other files.  I also have some concerns about whether
> the front end would need to retain some intermediate data constructors in
order
> to be able to do incremental compilation efficiently, but these intermediate
> data structures would increase memory pressure if not needed.
> 
> In any case, it probably is jumping ahead a little bit right now.  If you
want,
> I'd be glad to brainstorm about it over VC sometime.

Sure - let's chat sometime later, maybe after the summit.

https://codereview.chromium.org/2417043003/diff/1/pkg/front_end/lib/compiler_...
pkg/front_end/lib/compiler_api.dart:28: abstract class CompilerApi {
On 2016/10/14 00:09:36, Paul Berry wrote:
> On 2016/10/13 23:22:16, Siggi Cherem (dart-lang) wrote:
> > minor nit: what do you think about renaming this as [FrontEnd]?
> 
> I'd prefer not to, since the front end also needs APIs for use by analyzer,
and
> they are probably going to be different.

Ok, makes sense.

My minor concern with [CompilerApi] is that it sounds like this is an API of a
compiler, rather than an API for compiler backends. Technically the frontend is
a compiler, so it's not wrong.

One option here could be [CompilerFrontEnd], then a front-end with extra APIs
for analyzer would be the AnalyzerFrontEnd.

Another idea could be to name it in terms of kernel (e.g. KernelFrontEnd,
KernelCompiler, or KernelGenerator).

https://codereview.chromium.org/2417043003/diff/1/pkg/front_end/lib/compiler_...
pkg/front_end/lib/compiler_api.dart:54: void set inputSummaries(List<String>
paths);
On 2016/10/14 14:54:45, Brian Wilkerson wrote:
> > > Is using summaries for pub packages an implementation detail or not
allowed?
> > 
> > I think it should be not allowed; I don't think we should bake any knowledge
> > about pub into the front end.
> 
> While it might be reasonable for the front end to not have any knowledge of
pub
> (although I reserve the right to ponder the implications before committing to
> that :-), I don't think it's reasonable to not support the association of
> summary files with packages.
> 
> If we're going to allow the specification of non-sdk summary files (see my
> earlier question), then it seems like we ought to be able to do so in a way
that
> is general enough that a client (like analysis server) ought to be able to
> specify summary files associated with packages, even if the front end doesn't
> have any concept of what a package is (not sure whether "knowledge of pub"
> includes knowledge of pub-style packages).
> 
> Are we going to follow the same philosophy when it comes to baking in
knowledge
> of Bazel?

My assumption here was also like Brian: this list of paths includes summaries
for everything that we might want to build separately/modularly - sdk and
non-sdk alike. 

I agree with Paul though that there shouldn't be any association internally
between packages and summaries. Summaries are an artifact that works at a
build-unit level, which can be bigger or smaller than a package. The frontend
shouldn't care.

https://codereview.chromium.org/2417043003/diff/1/pkg/front_end/lib/compiler_...
pkg/front_end/lib/compiler_api.dart:61: void set uriOverride(Map<Uri, String>
mapping);
On 2016/10/14 00:09:36, Paul Berry wrote:
> On 2016/10/13 23:22:15, Siggi Cherem (dart-lang) wrote:
> > is this for dart:* libraries or for mappings like we had to do for summaries
> in
> > blaze?
> 
> It's for the mappings we have to do for Bazel.  It corresponds precisely to
the
> DDC command line option "--url-mapping".  Analyzer_cli's build mode has a
> similar feature, but it's more clumsy IMHO (it requires all input files to be
> specified on the command line as URI/pathname pairs).
> 
> It's not intended for redirecting dart:* URIs, and it shouldn't be necessary,
> since the use cases where it will be used are for modular compilation, in
which
> case the dart:* URIs are coming from summaries, in which case the translation
> from URI to file path will never happen for dart:* URIs.  But I can't decide
> whether we should prohibit dart:* URIs or not--we might need to allow them as
> part of a bootstrapping process.

Ah - if it is mainly for bazel, this might be a good time to see if it is
feasible to move towards the approach we used in dart2js that I was telling you
about a few weeks ago (where we specify "file://bazel-root/" and search within a
handful of bazel-roots).

https://codereview.chromium.org/2417043003/diff/1/pkg/front_end/lib/compiler_...
pkg/front_end/lib/compiler_api.dart:64: /// applied to the SDK.
On 2016/10/14 00:09:36, Paul Berry wrote:
> On 2016/10/13 23:58:17, scheglov wrote:
> > It is always a single platform, right?
> > The phrase "bit mask" sounds as it might be a combination of several bits.
> 
> It's always a single platform, but it must be a power of two, because that's
> necessary to allow bit masks to be used in libraries.dart to share patch files
> among platforms.
> 
> I'll improve the wording to clarify.

I wonder if we should add an enum to libraries.dart and import it here, so we
can expose this in a way that is easier to understand.

enum Platforms {
  dart2js, vm
}

https://codereview.chromium.org/2417043003/diff/1/pkg/front_end/lib/compiler_...
pkg/front_end/lib/compiler_api.dart:75: void set permittedFiles(Iterable<String>
files);
On 2016/10/14 00:09:36, Paul Berry wrote:
> On 2016/10/13 23:22:15, Siggi Cherem (dart-lang) wrote:
> > is this mainly for validation in blaze? (to check that we don't have read
both
> > from sources + summaries for the same library?)
> 
> It's for Bazel workers.  Bazel workers aren't hermetically sealed like normal
> Bazel actions, so they are responsible for ensuring that they only access the
> files that Bazel instructs them to access.  I can't recall if analyzer_cli's
> build mode implements this functionality or not, but it should, and so should
> DDC, because it is necessary for reproducible builds.

What do you think about making the front-end hermetic always?

In particular, I'd:
  - rename [permittedFiles] to [sources]
  - make "sources" a mandatory argument
  - rename the argument in sourceToKernel from [sources] to [entryPoints] or
[entrySources]

Then rather than saying that the front-end will pretend that no other file
exists, we would say that the frontend only loads the files that were listed. It
also conveys that the "entryPoints" must be a subset of these sources.

https://codereview.chromium.org/2417043003/diff/1/pkg/front_end/lib/compiler_...
pkg/front_end/lib/compiler_api.dart:90: /// Dart Kernel.
On 2016/10/14 00:09:36, Paul Berry wrote:
> On 2016/10/13 23:22:16, Siggi Cherem (dart-lang) wrote:
> > + and will be deperecated in the future?
> 
> Yes that's my intent.
> 
> > 
> > OR, should we move this to a subclass of CompilerApi so that it is not
exposed
> > by default (less breaking changes when we deprecate it?)
> 
> Hmm, good question.  I don't know what would be best.

Let's keep it separate then (either on a separate top-level or class depending
on what we decide above)

Bob Nystrom

https://codereview.chromium.org/2417043003/diff/1/pkg/front_end/lib/compiler_api.dart File pkg/front_end/lib/compiler_api.dart (right): https://codereview.chromium.org/2417043003/diff/1/pkg/front_end/lib/compiler_api.dart#newcode5 pkg/front_end/lib/compiler_api.dart:5: /// Defines the front end API to be used ...

4 years, 2 months ago (2016-10-14 16:12:18 UTC) #11

Paul Berry

Thanks for the comments, everyone. I will rework this and publish a version 2 as ...

4 years, 2 months ago (2016-10-14 21:32:35 UTC) #12

Thanks for the comments, everyone.  I will rework this and publish a version 2
as soon as I can.

https://codereview.chromium.org/2417043003/diff/1/pkg/front_end/lib/compilati...
File pkg/front_end/lib/compilation_error.dart (right):

https://codereview.chromium.org/2417043003/diff/1/pkg/front_end/lib/compilati...
pkg/front_end/lib/compilation_error.dart:14: /// TODO(paulberry): add a
reference to the analyzer error code.
On 2016/10/14 00:09:35, Paul Berry wrote:
> On 2016/10/13 23:37:01, Brian Wilkerson wrote:
> > We also need the correction message and support for properties.
> 
> I'm not convinced that we need these--they sound like analyzer-centric use
cases
> so they shouldn't be exposed through this API.  But maybe I don't understand
> what they're used for.  Let's talk about it these questions in person
tomorrow.
> 
> > 
> > Is there a reason why we're not using AnalysisError, which already has this
> > additional support?
> 
> AnalysisError has a bunch of static methods that don't seem like they belong
in
> the front end API, as well as the correction message and properties mentioned
> above.  Unless we're certain that all of the features of AnalysisError make
> sense for back ends other than analyzer, I'd rather not presume to put them in
> this API.
> 
> However, I see no problem making AnalysisError *implement* this API.

After further discussion this morning, I'm ok adding the correction message once
most errors actually use it (which Brian says he hopes to have implemented
soon).  The properties are currently only used for analyzer's use case, so I'm
leaving them out of this API for now; if they wind up being needed by
compilation back-ends, we can always add them.

https://codereview.chromium.org/2417043003/diff/1/pkg/front_end/lib/compiler_...
File pkg/front_end/lib/compiler_api.dart (right):

https://codereview.chromium.org/2417043003/diff/1/pkg/front_end/lib/compiler_...
pkg/front_end/lib/compiler_api.dart:5: /// Defines the front end API to be used
by compiler back-ends.
On 2016/10/14 16:00:49, Siggi Cherem (dart-lang) wrote:
> On 2016/10/14 00:09:35, Paul Berry wrote:
> > On 2016/10/13 23:22:15, Siggi Cherem (dart-lang) wrote:
> > > nit: front end => front-end ?
> > > (that would match back-ends too :))
> > 
> > I don't have a strong preference, but in my mind they're not hyphenated, and
> > Wikipedia seems to agree with me:
> > https://en.wikipedia.org/wiki/Front_and_back_ends (although there is some
> > disagreement on the talk page).
> 
> To make sure all options are on the table: my leanding is towards "frontend"
> (one word). We should ask Kathy, I recall when we debated "entry point" vs
> "entrypoint" she leaned towards the latter in technical writing. 

I'm happy to defer to Kathy on this question.  I'll reach out to her.

https://codereview.chromium.org/2417043003/diff/1/pkg/front_end/lib/compiler_...
pkg/front_end/lib/compiler_api.dart:17: /// Intended use: create a CompilerApi
object, set any desired options using
On 2016/10/14 16:00:48, Siggi Cherem (dart-lang) wrote:
> On 2016/10/14 14:54:45, Brian Wilkerson wrote:
> > If you intend to deprecate the sourcesToResolvedAsts methods, and if all of
> the
> > setters become named parameters to the constructor, then you're effectively
> down
> > to an abstract class with a single method. Do you anticipate adding more
> > methods, or should we consider defining a top-level function instead?
> 
> Good point. +1 on making it a single top-level function if we only have a
single
> function.
> 
> A related question I have: should we expose some way to build summaries as
well?
> Depending on whether it makes sense to do both kernel + summaries together,
this
> could be either by changing the return type of sourcesToKernel to include
both,
> or we'd have a second API.

After further discussion, I'm going to provisionally try going with an options
class and top level functions.  Rationale for using an options class rather than
a set of named parameters: (a) it allows the common parameters to be shared
amongst multiple top level functions, (b) it allows the options to be passed
around by the caller (e.g. a set of unit tests might create a single options
struct and then re-use it for all tests), (c) it allows the options to be passed
around inside the implementation.

In today's in-person discussion, a disadvantage of top level functions was
raised: they can't be easily mocked out in unit testing scenarios.  I believe
that for the kinds of unit tests we are going to want to write for the front end
(and its clients), this won't be a problem.  But before committing to this
approach, I will sketch out some unit testing scenarios to make sure.

https://codereview.chromium.org/2417043003/diff/1/pkg/front_end/lib/compiler_...
pkg/front_end/lib/compiler_api.dart:28: abstract class CompilerApi {
On 2016/10/14 16:00:49, Siggi Cherem (dart-lang) wrote:
> On 2016/10/14 00:09:36, Paul Berry wrote:
> > On 2016/10/13 23:22:16, Siggi Cherem (dart-lang) wrote:
> > > minor nit: what do you think about renaming this as [FrontEnd]?
> > 
> > I'd prefer not to, since the front end also needs APIs for use by analyzer,
> and
> > they are probably going to be different.
> 
> Ok, makes sense.
> 
> My minor concern with [CompilerApi] is that it sounds like this is an API of a
> compiler, rather than an API for compiler backends. Technically the frontend
is
> a compiler, so it's not wrong.
> 
> One option here could be [CompilerFrontEnd], then a front-end with extra APIs
> for analyzer would be the AnalyzerFrontEnd.
> 
> Another idea could be to name it in terms of kernel (e.g. KernelFrontEnd,
> KernelCompiler, or KernelGenerator).

Good suggestions.  Since I'm going to try re-working with top level functions,
the class will go away, so it's probably moot.

https://codereview.chromium.org/2417043003/diff/1/pkg/front_end/lib/compiler_...
pkg/front_end/lib/compiler_api.dart:49: void set packages(String path);
On 2016/10/14 14:54:45, Brian Wilkerson wrote:
> > Since the front end isn't likely to be used until Dart 2.0, I figured we
> > shouldn't support deprecated functionality like the "packages" directory.
> 
> I have asked several people, and as far as I can tell there is no plan to
> disallow the use of a packages directory. Pub won't create one for you, but as
> far as I can tell it will be allowed. Do you have a different understanding?

We had a bunch of discussion about this, and agreed that we don't know whether
Dart 2.0 will need to support a packages directory or not.  For the short term,
I'm going to go with the assumption that it won't (at least for the API), on the
grounds that it will try to push people in a direction that I think is good for
Dart.  If I turn out to be incorrect, I believe the amount of additional effort
to add package directory support back into the API.

https://codereview.chromium.org/2417043003/diff/1/pkg/front_end/lib/compiler_...
pkg/front_end/lib/compiler_api.dart:61: void set uriOverride(Map<Uri, String>
mapping);
On 2016/10/14 16:00:48, Siggi Cherem (dart-lang) wrote:
> On 2016/10/14 00:09:36, Paul Berry wrote:
> > On 2016/10/13 23:22:15, Siggi Cherem (dart-lang) wrote:
> > > is this for dart:* libraries or for mappings like we had to do for
summaries
> > in
> > > blaze?
> > 
> > It's for the mappings we have to do for Bazel.  It corresponds precisely to
> the
> > DDC command line option "--url-mapping".  Analyzer_cli's build mode has a
> > similar feature, but it's more clumsy IMHO (it requires all input files to
be
> > specified on the command line as URI/pathname pairs).
> > 
> > It's not intended for redirecting dart:* URIs, and it shouldn't be
necessary,
> > since the use cases where it will be used are for modular compilation, in
> which
> > case the dart:* URIs are coming from summaries, in which case the
translation
> > from URI to file path will never happen for dart:* URIs.  But I can't decide
> > whether we should prohibit dart:* URIs or not--we might need to allow them
as
> > part of a bootstrapping process.
> 
> Ah - if it is mainly for bazel, this might be a good time to see if it is
> feasible to move towards the approach we used in dart2js that I was telling
you
> about a few weeks ago (where we specify "file://bazel-root/" and search within
a
> handful of bazel-roots). 

I would like to learn more about this from you.

https://codereview.chromium.org/2417043003/diff/1/pkg/front_end/lib/compiler_...
pkg/front_end/lib/compiler_api.dart:64: /// applied to the SDK.
On 2016/10/14 16:00:49, Siggi Cherem (dart-lang) wrote:
> On 2016/10/14 00:09:36, Paul Berry wrote:
> > On 2016/10/13 23:58:17, scheglov wrote:
> > > It is always a single platform, right?
> > > The phrase "bit mask" sounds as it might be a combination of several bits.
> > 
> > It's always a single platform, but it must be a power of two, because that's
> > necessary to allow bit masks to be used in libraries.dart to share patch
files
> > among platforms.
> > 
> > I'll improve the wording to clarify.
> 
> I wonder if we should add an enum to libraries.dart and import it here, so we
> can expose this in a way that is easier to understand.
> 
> enum Platforms {
>   dart2js, vm
> }

That's a good idea, but I'm not sure how it would work since the platforms need
to be powers of two.

https://codereview.chromium.org/2417043003/diff/1/pkg/front_end/lib/compiler_...
pkg/front_end/lib/compiler_api.dart:75: void set permittedFiles(Iterable<String>
files);
On 2016/10/14 16:00:49, Siggi Cherem (dart-lang) wrote:
> On 2016/10/14 00:09:36, Paul Berry wrote:
> > On 2016/10/13 23:22:15, Siggi Cherem (dart-lang) wrote:
> > > is this mainly for validation in blaze? (to check that we don't have read
> both
> > > from sources + summaries for the same library?)
> > 
> > It's for Bazel workers.  Bazel workers aren't hermetically sealed like
normal
> > Bazel actions, so they are responsible for ensuring that they only access
the
> > files that Bazel instructs them to access.  I can't recall if analyzer_cli's
> > build mode implements this functionality or not, but it should, and so
should
> > DDC, because it is necessary for reproducible builds.
> 
> What do you think about making the front-end hermetic always?
> 
> In particular, I'd:
>   - rename [permittedFiles] to [sources]
>   - make "sources" a mandatory argument
>   - rename the argument in sourceToKernel from [sources] to [entryPoints] or
> [entrySources]
> 
> Then rather than saying that the front-end will pretend that no other file
> exists, we would say that the frontend only loads the files that were listed.
It
> also conveys that the "entryPoints" must be a subset of these sources.

If we do this we won't be able to support the use case where the VM is pointed
to a single file and it's expected to chase imports to find all imported files. 
IMHO that's a pretty important use case.

However, I think it does highlight that we have essentially two different use
cases here: import chasing and non-import chasing, and some combinations of
options make sense for one use case but not the other.  I will try to see if the
set of options can be simplified based on this insight.

https://codereview.chromium.org/2417043003/diff/1/pkg/front_end/lib/compiler_...
pkg/front_end/lib/compiler_api.dart:90: /// Dart Kernel.
On 2016/10/14 16:00:48, Siggi Cherem (dart-lang) wrote:
> On 2016/10/14 00:09:36, Paul Berry wrote:
> > On 2016/10/13 23:22:16, Siggi Cherem (dart-lang) wrote:
> > > + and will be deperecated in the future?
> > 
> > Yes that's my intent.
> > 
> > > 
> > > OR, should we move this to a subclass of CompilerApi so that it is not
> exposed
> > > by default (less breaking changes when we deprecate it?)
> > 
> > Hmm, good question.  I don't know what would be best.
> 
> Let's keep it separate then (either on a separate top-level or class depending
> on what we decide above)

Agreed.

scheglov

lgtm https://codereview.chromium.org/2417043003/diff/40001/pkg/front_end/lib/options.dart File pkg/front_end/lib/options.dart (right): https://codereview.chromium.org/2417043003/diff/40001/pkg/front_end/lib/options.dart#newcode19 pkg/front_end/lib/options.dart:19: /// If `null`, the SDK will be searched ...

4 years, 2 months ago (2016-10-16 21:19:39 UTC) #14

Paul Berry

https://codereview.chromium.org/2417043003/diff/40001/pkg/front_end/lib/options.dart File pkg/front_end/lib/options.dart (right): https://codereview.chromium.org/2417043003/diff/40001/pkg/front_end/lib/options.dart#newcode19 pkg/front_end/lib/options.dart:19: /// If `null`, the SDK will be searched for ...

4 years, 2 months ago (2016-10-17 12:54:39 UTC) #15

Brian Wilkerson

https://codereview.chromium.org/2417043003/diff/60001/pkg/front_end/lib/kernel_generator.dart File pkg/front_end/lib/kernel_generator.dart (right): https://codereview.chromium.org/2417043003/diff/60001/pkg/front_end/lib/kernel_generator.dart#newcode23 pkg/front_end/lib/kernel_generator.dart:23: Future<kernel.Program> compileProgram(Options options, Uri source) => Using "compile" seems ...

4 years, 2 months ago (2016-10-17 15:08:14 UTC) #16

https://codereview.chromium.org/2417043003/diff/60001/pkg/front_end/lib/kerne...
File pkg/front_end/lib/kernel_generator.dart (right):

https://codereview.chromium.org/2417043003/diff/60001/pkg/front_end/lib/kerne...
pkg/front_end/lib/kernel_generator.dart:23: Future<kernel.Program>
compileProgram(Options options, Uri source) =>
Using "compile" seems wrong here (and below), because this is only doing part of
the work that I think "compile" is typically used to mean. I don't like
"analyze" any better, because this is still a subset. I'm not sure what to
suggest, but I think it would be worth trying to find a better name. I'd even
prefer something generic, such as "process" or "create", or a made-up term, such
as "kernelize". One of the properties of the Kernel form is that it lowers Dart
to a more primitive form, so perhaps something like "lower".

https://codereview.chromium.org/2417043003/diff/60001/pkg/front_end/lib/kerne...
pkg/front_end/lib/kernel_generator.dart:32: /// compiled to Dart Kernel objects.
nit: Does Kernel have a name for its representation of a library other than
"object"? If so, it would be better to be more specific.

https://codereview.chromium.org/2417043003/diff/60001/pkg/front_end/lib/optio...
File pkg/front_end/lib/options.dart (right):

https://codereview.chromium.org/2417043003/diff/60001/pkg/front_end/lib/optio...
pkg/front_end/lib/options.dart:12: /// Front-end Analysis options relevant to
compiler back ends.
nit: Remove "Analysis"? It doesn't appear to be adding anything.

https://codereview.chromium.org/2417043003/diff/60001/pkg/front_end/lib/optio...
pkg/front_end/lib/options.dart:16: class Options {
"Options" is really generic. I doubt that you want to add all possible options
for all of the API's that the front end will eventually support to one class, so
consider qualifying the name.

https://codereview.chromium.org/2417043003/diff/60001/pkg/front_end/lib/optio...
pkg/front_end/lib/options.dart:36: List<String> inputSummaries = [];
Is there a strong connection between inputSummaries and packagesFilePath? I'm
assuming that the summary files in this list are like the sdk summary file in
that they don't contain any information about the bodies of functions. If that's
true, then we need to be able to locate the source associated with any given
library. Is that done via the .packages file (for non-bazel packages) and
bazelRoots (for bazel packages)?

If so, I think this needs to be made explicit. If not, then how are sources for
file in summaries found?

https://codereview.chromium.org/2417043003/diff/60001/pkg/front_end/lib/optio...
pkg/front_end/lib/options.dart:42: String sdkSummary;
Perhaps mention that it is critical that the sdkSummary (if set) is consistent
with the contents of the SDK in sdkPath.

Given the tight coupling, should this even be an option? We will always find the
SDK sources; do we really need to allow a summary file outside the SDK to be
used?

https://codereview.chromium.org/2417043003/diff/60001/pkg/front_end/lib/optio...
pkg/front_end/lib/options.dart:51: /// "file:///bazel-root" mechanism, and then
remove this.
Is this mechanism one that you created, or is this in use elsewhere? (Perhaps
best discussed off-line.)

Siggi Cherem (dart-lang)

looks great Paul, a few minor comments and replies below https://codereview.chromium.org/2417043003/diff/1/pkg/front_end/lib/compiler_api.dart File pkg/front_end/lib/compiler_api.dart (right): https://codereview.chromium.org/2417043003/diff/1/pkg/front_end/lib/compiler_api.dart#newcode64 ...

4 years, 2 months ago (2016-10-17 16:27:30 UTC) #17

looks great Paul, a few minor comments and replies below

https://codereview.chromium.org/2417043003/diff/1/pkg/front_end/lib/compiler_...
File pkg/front_end/lib/compiler_api.dart (right):

https://codereview.chromium.org/2417043003/diff/1/pkg/front_end/lib/compiler_...
pkg/front_end/lib/compiler_api.dart:64: /// applied to the SDK.
On 2016/10/14 21:32:34, Paul Berry wrote:
> On 2016/10/14 16:00:49, Siggi Cherem (dart-lang) wrote:
> > On 2016/10/14 00:09:36, Paul Berry wrote:
> > > On 2016/10/13 23:58:17, scheglov wrote:
> > > > It is always a single platform, right?
> > > > The phrase "bit mask" sounds as it might be a combination of several
bits.
> > > 
> > > It's always a single platform, but it must be a power of two, because
that's
> > > necessary to allow bit masks to be used in libraries.dart to share patch
> files
> > > among platforms.
> > > 
> > > I'll improve the wording to clarify.
> > 
> > I wonder if we should add an enum to libraries.dart and import it here, so
we
> > can expose this in a way that is easier to understand.
> > 
> > enum Platforms {
> >   dart2js, vm
> > }
> 
> That's a good idea, but I'm not sure how it would work since the platforms
need
> to be powers of two.

[sorry I forgot to send this comment earlier]:
To me the fact that PLATFORM are bit flags in libraries is an implementation
detail of libraries.dart we don't necessarily have to carry over here. If we
used an enum, internally the front end would map it to the appropriate bit
masks.

https://codereview.chromium.org/2417043003/diff/60001/pkg/front_end/lib/compi...
File pkg/front_end/lib/compilation_error.dart (right):

https://codereview.chromium.org/2417043003/diff/60001/pkg/front_end/lib/compi...
pkg/front_end/lib/compilation_error.dart:23: FileSpan get location;
consider switching to just use the interface (SourceSpan) for the API
declaration?

Most of the time we'll likely use FileSpan, but I can imagine cases where we
might want to report an error with information derived from summaries or other
kernel files. In those cases, we might need a different implementation of
SourceSpan.

https://codereview.chromium.org/2417043003/diff/60001/pkg/front_end/lib/kerne...
File pkg/front_end/lib/kernel_generator.dart (right):

https://codereview.chromium.org/2417043003/diff/60001/pkg/front_end/lib/kerne...
pkg/front_end/lib/kernel_generator.dart:12: /// Processes the program whose main
library is in the given [source].
Processes => Generate Kernel for ...

https://codereview.chromium.org/2417043003/diff/60001/pkg/front_end/lib/kerne...
pkg/front_end/lib/kernel_generator.dart:23: Future<kernel.Program>
compileProgram(Options options, Uri source) =>
On 2016/10/17 15:08:14, Brian Wilkerson wrote:
> Using "compile" seems wrong here (and below), because this is only doing part
of
> the work that I think "compile" is typically used to mean. I don't like
> "analyze" any better, because this is still a subset. I'm not sure what to
> suggest, but I think it would be worth trying to find a better name. I'd even
> prefer something generic, such as "process" or "create", or a made-up term,
such
> as "kernelize". One of the properties of the Kernel form is that it lowers
Dart
> to a more primitive form, so perhaps something like "lower".

other ideas:
 - generateKernel
 - programToKernel
 - lowerProgramToKernel
 - programKernelFor

another nit: consider moving options as a second argument.

https://codereview.chromium.org/2417043003/diff/60001/pkg/front_end/lib/kerne...
pkg/front_end/lib/kernel_generator.dart:49: Future<kernel.Program>
compileBuildUnit(Options options, List<Uri> sources) =>
nit: change the return type, since a build-unit is not a kernel.Program, maybe
List<kernel.Library>? (not sure if this is related to your TODO above)

https://codereview.chromium.org/2417043003/diff/60001/pkg/front_end/lib/kerne...
pkg/front_end/lib/kernel_generator.dart:49: Future<kernel.Program>
compileBuildUnit(Options options, List<Uri> sources) =>
if we rename "compileProgram", I'd rename this too to match. My thinking is:

programKernelFor(source, options);
buildUnitKernelFor(List sources, options); // or moduleKernelFor(List sources,
options);

I've seen the style "somethingFor" a lot in dart2js, so I've grown to like it.
If we pick that, I'd suggest renaming the others (resolveBuildUnit =>
resolvedAstFor, summarizeBuildUnit => summaryFor)

https://codereview.chromium.org/2417043003/diff/60001/pkg/front_end/lib/optio...
File pkg/front_end/lib/options.dart (right):

https://codereview.chromium.org/2417043003/diff/60001/pkg/front_end/lib/optio...
pkg/front_end/lib/options.dart:36: List<String> inputSummaries = [];
On 2016/10/17 15:08:14, Brian Wilkerson wrote:
> Is there a strong connection between inputSummaries and packagesFilePath?

I don't believe there should be any connection.

> I'm
> assuming that the summary files in this list are like the sdk summary file in
> that they don't contain any information about the bodies of functions. 

Correct.

> If that's
> true, then we need to be able to locate the source associated with any given
> library. Is that done via the .packages file (for non-bazel packages) and
> bazelRoots (for bazel packages)?

Yes - summaries will have a resolved URL of the library (either a package:url or
a file:/ url for which we know where to locate on disk (either via the
uriOverrides or via the bazel-root mechanism below).

> If so, I think this needs to be made explicit. If not, then how are sources
for
> file in summaries found?

Would it be fair to say that summaries can contain a URI for the sources where
they are derived from and that these URIs can be resolved using the same
resolution mechanism that we would use if that URI was imported in the code?

https://codereview.chromium.org/2417043003/diff/60001/pkg/front_end/lib/optio...
pkg/front_end/lib/options.dart:42: String sdkSummary;
> Given the tight coupling, should this even be an option? We will always find
the
> SDK sources; do we really need to allow a summary file outside the SDK to be
> used?

I think this option is important to have:
 - it helps on certain build systems to be able to store the sdk-summary file in
a different location than the SDK itself.
 - for modular compilation we'll use an sdk summary and generate kernel that
refer to sdk symbols but will not include the kernel code for the sdk itself. In
those cases I'd like to only specify where to find this file.

In the past, when integrating with bazel, we have had to create a fake sdk
folder structure just to be able to load the sdk summaries. I'd like to avoid
that in the future.

https://codereview.chromium.org/2417043003/diff/60001/pkg/front_end/lib/optio...
pkg/front_end/lib/options.dart:51: /// "file:///bazel-root" mechanism, and then
remove this.
On 2016/10/17 15:08:14, Brian Wilkerson wrote:
> Is this mechanism one that you created, or is this in use elsewhere? (Perhaps
> best discussed off-line.)

This is a mechanism we introduced in dart2js when integrating our modular mode
with bazel. I'm happy to share it more details offline, but it's well summarized
in the bazelRoots dart doc below.

https://codereview.chromium.org/2417043003/diff/60001/pkg/front_end/lib/summa...
File pkg/front_end/lib/summary_generator.dart (right):

https://codereview.chromium.org/2417043003/diff/60001/pkg/front_end/lib/summa...
pkg/front_end/lib/summary_generator.dart:29: /// The return value is a list of
bytes to write to the summary file.
Not sure yet about the use cases, but are there situations where it could be
useful to return the in-memory summary representation here? (so we would do the
encoding to bytes separately?)

Paul Berry

PATL https://codereview.chromium.org/2417043003/diff/60001/pkg/front_end/lib/compilation_error.dart File pkg/front_end/lib/compilation_error.dart (right): https://codereview.chromium.org/2417043003/diff/60001/pkg/front_end/lib/compilation_error.dart#newcode23 pkg/front_end/lib/compilation_error.dart:23: FileSpan get location; On 2016/10/17 16:27:29, Siggi Cherem ...

4 years, 2 months ago (2016-10-17 17:04:28 UTC) #18

PATL

https://codereview.chromium.org/2417043003/diff/60001/pkg/front_end/lib/compi...
File pkg/front_end/lib/compilation_error.dart (right):

https://codereview.chromium.org/2417043003/diff/60001/pkg/front_end/lib/compi...
pkg/front_end/lib/compilation_error.dart:23: FileSpan get location;
On 2016/10/17 16:27:29, Siggi Cherem (dart-lang) wrote:
> consider switching to just use the interface (SourceSpan) for the API
> declaration?
> 
> Most of the time we'll likely use FileSpan, but I can imagine cases where we
> might want to report an error with information derived from summaries or other
> kernel files. In those cases, we might need a different implementation of
> SourceSpan.

Done.

https://codereview.chromium.org/2417043003/diff/60001/pkg/front_end/lib/kerne...
File pkg/front_end/lib/kernel_generator.dart (right):

https://codereview.chromium.org/2417043003/diff/60001/pkg/front_end/lib/kerne...
pkg/front_end/lib/kernel_generator.dart:12: /// Processes the program whose main
library is in the given [source].
On 2016/10/17 16:27:29, Siggi Cherem (dart-lang) wrote:
> Processes => Generate Kernel for ... 

Done.

https://codereview.chromium.org/2417043003/diff/60001/pkg/front_end/lib/kerne...
pkg/front_end/lib/kernel_generator.dart:23: Future<kernel.Program>
compileProgram(Options options, Uri source) =>
On 2016/10/17 16:27:29, Siggi Cherem (dart-lang) wrote:
> On 2016/10/17 15:08:14, Brian Wilkerson wrote:
> > Using "compile" seems wrong here (and below), because this is only doing
part
> of
> > the work that I think "compile" is typically used to mean. I don't like
> > "analyze" any better, because this is still a subset. I'm not sure what to
> > suggest, but I think it would be worth trying to find a better name. I'd
even
> > prefer something generic, such as "process" or "create", or a made-up term,
> such
> > as "kernelize". One of the properties of the Kernel form is that it lowers
> Dart
> > to a more primitive form, so perhaps something like "lower".
> 
> other ideas:
>  - generateKernel
>  - programToKernel
>  - lowerProgramToKernel
>  - programKernelFor

I like the "fooFor" convention, but when we need to disambiguate possible things
that might be converted to form "foo", it's not obvious to me that "barFooFor"
means "convert a bar to a foo".  How about "kernelForProgram"?  I think it's
pretty intuitive for that to mean "convert a program to kernel form".

> another nit: consider moving options as a second argument.

Done.

https://codereview.chromium.org/2417043003/diff/60001/pkg/front_end/lib/kerne...
pkg/front_end/lib/kernel_generator.dart:32: /// compiled to Dart Kernel objects.
On 2016/10/17 15:08:14, Brian Wilkerson wrote:
> nit: Does Kernel have a name for its representation of a library other than
> "object"? If so, it would be better to be more specific.

Kernel calls them "Library".  Changed to "Dart Kernel Library objects".

https://codereview.chromium.org/2417043003/diff/60001/pkg/front_end/lib/kerne...
pkg/front_end/lib/kernel_generator.dart:49: Future<kernel.Program>
compileBuildUnit(Options options, List<Uri> sources) =>
On 2016/10/17 16:27:29, Siggi Cherem (dart-lang) wrote:
> if we rename "compileProgram", I'd rename this too to match. My thinking is:
> 
> programKernelFor(source, options);
> buildUnitKernelFor(List sources, options); // or moduleKernelFor(List sources,
> options);
> 
> I've seen the style "somethingFor" a lot in dart2js, so I've grown to like it.
> If we pick that, I'd suggest renaming the others (resolveBuildUnit =>
> resolvedAstFor, summarizeBuildUnit => summaryFor)

Went with "kernelForBuildUnit" here, and "resolvedAstFor"/"summaryFor" in the
other files (where there is no ambiguity).

https://codereview.chromium.org/2417043003/diff/60001/pkg/front_end/lib/kerne...
pkg/front_end/lib/kernel_generator.dart:49: Future<kernel.Program>
compileBuildUnit(Options options, List<Uri> sources) =>
On 2016/10/17 16:27:29, Siggi Cherem (dart-lang) wrote:
> nit: change the return type, since a build-unit is not a kernel.Program, maybe
> List<kernel.Library>? (not sure if this is related to your TODO above)

kernel.Program also contains a `uriToLineStarts` map, which I think will still
be needed in the case of modular compilation.  But it might be even better to
add a data type to kernel to represent the result of compiling a build unit. 
I've added a TODO to that effect.

https://codereview.chromium.org/2417043003/diff/60001/pkg/front_end/lib/optio...
File pkg/front_end/lib/options.dart (right):

https://codereview.chromium.org/2417043003/diff/60001/pkg/front_end/lib/optio...
pkg/front_end/lib/options.dart:12: /// Front-end Analysis options relevant to
compiler back ends.
On 2016/10/17 15:08:14, Brian Wilkerson wrote:
> nit: Remove "Analysis"? It doesn't appear to be adding anything.

Done.

https://codereview.chromium.org/2417043003/diff/60001/pkg/front_end/lib/optio...
pkg/front_end/lib/options.dart:16: class Options {
On 2016/10/17 15:08:14, Brian Wilkerson wrote:
> "Options" is really generic. I doubt that you want to add all possible options
> for all of the API's that the front end will eventually support to one class,
so
> consider qualifying the name.

Ok, renamed to "CompilerOptions".

https://codereview.chromium.org/2417043003/diff/60001/pkg/front_end/lib/optio...
pkg/front_end/lib/options.dart:36: List<String> inputSummaries = [];
On 2016/10/17 16:27:29, Siggi Cherem (dart-lang) wrote:
> On 2016/10/17 15:08:14, Brian Wilkerson wrote:
> > Is there a strong connection between inputSummaries and packagesFilePath?
> 
> I don't believe there should be any connection.
> 
> > I'm
> > assuming that the summary files in this list are like the sdk summary file
in
> > that they don't contain any information about the bodies of functions. 
> 
> Correct.
> 
> > If that's
> > true, then we need to be able to locate the source associated with any given
> > library. Is that done via the .packages file (for non-bazel packages) and
> > bazelRoots (for bazel packages)?
> 
> Yes - summaries will have a resolved URL of the library (either a package:url
or
> a file:/ url for which we know where to locate on disk (either via the
> uriOverrides or via the bazel-root mechanism below).
> 
> > If so, I think this needs to be made explicit. If not, then how are sources
> for
> > file in summaries found?
> 
> Would it be fair to say that summaries can contain a URI for the sources where
> they are derived from and that these URIs can be resolved using the same
> resolution mechanism that we would use if that URI was imported in the code?

Yes, that's true.  Although in practice I'm not sure how important this is.  In
the modular compilation case, the compiler isn't going to need to look at the
sources corresponding to a summary, because the information in the summary file
will be sufficient.  In the whole program compilation case, we're not going to
gain much benefit from using summaries anyhow.

I'm leaving the comment as is for now.  If it's still unclear, I'm happy to
discuss in person and we can decide exactly what we want to say in the comment.

https://codereview.chromium.org/2417043003/diff/60001/pkg/front_end/lib/optio...
pkg/front_end/lib/options.dart:42: String sdkSummary;
On 2016/10/17 15:08:14, Brian Wilkerson wrote:
> Perhaps mention that it is critical that the sdkSummary (if set) is consistent
> with the contents of the SDK in sdkPath.

Actually this is not critical.  As with the situation above, in the modular
compilation case, if the sdk summary is set, then the SDK source files won't
need to be used.  In the whole program compilation case, the summaries are of
little benefit.

> 
> Given the tight coupling, should this even be an option? We will always find
the
> SDK sources; do we really need to allow a summary file outside the SDK to be
> used?

In addition to the reasons Siggi stated, we need this to cover the case where
the version of the front end being used does not match the version published in
the SDK; in that case we will need to use the front end to create its own
summary of the SDK which is independent of the summary stored in the SDK itself;
then we will need to pass this summary into dependent build steps.

https://codereview.chromium.org/2417043003/diff/60001/pkg/front_end/lib/summa...
File pkg/front_end/lib/summary_generator.dart (right):

https://codereview.chromium.org/2417043003/diff/60001/pkg/front_end/lib/summa...
pkg/front_end/lib/summary_generator.dart:29: /// The return value is a list of
bytes to write to the summary file.
On 2016/10/17 16:27:29, Siggi Cherem (dart-lang) wrote:
> Not sure yet about the use cases, but are there situations where it could be
> useful to return the in-memory summary representation here? (so we would do
the
> encoding to bytes separately?)

At the moment I can't imagine a scenario where it will be necessary, and even if
it does become necessary I believe the performance impact of extra
serialization/deserialization would be small.  I'd prefer to stick with the
simpler API for now.

Brian Wilkerson

https://codereview.chromium.org/2417043003/diff/60001/pkg/front_end/lib/options.dart File pkg/front_end/lib/options.dart (right): https://codereview.chromium.org/2417043003/diff/60001/pkg/front_end/lib/options.dart#newcode36 pkg/front_end/lib/options.dart:36: List<String> inputSummaries = []; > > If that's true, ...

4 years, 2 months ago (2016-10-17 17:09:16 UTC) #19

https://codereview.chromium.org/2417043003/diff/60001/pkg/front_end/lib/optio...
File pkg/front_end/lib/options.dart (right):

https://codereview.chromium.org/2417043003/diff/60001/pkg/front_end/lib/optio...
pkg/front_end/lib/options.dart:36: List<String> inputSummaries = [];
> > If that's true, then we need to be able to locate the source associated with
any given
> > library. Is that done via the .packages file (for non-bazel packages) and
> > bazelRoots (for bazel packages)?
> 
> Yes - summaries will have a resolved URL of the library (either a package:url
or
> a file:/ url for which we know where to locate on disk (either via the
> uriOverrides or via the bazel-root mechanism below).

So, the packagesFilePath is used to resolve package: URI's found in source code
(unless a uriOverride is provided), but not those in summaries? Are
packageFilePath and uriOverride mutually exclusive?

> > If so, I think this needs to be made explicit. If not, then how are sources
for
> > file in summaries found?
> 
> Would it be fair to say that summaries can contain a URI for the sources where
> they are derived from and that these URIs can be resolved using the same
> resolution mechanism that we would use if that URI was imported in the code?

Given that we don't know, at the time we're producing a summary, where the file
containing that summary will be located, all of the URI's in a summary file
would need to be absolute. Given that all of the URI's in a summary file are
absolute, then, yes, I think that would work.

I still think this should be carefully explained in the docs, because it isn't
obvious to everyone.

https://codereview.chromium.org/2417043003/diff/60001/pkg/front_end/lib/optio...
pkg/front_end/lib/options.dart:42: String sdkSummary;
> I think this option is important to have:
>  - it helps on certain build systems to be able to store the sdk-summary file
in
> a different location than the SDK itself.

Are we planning on shipping a version of the SDK that doesn't include a summary?

>  - for modular compilation we'll use an sdk summary and generate kernel that
> refer to sdk symbols but will not include the kernel code for the sdk itself.
In
> those cases I'd like to only specify where to find this file.

If there's a reason why we can't get the SDK summary from the SDK, then it makes
sense, but I don't understand why would we be unable to get the summary from the
SDK. Sounds like we should talk.

> In the past, when integrating with bazel, we have had to create a fake sdk
> folder structure just to be able to load the sdk summaries. I'd like to avoid
> that in the future.

Yeah, that sounds pretty awful. I don't understand why that was necessary,
though.

https://codereview.chromium.org/2417043003/diff/60001/pkg/front_end/lib/optio...
pkg/front_end/lib/options.dart:51: /// "file:///bazel-root" mechanism, and then
remove this.
Yes, I would like to get more info. Thanks!

Analyzer has a different way of dealing with this, so I'd like to understand the
trade-offs of the two approaches.

Brian Wilkerson

https://codereview.chromium.org/2417043003/diff/60001/pkg/front_end/lib/options.dart File pkg/front_end/lib/options.dart (right): https://codereview.chromium.org/2417043003/diff/60001/pkg/front_end/lib/options.dart#newcode42 pkg/front_end/lib/options.dart:42: String sdkSummary; > ... in the modular compilation case, ...

4 years, 2 months ago (2016-10-17 17:20:44 UTC) #20

Paul Berry

https://codereview.chromium.org/2417043003/diff/60001/pkg/front_end/lib/options.dart File pkg/front_end/lib/options.dart (right): https://codereview.chromium.org/2417043003/diff/60001/pkg/front_end/lib/options.dart#newcode36 pkg/front_end/lib/options.dart:36: List<String> inputSummaries = []; On 2016/10/17 17:09:16, Brian Wilkerson ...

4 years, 2 months ago (2016-10-17 17:32:06 UTC) #21

https://codereview.chromium.org/2417043003/diff/60001/pkg/front_end/lib/optio...
File pkg/front_end/lib/options.dart (right):

https://codereview.chromium.org/2417043003/diff/60001/pkg/front_end/lib/optio...
pkg/front_end/lib/options.dart:36: List<String> inputSummaries = [];
On 2016/10/17 17:09:16, Brian Wilkerson wrote:
> > > If that's true, then we need to be able to locate the source associated
with
> any given
> > > library. Is that done via the .packages file (for non-bazel packages) and
> > > bazelRoots (for bazel packages)?
> > 
> > Yes - summaries will have a resolved URL of the library (either a
package:url
> or
> > a file:/ url for which we know where to locate on disk (either via the
> > uriOverrides or via the bazel-root mechanism below).
> 
> So, the packagesFilePath is used to resolve package: URI's found in source
code
> (unless a uriOverride is provided), but not those in summaries?

Correct.

> Are
> packageFilePath and uriOverride mutually exclusive?

No, packageFilePath and uriOverride are not mutually exclusive.  One overrides
the other.

Here's the resolution process as I understand it in pseudocode:

resolveUriToLibrary(uri) {
  if (any summary refers to uri) {
    return resynthesizeLibraryFromSummary(uri);
  }
  String filePath;
  if (uriOverride.containsKey(uri)) {
    filePath = uriOverride[uri];
  } else {
    if (uri.scheme == 'package') {
      uri = resolve package URI using packagesFilePath;
    }
    assert(uri.scheme == 'file');
    filePath = uri.path; // Neglecting Windows subtleties
    if (filePath starts with "/bazel-root/") {
      String pathRelativeToRoot = remainder of filePath after "/bazel-root/";
      for (bazelRoot in bazelRoots) {
        String candidatePath = path.join(bazelRoot, pathRelativeToRoot);
        if (a file exists at candidatePath) {
          filePath = candidatePath;
          break;
        }
      }
    }
  }
  return createElementModelForLibrary(filePath);
}

> 
> > > If so, I think this needs to be made explicit. If not, then how are
sources
> for
> > > file in summaries found?
> > 
> > Would it be fair to say that summaries can contain a URI for the sources
where
> > they are derived from and that these URIs can be resolved using the same
> > resolution mechanism that we would use if that URI was imported in the code?
> 
> Given that we don't know, at the time we're producing a summary, where the
file
> containing that summary will be located, all of the URI's in a summary file
> would need to be absolute. Given that all of the URI's in a summary file are
> absolute, then, yes, I think that would work.
> 
> I still think this should be carefully explained in the docs, because it isn't
> obvious to everyone.

Would you mind sitting with me at my desk and helping me craft the necessary
documentation?  I want to make sure I write something that will adequately
explain the situation, and I suspect the most efficient way to do that would be
to do it "pair programming" style, in person, so that we can reduce the amount
of back and forth review.

https://codereview.chromium.org/2417043003/diff/60001/pkg/front_end/lib/optio...
pkg/front_end/lib/options.dart:42: String sdkSummary;
On 2016/10/17 17:09:16, Brian Wilkerson wrote:
> > I think this option is important to have:
> >  - it helps on certain build systems to be able to store the sdk-summary
file
> in
> > a different location than the SDK itself.
> 
> Are we planning on shipping a version of the SDK that doesn't include a
summary?
> 
> >  - for modular compilation we'll use an sdk summary and generate kernel that
> > refer to sdk symbols but will not include the kernel code for the sdk
itself.
> In
> > those cases I'd like to only specify where to find this file.
> 
> If there's a reason why we can't get the SDK summary from the SDK, then it
makes
> sense, but I don't understand why would we be unable to get the summary from
the
> SDK. Sounds like we should talk.
> 
> > In the past, when integrating with bazel, we have had to create a fake sdk
> > folder structure just to be able to load the sdk summaries. I'd like to
avoid
> > that in the future.
> 
> Yeah, that sounds pretty awful. I don't understand why that was necessary,
> though.

Some of these are Google-internal use cases.  Let's discuss in person.

https://codereview.chromium.org/2417043003/diff/60001/pkg/front_end/lib/optio...
pkg/front_end/lib/options.dart:51: /// "file:///bazel-root" mechanism, and then
remove this.
On 2016/10/17 17:09:16, Brian Wilkerson wrote:
> Yes, I would like to get more info. Thanks!
> 
> Analyzer has a different way of dealing with this, so I'd like to understand
the
> trade-offs of the two approaches.

Since we already have some discussion to do, I'm happy to explain this.

Siggi Cherem (dart-lang)

https://codereview.chromium.org/2417043003/diff/60001/pkg/front_end/lib/options.dart File pkg/front_end/lib/options.dart (right): https://codereview.chromium.org/2417043003/diff/60001/pkg/front_end/lib/options.dart#newcode36 pkg/front_end/lib/options.dart:36: List<String> inputSummaries = []; On 2016/10/17 17:32:06, Paul Berry ...

4 years, 2 months ago (2016-10-17 18:25:31 UTC) #22

https://codereview.chromium.org/2417043003/diff/60001/pkg/front_end/lib/optio...
File pkg/front_end/lib/options.dart (right):

https://codereview.chromium.org/2417043003/diff/60001/pkg/front_end/lib/optio...
pkg/front_end/lib/options.dart:36: List<String> inputSummaries = [];
On 2016/10/17 17:32:06, Paul Berry wrote:
> On 2016/10/17 17:09:16, Brian Wilkerson wrote:
> > > > If that's true, then we need to be able to locate the source associated
> with
> > any given
> > > > library. Is that done via the .packages file (for non-bazel packages)
and
> > > > bazelRoots (for bazel packages)?
> > > 
> > > Yes - summaries will have a resolved URL of the library (either a
> package:url
> > or
> > > a file:/ url for which we know where to locate on disk (either via the
> > > uriOverrides or via the bazel-root mechanism below).
> > 
> > So, the packagesFilePath is used to resolve package: URI's found in source
> code
> > (unless a uriOverride is provided), but not those in summaries?
> 
> Correct.
> 
> > Are
> > packageFilePath and uriOverride mutually exclusive?
> 
> No, packageFilePath and uriOverride are not mutually exclusive.  One overrides
> the other.
> 
> Here's the resolution process as I understand it in pseudocode:
> 
> resolveUriToLibrary(uri) {
>   if (any summary refers to uri) {
>     return resynthesizeLibraryFromSummary(uri);
>   }


Thanks for writing the pseudocode, it reminds me that the use of URIs in
summaries is even more limited than I thought: we don't even try to resolve it
to a disk location. It's mainly a lookup to check whether or not an (unresolved)
URI is covered by a specific summary.

>   String filePath;
>   if (uriOverride.containsKey(uri)) {
>     filePath = uriOverride[uri];
>   } else {
>     if (uri.scheme == 'package') {
>       uri = resolve package URI using packagesFilePath;
>     }
>     assert(uri.scheme == 'file');
>     filePath = uri.path; // Neglecting Windows subtleties
>     if (filePath starts with "/bazel-root/") {
>       String pathRelativeToRoot = remainder of filePath after "/bazel-root/";
>       for (bazelRoot in bazelRoots) {
>         String candidatePath = path.join(bazelRoot, pathRelativeToRoot);
>         if (a file exists at candidatePath) {
>           filePath = candidatePath;
>           break;
>         }
>       }
>     }
>   }
>   return createElementModelForLibrary(filePath);
> }
> 
> > 
> > > > If so, I think this needs to be made explicit. If not, then how are
> sources
> > for
> > > > file in summaries found?
> > > 
> > > Would it be fair to say that summaries can contain a URI for the sources
> where
> > > they are derived from and that these URIs can be resolved using the same
> > > resolution mechanism that we would use if that URI was imported in the
code?
> > 
> > Given that we don't know, at the time we're producing a summary, where the
> file
> > containing that summary will be located, all of the URI's in a summary file
> > would need to be absolute. Given that all of the URI's in a summary file are
> > absolute, then, yes, I think that would work.
> > 
> > I still think this should be carefully explained in the docs, because it
isn't
> > obvious to everyone.
> 
> Would you mind sitting with me at my desk and helping me craft the necessary
> documentation?  I want to make sure I write something that will adequately
> explain the situation, and I suspect the most efficient way to do that would
be
> to do it "pair programming" style, in person, so that we can reduce the amount
> of back and forth review.

Lets also consider whether all this information needs to be in this frontend API
or documented to somewhere else (e.g. design docs for summaries?). In
particular, I'd only include here what the user of this API needs to know or
provide.

https://codereview.chromium.org/2417043003/diff/60001/pkg/front_end/lib/optio...
pkg/front_end/lib/options.dart:51: /// "file:///bazel-root" mechanism, and then
remove this.
On 2016/10/17 17:32:06, Paul Berry wrote:
> On 2016/10/17 17:09:16, Brian Wilkerson wrote:
> > Yes, I would like to get more info. Thanks!
> > 
> > Analyzer has a different way of dealing with this, so I'd like to understand
> the
> > trade-offs of the two approaches.
> 
> Since we already have some discussion to do, I'm happy to explain this.

Sounds good - I'm happy to join on VC if you like. Just let me know.

Paul Berry

PTAL https://codereview.chromium.org/2417043003/diff/60001/pkg/front_end/lib/options.dart File pkg/front_end/lib/options.dart (right): https://codereview.chromium.org/2417043003/diff/60001/pkg/front_end/lib/options.dart#newcode36 pkg/front_end/lib/options.dart:36: List<String> inputSummaries = []; On 2016/10/17 18:25:31, Siggi ...

4 years, 2 months ago (2016-10-17 18:38:43 UTC) #23

PTAL

https://codereview.chromium.org/2417043003/diff/60001/pkg/front_end/lib/optio...
File pkg/front_end/lib/options.dart (right):

https://codereview.chromium.org/2417043003/diff/60001/pkg/front_end/lib/optio...
pkg/front_end/lib/options.dart:36: List<String> inputSummaries = [];
On 2016/10/17 18:25:31, Siggi Cherem (dart-lang) wrote:
> On 2016/10/17 17:32:06, Paul Berry wrote:
> > On 2016/10/17 17:09:16, Brian Wilkerson wrote:
> > > > > If that's true, then we need to be able to locate the source
associated
> > with
> > > any given
> > > > > library. Is that done via the .packages file (for non-bazel packages)
> and
> > > > > bazelRoots (for bazel packages)?
> > > > 
> > > > Yes - summaries will have a resolved URL of the library (either a
> > package:url
> > > or
> > > > a file:/ url for which we know where to locate on disk (either via the
> > > > uriOverrides or via the bazel-root mechanism below).
> > > 
> > > So, the packagesFilePath is used to resolve package: URI's found in source
> > code
> > > (unless a uriOverride is provided), but not those in summaries?
> > 
> > Correct.
> > 
> > > Are
> > > packageFilePath and uriOverride mutually exclusive?
> > 
> > No, packageFilePath and uriOverride are not mutually exclusive.  One
overrides
> > the other.
> > 
> > Here's the resolution process as I understand it in pseudocode:
> > 
> > resolveUriToLibrary(uri) {
> >   if (any summary refers to uri) {
> >     return resynthesizeLibraryFromSummary(uri);
> >   }
> 
> 
> Thanks for writing the pseudocode, it reminds me that the use of URIs in
> summaries is even more limited than I thought: we don't even try to resolve it
> to a disk location. It's mainly a lookup to check whether or not an
(unresolved)
> URI is covered by a specific summary.
> 
> >   String filePath;
> >   if (uriOverride.containsKey(uri)) {
> >     filePath = uriOverride[uri];
> >   } else {
> >     if (uri.scheme == 'package') {
> >       uri = resolve package URI using packagesFilePath;
> >     }
> >     assert(uri.scheme == 'file');
> >     filePath = uri.path; // Neglecting Windows subtleties
> >     if (filePath starts with "/bazel-root/") {
> >       String pathRelativeToRoot = remainder of filePath after
"/bazel-root/";
> >       for (bazelRoot in bazelRoots) {
> >         String candidatePath = path.join(bazelRoot, pathRelativeToRoot);
> >         if (a file exists at candidatePath) {
> >           filePath = candidatePath;
> >           break;
> >         }
> >       }
> >     }
> >   }
> >   return createElementModelForLibrary(filePath);
> > }
> > 
> > > 
> > > > > If so, I think this needs to be made explicit. If not, then how are
> > sources
> > > for
> > > > > file in summaries found?
> > > > 
> > > > Would it be fair to say that summaries can contain a URI for the sources
> > where
> > > > they are derived from and that these URIs can be resolved using the same
> > > > resolution mechanism that we would use if that URI was imported in the
> code?
> > > 
> > > Given that we don't know, at the time we're producing a summary, where the
> > file
> > > containing that summary will be located, all of the URI's in a summary
file
> > > would need to be absolute. Given that all of the URI's in a summary file
are
> > > absolute, then, yes, I think that would work.
> > > 
> > > I still think this should be carefully explained in the docs, because it
> isn't
> > > obvious to everyone.
> > 
> > Would you mind sitting with me at my desk and helping me craft the necessary
> > documentation?  I want to make sure I write something that will adequately
> > explain the situation, and I suspect the most efficient way to do that would
> be
> > to do it "pair programming" style, in person, so that we can reduce the
amount
> > of back and forth review.
> 
> Lets also consider whether all this information needs to be in this frontend
API
> or documented to somewhere else (e.g. design docs for summaries?). In
> particular, I'd only include here what the user of this API needs to know or
> provide.
> 

Acknowledged.  In the interest of trying to get this CL landed, I'd prefer to
leave things documented here for now.  If/when we publish an official design doc
for summaries, I'm happy to change the comments here to point to the published
doc.

Brian Wilkerson

lgtm https://codereview.chromium.org/2417043003/diff/120001/pkg/front_end/lib/resolved_ast_generator.dart File pkg/front_end/lib/resolved_ast_generator.dart (right): https://codereview.chromium.org/2417043003/diff/120001/pkg/front_end/lib/resolved_ast_generator.dart#newcode36 pkg/front_end/lib/resolved_ast_generator.dart:36: Future<ResolvedAsts> resolvedAstFor( "resolvedAstFor" --> "resolvedAstsFor"

4 years, 2 months ago (2016-10-17 19:24:55 UTC) #24

Paul Berry

https://codereview.chromium.org/2417043003/diff/120001/pkg/front_end/lib/resolved_ast_generator.dart File pkg/front_end/lib/resolved_ast_generator.dart (right): https://codereview.chromium.org/2417043003/diff/120001/pkg/front_end/lib/resolved_ast_generator.dart#newcode36 pkg/front_end/lib/resolved_ast_generator.dart:36: Future<ResolvedAsts> resolvedAstFor( On 2016/10/17 19:24:55, Brian Wilkerson wrote: > ...

4 years, 2 months ago (2016-10-17 19:28:43 UTC) #25

Paul Berry

Description was changed from ========== Initial API for the Dart front_end package. ========== to ========== ...

4 years, 2 months ago (2016-10-17 19:36:11 UTC) #26

Paul Berry

4 years, 2 months ago (2016-10-17 19:36:13 UTC) #27

Message was sent while issue was closed.

Committed patchset #7 (id:120001) manually as
0753607deae0fae027da707d013ad2cdf1503414 (presubmit successful).

Issue 2417043003: Initial API for the Dart front_end package. (Closed)

Description

Patch Set 1 #

Patch Set 2 : Rework based on review comments. #

Patch Set 3 : Fixes #

Patch Set 4 : Address code review comments #

Patch Set 5 : Address further review comments. #

Patch Set 6 : Fix trivial error in resolved_ast_generator.dart #

Patch Set 7 : Rework with Brianwilkerson #

Messages