support --options flag and other analysis options flags in DDC

support --options flag and other analysis options flags in DDC

This updates DDC options processing to support:
* --options /path/to/analysis/options/file.yaml
* --enable-strict-call-checks
* --supermixin
* --no-implicit-casts
* --no-implicit-dynamic

As well as general refactoring and cleanup of DDC options processing

R=vsm@google.com

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

[danrubel, 2016-12-21 18:10:04]

danrubel@google.com changed reviewers:
+ vsm@google.com

[danrubel, 2016-12-21 18:10:19]

danrubel@google.com changed reviewers:
+ jmesserly@google.com

[vsm, 2017-01-03 17:56:28]

https://codereview.chromium.org/2598593003/diff/20001/pkg/dev_compiler/lib/sr...
File pkg/dev_compiler/lib/src/analyzer/context.dart (right):

https://codereview.chromium.org/2598593003/diff/20001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/analyzer/context.dart:64: ..strongMode = true;
Maybe

..defaultOptions = new AnalysisOptionsImpl()..strongMode = true

and delete this last line?

https://codereview.chromium.org/2598593003/diff/20001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/analyzer/context.dart:76: ..strongMode = true;
Would be nice to avoid the cast to the Impl type here as well.  Perhaps this
could be forced in args or passed as a separate param to
createContextBuilderOptions?

https://codereview.chromium.org/2598593003/diff/20001/pkg/dev_compiler/lib/sr...
File pkg/dev_compiler/lib/src/compiler/compiler.dart (right):

https://codereview.chromium.org/2598593003/diff/20001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/compiler/compiler.dart:77: var analysisOptions =
contextBuilder.getAnalysisOptions(projRoot);
Hmm, looks like this is the library-root being passed in as the place to get the
analysis options file?

https://codereview.chromium.org/2598593003/diff/20001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/compiler/compiler.dart:79: ..strongMode = true
Why do we need to set this here and in the context file?

[danrubel, 2017-01-03 18:51:48]

Comments addressed. PTAL.

https://codereview.chromium.org/2598593003/diff/20001/pkg/dev_compiler/lib/sr...
File pkg/dev_compiler/lib/src/analyzer/context.dart (right):

https://codereview.chromium.org/2598593003/diff/20001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/analyzer/context.dart:64: ..strongMode = true;
On 2017/01/03 17:56:28, vsm wrote:
> Maybe
> 
> ..defaultOptions = new AnalysisOptionsImpl()..strongMode = true
> 
> and delete this last line?

Good point. Restructured to avoid cast.

https://codereview.chromium.org/2598593003/diff/20001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/analyzer/context.dart:76: ..strongMode = true;
On 2017/01/03 17:56:28, vsm wrote:
> Would be nice to avoid the cast to the Impl type here as well.  Perhaps this
> could be forced in args or passed as a separate param to
> createContextBuilderOptions?

Done.

https://codereview.chromium.org/2598593003/diff/20001/pkg/dev_compiler/lib/sr...
File pkg/dev_compiler/lib/src/compiler/compiler.dart (right):

https://codereview.chromium.org/2598593003/diff/20001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/compiler/compiler.dart:77: var analysisOptions =
contextBuilder.getAnalysisOptions(projRoot);
On 2017/01/03 17:56:28, vsm wrote:
> Hmm, looks like this is the library-root being passed in as the place to get
the
> analysis options file?

Correct. It will start looking in the projRoot directory (the library-root) and
up the parent tree until one is found.

https://codereview.chromium.org/2598593003/diff/20001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/compiler/compiler.dart:79: ..strongMode = true
On 2017/01/03 17:56:28, vsm wrote:
> Why do we need to set this here and in the context file?

Hmmm... good point. Removed.

And I moved trackCacheDependencies into the context file alongside strongMode.

[vsm, 2017-01-04 20:20:39]

https://codereview.chromium.org/2598593003/diff/80001/pkg/dev_compiler/lib/sr...
File pkg/dev_compiler/lib/src/compiler/compiler.dart (right):

https://codereview.chromium.org/2598593003/diff/80001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/compiler/compiler.dart:67: factory
ModuleCompiler(String projRoot, AnalyzerOptions options,
Perhaps rename projRoot to analysisOptionsPath or Root or similar?

I'm not really sure it makes sense to make both libraryRoot and this the same. 
There is a proposal to get rid of libraryRoot
(https://github.com/dart-lang/sdk/issues/27262).  How does this work for
analyzer-cli?  Is the directory passed in?

https://codereview.chromium.org/2598593003/diff/80001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/compiler/compiler.dart:69:
AnalysisEngine.instance.processRequiredPlugins();
Would it make sense to push much of the logic here into analyzer proper?  Seems
like it must duplicate analyzer-cli?  Everything up to the dart.isVm, etc.,
looks fairly generic boilerplate.

[danrubel, 2017-01-04 20:41:20]

See answers inline. PLMK.

https://codereview.chromium.org/2598593003/diff/80001/pkg/dev_compiler/lib/sr...
File pkg/dev_compiler/lib/src/compiler/compiler.dart (right):

https://codereview.chromium.org/2598593003/diff/80001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/compiler/compiler.dart:67: factory
ModuleCompiler(String projRoot, AnalyzerOptions options,
On 2017/01/04 20:20:39, vsm wrote:
> Perhaps rename projRoot to analysisOptionsPath or Root or similar?
> 
> I'm not really sure it makes sense to make both libraryRoot and this the same.

> There is a proposal to get rid of libraryRoot
> (https://github.com/dart-lang/sdk/issues/27262).  How does this work for
> analyzer-cli?  Is the directory passed in?

The analyzer-cli looks in the current working directory for the
.analyzer_options or analyzer_options.yaml file. Should we do the same for DDC ?

https://codereview.chromium.org/2598593003/diff/80001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/compiler/compiler.dart:69:
AnalysisEngine.instance.processRequiredPlugins();
On 2017/01/04 20:20:39, vsm wrote:
> Would it make sense to push much of the logic here into analyzer proper? 
Seems
> like it must duplicate analyzer-cli?  Everything up to the dart.isVm, etc.,
> looks fairly generic boilerplate.

Yes, that's my long term goal, but I'm working towards it incrementally so that
I don't break things in the process :)

[vsm, 2017-01-04 20:46:20]

https://codereview.chromium.org/2598593003/diff/80001/pkg/dev_compiler/lib/sr...
File pkg/dev_compiler/lib/src/compiler/compiler.dart (right):

https://codereview.chromium.org/2598593003/diff/80001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/compiler/compiler.dart:67: factory
ModuleCompiler(String projRoot, AnalyzerOptions options,
On 2017/01/04 20:41:20, danrubel wrote:
> On 2017/01/04 20:20:39, vsm wrote:
> > Perhaps rename projRoot to analysisOptionsPath or Root or similar?
> > 
> > I'm not really sure it makes sense to make both libraryRoot and this the
same.
> 
> > There is a proposal to get rid of libraryRoot
> > (https://github.com/dart-lang/sdk/issues/27262).  How does this work for
> > analyzer-cli?  Is the directory passed in?
> 
> The analyzer-cli looks in the current working directory for the
> .analyzer_options or analyzer_options.yaml file. Should we do the same for DDC
?

Yes, I think it would make sense to act the same as analyzer-cli.  And, that
way, this won't interfere with getting rid of libraryRoot.  :-)

https://codereview.chromium.org/2598593003/diff/80001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/compiler/compiler.dart:69:
AnalysisEngine.instance.processRequiredPlugins();
On 2017/01/04 20:41:19, danrubel wrote:
> On 2017/01/04 20:20:39, vsm wrote:
> > Would it make sense to push much of the logic here into analyzer proper? 
> Seems
> > like it must duplicate analyzer-cli?  Everything up to the dart.isVm, etc.,
> > looks fairly generic boilerplate.
> 
> Yes, that's my long term goal, but I'm working towards it incrementally so
that
> I don't break things in the process :)

SGTM :-)  Do you mind adding a comment / todo here to that effect?

[danrubel, 2017-01-04 21:37:42]

Comments addressed. PTAL.

https://codereview.chromium.org/2598593003/diff/80001/pkg/dev_compiler/lib/sr...
File pkg/dev_compiler/lib/src/compiler/compiler.dart (right):

https://codereview.chromium.org/2598593003/diff/80001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/compiler/compiler.dart:67: factory
ModuleCompiler(String projRoot, AnalyzerOptions options,
On 2017/01/04 20:46:20, vsm wrote:
> On 2017/01/04 20:41:20, danrubel wrote:
> > On 2017/01/04 20:20:39, vsm wrote:
> > > Perhaps rename projRoot to analysisOptionsPath or Root or similar?
> > > 
> > > I'm not really sure it makes sense to make both libraryRoot and this the
> same.
> > 
> > > There is a proposal to get rid of libraryRoot
> > > (https://github.com/dart-lang/sdk/issues/27262).  How does this work for
> > > analyzer-cli?  Is the directory passed in?
> > 
> > The analyzer-cli looks in the current working directory for the
> > .analyzer_options or analyzer_options.yaml file. Should we do the same for
DDC
> ?
> 
> Yes, I think it would make sense to act the same as analyzer-cli.  And, that
> way, this won't interfere with getting rid of libraryRoot.  :-)

Done.

https://codereview.chromium.org/2598593003/diff/80001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/compiler/compiler.dart:69:
AnalysisEngine.instance.processRequiredPlugins();
On 2017/01/04 20:46:20, vsm wrote:
> On 2017/01/04 20:41:19, danrubel wrote:
> > On 2017/01/04 20:20:39, vsm wrote:
> > > Would it make sense to push much of the logic here into analyzer proper? 
> > Seems
> > > like it must duplicate analyzer-cli?  Everything up to the dart.isVm,
etc.,
> > > looks fairly generic boilerplate.
> > 
> > Yes, that's my long term goal, but I'm working towards it incrementally so
> that
> > I don't break things in the process :)
> 
> SGTM :-)  Do you mind adding a comment / todo here to that effect?

Good point. Done.

[vsm, 2017-01-04 22:15:53]

lgtm

[danrubel, 2017-01-04 23:15:32]

Description was changed from

==========
support --options flag and other analysis options flags in DDC

This updates DDC options processing to support:
* --options /path/to/analysis/options/file.yaml
* --enable-strict-call-checks
* --supermixin
* --no-implicit-casts
* --no-implicit-dynamic

As well as general refactoring and cleanup of DDC options processing
==========

to

==========
support --options flag and other analysis options flags in DDC

This updates DDC options processing to support:
* --options /path/to/analysis/options/file.yaml
* --enable-strict-call-checks
* --supermixin
* --no-implicit-casts
* --no-implicit-dynamic

As well as general refactoring and cleanup of DDC options processing

R=vsm@google.com

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

[danrubel, 2017-01-04 23:15:33]

Committed patchset #7 (id:120001) manually as
55ef6e193c21d4f1ae976c08645609c0c969db1b (presubmit successful).