Reduce strong mode errors and warnings

Reduce strong mode errors and warnings

R=ahe@google.com

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

[Siggi Cherem (dart-lang), 2017-02-02 04:26:58]

https://codereview.chromium.org/2675603002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/builder/library_builder.dart (right):

https://codereview.chromium.org/2675603002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/builder/library_builder.dart:60: Builder
buildAmbiguousBuilder(
one of the subclasses returns a MixinAccessor, which is not an
InvalidTypeBuilder

https://codereview.chromium.org/2675603002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/builder/member_builder.dart (right):

https://codereview.chromium.org/2675603002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/builder/member_builder.dart:19: String get name;
I wasn't sure about adding it here, but there wasn't any common superclass of
Field and Procedure that could have this (there is one place where you read
member.name to provide an error message)

https://codereview.chromium.org/2675603002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/kernel/body_builder.dart (right):

https://codereview.chromium.org/2675603002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:227: List statements =
popList(count) ?? <Statement>[];
this is a temporary way of hiding some errors, but not the ideal fix. I think
here (as well as in how we process optional arguments) we might need a different
way to represent this kind of union type.

Note that this change (and the related changes below) produce warnings in strong
mode, but at least not an error.

https://codereview.chromium.org/2675603002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:993:
List<VariableDeclaration> vars = variableOrExpression;
This has an implicit downcast warning, without this fix, the argument couldn't
be assigned to the `addAll` below.

An alternative here would be to change the is check to `is
List<VariableDeclaration>` - that would work if the list has the reified type,
which I think it does.

https://codereview.chromium.org/2675603002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:1194: } else if (name is
KernelTypeBuilder) {
.build is not defined in TypeBuilder - I was expecting the error to be visible
also without strong mode, but it wasn't

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

https://codereview.chromium.org/2675603002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/kernel_enum_builder.dart:139:
members.forEach((String name, Builder builder) {
it would be nice to be able to use `covariant` in this context too.

https://codereview.chromium.org/2675603002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/parser/parser.dart (right):

https://codereview.chromium.org/2675603002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/parser/parser.dart:2458: BeginGroupToken begin =
token;
strong mode infers `var` to be the type of the rhs, which is Token.

https://codereview.chromium.org/2675603002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/source/outline_builder.dart (right):

https://codereview.chromium.org/2675603002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/source/outline_builder.dart:364:
List<FormalParameterBuilder> l = last;
I don't want to submit this, this was just a test to see if I could get rid of
the errors. I think it's more viable to do something like what I propose above
so we get rid of this implicit union type here.

https://codereview.chromium.org/2675603002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/source/stack_listener.dart (right):

https://codereview.chromium.org/2675603002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/source/stack_listener.dart:89: List<T>
popList<T>(int n) {
these type-args get rid of many warnings where these APIs are used. We still
have a warning inside peek and pop, we could get rid of it by adding a cast `as
T`

[Siggi Cherem (dart-lang), 2017-02-02 04:29:12]

Patchset #2 (id:20001) has been deleted

[ahe, 2017-02-02 07:28:29]

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

[ahe, 2017-02-02 07:28:30]

https://codereview.chromium.org/2675603002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/analyzer/mock_element.dart (right):

https://codereview.chromium.org/2675603002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/analyzer/mock_element.dart:68: T
accept<T>(ElementVisitor<T> visitor) => internalError("not supported.");
The types added in this file should be inferred from supertypes.

https://codereview.chromium.org/2675603002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/builder/type_declaration_builder.dart (right):

https://codereview.chromium.org/2675603002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/builder/type_declaration_builder.dart:26: //
TODO(sigmund): remove @virtual, should not be needed, see #28601
I'm patient. I can wait until issue 28601 is fixed.

I don't want to import random packages for no other reason than working around
bugs in the analyzer.

https://codereview.chromium.org/2675603002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/kernel/body_builder.dart (right):

https://codereview.chromium.org/2675603002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:227: List statements =
popList(count) ?? <Statement>[];
On 2017/02/02 04:26:58, Siggi Cherem (dart-lang) wrote:
> this is a temporary way of hiding some errors, but not the ideal fix. I think
> here (as well as in how we process optional arguments) we might need a
different
> way to represent this kind of union type.
> 
> Note that this change (and the related changes below) produce warnings in
strong
> mode, but at least not an error.

I'm strongly opposed to removing types to appease a buggy type checker.

https://codereview.chromium.org/2675603002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:993:
List<VariableDeclaration> vars = variableOrExpression;
On 2017/02/02 04:26:58, Siggi Cherem (dart-lang) wrote:
> This has an implicit downcast warning, without this fix, the argument couldn't
> be assigned to the `addAll` below.
> 
> An alternative here would be to change the is check to `is
> List<VariableDeclaration>` - that would work if the list has the reified type,
> which I think it does.

Implicit downcast warning?!?

https://codereview.chromium.org/2675603002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:1194: } else if (name is
KernelTypeBuilder) {
On 2017/02/02 04:26:58, Siggi Cherem (dart-lang) wrote:
> .build is not defined in TypeBuilder - I was expecting the error to be visible
> also without strong mode, but it wasn't

This fix isn't correct. It potentially causes a TypeBuilder to passed to
toKernelType.

https://codereview.chromium.org/2675603002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/kernel/builder_accessors.dart (right):

https://codereview.chromium.org/2675603002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/builder_accessors.dart:103: doInvocation(int
charOffset, Arguments arguments);
Please don't remove explicit types.

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

https://codereview.chromium.org/2675603002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/kernel_enum_builder.dart:139:
members.forEach((String name, Builder builder) {
On 2017/02/02 04:26:58, Siggi Cherem (dart-lang) wrote:
> it would be nice to be able to use `covariant` in this context too.

Please don't change explicit types.

https://codereview.chromium.org/2675603002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/parser/parser.dart (right):

https://codereview.chromium.org/2675603002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/parser/parser.dart:2458: BeginGroupToken begin =
token;
On 2017/02/02 04:26:58, Siggi Cherem (dart-lang) wrote:
> strong mode infers `var` to be the type of the rhs, which is Token.

The type should be dynamic. This cast isn't correct. Take a look at the comment
above.

https://codereview.chromium.org/2675603002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/source/stack_listener.dart (right):

https://codereview.chromium.org/2675603002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/source/stack_listener.dart:75: T peek<T>() {
This is precisely the kind of changes that I'm very concerned about.

Generic methods will add an overhead at runtime when strong mode is fully
implemented. These methods are extremely hot and I'm doubtful that generic
methods will have the right performance characteristics.

[kasperl, 2017-02-02 07:51:30]

kasperl@google.com changed reviewers:
+ kasperl@google.com

[kasperl, 2017-02-02 07:51:31]

Couldn't help but follow along here! Very interesting and a good way for us to
learn about migrating to strong mode.

https://codereview.chromium.org/2675603002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/kernel/body_builder.dart (right):

https://codereview.chromium.org/2675603002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:993:
List<VariableDeclaration> vars = variableOrExpression;
On 2017/02/02 07:28:30, ahe wrote:
> On 2017/02/02 04:26:58, Siggi Cherem (dart-lang) wrote:
> > This has an implicit downcast warning, without this fix, the argument
couldn't
> > be assigned to the `addAll` below.
> > 
> > An alternative here would be to change the is check to `is
> > List<VariableDeclaration>` - that would work if the list has the reified
type,
> > which I think it does.
> 
> Implicit downcast warning?!?

Is this the warning that will go away when we implement the correct runtime
checks in Dart 2.0? See https://github.com/dart-lang/sdk/issues/28588.

https://codereview.chromium.org/2675603002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/parser/parser.dart (right):

https://codereview.chromium.org/2675603002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/parser/parser.dart:2458: BeginGroupToken begin =
token;
On 2017/02/02 07:28:30, ahe wrote:
> On 2017/02/02 04:26:58, Siggi Cherem (dart-lang) wrote:
> > strong mode infers `var` to be the type of the rhs, which is Token.
> 
> The type should be dynamic. This cast isn't correct. Take a look at the
comment
> above.

var first = token;
token = expect('(', token);
BeginGroupToken begin = first;

seems cleaner to me.

[Siggi Cherem (dart-lang), 2017-02-02 16:30:35]

https://codereview.chromium.org/2675603002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/analyzer/mock_element.dart (right):

https://codereview.chromium.org/2675603002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/analyzer/mock_element.dart:68: T
accept<T>(ElementVisitor<T> visitor) => internalError("not supported.");
On 2017/02/02 07:28:30, ahe wrote:
> The types added in this file should be inferred from supertypes.

Agree - I was surprised that this wasn't working. I'll follow up to see why.

https://codereview.chromium.org/2675603002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/builder/type_declaration_builder.dart (right):

https://codereview.chromium.org/2675603002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/builder/type_declaration_builder.dart:26: //
TODO(sigmund): remove @virtual, should not be needed, see #28601
On 2017/02/02 07:28:30, ahe wrote:
> I'm patient. I can wait until issue 28601 is fixed.
> 
> I don't want to import random packages for no other reason than working around
> bugs in the analyzer.

I understand, note that it's a 1-file package with just a few top-level
constants that are used for annotations. Nothing else:
https://github.com/dart-lang/sdk/blob/master/pkg/meta/lib/meta.dart

It was a temporary way to test out annotations for the type checker without
having to add syntax to the language until it gets finalized.

https://codereview.chromium.org/2675603002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/kernel/body_builder.dart (right):

https://codereview.chromium.org/2675603002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:227: List statements =
popList(count) ?? <Statement>[];
On 2017/02/02 07:28:30, ahe wrote:
> On 2017/02/02 04:26:58, Siggi Cherem (dart-lang) wrote:
> > this is a temporary way of hiding some errors, but not the ideal fix. I
think
> > here (as well as in how we process optional arguments) we might need a
> different
> > way to represent this kind of union type.
> > 
> > Note that this change (and the related changes below) produce warnings in
> strong
> > mode, but at least not an error.
> 
> I'm strongly opposed to removing types to appease a buggy type checker.

in this case I don't believe it's a bug in the checker: it seems that the type
is actually not accurate in this case. The `statements` variable is not a
List<Statement>, but a List< {Statement | List<Statement> } >, correct?

https://codereview.chromium.org/2675603002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:1194: } else if (name is
KernelTypeBuilder) {
On 2017/02/02 07:28:30, ahe wrote:
> On 2017/02/02 04:26:58, Siggi Cherem (dart-lang) wrote:
> > .build is not defined in TypeBuilder - I was expecting the error to be
visible
> > also without strong mode, but it wasn't
> 
> This fix isn't correct. It potentially causes a TypeBuilder to passed to
> toKernelType.

Shold `TypeBuilder` declare a `build()` method instead?

https://codereview.chromium.org/2675603002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/kernel/builder_accessors.dart (right):

https://codereview.chromium.org/2675603002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/builder_accessors.dart:103: doInvocation(int
charOffset, Arguments arguments);
On 2017/02/02 07:28:30, ahe wrote:
> Please don't remove explicit types.

There is a subclass that returns a non-expression here

[Siggi Cherem (dart-lang), 2017-02-06 22:26:25]

Patchset #2 (id:40001) has been deleted

[Siggi Cherem (dart-lang), 2017-02-06 22:26:32]

Patchset #2 (id:60001) has been deleted

[Siggi Cherem (dart-lang), 2017-02-06 22:26:44]

Patchset #2 (id:80001) has been deleted

[Siggi Cherem (dart-lang), 2017-02-06 22:29:49]

Patchset #2 (id:100001) has been deleted

[Siggi Cherem (dart-lang), 2017-02-06 22:34:37]

Patchset #2 (id:120001) has been deleted

[Siggi Cherem (dart-lang), 2017-02-06 23:35:51]

PTAL

I believe I addressed all the comments from here and that we discussed offline.
There are no added casts and no extra type-arguments on methods. A couple
changes feel like workarounds to make the type-system  happy, but they should
have no runtime effect. If for any reason you prefer to leave some of those out,
I'm happy to do so too.

Most of the code has a couple kind of warnings that are disabled in
.analysis_options. To run with these options on the command line, do:

sdk/bin/dartanalyzer --options pkg/front_end/ pkg/front_end/lib/

With those options, this CL brings us to 0 warnings and errors (note there is a
bug fixed in bleeding-edge that gets rid of a couple errors). We should take
that with a Kg of salt: if we were to insert all the reified checks I expect
lots of failures and the fixes would require more work. As we discussed, we can
look at those as a separate task (possibly by either me or someone on the
language team to assess the impact of this language change).

https://codereview.chromium.org/2675603002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/analyzer/mock_element.dart (right):

https://codereview.chromium.org/2675603002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/analyzer/mock_element.dart:68: T
accept<T>(ElementVisitor<T> visitor) => internalError("not supported.");
On 2017/02/02 16:30:35, Siggi Cherem (dart-lang) wrote:
> On 2017/02/02 07:28:30, ahe wrote:
> > The types added in this file should be inferred from supertypes.
> 
> Agree - I was surprised that this wasn't working. I'll follow up to see why.

Fixed. There were two issues:
 - some methods were declared in this class but not in `Element`, so there was
nothing to inherit in those cases, I moved those to the subclasses where they
are first needed and added an extra interface impementation to MockElement as
well.
 - arity of type-arguments had to match too. In this case I had to add <T>, but
I can still omit the types on the arguments and return value.

https://codereview.chromium.org/2675603002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/kernel/body_builder.dart (right):

https://codereview.chromium.org/2675603002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:993:
List<VariableDeclaration> vars = variableOrExpression;
On 2017/02/02 07:51:30, kasperl wrote:
> On 2017/02/02 07:28:30, ahe wrote:
> > On 2017/02/02 04:26:58, Siggi Cherem (dart-lang) wrote:
> > > This has an implicit downcast warning, without this fix, the argument
> couldn't
> > > be assigned to the `addAll` below.
> > > 
> > > An alternative here would be to change the is check to `is
> > > List<VariableDeclaration>` - that would work if the list has the reified
> type,
> > > which I think it does.
> > 
> > Implicit downcast warning?!?
> 
> Is this the warning that will go away when we implement the correct runtime
> checks in Dart 2.0? See https://github.com/dart-lang/sdk/issues/28588.

It's quite related, but there is an extra nuance here because the argument is
List and the parameter is Iterable. I filed this bug to the language team:
https://github.com/dart-lang/sdk/issues/28651

For now, I made a small change here to introduce an intermediate assignment to
Iterable to turn the error into a warning that we ignore and a TODO to remove
it.

https://codereview.chromium.org/2675603002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:1194: } else if (name is
KernelTypeBuilder) {
On 2017/02/02 16:30:35, Siggi Cherem (dart-lang) wrote:
> On 2017/02/02 07:28:30, ahe wrote:
> > On 2017/02/02 04:26:58, Siggi Cherem (dart-lang) wrote:
> > > .build is not defined in TypeBuilder - I was expecting the error to be
> visible
> > > also without strong mode, but it wasn't
> > 
> > This fix isn't correct. It potentially causes a TypeBuilder to passed to
> > toKernelType.
> 
> Shold `TypeBuilder` declare a `build()` method instead?

Done - I added `build()` to TypeBuilder as discussed. I didn't have a return
type for it though, I left it dynamic but let me know if there is something
better for it.

https://codereview.chromium.org/2675603002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/kernel/builder_accessors.dart (right):

https://codereview.chromium.org/2675603002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/builder_accessors.dart:103: doInvocation(int
charOffset, Arguments arguments);
On 2017/02/02 16:30:35, Siggi Cherem (dart-lang) wrote:
> On 2017/02/02 07:28:30, ahe wrote:
> > Please don't remove explicit types.
> 
> There is a subclass that returns a non-expression here

To be specific `ThisAccessor` might return an Initializer. Let me know if you'd
like me to add a dartdoc about it here.

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

https://codereview.chromium.org/2675603002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/kernel_enum_builder.dart:139:
members.forEach((String name, Builder builder) {
On 2017/02/02 07:28:30, ahe wrote:
> On 2017/02/02 04:26:58, Siggi Cherem (dart-lang) wrote:
> > it would be nice to be able to use `covariant` in this context too.
> 
> Please don't change explicit types.

OK - I switched it to dynamic (I believe this is what you suggested, but please
correct me if this is wrong)

https://codereview.chromium.org/2675603002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/parser/parser.dart (right):

https://codereview.chromium.org/2675603002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/parser/parser.dart:2458: BeginGroupToken begin =
token;
On 2017/02/02 07:51:30, kasperl wrote:
> On 2017/02/02 07:28:30, ahe wrote:
> > On 2017/02/02 04:26:58, Siggi Cherem (dart-lang) wrote:
> > > strong mode infers `var` to be the type of the rhs, which is Token.
> > 
> > The type should be dynamic. This cast isn't correct. Take a look at the
> comment
> > above.
> 
> var first = token;
> token = expect('(', token);
> BeginGroupToken begin = first;
> 
> seems cleaner to me. 

Sorry - I totally missed the comment. Thanks for clarifying. I went ahead with
Kasper's suggestion here.

[ahe, 2017-02-07 11:03:08]

This seems fine, but there's a few cases that still concern.

In particular forEach and changes to types in is-checks.

https://codereview.chromium.org/2675603002/diff/140001/pkg/front_end/lib/src/...
File pkg/front_end/lib/src/fasta/analyzer/mock_type.dart (right):

https://codereview.chromium.org/2675603002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/analyzer/mock_type.dart:40: internalError("not
supported.");
Would you mind using `{ return expr; }` when `=> expr;` doesn't fit on one line?

https://codereview.chromium.org/2675603002/diff/140001/pkg/front_end/lib/src/...
File pkg/front_end/lib/src/fasta/builder/builder.dart (right):

https://codereview.chromium.org/2675603002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/builder/builder.dart:92: int resolveTypes(covariant
Builder parent) => 0;
Do you have to add "covariant" here?

https://codereview.chromium.org/2675603002/diff/140001/pkg/front_end/lib/src/...
File pkg/front_end/lib/src/fasta/kernel/body_builder.dart (right):

https://codereview.chromium.org/2675603002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:244: // List<Statement>.
How about:

List<dynamic /* Statement | List<Statement> */> statements = popList(count) ??
<Statement>[];

And remove the todo.

https://codereview.chromium.org/2675603002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:245: List copy;
This should be List<Statement>.

https://codereview.chromium.org/2675603002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:1285: var builder =
formalParameterScope.lookup(name.name);
dynamic.

https://codereview.chromium.org/2675603002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:1291:
KernelFormalParameterBuilder formalBuilder = builder;
Remove this.

https://codereview.chromium.org/2675603002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:1641: if (type is
KernelClassBuilder) {
Change this back.

https://codereview.chromium.org/2675603002/diff/140001/pkg/front_end/lib/src/...
File pkg/front_end/lib/src/fasta/kernel/builder_accessors.dart (right):

https://codereview.chromium.org/2675603002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/kernel/builder_accessors.dart:100: doInvocation(int
charOffset, Arguments arguments);
TreeNode

https://codereview.chromium.org/2675603002/diff/140001/pkg/front_end/lib/src/...
File pkg/front_end/lib/src/fasta/kernel/kernel_enum_builder.dart (right):

https://codereview.chromium.org/2675603002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/kernel/kernel_enum_builder.dart:139:
members.forEach((String name, dynamic builder) {
I think I prefer this:

members.forEach((String name, dynamic b) {
  MemberBuilder builder = b;
...
}

Two or three other places. Thank you for adding the todo.

https://codereview.chromium.org/2675603002/diff/140001/pkg/front_end/lib/src/...
File pkg/front_end/lib/src/fasta/parser/parser.dart (right):

https://codereview.chromium.org/2675603002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/parser/parser.dart:2456: BeginGroupToken begin =
first;
I would rather keep the previous version using "dynamic". In this case, I feel
the documentation is adequate for explaining what is going on.

https://codereview.chromium.org/2675603002/diff/140001/pkg/front_end/lib/src/...
File pkg/front_end/lib/src/fasta/scanner/recover.dart (right):

https://codereview.chromium.org/2675603002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/scanner/recover.dart:57: var codeUnits = <int>[];
Restore type.

https://codereview.chromium.org/2675603002/diff/140001/pkg/front_end/lib/src/...
File pkg/front_end/lib/src/fasta/source/outline_builder.dart (right):

https://codereview.chromium.org/2675603002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/source/outline_builder.dart:386: List newList =
You don't need to add this comment, just do this:

List<FormalParameterBuilder> newList = ...;

https://codereview.chromium.org/2675603002/diff/140001/pkg/front_end/lib/src/...
File pkg/front_end/lib/src/fasta/source/source_loader.dart (right):

https://codereview.chromium.org/2675603002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/source/source_loader.dart:200: // TODO(sigmund):
should be covaraint SourceLibraryBuilder
covariant.

https://codereview.chromium.org/2675603002/diff/140001/pkg/front_end/lib/src/...
File pkg/front_end/lib/src/fasta/source/stack_listener.dart (right):

https://codereview.chromium.org/2675603002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/source/stack_listener.dart:59: covariant formals,
AsyncMarker asyncModifier, covariant body) {
Would dynamic work instead of covariant?

https://codereview.chromium.org/2675603002/diff/140001/pkg/front_end/lib/src/...
File pkg/front_end/lib/src/fasta/util/link.dart (right):

https://codereview.chromium.org/2675603002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/util/link.dart:136: K fold<K>(K initialValue, K
combine(K value, T element)) => _unsupported('fold');
Long line.

[Siggi Cherem (dart-lang), 2017-02-07 21:13:57]

Patchset #3 (id:160001) has been deleted

[Siggi Cherem (dart-lang), 2017-02-07 21:48:18]

Patchset #3 (id:180001) has been deleted

[Siggi Cherem (dart-lang), 2017-02-07 21:59:26]

https://codereview.chromium.org/2675603002/diff/140001/pkg/front_end/lib/src/...
File pkg/front_end/lib/src/fasta/analyzer/mock_type.dart (right):

https://codereview.chromium.org/2675603002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/analyzer/mock_type.dart:40: internalError("not
supported.");
On 2017/02/07 11:03:07, ahe wrote:
> Would you mind using `{ return expr; }` when `=> expr;` doesn't fit on one
line?

Sure, no problem.

FWIW - I personally prefer to continue using => when the rhs is a simple
expression.

https://codereview.chromium.org/2675603002/diff/140001/pkg/front_end/lib/src/...
File pkg/front_end/lib/src/fasta/builder/builder.dart (right):

https://codereview.chromium.org/2675603002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/builder/builder.dart:92: int resolveTypes(covariant
Builder parent) => 0;
On 2017/02/07 11:03:07, ahe wrote:
> Do you have to add "covariant" here?

There are two overrides that strengthen the argument type (in ClassBuilder and
FunctionTypeBuilder). However, both of them used LibraryBuilder as the type.

So we have 3 options:

a- make it LibraryBuilder here.
b- add covariant here.
c- add covariant on each override (the type system allows you to add the
covariant keyword on either the base definition or on each override).

Any preference? It seems like (a) should be fine here, but you know better.

https://codereview.chromium.org/2675603002/diff/140001/pkg/front_end/lib/src/...
File pkg/front_end/lib/src/fasta/kernel/body_builder.dart (right):

https://codereview.chromium.org/2675603002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:244: // List<Statement>.
On 2017/02/07 11:03:07, ahe wrote:
> How about:
> 
> List<dynamic /* Statement | List<Statement> */> statements = popList(count) ??
> <Statement>[];
> 
> And remove the todo.

Done. added the comment on the type, but had to do something else for the TODO
(below)

https://codereview.chromium.org/2675603002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:245: List copy;
On 2017/02/07 11:03:07, ahe wrote:
> This should be List<Statement>.

Done. Sorry for the confusion - the TODO was referring to this `List`. I
originally removed the explicit type arg as a way to workaround
https://github.com/dart-lang/sdk/issues/28651. (same issue as
`variableOrExpression` below and with formals in the outline_builder.)

There are two ways to work around it here, either removing the type-arg here or
adding a silly assignment further below. I just switched to the latter and made
the comment highlight the issue. Hope this is clearer.

https://codereview.chromium.org/2675603002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:1285: var builder =
formalParameterScope.lookup(name.name);
On 2017/02/07 11:03:08, ahe wrote:
> dynamic.

Done.

https://codereview.chromium.org/2675603002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:1291:
KernelFormalParameterBuilder formalBuilder = builder;
On 2017/02/07 11:03:08, ahe wrote:
> Remove this.

Done.

https://codereview.chromium.org/2675603002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:1641: if (type is
KernelClassBuilder) {
On 2017/02/07 11:03:07, ahe wrote:
> Change this back.

Sorry, I totally meant to ask you a follow up question about this: below there
are two accesses to `type.cls` that are producing an error because `cls` is only
defined in KernelClassBuilder.

On what cases can this be a non-kernel class builder?

https://codereview.chromium.org/2675603002/diff/140001/pkg/front_end/lib/src/...
File pkg/front_end/lib/src/fasta/kernel/builder_accessors.dart (right):

https://codereview.chromium.org/2675603002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/kernel/builder_accessors.dart:100: doInvocation(int
charOffset, Arguments arguments);
On 2017/02/07 11:03:08, ahe wrote:
> TreeNode

Done.

https://codereview.chromium.org/2675603002/diff/140001/pkg/front_end/lib/src/...
File pkg/front_end/lib/src/fasta/kernel/kernel_enum_builder.dart (right):

https://codereview.chromium.org/2675603002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/kernel/kernel_enum_builder.dart:139:
members.forEach((String name, dynamic builder) {
On 2017/02/07 11:03:08, ahe wrote:
> I think I prefer this:
> 
> members.forEach((String name, dynamic b) {
>   MemberBuilder builder = b;
> ...
> }
> 
> Two or three other places. Thank you for adding the todo.

Done. I had that in on my first change and I thought you wanted me to remove it
when switching to dynamic :), just added it back.

https://codereview.chromium.org/2675603002/diff/140001/pkg/front_end/lib/src/...
File pkg/front_end/lib/src/fasta/parser/parser.dart (right):

https://codereview.chromium.org/2675603002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/parser/parser.dart:2456: BeginGroupToken begin =
first;
On 2017/02/07 11:03:08, ahe wrote:
> I would rather keep the previous version using "dynamic". In this case, I feel
> the documentation is adequate for explaining what is going on.

Done.

https://codereview.chromium.org/2675603002/diff/140001/pkg/front_end/lib/src/...
File pkg/front_end/lib/src/fasta/scanner/recover.dart (right):

https://codereview.chromium.org/2675603002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/scanner/recover.dart:57: var codeUnits = <int>[];
On 2017/02/07 11:03:08, ahe wrote:
> Restore type.

Done.

Not sure if you know: in strong mode `var` gets inferred with the type of the
rhs, in this case List<int>, so it was treated as such below whether I include
the type or var.

Just to double check: do you prefer the type to be explicit in this case? Or
would you be OK omitting it given that we know it is inferred?

https://codereview.chromium.org/2675603002/diff/140001/pkg/front_end/lib/src/...
File pkg/front_end/lib/src/fasta/source/outline_builder.dart (right):

https://codereview.chromium.org/2675603002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/source/outline_builder.dart:386: List newList =
On 2017/02/07 11:03:08, ahe wrote:
> You don't need to add this comment, just do this:
> 
> List<FormalParameterBuilder> newList = ...;

Here too: `var` is inferred as `List<FormalParameterBuilder>` under strong mode.
The problem is that `formals` and `last` are List<dynamic> and cannot be passed
to .setRange due to https://github.com/dart-lang/sdk/issues/28651.

Options that I believe are available to us right now:
 a- I can do what I did in body_builder (add an assignment to Iterable for
formals/last before we pass them to .setRange)
 b- remove the generic like I do here
 c- fix the parser to do what I describe in the comment
 d- keep the code as it was, and for now keep the 2 errors messages below in the
codebase until #28651 is fixed or (c) is done.

Eventually I'd like to make (c) happen: (a) and (b) are hiding a problem that
will resurface when we get to the point of making this run with strong checks in
2.0. However, I figure it's better to make that kind of change separately - if
you'd like me to do it sooner, I'd be happy to look into that.

Of the remaining options, I prefer (a) or (b). I kept it as (b) to make a single
change rather than 2, but I'm happy to change it to (a) instead if you prefer.

Since the comment felt a bit long, I moved the details to a bug and kept it
shorter here. Let me know what you think

https://codereview.chromium.org/2675603002/diff/140001/pkg/front_end/lib/src/...
File pkg/front_end/lib/src/fasta/source/source_loader.dart (right):

https://codereview.chromium.org/2675603002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/source/source_loader.dart:200: // TODO(sigmund):
should be covaraint SourceLibraryBuilder
On 2017/02/07 11:03:08, ahe wrote:
> covariant.

Done.

https://codereview.chromium.org/2675603002/diff/140001/pkg/front_end/lib/src/...
File pkg/front_end/lib/src/fasta/source/stack_listener.dart (right):

https://codereview.chromium.org/2675603002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/source/stack_listener.dart:59: covariant formals,
AsyncMarker asyncModifier, covariant body) {
On 2017/02/07 11:03:08, ahe wrote:
> Would dynamic work instead of covariant?

not in this case: these were treated as dynamic, the problem is that there are 2
overrides that strenghten the types:
- analyzer/AstBuilder makes it dynamic x AsyncMarker x analyzer.Statement
- kernel/BodyBuilder makes it FormalParameters x AsyncMarker x kernel.Statement

I could however move the covariant down to each override if you prefer

https://codereview.chromium.org/2675603002/diff/140001/pkg/front_end/lib/src/...
File pkg/front_end/lib/src/fasta/util/link.dart (right):

https://codereview.chromium.org/2675603002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/util/link.dart:136: K fold<K>(K initialValue, K
combine(K value, T element)) => _unsupported('fold');
On 2017/02/07 11:03:08, ahe wrote:
> Long line.

Done.

[ahe, 2017-02-08 19:59:24]

https://codereview.chromium.org/2675603002/diff/140001/pkg/front_end/lib/src/...
File pkg/front_end/lib/src/fasta/analyzer/mock_type.dart (right):

https://codereview.chromium.org/2675603002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/analyzer/mock_type.dart:40: internalError("not
supported.");
On 2017/02/07 21:59:25, Siggi Cherem (dart-lang) wrote:
> On 2017/02/07 11:03:07, ahe wrote:
> > Would you mind using `{ return expr; }` when `=> expr;` doesn't fit on one
> line?
> 
> Sure, no problem.
> 
> FWIW - I personally prefer to continue using => when the rhs is a simple
> expression.

Let's pick the option that most people on team prefer and stick to that. I'm
used to what I suggested above, and kinda like it.

https://codereview.chromium.org/2675603002/diff/140001/pkg/front_end/lib/src/...
File pkg/front_end/lib/src/fasta/builder/builder.dart (right):

https://codereview.chromium.org/2675603002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/builder/builder.dart:92: int resolveTypes(covariant
Builder parent) => 0;
On 2017/02/07 21:59:25, Siggi Cherem (dart-lang) wrote:
> On 2017/02/07 11:03:07, ahe wrote:
> > Do you have to add "covariant" here?
> 
> There are two overrides that strengthen the argument type (in ClassBuilder and
> FunctionTypeBuilder). However, both of them used LibraryBuilder as the type.
> 
> So we have 3 options:
> 
> a- make it LibraryBuilder here.
> b- add covariant here.
> c- add covariant on each override (the type system allows you to add the
> covariant keyword on either the base definition or on each override).
> 
> Any preference? It seems like (a) should be fine here, but you know better.

I was curious and wasn't sure if c was an option. I'm glad that it is, but
covariant is fine here.

https://codereview.chromium.org/2675603002/diff/140001/pkg/front_end/lib/src/...
File pkg/front_end/lib/src/fasta/kernel/body_builder.dart (right):

https://codereview.chromium.org/2675603002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:1641: if (type is
KernelClassBuilder) {
On 2017/02/07 21:59:25, Siggi Cherem (dart-lang) wrote:
> On 2017/02/07 11:03:07, ahe wrote:
> > Change this back.
> 
> Sorry, I totally meant to ask you a follow up question about this: below there
> are two accesses to `type.cls` that are producing an error because `cls` is
only
> defined in KernelClassBuilder.
> 
> On what cases can this be a non-kernel class builder?

I don't think it can happen currently, but I'd like to keep the type test and
fix the problems below. I've added some suggestions.

https://codereview.chromium.org/2675603002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:1647: if
(type.cls.isAbstract) {
I don't think we have isAbstract on ClassBuilder, could you add that?

In ClassBuilder it would just be abstract and overridden by KernelClass builder.

https://codereview.chromium.org/2675603002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:1667: errorName =
debugName(type.cls.name, name);
I think debugName(type.name, name) should work instead.

https://codereview.chromium.org/2675603002/diff/140001/pkg/front_end/lib/src/...
File pkg/front_end/lib/src/fasta/kernel/kernel_enum_builder.dart (right):

https://codereview.chromium.org/2675603002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/kernel/kernel_enum_builder.dart:139:
members.forEach((String name, dynamic builder) {
On 2017/02/07 21:59:26, Siggi Cherem (dart-lang) wrote:
> On 2017/02/07 11:03:08, ahe wrote:
> > I think I prefer this:
> > 
> > members.forEach((String name, dynamic b) {
> >   MemberBuilder builder = b;
> > ...
> > }
> > 
> > Two or three other places. Thank you for adding the todo.
> 
> Done. I had that in on my first change and I thought you wanted me to remove
it
> when switching to dynamic :), just added it back.

Yeah, sorry about that. Sometimes stuffs needs to be really concrete for me to
understand the trade offs. This is part of the reason I'd preferred to do this
task myself at a later time.

https://codereview.chromium.org/2675603002/diff/140001/pkg/front_end/lib/src/...
File pkg/front_end/lib/src/fasta/scanner/recover.dart (right):

https://codereview.chromium.org/2675603002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/scanner/recover.dart:57: var codeUnits = <int>[];
On 2017/02/07 21:59:26, Siggi Cherem (dart-lang) wrote:
> On 2017/02/07 11:03:08, ahe wrote:
> > Restore type.
> 
> Done.
> 
> Not sure if you know: in strong mode `var` gets inferred with the type of the
> rhs, in this case List<int>, so it was treated as such below whether I include
> the type or var.
> 
> Just to double check: do you prefer the type to be explicit in this case? Or
> would you be OK omitting it given that we know it is inferred?

Thank you. I prefer for types to be explicit as I can't see the inferred type in
Emacs and code reviews.

https://codereview.chromium.org/2675603002/diff/140001/pkg/front_end/lib/src/...
File pkg/front_end/lib/src/fasta/source/stack_listener.dart (right):

https://codereview.chromium.org/2675603002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/source/stack_listener.dart:59: covariant formals,
AsyncMarker asyncModifier, covariant body) {
On 2017/02/07 21:59:26, Siggi Cherem (dart-lang) wrote:
> On 2017/02/07 11:03:08, ahe wrote:
> > Would dynamic work instead of covariant?
> 
> not in this case: these were treated as dynamic, the problem is that there are
2
> overrides that strenghten the types:
> - analyzer/AstBuilder makes it dynamic x AsyncMarker x analyzer.Statement
> - kernel/BodyBuilder makes it FormalParameters x AsyncMarker x
kernel.Statement
> 
> I could however move the covariant down to each override if you prefer

Sigh. Obviously someone doesn't know what "dynamic" means. So long, optional
types :-((((

[Siggi Cherem (dart-lang), 2017-02-09 04:44:02]

thanks Peter - I think I've addressed all the comments. PTAL

https://codereview.chromium.org/2675603002/diff/140001/pkg/front_end/lib/src/...
File pkg/front_end/lib/src/fasta/analyzer/mock_type.dart (right):

https://codereview.chromium.org/2675603002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/analyzer/mock_type.dart:40: internalError("not
supported.");
On 2017/02/08 19:59:23, ahe wrote:
> On 2017/02/07 21:59:25, Siggi Cherem (dart-lang) wrote:
> > On 2017/02/07 11:03:07, ahe wrote:
> > > Would you mind using `{ return expr; }` when `=> expr;` doesn't fit on one
> > line?
> > 
> > Sure, no problem.
> > 
> > FWIW - I personally prefer to continue using => when the rhs is a simple
> > expression.
> 
> Let's pick the option that most people on team prefer and stick to that. I'm
> used to what I suggested above, and kinda like it.

OK - I'll land this for now as you suggested since likely the rest of the code
is following this style, we can revisit this later on if we need to.

https://codereview.chromium.org/2675603002/diff/140001/pkg/front_end/lib/src/...
File pkg/front_end/lib/src/fasta/builder/builder.dart (right):

https://codereview.chromium.org/2675603002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/builder/builder.dart:92: int resolveTypes(covariant
Builder parent) => 0;
On 2017/02/08 19:59:23, ahe wrote:
> On 2017/02/07 21:59:25, Siggi Cherem (dart-lang) wrote:
> > On 2017/02/07 11:03:07, ahe wrote:
> > > Do you have to add "covariant" here?
> > 
> > There are two overrides that strengthen the argument type (in ClassBuilder
and
> > FunctionTypeBuilder). However, both of them used LibraryBuilder as the type.
> > 
> > So we have 3 options:
> > 
> > a- make it LibraryBuilder here.
> > b- add covariant here.
> > c- add covariant on each override (the type system allows you to add the
> > covariant keyword on either the base definition or on each override).
> > 
> > Any preference? It seems like (a) should be fine here, but you know better.
> 
> I was curious and wasn't sure if c was an option. I'm glad that it is, but
> covariant is fine here.

ok - switched back to covariant. If `LibraryBuilder` is a better match here let
me know.

https://codereview.chromium.org/2675603002/diff/140001/pkg/front_end/lib/src/...
File pkg/front_end/lib/src/fasta/kernel/body_builder.dart (right):

https://codereview.chromium.org/2675603002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:1641: if (type is
KernelClassBuilder) {
On 2017/02/08 19:59:23, ahe wrote:
> On 2017/02/07 21:59:25, Siggi Cherem (dart-lang) wrote:
> > On 2017/02/07 11:03:07, ahe wrote:
> > > Change this back.
> > 
> > Sorry, I totally meant to ask you a follow up question about this: below
there
> > are two accesses to `type.cls` that are producing an error because `cls` is
> only
> > defined in KernelClassBuilder.
> > 
> > On what cases can this be a non-kernel class builder?
> 
> I don't think it can happen currently, but I'd like to keep the type test and
> fix the problems below. I've added some suggestions.

Done.

https://codereview.chromium.org/2675603002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:1647: if
(type.cls.isAbstract) {
On 2017/02/08 19:59:23, ahe wrote:
> I don't think we have isAbstract on ClassBuilder, could you add that?
> 
> In ClassBuilder it would just be abstract and overridden by KernelClass
builder.

Done.

https://codereview.chromium.org/2675603002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:1667: errorName =
debugName(type.cls.name, name);
On 2017/02/08 19:59:23, ahe wrote:
> I think debugName(type.name, name) should work instead.

Done.

https://codereview.chromium.org/2675603002/diff/140001/pkg/front_end/lib/src/...
File pkg/front_end/lib/src/fasta/kernel/kernel_enum_builder.dart (right):

https://codereview.chromium.org/2675603002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/kernel/kernel_enum_builder.dart:139:
members.forEach((String name, dynamic builder) {
On 2017/02/08 19:59:23, ahe wrote:
> On 2017/02/07 21:59:26, Siggi Cherem (dart-lang) wrote:
> > On 2017/02/07 11:03:08, ahe wrote:
> > > I think I prefer this:
> > > 
> > > members.forEach((String name, dynamic b) {
> > >   MemberBuilder builder = b;
> > > ...
> > > }
> > > 
> > > Two or three other places. Thank you for adding the todo.
> > 
> > Done. I had that in on my first change and I thought you wanted me to remove
> it
> > when switching to dynamic :), just added it back.
> 
> Yeah, sorry about that. Sometimes stuffs needs to be really concrete for me to
> understand the trade offs. This is part of the reason I'd preferred to do this
> task myself at a later time.

No problem, I understand

[ahe, 2017-02-09 15:46:10]

lgtm

[Siggi Cherem (dart-lang), 2017-02-10 19:14:00]

Description was changed from

==========
Reduce strong mode errors and warnings
==========

to

==========
Reduce strong mode errors and warnings

R=ahe@google.com

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

[Siggi Cherem (dart-lang), 2017-02-10 19:14:01]

Committed patchset #5 (id:240001) manually as
935ec2bb8948f142c7843a4eaa6f1aa09213c408 (presubmit successful).

[Siggi Cherem (dart-lang), 2017-02-10 19:20:32]

https://codereview.chromium.org/2675603002/diff/240001/pkg/front_end/lib/src/...
File pkg/front_end/lib/src/fasta/kernel/body_builder.dart (right):

https://codereview.chromium.org/2675603002/diff/240001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:1644: if (type.isAbstract)
{
FYI - I reverted this one change for now so I could land this and investigate
later. I left it as type.cls.isAbstract, so this will have an analyzer error
(surprisingly only in strong mode)