Add support for SDK outline in IKG.

Add support for SDK outline in IKG.

R=ahe@google.com, paulberry@google.com, sigmund@google.com
BUG=

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

[Paul Berry, 2017-08-07 18:37:13]

lgtm assuming "solo_" is removed.

https://codereview.chromium.org/2999563002/diff/1/pkg/front_end/test/incremen...
File pkg/front_end/test/incremental_kernel_generator_test.dart (right):

https://codereview.chromium.org/2999563002/diff/1/pkg/front_end/test/incremen...
pkg/front_end/test/incremental_kernel_generator_test.dart:179:
solo_test_compile_useSdkOutline() async {
Remove "solo_"

[Siggi Cherem (dart-lang), 2017-08-07 19:12:52]

https://codereview.chromium.org/2999563002/diff/1/pkg/front_end/lib/increment...
File pkg/front_end/lib/incremental_kernel_generator.dart (right):

https://codereview.chromium.org/2999563002/diff/1/pkg/front_end/lib/increment...
pkg/front_end/lib/incremental_kernel_generator.dart:106: var sdkOutlineBytes =
await processedOptions.loadSdkSummaryBytes();
What do we want the default behavior to use a summary?

I'm guessing you want the default to be that we don't use a summary unless you
specified it explicitly, is that correct?

If so, this might not do quite what you want: if sdkSummary is not specified, we
will infer it based on the sdkRoot, so we might end up using a summary after
all.

I suggest that to address this we do something similar to what we did to have
different defaults in kernelForProgram and kernelForBuildUnit: 
 - add a flag to the ProcessedOptions constructor (we used `modularApi` before,
you might want to add `incremental`), we probably should switch to use
name-parameters, but I'm happy to address that in a separate change.
 - use the `incremental` option when inferring `ProcessedOptions.sdkSummary` 
 - add additional code in `validateOptions` to check for consistency of flags
and call it here. For example, we could add error messages if inputSummaries and
linkedSummaries are specified.
 - finally, add to the dartdoc in CompilerOptions.sdkSummary about the default,
and in CompilerOptions.inputSummaries and CompilerOptions.linkedDependencies
about what is not supported.

https://codereview.chromium.org/2999563002/diff/1/pkg/front_end/test/incremen...
File pkg/front_end/test/incremental_kernel_generator_test.dart (right):

https://codereview.chromium.org/2999563002/diff/1/pkg/front_end/test/incremen...
pkg/front_end/test/incremental_kernel_generator_test.dart:207: }
consider adding a test that we don't use the summary when we don't intend to.
Maybe by testing that you can modify the sdk itself (which I expect will show a
change in one scenario, but not in the other)?

[scheglov, 2017-08-07 20:47:51]

https://codereview.chromium.org/2999563002/diff/1/pkg/front_end/lib/increment...
File pkg/front_end/lib/incremental_kernel_generator.dart (right):

https://codereview.chromium.org/2999563002/diff/1/pkg/front_end/lib/increment...
pkg/front_end/lib/incremental_kernel_generator.dart:106: var sdkOutlineBytes =
await processedOptions.loadSdkSummaryBytes();
On 2017/08/07 19:12:52, Siggi Cherem (dart-lang) wrote:
> What do we want the default behavior to use a summary?
> 
> I'm guessing you want the default to be that we don't use a summary unless you
> specified it explicitly, is that correct?

Well, on this level it does not matter - IKG uses whatever values are returned
from ProcessedOptions.


> If so, this might not do quite what you want: if sdkSummary is not specified,
we
> will infer it based on the sdkRoot, so we might end up using a summary after
> all.

It seems strange to me - implement inference for sdkSummary, but then heroically
disable it :-)


> 
> I suggest that to address this we do something similar to what we did to have
> different defaults in kernelForProgram and kernelForBuildUnit: 
>  - add a flag to the ProcessedOptions constructor (we used `modularApi`
before,
> you might want to add `incremental`), we probably should switch to use
> name-parameters, but I'm happy to address that in a separate change.
>  - use the `incremental` option when inferring `ProcessedOptions.sdkSummary` 

I thought that the motivation for this change was to make it possible to use SDK
outline in IKG.
How come we now disable it for IKG?

Or is it just to disable _inference_?
Then maybe it should be controlled by a special "allowSdkOutlineInference" flag.
Or maybe make it an explicit call inferSdkOutline() method.


>  - add additional code in `validateOptions` to check for consistency of flags
> and call it here. For example, we could add error messages if inputSummaries
and
> linkedSummaries are specified.
>  - finally, add to the dartdoc in CompilerOptions.sdkSummary about the
default,
> and in CompilerOptions.inputSummaries and CompilerOptions.linkedDependencies
> about what is not supported.

This sounds good to me.

https://codereview.chromium.org/2999563002/diff/1/pkg/front_end/test/incremen...
File pkg/front_end/test/incremental_kernel_generator_test.dart (right):

https://codereview.chromium.org/2999563002/diff/1/pkg/front_end/test/incremen...
pkg/front_end/test/incremental_kernel_generator_test.dart:179:
solo_test_compile_useSdkOutline() async {
On 2017/08/07 18:37:13, Paul Berry wrote:
> Remove "solo_"

Done.

https://codereview.chromium.org/2999563002/diff/1/pkg/front_end/test/incremen...
pkg/front_end/test/incremental_kernel_generator_test.dart:207: }
On 2017/08/07 19:12:52, Siggi Cherem (dart-lang) wrote:
> consider adding a test that we don't use the summary when we don't intend to.
> Maybe by testing that you can modify the sdk itself (which I expect will show
a
> change in one scenario, but not in the other)?

Hm...
We could do this, but it seems a bit stretched.
When we don't intend to use it, we don't pass "sdkOutlineUri".

[Siggi Cherem (dart-lang), 2017-08-07 22:16:17]

https://codereview.chromium.org/2999563002/diff/1/pkg/front_end/lib/increment...
File pkg/front_end/lib/incremental_kernel_generator.dart (right):

https://codereview.chromium.org/2999563002/diff/1/pkg/front_end/lib/increment...
pkg/front_end/lib/incremental_kernel_generator.dart:106: var sdkOutlineBytes =
await processedOptions.loadSdkSummaryBytes();

> I thought that the motivation for this change was to make it possible to use
SDK
> outline in IKG.
> How come we now disable it for IKG?
> 
> Or is it just to disable _inference_?

Correct, this is about what do we want the default behavior to be if a user of
IKG doesn't provide any flags in CompilerOptions. If they explicitly provide the
URI to an sdk summary, we will use it, just like we do in all the other APIs.


> Then maybe it should be controlled by a special "allowSdkOutlineInference"
flag.
> Or maybe make it an explicit call inferSdkOutline() method.

We have something similar with the `compileSdk` option today. When compileSdk is
true: we don't infer the sdk summary, because your intent was to compile the
sdk, if compileSdk is false, we try to find a summary for you.

Note that `_ensureSdkDefaults()` method in ProcessedOptions might be similar to
what you were suggesting with `inferSdkOutline`. That's where we infer values
based on the options given to us. I prefer to keep it functional and not make it
procedural though: the value of sdkSummary shouldn't depend on whether you
called something in ProcessedOptions, but we should be able to compute it based
on the constructor arguments.


So the question is: what is the most common behavior we want for users of IKG?
If the most common behavior is to infer the location of the summary and use it,
then what you implemented here seems fine and we don't need to change the
defaults or add more public flags (only add errors for the other options that
IKG ignores). If the default should be to compileSdk from sources, then maybe
the fix should not be to change how we infer sdkSummary, but to make
`compileSdk` inferrable and infer true/false depending on the API.

[scheglov, 2017-08-07 23:34:12]

https://codereview.chromium.org/2999563002/diff/1/pkg/front_end/lib/increment...
File pkg/front_end/lib/incremental_kernel_generator.dart (right):

https://codereview.chromium.org/2999563002/diff/1/pkg/front_end/lib/increment...
pkg/front_end/lib/incremental_kernel_generator.dart:106: var sdkOutlineBytes =
await processedOptions.loadSdkSummaryBytes();
On 2017/08/07 22:16:17, Siggi Cherem (dart-lang) wrote:
> 
> > I thought that the motivation for this change was to make it possible to use
> SDK
> > outline in IKG.
> > How come we now disable it for IKG?
> > 
> > Or is it just to disable _inference_?
> 
> Correct, this is about what do we want the default behavior to be if a user of
> IKG doesn't provide any flags in CompilerOptions. If they explicitly provide
the
> URI to an sdk summary, we will use it, just like we do in all the other APIs.
> 
> 
> > Then maybe it should be controlled by a special "allowSdkOutlineInference"
> flag.
> > Or maybe make it an explicit call inferSdkOutline() method.
> 
> We have something similar with the `compileSdk` option today. When compileSdk
is
> true: we don't infer the sdk summary, because your intent was to compile the
> sdk, if compileSdk is false, we try to find a summary for you.
> 
> Note that `_ensureSdkDefaults()` method in ProcessedOptions might be similar
to
> what you were suggesting with `inferSdkOutline`. That's where we infer values
> based on the options given to us. I prefer to keep it functional and not make
it
> procedural though: the value of sdkSummary shouldn't depend on whether you
> called something in ProcessedOptions, but we should be able to compute it
based
> on the constructor arguments.
> 
> 
> So the question is: what is the most common behavior we want for users of IKG?
> If the most common behavior is to infer the location of the summary and use
it,
> then what you implemented here seems fine and we don't need to change the
> defaults or add more public flags (only add errors for the other options that
> IKG ignores). If the default should be to compileSdk from sources, then maybe
> the fix should not be to change how we infer sdkSummary, but to make
> `compileSdk` inferrable and infer true/false depending on the API.
> 
> 

Flutter tools is the only known client of IKG, and it wants us to use SDK
outline.
So, this is the most common behavior, and we should infer outline location.

[Siggi Cherem (dart-lang), 2017-08-07 23:37:14]

sgtm (and lgtm after removing the solo_ piece like Paul suggested too). I'm fine
adding error reporting in a separate CL if you prefer.

[scheglov, 2017-08-08 00:09:11]

Description was changed from

==========
Add support for SDK outline in IKG.

R=ahe@google.com, paulberry@google.com, sigmund@google.com
BUG=
==========

to

==========
Add support for SDK outline in IKG.

R=ahe@google.com, paulberry@google.com, sigmund@google.com
BUG=

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

[scheglov, 2017-08-08 00:09:13]

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