analyzer strong mode: infer final field from initializer if super getters are dynamic

pkg/analyzer/lib/src/task/strong_mode.dart

 // Copyright (c) 2015, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 library analyzer.src.task.strong_mode;
 
 import 'dart:collection';
 
 import 'package:analyzer/src/generated/ast.dart';
 import 'package:analyzer/src/generated/element.dart';
@@ skipping 279 matching lines @@
             .lookupOverrides(
                 fieldElement.enclosingElement, fieldElement.name + '=');
         if (!_isCompatible(newType, overriddenSetters)) {
           newType = null;
         }
       }
       //
       // Then, if none was found, infer the type from the initialization
       // expression.
       //
-      if (newType == null) {
+      // Also, if we're the field is final and overridden getter is dynamic,
+      // we can infer from the initialization without breaking subtyping rules
+      // (return type is covariant).
+      //
+      if (newType == null || newType.isDynamic) {

[Jennifer Messerly, 2015/09/25 21:41:55]

this fix is intended to match:
https://github.com/dart-lang/dev_compiler/blob/1549e6a5e7e07a7f0ccb630dbaa926...

To be honest, I'm not sure why we special case `dynamic` ... instead of either
using hasImplicitType, or just a subtype check? Thoughts?

[Brian Wilkerson, 2015/09/25 22:00:00]

hasImplicitType only applies to variables, not types.

What we're saying here is that we will try to infer the type from the
initializer if either there was no overridden getter or if the overridden getter
couldn't have its type inferred.

(But the comment you added makes no sense here, so please remove it.)

[Jennifer Messerly, 2015/09/25 22:22:11]

What doesn't make sense about the comment? That's basically Siggi's comment from
DDC's old resolver code ... it made sense to me. Let me try and reword ... he
was saying: because the supertype field's getter is `dynamic`, it is safe to
infer a stronger type in a derived class. This won't break any users of the
supertype, because they already have to expect it to be any possible type.

(and we get `newType` from `newType = _computeReturnType(overriddenGetters);`
above. In other words, it represents the return type of the overridden getters)

I'm happy to reword, but I don't want to just remove the comment...

[Leaf, 2015/09/25 22:36:32]

The comment has a wording problem in the first line ("if we're the field is
final"), but the gist of it makes sense to me.  I'm also not sure why we only
handle the dynamic case.

[Jennifer Messerly, 2015/09/25 22:48:23]

ah good catch. How about this:

Also infer from the initialization expression if the overridden getter's return
type is dynamic.
This is safe to do because we will infer a stronger return type (than dynamic).

[Brian Wilkerson, 2015/09/26 00:36:29]

Aside from the grammatical error, it seems to me that
- there is no "also", we're only doing one thing here,
- the condition given for being able to infer the return type is as incomplete
as the comment above,
- the role of covariance is misrepresented; we could potentially infer a
covariant type even if the overridden getter's type was not dynamic, but we're
choosing not to.
How about a single (hopefully more consistent) comment:

If there is no overridden getter or if the overridden getter's type is dynamic,
then we can infer the type from the initialization expression without breaking
subtype rules. We could potentially infer a consistent return type even if the
overridden getter's type was not dynamic, but choose not to for simplicity. The
field is required to be final to prevent choosing a type that is inconsistent
with assignments we cannot analyze.

[Jennifer Messerly, 2015/09/26 00:48:23]

"if <condition>, do this" "also, if <other condition> ..."

not sure why that's a problem. I didn't want to mess with the original comment,
so I added "also" to explain the (IMO, unrelated) additional rule.
fwiw, not sure I agree with those points, I don't understand what's incomplete
or misrepresented. But anyway, I can agree to disagree. Happy to use your
wording.
Sure, I can use that. Thanks.

         if (fieldElement.initializer != null &&
             (fieldElement.isFinal || overriddenGetters.isEmpty)) {
           newType = fieldElement.initializer.returnType;
         }
       }
       if (newType == null || newType.isBottom) {
         newType = typeProvider.dynamicType;
       }
       (fieldElement as FieldElementImpl).type = newType;
       setReturnType(fieldElement.getter, newType);
@@ skipping 161 matching lines @@
         results.add(element);
       }
     }
   }
 }
 
 /**
  * A class of exception that is not used anywhere else.
  */
 class _CycleException implements Exception {}