Introduce initial plumbing for type promotion in fasta.

Introduce initial plumbing for type promotion in fasta.

R=ahe@google.com

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

[Paul Berry, 2017-04-22 18:31:51]

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

[ahe, 2017-04-24 16:47:16]

Comments so far (only nits).

Note to self: I've reviewed up to and including
	pkg/front_end/lib/src/fasta/kernel/kernel_field_builder.dart.

https://codereview.chromium.org/2829223007/diff/20001/pkg/analysis_server/ana...
File pkg/analysis_server/analysis_server.iml (left):

https://codereview.chromium.org/2829223007/diff/20001/pkg/analysis_server/ana...
pkg/analysis_server/analysis_server.iml:60: <orderEntry type="library"
name="Dart Packages" level="project" />
What's this for?

https://codereview.chromium.org/2829223007/diff/20001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/builder/ast_factory.dart (right):

https://codereview.chromium.org/2829223007/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/builder/ast_factory.dart:49: /// Creates an if
statement.
`if`

https://codereview.chromium.org/2829223007/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/builder/ast_factory.dart:56: /// Creates an "is"
expression.
"is" -> `is`

https://codereview.chromium.org/2829223007/diff/20001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/kernel/fasta_accessors.dart (right):

https://codereview.chromium.org/2829223007/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/fasta_accessors.dart:40: abstract class
BuilderHelper {
BTW: If you have a suggestion for a better name for this interface, please let
me know.

[Paul Berry, 2017-04-24 18:20:30]

https://codereview.chromium.org/2829223007/diff/20001/pkg/analysis_server/ana...
File pkg/analysis_server/analysis_server.iml (left):

https://codereview.chromium.org/2829223007/diff/20001/pkg/analysis_server/ana...
pkg/analysis_server/analysis_server.iml:60: <orderEntry type="library"
name="Dart Packages" level="project" />
On 2017/04/24 16:47:16, ahe wrote:
> What's this for?

This is one of the data files used by IntelliJ to keep track of state.  For
reasons I don't entirely understand, IntelliJ periodically messes with them. 
The changes are pretty much always benign.

If you'd rather I can extract these changes to a separate CL (or just revert
them and wait for IntelliJ to make them again in another CL).  But I find that
it's easiest to just ignore them and let IntelliJ do its thing :)

https://codereview.chromium.org/2829223007/diff/20001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/kernel/fasta_accessors.dart (right):

https://codereview.chromium.org/2829223007/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/fasta_accessors.dart:40: abstract class
BuilderHelper {
On 2017/04/24 16:47:16, ahe wrote:
> BTW: If you have a suggestion for a better name for this interface, please let
> me know.

Ok, thanks.  So far I don't have any ideas.

[ahe, 2017-04-25 13:17:01]

lgtm

I'd like to go talk through the code at our next 1:1.

https://codereview.chromium.org/2829223007/diff/20001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/type_inference/type_inferrer.dart (right):

https://codereview.chromium.org/2829223007/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/type_inference/type_inferrer.dart:71: // set isAsync
and isGenerator to false in the outermost context.
Brittle?

https://codereview.chromium.org/2829223007/diff/20001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/type_inference/type_promotion.dart (right):

https://codereview.chromium.org/2829223007/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/type_inference/type_promotion.dart:79: abstract
class TypePromoterImpl<E, V> extends TypePromoter<E, V> {
Can this class be merged with TypePromoter to take advantage of a class is also
an interface in Dart?

https://codereview.chromium.org/2829223007/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/type_inference/type_promotion.dart:133: Map<V,
TypePromotionFact<V>> get _currentFactMap {
This is a potentially expensive operation, and in that case, we often follow the
convention of calling it "computeSomething" and ensuring that it is a method
(not a getter).

https://codereview.chromium.org/2829223007/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/type_inference/type_promotion.dart:275: void
_debugEvent(String name) {
If you make this public, it also works across subclasses in different libraries.

https://codereview.chromium.org/2829223007/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/type_inference/type_promotion.dart:283:
TypePromotionFact<V> _falseFacts(E e) {
How about factsWhenFalse (as to not imply that the facts are false).

https://codereview.chromium.org/2829223007/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/type_inference/type_promotion.dart:375: /// reaches
that point in the program.
I wonder how this definition of "apply" corresponds to things like when
[variable] is assigned to later?

https://codereview.chromium.org/2829223007/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/type_inference/type_promotion.dart:427: static void
_accessedInScope(TypePromotionFact fact, TypePromotionScope scope,
Consider changing name of function to "recordAccessedInScope". To me this sounds
like a function that returns a bool.

https://codereview.chromium.org/2829223007/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/type_inference/type_promotion.dart:447: static void
_mutatedInScope(
Ditto.

[Paul Berry, 2017-04-25 15:49:27]

https://codereview.chromium.org/2829223007/diff/20001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/builder/ast_factory.dart (right):

https://codereview.chromium.org/2829223007/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/builder/ast_factory.dart:49: /// Creates an if
statement.
On 2017/04/24 16:47:16, ahe wrote:
> `if`

Done.

https://codereview.chromium.org/2829223007/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/builder/ast_factory.dart:56: /// Creates an "is"
expression.
On 2017/04/24 16:47:16, ahe wrote:
> "is" -> `is`

Done.

https://codereview.chromium.org/2829223007/diff/20001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/type_inference/type_inferrer.dart (right):

https://codereview.chromium.org/2829223007/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/type_inference/type_inferrer.dart:71: // set isAsync
and isGenerator to false in the outermost context.
On 2017/04/25 13:17:00, ahe wrote:
> Brittle?

Good point.  I actually think it might be ok to just leave it null at this
point--it's only ever used when we're inside a closure.

I've added a TODO comment.  I'll clean this up in a follow-up CL.

https://codereview.chromium.org/2829223007/diff/20001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/type_inference/type_promotion.dart (right):

https://codereview.chromium.org/2829223007/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/type_inference/type_promotion.dart:79: abstract
class TypePromoterImpl<E, V> extends TypePromoter<E, V> {
On 2017/04/25 13:17:01, ahe wrote:
> Can this class be merged with TypePromoter to take advantage of a class is
also
> an interface in Dart?

It could, but it would blur the interface boundary because we would have to move
the abstract methods from TypePromoterImpl to TypePromoter, which would make
them callable from a lot more places.  Those abstract methods are intended only
for use by the class derived from TypePromoterImpl.

https://codereview.chromium.org/2829223007/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/type_inference/type_promotion.dart:133: Map<V,
TypePromotionFact<V>> get _currentFactMap {
On 2017/04/25 13:17:01, ahe wrote:
> This is a potentially expensive operation, and in that case, we often follow
the
> convention of calling it "computeSomething" and ensuring that it is a method
> (not a getter).

Done.

https://codereview.chromium.org/2829223007/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/type_inference/type_promotion.dart:275: void
_debugEvent(String name) {
On 2017/04/25 13:17:01, ahe wrote:
> If you make this public, it also works across subclasses in different
libraries.

Done.

https://codereview.chromium.org/2829223007/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/type_inference/type_promotion.dart:283:
TypePromotionFact<V> _falseFacts(E e) {
On 2017/04/25 13:17:01, ahe wrote:
> How about factsWhenFalse (as to not imply that the facts are false).

Done.

https://codereview.chromium.org/2829223007/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/type_inference/type_promotion.dart:375: /// reaches
that point in the program.
On 2017/04/25 13:17:01, ahe wrote:
> I wonder how this definition of "apply" corresponds to things like when
> [variable] is assigned to later?

Yeah, that deserves some clarification.  The idea I'm going for is that the
notion of "applies" only includes things we can figure out at the time of
parsing, so when [variable] is assigned to later, the fact is still said to
"apply", but we update the fact's [_mutatedInScopes] so that when we later come
back during type inference, _IsCheck._computePromotedType will decide not to
promote the type.

I've updated the comment to clarify this.

https://codereview.chromium.org/2829223007/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/type_inference/type_promotion.dart:427: static void
_accessedInScope(TypePromotionFact fact, TypePromotionScope scope,
On 2017/04/25 13:17:00, ahe wrote:
> Consider changing name of function to "recordAccessedInScope". To me this
sounds
> like a function that returns a bool.

Done.

https://codereview.chromium.org/2829223007/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/type_inference/type_promotion.dart:447: static void
_mutatedInScope(
On 2017/04/25 13:17:00, ahe wrote:
> Ditto.

Done.

[Paul Berry, 2017-04-25 16:06:57]

Description was changed from

==========
Introduce initial plumbing for type promotion in fasta.
==========

to

==========
Introduce initial plumbing for type promotion in fasta.

R=ahe@google.com

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

[Paul Berry, 2017-04-25 16:06:58]

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