Introduces `ParserOptions`.

Introduces `ParserOptions`.

In preparation for support for generic methods (with an associated
command line option), this CL introduces a `ParserOptions` class and
uses that to pass the existing option `enableConditionalDirectives` to
all parsers. With that, the addition of an `enableGenericMethodSyntax`
option will be concise and well localized.

It is necessary to keep a `ParserOptions` object in `Parsing`, because
that's the only convenient channel for providing the options to the new
`Parser` and `ClassElementParser` created from the top-level function
`parse` in 'partial_elements.dart', and a from
`PartialClassElement.parseNode`, respectively.

The `ParserOptions` class is located in 'parser.dart'; from the current
import structure the most natural choice might be to put it in
'element_listener.dart', but considering the nature of that file it
seems less natural: What does `ParserOptions` have to do with element
listeners? So I put it in 'parser.dart', even though this causes a few
additional import statements.

R=johnniwinther@google.com, sigmund@google.com

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

[eernst, 2016-03-11 12:54:11]

eernst@google.com changed reviewers:
+ johnniwinther@google.com

[eernst, 2016-03-11 12:54:12]

`ParserOptions` related code from https://codereview.chromium.org/1723443003/
extracted and improved, ready for adding new options and providing them to all
parsers.

[Johnni Winther, 2016-03-11 13:43:39]

lgtm

https://codereview.chromium.org/1789553003/diff/1/pkg/compiler/lib/src/parser...
File pkg/compiler/lib/src/parser/parser.dart (right):

https://codereview.chromium.org/1789553003/diff/1/pkg/compiler/lib/src/parser...
pkg/compiler/lib/src/parser/parser.dart:81: const
ParserOptions({this.enableConditionalDirectives: false});
Nit: Add an empty line before the constructor.

[eernst, 2016-03-21 13:03:29]

eernst@google.com changed reviewers:
+ sigmund@google.com

[eernst, 2016-03-21 13:03:35]

Hi Siggy,

I'm adding you as a reviewer because this CL clashes with some work that you
have been doing since I wrote it (commit
4f3a7cb86e04bb1d4d06902924d9c7275a3b6527). I'm currently trying to adjust this
CL such that it fits well with the changes you made, and with the plans you have
in this area (Kevin M just told me something about that, hope I got it right).
So don't judge the situation only by looking at the patches in this CL up until
now (I'll upload some more patches later today, but there are several meetings
this afternoon as well, so there will be some delays).

In this CL, I added a `ParserOptions` class and used it to carry all parser
related options, such that they are available for various `..ParserTask`s and
for `Parsing`. Since I wrote that, you added the `options` object to `Compiler`.
These two objects overlap too much, conceptually at least, to just merge the
code and have both.

As far as I understood, you wanted to narrow the interfaces seen by different
parts of the compiler in order to improve on its modularity (so we would deliver
the `options` to parts of the compiler, rather than delivering the `compiler` to
them and letting them dig into it in arbitrary ways). From that point of view, a
`ParserOptions` object would make sense: It is an even more restricted view on
the context, suitable for a parser related object (e.g., a `DietParserTask` or a
`Parser`). But it certainly does not make sense to have two copies of `bool
enableConditionalDirectives`, with the obvious potential for inconsistencies and
confusion. (We both took that flag and moved it into an "options" object, so a
naive merge produces the duplicate representation).

One approach that would make sense is to compute the `ParserOptions` object on
demand, just like the `ScannerOptions` object which is returned by
`Parsing.getScannerOptionsFor(Element)`. We would then confine the duplication
to this well-defined setup: We do have multiple copies of
`enableConditionalDirectives`, but the "original" storage location is
`Compiler.options`, and every `ParserOptions` object is just a projection of the
compiler options down to this smaller set of options that are relevant to
parsing. Instances of `DietParserTask` et al. will receive the `ParserOptions`
and be happy with that narrow view on the configuration.

Another approach would be to partition `Compiler.options` into smaller objects,
such as `Compiler.parserOptions`, `Compiler.scannerOptions`, etc. However, I
suspect that certain options (say `enableGenericMethods`) would be relevant for
several parts of the compiler, which means that it would be difficult to
actually achieve a partitioning of `Compiler.options`, we would more likely end
up with a bunch of overlapping subsets. The overlaps would not cause any
problems for a set of projections: We can certainly produce `ParserOptions` and,
say, `TypeCheckerOptions` containing overlapping information (the consistency
problems only arise when we have multiple "original" copies of a given piece of
information, it is no problem to include the same thing in multiple
projections).

Based on this, I'd suggest that we keep all the options in `Compiler.options`,
and then compute more narrow views on demand. We would then deliver a
`ParserOptions` to each parser/parser-task and a `ScannerOptions` to other parts
of the compiler, and we may well design additional projections from
`Compiler.options` that we can deliver to yet other parts of the compiler. This
allows for tailoring narrow and consistent views on the configuration for all
parts of the compiler.

WDYT?

https://codereview.chromium.org/1789553003/diff/1/pkg/compiler/lib/src/parser...
File pkg/compiler/lib/src/parser/parser.dart (right):

https://codereview.chromium.org/1789553003/diff/1/pkg/compiler/lib/src/parser...
pkg/compiler/lib/src/parser/parser.dart:81: const
ParserOptions({this.enableConditionalDirectives: false});
On 2016/03/11 13:43:39, Johnni Winther wrote:
> Nit: Add an empty line before the constructor.

Done.

[Siggi Cherem (dart-lang), 2016-03-21 16:20:10]

Hi Erik

Thanks for bringing this up!

I am totally in favor of projecting the subset of options we need for each task.
As of how we do that, I'd like to simply project the options using an interface
(rather than coping the projected options).

I think using the interface will give us the best of both worlds:
 - each task only knows about the options it cares about and we get the
separation we want.
 - we can simplify how we parse and manipulate options and iterate faster until
we figure out the right partition of the options.

It's true that people could cheat and read properties that were not exposed, but
our tests would fail in that case (as long as we are good about typing the
interface, which I am not worried about). 

In the case of the parser options, this would look something like:

parser.dart*:

  abstract class ParserOptions {
    bool enableConditionalImports;
  }


compiler_new.dart:

  class CompilerOptions implements ParserOptions {
    ...
  }


* as you suggested, `parser.dart` seems like a good place for it. Ideally each
task can become self-contained, that includes declaring what options it expects
to use.

WDYT?

[eernst, 2016-03-22 10:40:24]

On 2016/03/21 16:20:10, Siggi Cherem (dart-lang) wrote:
> Hi Erik
> 
> Thanks for bringing this up!
> 
> I am totally in favor of projecting the subset of options we need for each
task.
> As of how we do that, I'd like to simply project the options using an
interface
> (rather than coping the projected options).
> 
> I think using the interface will give us the best of both worlds:
>  - each task only knows about the options it cares about and we get the
> separation we want.
>  - we can simplify how we parse and manipulate options and iterate faster
until
> we figure out the right partition of the options.
> 
> It's true that people could cheat and read properties that were not exposed,
but
> our tests would fail in that case (as long as we are good about typing the
> interface, which I am not worried about). 
> 
> In the case of the parser options, this would look something like:
> 
> parser.dart*:
> 
>   abstract class ParserOptions {
>     bool enableConditionalImports;
>   }
> 
> 
> compiler_new.dart:
> 
>   class CompilerOptions implements ParserOptions {
>     ...
>   }
> 
> 
> * as you suggested, `parser.dart` seems like a good place for it. Ideally each
> task can become self-contained, that includes declaring what options it
expects
> to use.
> 
> WDYT?

Interesting idea! We do indeed avoid copying data, but in return we have to live
with some extra imports (which translate into a certain loss of modularity):

  compiler_new.dart --> parser.dart  // To see `ParserOptions`.
  compiler_new.dart --> element_listener.dart  // To see `ScannerOptions`.
  ...  // Similar imports to see option classes for other tasks.

or we would need to move those `..Options` classes elsewhere (in which case we
would have to import those "elsewheres" into 'parser.dart',
'element_listener.dart' etc. Interestingly, we already have an import of
'diagnostic_listener.dart' in 'compiler_new.dart', in order to see
`DiagnosticOptions`. Here is a different approach, by the way:
`DiagnosticOptions` is included into `CompilerOptions` as a field, which is
of course yet another approach that we could use.

But there is another issue as well: `ScannerOptions` won't work as a supertype
of `CompilerOptions`, because it is being computed relative to an `Element`
(or at least its library), rather than being a simple subset of the fields of
`CompilerOptions`:

  ScannerOptions getScannerOptionsFor(Element element) {
    return new ScannerOptions(
      canUseNative: compiler.backend.canLibraryUseNative(element.library));
  }

Hence, `ScannerOptions` and its usages would need to be redesigned in order to
make it fit into this approach.

The only matches for class names ending in `Options` are `CompilerOptions`,
`ScannerOptions`, `DiagnosticOptions`, and `ParserOptions`, so the above should
cover the situation for now.

To summarize:
  - The models on the table are the following:
      1. Cluster options directly in `CompilerOptions` using objects, like 
         the field `diagnosticOptions`.
      2. Provide access to clusters of options via supertypes, as in
         `CompilerOptions implements ParserOptions, DiagnosticOptions, ..`.
      3. Compute objects representing clusters of options on demand, e.g.,
         `ParserOptions get parserOptions` in `Parsing` which will use its
         access to a `CompilerOptions` to compute a suitable `ParserOptions`.
  - We should probably treat all groups of options similarly, no matter which
    model we choose in the end.

I think (1) is a little bit fragile as a general principle, because it
breaks down when the complete configuration contains settings which are
relevant to more than one compiler task (e.g., an upcoming "enable
generic methods" flag is relevant to the parser as well as the type
checker). Model (2) allows for overlaps (it is not a problem if both
`ParserOptions` and a hypothetical `TypeCheckerOptions` define an abstract
`bool get enableGenericMethods`). However, it does not easily allow for any
other transformation than selecting a simple subset of the fields of
`CompilerOptions`, e.g., it would be difficult to use this approach to get
anything like the `getScannerOptionsFor` method I mentioned above. Finally,
model (3) allows for greater flexibility (`ScannerOptions` already fits into
this model, and it allows for arbitrary computation in order to compute, say,
a `ParserOptions` object based on the given `CompilerOptions`), and this
model also allows 'new_compiler.dart' to remain independent of all the
projections of `CompilerOptions` into task specific `XOptions`---they would
just need to be known by the producers of `xOptions` instances and the
relevant tasks.

One thing that comes to mind here is the general approach taken in the "Gang
of Four" book
(http://www.amazon.com/Design-Patterns-Elements-Reusable-Object-Oriented/dp/02...).
They tend to emphasize the dynamic flexibility over the statically declared
relations in a number of cases where the static approach does not improve on
correctness: Approach (2) and (3) do not differ with respect to type safety,
but (3) does pay a small runtime penalty in return for the added flexibility.
I suspect that we may be willing to pay that runtime price in the longer run,
because the added flexibility is non-trivial.

So I'll return the WDYT an extra time..  ;-)

[eernst, 2016-03-22 13:00:25]

Forgot to mention this:

A challenge in model (3) is that the code needed to create the
projection from a given `CompilerOptions` to a task specific instance
of `XOptions` for some task `X` will sometimes be non-trivial. E.g.,
`getScannerOptionsFor` is not a piece of code that one would immediately
come up with. This knowledge needs to be placed in a suitable location.
It is concerned with options and tasks, so that location could be the
task itself (e.g., 'parser.dart'). For the task `X`, we would then..:

  - locate `XOptions` in a core file for the task `X` (e.g., 'parser.dart');
  - write the code needed to project a `CompilerOptions` to an `XOptions`
    instance as a constructor in the class `XOptions`, such that the
    projection can be fully encapsulated in that class (and "in the task");
  - use that constructor (or those constructors) in `Parsing` and
    elsewhere, when the need arises to provide such an `XOptions` object.

This means that the "task files" (such as 'parser.dart') need to depend on
`CompilerOptions`, but having several "client" entities depend on one
centralized entity seems less detrimental to modularity than having one
centralized entity depend on several others.

I'm currently writing a patch which is built on these principles, will
upload it later today.

[eernst, 2016-03-22 14:11:59]

On 2016/03/22 13:00:25, eernst wrote:
> Forgot to mention this:
> 
> A challenge in model (3) is that the code needed to create the
> projection from a given `CompilerOptions` to a task specific instance
> of `XOptions` for some task `X` will sometimes be non-trivial.

Followup:

I found an expression used to create instances of `ScannerOptions` in 3 
locations ('compiler.dart', 'diet_parser_task.dart', and 'parser_task.dart'),
and it is not completely obvious why the last one has `false` for
`canUseNative`, whereas the other two invoke `canLibraryUseNative` to make
the choice. Presumably the unconditionally `false` variant is never used
with built-in libraries?

[Siggi Cherem (dart-lang), 2016-03-22 15:17:35]

Good questions Erik!

The case you found with canUseNative default to false happens to be unreachable
in dart2js. It's only used for dart2dart, which doesn't have any native
keywords. It's not obvious because parseCompilationUnit sounds like a very
general thing, but it seems to only be called from dart2dart, so I expect we'll
delete it once we get rid of the dart backend (very soon).

Here is what I think we should do:
a- let's do (2) as our general design (declare interfaces for the different
suboptions)
b- inline DiagnosticOptions into CompilerOptions
c- use the word "option" for those options that will be invariant throughout the
compilation. 

There are so many options for what to do with the word "option"  ;-)... the case
of ScannerOptions, for example, is one we should replace. ScannerOptions seems
more like a configuration on the node/element listeners. For this specific case,
an object is practically overkill. I propose here that we delete the
ScannerOptions object, replace it with a boolean flag (canUseNative) on the
ElementVisitor directly. The helper:
   getScannerOptions(element)

becomes

   isNativeAllowedOn(element)

I'm not as concerned about the extra imports, but I'd like to avoid a circular
import graph. My proposal:

d- move all options out of compiler_new into a separate file (src/options.dart)
e- move all the XOptions into that file too. Now options is a sink library, all
tasks import it to know what options they can use. The compiler imports it to
parse/configure the options.

[eernst, 2016-03-22 17:37:09]

On 2016/03/22 15:17:35, Siggi Cherem (dart-lang) wrote:
> Good questions Erik!

Thanks! ;-)

I just uploaded the patch which changes a number of things in order to
make the approach to options objects more uniform.

It changes `DiagnosticOptions`, so I sent Johnni a separate 'PTAL' by email.

> The case you found with canUseNative default to false happens to be
unreachable
> in dart2js. It's only used for dart2dart, which doesn't have any native
> keywords. It's not obvious because parseCompilationUnit sounds like a very
> general thing, but it seems to only be called from dart2dart, so I expect
we'll
> delete it once we get rid of the dart backend (very soon).

Aha, that wasn't obvious. But the semantics has been preserved in the new patch.

> Here is what I think we should do:
> a- let's do (2) as our general design (declare interfaces for the different
> suboptions)

I actually used (3) in the patch, because (2) wouldn't accommodate
`ScannerOptions`. But I agree with your argument below that it isn't actually
related to "options", it is (presumably?) a language property that some
libraries admit the keyword `native` and others don't (but the spec never
mentions such a keyword, which seems to confirm that the libraries where
`native` is allowed are the built-in ones).

So you are not worried about the remaining issues that I mentioned?:

  - we may wish to present options to subtasks in a way that differs from
    the format which is given on the command line (which will be reflected
    rather directly by `CompilerOptions);

  - the dependency structure where `CompilerOptions` depends on all the
    `XOptions` classes, and each `XOptions` class depends on the specific
    compiler subtask that it serves seems to make "everything depend on
    everything".

Maybe those dependencies don't matter after all? We don't have transitivity
with respect to visible declarations (so importers of 'compiler_new.dart'
will not see names from 'parser.dart' etc. unless they import it separately).

> b- inline DiagnosticOptions into CompilerOptions

Did that.

> c- use the word "option" for those options that will be invariant throughout
the
> compilation.

That makes sense, but I'm not quite sure where that criterion is satisfied.

> There are so many options for what to do with the word "option"  ;-)

Indeed! 8^)

> ... the case
> of ScannerOptions, for example, is one we should replace. ScannerOptions seems
> more like a configuration on the node/element listeners. For this specific
case,
> an object is practically overkill. I propose here that we delete the
> ScannerOptions object, replace it with a boolean flag (canUseNative) on the
> ElementVisitor directly. The helper:
>    getScannerOptions(element)
> 
> becomes
> 
>    isNativeAllowedOn(element)

I'll try that, it definitely sounds more consistent than trying to make it fit
as an `XOptions` class.

> I'm not as concerned about the extra imports, but I'd like to avoid a circular
> import graph. My proposal:
> 
> d- move all options out of compiler_new into a separate file
(src/options.dart)

That sounds good!

> e- move all the XOptions into that file too. Now options is a sink library,
all
> tasks import it to know what options they can use. The compiler imports it to
> parse/configure the options.

OK, I'll do those things tomorrow, including the switch from model (3) to (2).
I think there are no serious obstacles when `ScannerOptions` is gone.

[Johnni Winther, 2016-03-23 08:01:40]

LGTM

[eernst, 2016-03-23 14:15:25]

Refactored substantially to have `CompilerOptions implements DiagnosticOptions,
ParserOptions` in a new 'options.dart' library. `ScannerOptions` not yet
modified, will be in upcoming patch.

[eernst, 2016-04-01 16:18:46]

On 2016/03/23 14:15:25, eernst wrote:
> Refactored substantially to have `CompilerOptions implements
DiagnosticOptions,
> ParserOptions` in a new 'options.dart' library. `ScannerOptions` not yet
> modified, will be in upcoming patch.

Hi Siggy,

PTAL:

I discussed `ScannerOptions` with Johnni, and we agreed that the most useful
thing to do now would be to add a TODO for Johnni about changes relating to
that; so I did that. With respect to the redesign of libraries and class
inheritance, we are now using the model you proposed. Florian had some input
on the resulting library structure, but he'll return to that later, and he
proposed that we should land this now.

The latest patch brings this CL on par with the current sdk (it's a branch on
46beb25f43018b2b6ed036b7b0d68e600990ea7f, landed by Johnni 9:46am CET this
morning).

Tests are currently running, they'll finish sometime this evening. The
language and corelib tests have already finished, they are OK.

So, assuming that nothing else has been broken, are you satisfied with
the CL at this point?

[Siggi Cherem (dart-lang), 2016-04-01 16:34:01]

LGTM!

Thanks Erik, looks great!

Sorry for the delay, this got buried in my inbox and I didn't know I was
blocking you.

[eernst, 2016-04-04 12:44:42]

On 2016/04/01 16:34:01, Siggi Cherem (dart-lang) wrote:
> LGTM!
> 
> Thanks Erik, looks great!
> 
> Sorry for the delay, this got buried in my inbox and I didn't know I was
> blocking you.

Thanks, and no problem! ;)

[eernst, 2016-04-04 12:44:45]

On 2016/04/01 16:34:01, Siggi Cherem (dart-lang) wrote:
> LGTM!
> 
> Thanks Erik, looks great!
> 
> Sorry for the delay, this got buried in my inbox and I didn't know I was
> blocking you.

Thanks, and no problem! ;)

[eernst, 2016-04-04 12:45:30]

Description was changed from

==========
Introduces `ParserOptions`.

In preparation for support for generic methods (with an associated
command line option), this CL introduces a `ParserOptions` class and
uses that to pass the existing option `enableConditionalDirectives` to
all parsers. With that, the addition of an `enableGenericMethodSyntax`
option will be concise and well localized.

It is necessary to keep a `ParserOptions` object in `Parsing`, because
that's the only convenient channel for providing the options to the new
`Parser` and `ClassElementParser` created from the top-level function
`parse` in 'partial_elements.dart', and a from
`PartialClassElement.parseNode`, respectively.

The `ParserOptions` class is located in 'parser.dart'; from the current
import structure the most natural choice might be to put it in
'element_listener.dart', but considering the nature of that file it
seems less natural: What does `ParserOptions` have to do with element
listeners? So I put it in 'parser.dart', even though this causes a few
additional import statements.
==========

to

==========
Introduces `ParserOptions`.

In preparation for support for generic methods (with an associated
command line option), this CL introduces a `ParserOptions` class and
uses that to pass the existing option `enableConditionalDirectives` to
all parsers. With that, the addition of an `enableGenericMethodSyntax`
option will be concise and well localized.

It is necessary to keep a `ParserOptions` object in `Parsing`, because
that's the only convenient channel for providing the options to the new
`Parser` and `ClassElementParser` created from the top-level function
`parse` in 'partial_elements.dart', and a from
`PartialClassElement.parseNode`, respectively.

The `ParserOptions` class is located in 'parser.dart'; from the current
import structure the most natural choice might be to put it in
'element_listener.dart', but considering the nature of that file it
seems less natural: What does `ParserOptions` have to do with element
listeners? So I put it in 'parser.dart', even though this causes a few
additional import statements.

R=johnniwinther@google.com, sigmund@google.com

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

[eernst, 2016-04-04 12:45:31]

Committed patchset #4 (id:60001) manually as
d4c9a499d534f1a1c0147a1698a3b39ba0038d54 (presubmit successful).