Fasta kernel builders.

Fasta kernel builders.

R=johnniwinther@google.com

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

[ahe, 2017-01-13 12:29:29]

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

[Johnni Winther, 2017-01-18 12:42:40]

lgtm

https://codereview.chromium.org/2631693002/diff/1/pkg/fasta/lib/src/kernel/ke...
File pkg/fasta/lib/src/kernel/kernel_builder.dart (right):

https://codereview.chromium.org/2631693002/diff/1/pkg/fasta/lib/src/kernel/ke...
pkg/fasta/lib/src/kernel/kernel_builder.dart:78: for (int i = arguments.length;
i < typeParameters.length; i++) {
Move the for loop into the then-branch above.

https://codereview.chromium.org/2631693002/diff/1/pkg/fasta/lib/src/kernel/ke...
pkg/fasta/lib/src/kernel/kernel_builder.dart:81: if (arguments.length >
typeParameters.length) {
Make this an else branch of the if statement above.

https://codereview.chromium.org/2631693002/diff/1/pkg/fasta/lib/src/kernel/ke...
File pkg/fasta/lib/src/kernel/kernel_class_builder.dart (right):

https://codereview.chromium.org/2631693002/diff/1/pkg/fasta/lib/src/kernel/ke...
pkg/fasta/lib/src/kernel/kernel_class_builder.dart:55:
computeDefaultTypeArguments(cls.typeParameters, arguments));
Why do we allow discrepancies and effective treat `Map<String>` as `Map<String,
dynamic>` ?

https://codereview.chromium.org/2631693002/diff/1/pkg/fasta/lib/src/kernel/ke...
pkg/fasta/lib/src/kernel/kernel_class_builder.dart:101: Member target =
targetBuilder.target;
Does this handle multiple steps (is .target the effective or immediate target):

class A {}
class B implements A { factory B = A; }
class C implements B { factory C = B; }

https://codereview.chromium.org/2631693002/diff/1/pkg/fasta/lib/src/kernel/ke...
File pkg/fasta/lib/src/kernel/kernel_enum_builder.dart (right):

https://codereview.chromium.org/2631693002/diff/1/pkg/fasta/lib/src/kernel/ke...
pkg/fasta/lib/src/kernel/kernel_enum_builder.dart:115: String className = name;
Move declaration to the for-loop where it is used.

https://codereview.chromium.org/2631693002/diff/1/pkg/fasta/lib/src/kernel/ke...
pkg/fasta/lib/src/kernel/kernel_enum_builder.dart:150: Class
build(KernelLibraryBuilder library) {
Maybe rename 'library' to 'libraryBuilder'; it's confusing to have
'library.library' in the code below.

https://codereview.chromium.org/2631693002/diff/1/pkg/fasta/lib/src/kernel/ke...
pkg/fasta/lib/src/kernel/kernel_enum_builder.dart:172: int index = 0;
Move declaration to the for-loop where it is used.

https://codereview.chromium.org/2631693002/diff/1/pkg/fasta/lib/src/kernel/ke...
File pkg/fasta/lib/src/kernel/kernel_function_type_alias_builder.dart (right):

https://codereview.chromium.org/2631693002/diff/1/pkg/fasta/lib/src/kernel/ke...
pkg/fasta/lib/src/kernel/kernel_function_type_alias_builder.dart:107: for (int i
= 0; i < arguments.length; i++) {
Wrap this in a `if (arguments != null) {...}` instead creating `arguments ??=
<DartType>[]` needlessly.

https://codereview.chromium.org/2631693002/diff/1/pkg/fasta/lib/src/kernel/ke...
File pkg/fasta/lib/src/kernel/kernel_interface_type_builder.dart (right):

https://codereview.chromium.org/2631693002/diff/1/pkg/fasta/lib/src/kernel/ke...
pkg/fasta/lib/src/kernel/kernel_interface_type_builder.dart:26: class
KernelInterfaceTypeBuilder extends InterfaceTypeBuilder<KernelTypeBuilder>
KernelNamedTypeBuilder?

https://codereview.chromium.org/2631693002/diff/1/pkg/fasta/lib/src/kernel/ke...
pkg/fasta/lib/src/kernel/kernel_interface_type_builder.dart:71: TypeBuilder
result = substitution[builder];
Add comment that this is for the case where [builder] is a TypeVariableBuilder.

https://codereview.chromium.org/2631693002/diff/1/pkg/fasta/lib/src/kernel/ke...
File pkg/fasta/lib/src/kernel/kernel_library_builder.dart (right):

https://codereview.chromium.org/2631693002/diff/1/pkg/fasta/lib/src/kernel/ke...
pkg/fasta/lib/src/kernel/kernel_library_builder.dart:107: endNestedScope();
Add a comment of where the nested scope begins, i.e. what is the invariant.

Also below.

https://codereview.chromium.org/2631693002/diff/1/pkg/fasta/lib/src/kernel/ke...
pkg/fasta/lib/src/kernel/kernel_library_builder.dart:199:
library.addMember(builder.build(library));
Shouldn't this be tagged as static?

https://codereview.chromium.org/2631693002/diff/1/pkg/fasta/lib/src/kernel/ke...
pkg/fasta/lib/src/kernel/kernel_library_builder.dart:201:
library.addMember(builder.build(library)..isStatic = true);
Shouldn't `isStatic = true` be given to the KernelProcedureBuilder at the
construction instead?

https://codereview.chromium.org/2631693002/diff/1/pkg/fasta/lib/src/kernel/ke...
pkg/fasta/lib/src/kernel/kernel_library_builder.dart:254: for (int i =
function.requiredParameterCount; i < function.positionalParameters.length; i++)
{
Long lines.

https://codereview.chromium.org/2631693002/diff/1/pkg/fasta/lib/src/kernel/ke...
File pkg/fasta/lib/src/kernel/kernel_procedure_builder.dart (right):

https://codereview.chromium.org/2631693002/diff/1/pkg/fasta/lib/src/kernel/ke...
pkg/fasta/lib/src/kernel/kernel_procedure_builder.dart:317:
assert(initializers[initializers.length - 1] == superInitializer);
Add documentation for what this case is.

https://codereview.chromium.org/2631693002/diff/1/pkg/fasta/lib/src/kernel/ke...
pkg/fasta/lib/src/kernel/kernel_procedure_builder.dart:324: for (int i = 0; i <
positional.length; i++) {
Add documentation for what you are doing here and in the loop below.

[ahe, 2017-01-18 15:21:50]

Thank you, Johnni!

https://codereview.chromium.org/2631693002/diff/1/pkg/fasta/lib/src/kernel/ke...
File pkg/fasta/lib/src/kernel/kernel_builder.dart (right):

https://codereview.chromium.org/2631693002/diff/1/pkg/fasta/lib/src/kernel/ke...
pkg/fasta/lib/src/kernel/kernel_builder.dart:78: for (int i = arguments.length;
i < typeParameters.length; i++) {
On 2017/01/18 12:42:39, Johnni Winther wrote:
> Move the for loop into the then-branch above.

Done.

https://codereview.chromium.org/2631693002/diff/1/pkg/fasta/lib/src/kernel/ke...
pkg/fasta/lib/src/kernel/kernel_builder.dart:81: if (arguments.length >
typeParameters.length) {
On 2017/01/18 12:42:39, Johnni Winther wrote:
> Make this an else branch of the if statement above.

Done.

https://codereview.chromium.org/2631693002/diff/1/pkg/fasta/lib/src/kernel/ke...
File pkg/fasta/lib/src/kernel/kernel_class_builder.dart (right):

https://codereview.chromium.org/2631693002/diff/1/pkg/fasta/lib/src/kernel/ke...
pkg/fasta/lib/src/kernel/kernel_class_builder.dart:55:
computeDefaultTypeArguments(cls.typeParameters, arguments));
On 2017/01/18 12:42:39, Johnni Winther wrote:
> Why do we allow discrepancies and effective treat `Map<String>` as
`Map<String,
> dynamic>` ?

We need to do something, and I'm not sure what the right thing to do is. I've
added a TODO stating this.

https://codereview.chromium.org/2631693002/diff/1/pkg/fasta/lib/src/kernel/ke...
pkg/fasta/lib/src/kernel/kernel_class_builder.dart:101: Member target =
targetBuilder.target;
On 2017/01/18 12:42:39, Johnni Winther wrote:
> Does this handle multiple steps (is .target the effective or immediate
target):
> 
> class A {}
> class B implements A { factory B = A; }
> class C implements B { factory C = B; }

It computes the immediate target. Added a comment about that.

It would require another phase to compute the effective target.

https://codereview.chromium.org/2631693002/diff/1/pkg/fasta/lib/src/kernel/ke...
File pkg/fasta/lib/src/kernel/kernel_enum_builder.dart (right):

https://codereview.chromium.org/2631693002/diff/1/pkg/fasta/lib/src/kernel/ke...
pkg/fasta/lib/src/kernel/kernel_enum_builder.dart:115: String className = name;
On 2017/01/18 12:42:39, Johnni Winther wrote:
> Move declaration to the for-loop where it is used.

Done.

https://codereview.chromium.org/2631693002/diff/1/pkg/fasta/lib/src/kernel/ke...
pkg/fasta/lib/src/kernel/kernel_enum_builder.dart:150: Class
build(KernelLibraryBuilder library) {
On 2017/01/18 12:42:39, Johnni Winther wrote:
> Maybe rename 'library' to 'libraryBuilder'; it's confusing to have
> 'library.library' in the code below.

Done.

https://codereview.chromium.org/2631693002/diff/1/pkg/fasta/lib/src/kernel/ke...
pkg/fasta/lib/src/kernel/kernel_enum_builder.dart:172: int index = 0;
On 2017/01/18 12:42:39, Johnni Winther wrote:
> Move declaration to the for-loop where it is used.

Done.

https://codereview.chromium.org/2631693002/diff/1/pkg/fasta/lib/src/kernel/ke...
File pkg/fasta/lib/src/kernel/kernel_function_type_alias_builder.dart (right):

https://codereview.chromium.org/2631693002/diff/1/pkg/fasta/lib/src/kernel/ke...
pkg/fasta/lib/src/kernel/kernel_function_type_alias_builder.dart:107: for (int i
= 0; i < arguments.length; i++) {
On 2017/01/18 12:42:40, Johnni Winther wrote:
> Wrap this in a `if (arguments != null) {...}` instead creating `arguments ??=
> <DartType>[]` needlessly.

Done.

https://codereview.chromium.org/2631693002/diff/1/pkg/fasta/lib/src/kernel/ke...
File pkg/fasta/lib/src/kernel/kernel_interface_type_builder.dart (right):

https://codereview.chromium.org/2631693002/diff/1/pkg/fasta/lib/src/kernel/ke...
pkg/fasta/lib/src/kernel/kernel_interface_type_builder.dart:26: class
KernelInterfaceTypeBuilder extends InterfaceTypeBuilder<KernelTypeBuilder>
On 2017/01/18 12:42:40, Johnni Winther wrote:
> KernelNamedTypeBuilder?

Yes. In a follow-up.

https://codereview.chromium.org/2631693002/diff/1/pkg/fasta/lib/src/kernel/ke...
pkg/fasta/lib/src/kernel/kernel_interface_type_builder.dart:71: TypeBuilder
result = substitution[builder];
On 2017/01/18 12:42:40, Johnni Winther wrote:
> Add comment that this is for the case where [builder] is a
TypeVariableBuilder.

I added an assertion instead. WDYT?

https://codereview.chromium.org/2631693002/diff/1/pkg/fasta/lib/src/kernel/ke...
File pkg/fasta/lib/src/kernel/kernel_library_builder.dart (right):

https://codereview.chromium.org/2631693002/diff/1/pkg/fasta/lib/src/kernel/ke...
pkg/fasta/lib/src/kernel/kernel_library_builder.dart:107: endNestedScope();
On 2017/01/18 12:42:40, Johnni Winther wrote:
> Add a comment of where the nested scope begins, i.e. what is the invariant.
> 
> Also below.

Done.

https://codereview.chromium.org/2631693002/diff/1/pkg/fasta/lib/src/kernel/ke...
pkg/fasta/lib/src/kernel/kernel_library_builder.dart:199:
library.addMember(builder.build(library));
On 2017/01/18 12:42:40, Johnni Winther wrote:
> Shouldn't this be tagged as static?

Done.

https://codereview.chromium.org/2631693002/diff/1/pkg/fasta/lib/src/kernel/ke...
pkg/fasta/lib/src/kernel/kernel_library_builder.dart:201:
library.addMember(builder.build(library)..isStatic = true);
On 2017/01/18 12:42:40, Johnni Winther wrote:
> Shouldn't `isStatic = true` be given to the KernelProcedureBuilder at the
> construction instead?

Not sure. Perhaps `isTopLevel = true`. Kernel marks all non-instance members as
static. I'm not sure I want to copy that (although it feels simpler).

https://codereview.chromium.org/2631693002/diff/1/pkg/fasta/lib/src/kernel/ke...
pkg/fasta/lib/src/kernel/kernel_library_builder.dart:254: for (int i =
function.requiredParameterCount; i < function.positionalParameters.length; i++)
{
On 2017/01/18 12:42:40, Johnni Winther wrote:
> Long lines.

Done.

https://codereview.chromium.org/2631693002/diff/1/pkg/fasta/lib/src/kernel/ke...
File pkg/fasta/lib/src/kernel/kernel_procedure_builder.dart (right):

https://codereview.chromium.org/2631693002/diff/1/pkg/fasta/lib/src/kernel/ke...
pkg/fasta/lib/src/kernel/kernel_procedure_builder.dart:317:
assert(initializers[initializers.length - 1] == superInitializer);
On 2017/01/18 12:42:40, Johnni Winther wrote:
> Add documentation for what this case is.

Done.

Writing the documentation made me realize a mistake, so the code was tweaked.

https://codereview.chromium.org/2631693002/diff/1/pkg/fasta/lib/src/kernel/ke...
pkg/fasta/lib/src/kernel/kernel_procedure_builder.dart:324: for (int i = 0; i <
positional.length; i++) {
On 2017/01/18 12:42:40, Johnni Winther wrote:
> Add documentation for what you are doing here and in the loop below.

Done.

[ahe, 2017-01-18 15:22:14]

Description was changed from

==========
Fasta kernel builders.
==========

to

==========
Fasta kernel builders.

R=johnniwinther@google.com

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

[ahe, 2017-01-18 15:22:16]

Committed patchset #2 (id:20001) manually as
3e75862e2e24932b3862396ff65b374f95e16e36 (presubmit successful).