Initial API for the Dart front_end package.

pkg/front_end/lib/compiler_api.dart

Index: pkg/front_end/lib/compiler_api.dart
diff --git a/pkg/front_end/lib/compiler_api.dart b/pkg/front_end/lib/compiler_api.dart
new file mode 100644
index 0000000000000000000000000000000000000000..6b697768789c8e752e265ae59eac5fcdb0ef6bbe
--- /dev/null
+++ b/pkg/front_end/lib/compiler_api.dart
@@ -0,0 +1,92 @@
+// Copyright (c) 2016, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+/// Defines the front end API to be used by compiler back-ends.

[Siggi Cherem (dart-lang), 2016/10/13 23:22:15]

nit: front end => front-end ?
(that would match back-ends too :))

[Bob Nystrom, 2016/10/13 23:56:11]

/me puts on grammarian hat.

(I'm kidding, of course. I NEVER TAKE THAT HAT OFF.)

I believe most editors would suggest:

"Defines the front-end API to be used by compiler back ends."

You hyphenate when the compound phrase is used as an adjective or adverb (here
"front-end" modifies "API) but not when using it as a noun ("back ends").

[Paul Berry, 2016/10/14 00:09:35]

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).

[Siggi Cherem (dart-lang), 2016/10/14 16:00:49]

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. 

[Bob Nystrom, 2016/10/14 16:12:18]

"frontend" doesn't collapse well as a single word because it can be misread as
"fron tend". Webster's and OED both say "front end":

http://www.merriam-webster.com/dictionary/frontend
https://en.oxforddictionaries.com/definition/front_end

Chicago Manual of Style says it should be hyphenated when before a noun but not
after. See the "adjective + noun" section here:

http://www.chicagomanualofstyle.org/16/images/ch07_tab01.pdf

So:

"Defines the front-end API to be used by compiler back ends."

:)

[Paul Berry, 2016/10/14 21:32:34]

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

+library front_end.compiler_api;
+
+import 'compilation_error.dart';
+import 'package:analyzer/dart/ast/ast.dart' show CompilationUnit;
+
+/// Callback used by [CompilerApi] to report errors encountered during
+/// compilation.
+typedef void ErrorHandler(CompilationError);

[scheglov, 2016/10/13 23:58:17]

typedef void ErrorHandler(CompilationError error);

[Paul Berry, 2016/10/14 00:09:36]

Oops.  Thank you.

+
+/// Front end API for use by compiler back-ends.
+///
+/// Intended use: create a CompilerApi object, set any desired options using

[Siggi Cherem (dart-lang), 2016/10/13 23:22:15]

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, ...});

[Paul Berry, 2016/10/14 00:09:36]

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.

[Brian Wilkerson, 2016/10/14 14:54:45]

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?

[Siggi Cherem (dart-lang), 2016/10/14 16:00:48]

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.

[Paul Berry, 2016/10/14 21:32:34]

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.

+/// setters, and then call [sourcesToKernel] to obtain a Dart Kernel
+/// representation of input source files.
+///
+/// At the moment, this API only supports batch-mode compilation.  Its methods
+/// operate on whole source files, and do not cache intermediate results between

[Siggi Cherem (dart-lang), 2016/10/13 23:22:16]

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?

[Paul Berry, 2016/10/14 00:09:36]

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.

[Siggi Cherem (dart-lang), 2016/10/14 16:00:49]

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

+/// method calls.  Support for incremental updates will be added in the future
+/// (e.g. to allow for hot reload features).
+///
+/// All of the options have reasonable defaults, so a simple client may simply
+/// do: `var kernel = new CompilerApi().sourcesToKernel([Uri.parse(...)]);`
+abstract class CompilerApi {

[Siggi Cherem (dart-lang), 2016/10/13 23:22:16]

minor nit: what do you think about renaming this as [FrontEnd]?

[Brian Wilkerson, 2016/10/13 23:37:02]

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.

[Paul Berry, 2016/10/14 00:09:36]

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).

[Paul Berry, 2016/10/14 00:09:36]

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

[Siggi Cherem (dart-lang), 2016/10/14 16:00:49]

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).

[Paul Berry, 2016/10/14 21:32:34]

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

+  /// Creates an instance of [CompilerApi] with all options set to default
+  /// values.
+  factory CompilerApi() => throw new UnimplementedError();
+
+  /// Sets the path to the Dart SDK.
+  ///
+  /// If not set, the SDK will be searched for using [Platform.script] as a
+  /// starting point.
+  void set sdkPath(String path);
+
+  /// Sets a callback to which compilation errors should be delivered.
+  ///
+  /// If not set, the first error will be reported by throwing an exception of
+  /// type [CompilationError].
+  void set onError(ErrorHandler onError);
+
+  /// Sets the path to the ".packages" file.
+  ///
+  /// If not set, the ".packages" file will be found via the standard search
+  /// algorithm.
+  void set packages(String path);

[Brian Wilkerson, 2016/10/13 23:37:02]

Analyzer currently supports setting a 'packages' directory as well; we might
well need support for that in kernel.

[Paul Berry, 2016/10/14 00:09:36]

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.

[Brian Wilkerson, 2016/10/14 14:54:45]

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?

[Paul Berry, 2016/10/14 21:32:34]

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.

+
+  /// Sets the paths to the input summary files.
+  ///
+  /// If not set, no input summaries will be used.
+  void set inputSummaries(List<String> paths);

[Brian Wilkerson, 2016/10/13 23:37:02]

Does this include all summary files: sdk and non-sdk?

[scheglov, 2016/10/13 23:58:17]

Is using summaries for pub packages an implementation detail or not allowed?
 

[Paul Berry, 2016/10/14 00:09:35]

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

[Paul Berry, 2016/10/14 00:09:35]

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

[Brian Wilkerson, 2016/10/14 14:54:45]

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), 2016/10/14 16:00:49]

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.

+
+  /// Sets the URI override map.
+  ///
+  /// This is a map from Uri to file path which overrides the normal URI
+  /// resolution algorithm.  If not set, the normal URI resolution algorithm
+  /// will always be used.
+  void set uriOverride(Map<Uri, String> mapping);

[Siggi Cherem (dart-lang), 2016/10/13 23:22:15]

is this for dart:* libraries or for mappings like we had to do for summaries in
blaze?

[Brian Wilkerson, 2016/10/13 23:37:01]

What is this used for? (I don't remember seeing similar support in analyzer.)

Does it apply to any kind of URI (dart:, package:, http:)?

[Paul Berry, 2016/10/14 00:09:36]

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.

[Siggi Cherem (dart-lang), 2016/10/14 16:00:48]

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). 

[Paul Berry, 2016/10/14 21:32:34]

I would like to learn more about this from you.

+
+  /// Sets the platform bit mask, which determines which patch files should be
+  /// applied to the SDK.

[scheglov, 2016/10/13 23:58:17]

It is always a single platform, right?
The phrase "bit mask" sounds as it might be a combination of several bits.

[Paul Berry, 2016/10/14 00:09:36]

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.

[Siggi Cherem (dart-lang), 2016/10/14 16:00:49]

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
}

[Paul Berry, 2016/10/14 21:32:34]

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

[Siggi Cherem (dart-lang), 2016/10/17 16:27:29]

[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.

+  ///
+  /// The value should match the `PLATFORM` bit flags in
+  /// sdk/lib/_internal/sdk_library_metadata/lib/libraries.dart.  If not set, no
+  /// patch files will be applied.
+  void set platform(int value);

[Brian Wilkerson, 2016/10/13 23:37:02]

Note that there is another definition of "platform" being proposed, so this name
is (or soon will be) ambiguous.

[Paul Berry, 2016/10/14 00:09:36]

Acknowledged.

+
+  /// Limits the set of files which the front end is allowed to read.
+  ///
+  /// The front end will behave as though files not in this set do not exist.
+  /// If not set, the front end is allowed to read all files in the filesystem.
+  void set permittedFiles(Iterable<String> files);

[Siggi Cherem (dart-lang), 2016/10/13 23:22:15]

is this mainly for validation in blaze? (to check that we don't have read both
from sources + summaries for the same library?)

[Brian Wilkerson, 2016/10/13 23:37:02]

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.)

[Paul Berry, 2016/10/14 00:09:36]

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).

[Paul Berry, 2016/10/14 00:09:36]

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.

[Siggi Cherem (dart-lang), 2016/10/14 16:00:49]

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.

[Paul Berry, 2016/10/14 21:32:34]

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.

+
+  /// Sets the declared variables for use by configurable imports and constant
+  /// evaluation.
+  void set declaredVariables(Map<String, String> variables);
+
+  /// Processes the given [sources] and converts them to Dart Kernel format.
+  ///
+  /// TODO(paulberry): once Dart Kernel has been merged into the SDK, set the

[Siggi Cherem (dart-lang), 2016/10/13 23:22:16]

since it's already pulled into DEPS, you might be able to use
List<kernel.Library> here (or kernel.Program?)

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

[Paul Berry, 2016/10/14 00:09:36]

Oh, I didn't realize that.  I will make that change.
I don't know.  I'll add a TODO.

+  /// return type correctly.
+  dynamic sourcesToKernel(List<Uri> sources);
+
+  /// Processes the given [sources] and converts them to resolved ASTs.
+  ///
+  /// This is intended to be used by dev_compiler prior to its conversion to
+  /// Dart Kernel.

[Siggi Cherem (dart-lang), 2016/10/13 23:22:16]

+ and will be deperecated in the future?

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?)

[Paul Berry, 2016/10/14 00:09:36]

Yes that's my intent.
Hmm, good question.  I don't know what would be best.

[Siggi Cherem (dart-lang), 2016/10/14 16:00:48]

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

[Paul Berry, 2016/10/14 21:32:34]

Agreed.

+  List<CompilationUnit> sourcesToResolvedAsts(List<Uri> sources);
+}