Using a toplevel variable changes inference results in CFE

[leafpetersen]

Top level variable inference in the CFE seems to be going wrong in very non-local ways. For this program:

typedef T F<T>(T b);

class A<T> {
  final F<T> f;
  A(this.f);
}

int foo(int a) {
  return 3;
}
var a = new A(foo);

// LINE 1: uncomment the next line and the definition of a (above) gets an error!
// var b = [a];
void main() {
  print(a.runtimeType); // prints A<int>
  // LINE 2: uncomment the next line, and the error message describes a as having type A<dynamic>
  // String s = a;
  // Line 3: uncomment the next line and there is no error message at all
  //   A<String> s = a;
}

This program correctly prints A<int>.

1. Uncommenting the line following LINE 1 causes the definition of a to suddenly become an error indicating that a the type argument of the constructor is being inferred as dynamic:

leafp-macbookpro:dart leafp$ rundart.sh dart --preview-dart-2  ~/tmp/ddctest.dart
file:///Users/leafp/tmp/ddctest.dart:11:15: Error: A value of type '(dart.core::int) → dart.core::int' can't be assigned to a variable of type '(dynamic) → dynamic'.
Try changing the type of the left hand side, or casting the right hand side to '(dynamic) → dynamic'.
var a = new A(foo);
              ^

2. Uncommenting the line following LINE 2 causes an error message that implies that a is being inferred as having type A<dynamic>.

file:///Users/leafp/tmp/ddctest.dart:17:14: Error: A value of type '#libnull::A<dynamic>' can't be assigned to a variable of type 'dart.core::String'.
Try changing the type of the left hand side, or casting the right hand side to 'dart.core::String'.
  String s = a;
             ^

3. Uncommenting the line following LINE 3 results in no error message (implying that a is incorrectly inferred as A<dynamic>), but the runtime cast failure indicates that the reified type is A<int>.

leafp-macbookpro:dart leafp$ rundart.sh dart --preview-dart-2  ~/tmp/ddctest.dart
A<int>
Unhandled exception:
type 'A<int>' is not a subtype of type 'A<String>' where

This showed up as an unexpected failure when testing dart2js with preview-dart-2 on internal code.

cc @srawlins @sigmundch @stereotype441

[kmillikin]

I'll take this one.

It looks like our implementation only infers in types of initializing formals after it has completed top-level inference for fields, getters, and setters. What you are seeing is a consequence of performing inference based on the parameter type of dynamic for the constructor A.

What it seems like should happen is that when inferring the type of the static field a based on its initializer we have a dependency on the type for the initializing formal of the constructor A. @leafpetersen the PR #28218 doesn't seem to mention this case.

[leafpetersen]

Note that that PR is very obsolete, we got strong push back on it and @stereotype441 agreed to implement something more general. What he implemented hasn't been specified yet, but I don't think we considered that inference of fields feeds into the types of constructors. I don't think this is problematic, but if it is we can consider rejecting this case.

Here's an algorithmic sketch of what I think top level inference should be doing now, with initializing formals considered:

• Do all override based inference
• Mark every top level variable declaration, static or instance field declaration, and constructor declaration with an explicit type as available.
• For each top level variable declaration, static or instance field declaration, and constructor declaration D which is not available
  • If D is a constructor declaration C(...) for which one or more of the parameters is declared as an initializing formal without an explicit type:
    • Mark C as unavailable
    • Perform top level inference on each of the fields used as an initializing formal for C (without an explicit type)
    • Record the inferred type of C, and mark it as available
  • If D is a declaration of a top level variable or static or instance field declaration declaring the name x and if the declaration does not have an explicit type and is initialized with e
    • Mark x as unavailable
    • Perform local type inference on e, with the following modifications:
      • If local type inference requires looking up the type of a variable or field y whose type has not yet been inferred and y is not unavailable then recursively perform inference on y before continuing.
      • If local type inference requires looking up the type of a variable or field y whose type has not yet been inferred and y is unavailable, it is an error and inference fails
      • If local type inference requires looking up the type of constructor C whose type has not yet been inferred and C is not unavailable then recursively perform inference on C before continuing.
      • If local type inference requires looking up the type of a constructor C whose type has not yet been inferred and C is unavailable, it is an error and inference fails
    • Record the inferred type of x and mark it available

cc @lrhn

[lrhn]

I'll read it more closely, but I have an immediate concern with the specified algorithm:

• Do all override based inference
• Mark every top level variable declaration, static or instance field declaration, and constructor declaration with an explicit type as available.
  ...

I'm not sure it gives a satisfactory result to do override based "inference" first because it means you can't inherit an inferred field type. Example:

class C {
  var x = new B();  // Inference finds x as B
}
class D {
  get x { log("getting x"); return super.x; }
  set x(value) { log("setting x"); super.x = value; }
}

Here I would very much like the setter and getter to inherit the type B.

Would it be possible to include override "inference" in the inference loop, and handle it like any other inference (with cycle detection, but since it usually only depends on the superclass, and we don't have cyclic class hierarchies, that's rarely going to be an issue).
That would mean:

• Mark every fully typed instance method/getter/setter as available.
• For each instance method/getter/setter not marked available:
  • Mark it unavailable
  • Perform top-level inference on every instance member with the same name in all (immediate?) super-interface classes of the containing class. For setters and getters, this includes doing inference on field declarations which introduce such setters and getters.
  • Find a most specific of those declarations. It's an inference error if none exists, or if more exists with different signatures.
  • Inherit types from that most specific declaration.
  • mark it available.

(or something like that.)

As for the initializing formals, it seems fine, even if it'll rarely be useful. We don't see many fields that both have initializer expressions and are also initialized by a constructor.

[leafpetersen]

Yes, @lrhn is correct, you can't separate override out entirely. Let's try that again, maybe something like this? @stereotype441 does this look reasonably close to what you implemented (semantically - I'm sure the actual algorithm is different)?

• Mark every top level, static or instance declaration(fields, setters, getters, constructors, methods) with an explicit type as available.
• For each declaration D which is not available
  • If D is a method, setter, or getter declaration with name x:
    • Mark x as unavailable
    • Perform override inference for x
    • Record the inferred type for x and mark x as available
  • If D is a constructor declaration C(...) for which one or more of the parameters is declared as an initializing formal without an explicit type:
    • Mark C as unavailable
    • Perform top level inference on each of the fields used as an initializing formal for C (without an explicit type)
    • Record the inferred type of C, and mark it as available
  • If D is a declaration of an instance field declaration declaring the name x, which overrides another field, setter, or getter:
    • Mark x as unavailable
    • Perform override inference on x
    • Record the type of x and mark x as available
  • If D is a declaration of a top level variable or static field declaration or a non-overriding instance field declaration, declaring the name x, and which is initialized with e:
    • Mark x as unavailable
    • Perform local type inference on e, with the following modifications:
      • If local type inference requires looking up the type of a name y whose type has not yet been inferred and y is not unavailable then recursively perform inference on y before continuing.
      • If local type inference requires looking up the type of a name y whose type has not yet been inferred and y is unavailable, it is an error and inference fails
    • Record the inferred type of x and mark it available

[leafpetersen]

cc @eernstg

[kmillikin]

It seems like what we have implemented, except that as I noted above we don't handle initializing formals properly.

I'm going to lower the priority because I don't think this is affecting users in production, but bump it back up or let me know if it is affecting users.

[sigmundch]

Re priority: this shows up when enabling strong-mode in dart2js for internal customers (that's how we discovered the issue), it happens on a couple targets only, though.

[sigmundch]

Just wanted to raise attention to this again - I've seen this come up in a couple more scenarios in customer apps.

[dgrove]

Bumping up given @sigmundch 's comment.

[sigmundch]

FWIW - I sent out CLs to our internal customers to workaround it, so we are not blocked. There were about 10 files that required modifications due to this.

[kmillikin]

I'm keeping the high priority but moving this to the Dart2Stable milestone.

[kmillikin]

Proposed fix is here: https://dart-review.googlesource.com/c/sdk/+/60140

[sigmundch]

We seem to be running into more issues of this kind. This is currently showing up on internal customer apps. Here is a small repro:

abstract class B {
  String get f;
}

class A implements B {
  final f;
  A(this.f);
}

var a = new A("foo");
main() => print(a);

This yields a compile-time error of this form:

> pkg/front_end/tool/fasta compile --strong a.dart
a.dart:7:10: Error: The type of parameter 'f' (dynamic) is not a subtype of the corresponding field's type (dart.core::String).
Try changing the type of parameter 'f' to a subtype of dart.core::String.
  A(this.f);
         ^
a.dart:6:9: Context: The field that corresponds to the parameter.
  final f;
        ^

If you remove a and simply allocate A in main, you'll see no compile-time errors.

[kmillikin]

This is a front-end bug in the implementation of inference for initializing formals. When I implemented it I hadn't noticed the feature of our type inference implementation that it records that some, but not all, fields need to have their type inferred.

In this case, the field A.f doesn't even know that it needs to have override inference performed to determine its type.