Bad errors on generic bounding chains following fix for #84, strong mode incompatible

[MichaelRFairhurst]

In the branch with fix for #84, the type checks are working properly for no strong mode, but the errors are not fully clear/resolved.

MyComponent<A extends B, B extends C, C extends String> {
  @Input A stringBound;
}

When you use this directive in a template, we don't have type parameters but we do have the lower bounds.

<my-component [stringBound]="'thisShouldWork'">...

With the fix for #86, pass in a string, without strong mode, you get no errors. So that works. Pass in an integer, you get an error, so that works. Even in strong mode, I believe outputs work when they work.

However, the errors you get will say "int is not assignable to B" because stringBound is of type A, and A extends B and that's as far as we resolved it. The typing works because we replaced "A" on the type which had no "bound" set, to B from the ClassElement which did, and that bound makes it more or less act like its own bound when not in strong mode.

However, when in strong mode, String is not assignable to T extends String, even though the reverse is true.

The problem comes when calling substitute2, and it doesn't substitute recursively. Not sure if the fix for this should be in substitute2 in the analyzer repo?

There are tests for this in tasks_test.dart, referencing this ticket number. Uncomment them to do the work.

[scheglov]

@jmesserly @leafpetersen Who might know better about types in analyzer.

[leafpetersen]

However, when in strong mode, String is not assignable to T extends String, even though the reverse is true.

This is correct. If T extends String, then String may not be a subtype of T. Can you be more concrete about what code is generating the error, and what you expect to happen?

The problem comes when calling substitute2, and it doesn't substitute recursively. Not sure if the fix for this should be in substitute2 in the analyzer repo?

I don't know what this means, can you give me a concrete example of what you expect to happen vs what is happening?

[jmesserly]

Yeah I want to add on to Leaf's question ... could you link to the code where you're implementing this? Are you trying to see if the string value "thisShouldWork" can be assigned to the "stringBound" field?

[MichaelRFairhurst]

The example I gave where 'A extends B, B extends C'

I'm accessing a member of the class, and getting its type. That type is A, which won't work in the template typechecking, and correctly so as you confirmed.

So I call type.substitute2(classElement.typeParameters.map((p) => p.bound), classElement.typeParameters.map((p) => p.type)). The result I get is type B! But B resolves to C, so I should get C. The fact that B should be substituted to C was passed in too.

Is there a better way to do this? I think the template is roughly equivalent to

MyComponent /* without arguments */ comp = new MyComponent /* no arguments */();
comp.stringBound = "blah"; //correct
comp.stringBound = 5; // int not assignable to string

Instead I'm getting the error B is not assignable to string.

The plot thickens as well, looks like it doesn't work without weak mode in my code, but I had a hack left in place

   while (inputType is TypeParameterType) {
     inputType = inputType.bound;
   }

I took the wrong conclusion while trying to figure out why THAT test was passing.

Edit: note too that that hack won't work for List<A>, which becomes List<B>, but is not a TypeParameterType

[jmesserly]

instantiateToBounds on TypeSystem (which should be an instance of StrongTypeSystemImpl in strong mode) will do what your first line does, and get the instantiated type. I think the answer in strong mode was supposed to be MyComponent<String, String, String> but I'm not sure if we implemented that or if the design has changed recently based on language meetings. Leaf would know for sure.

[jmesserly]

it does not currently work in strong mode:

class MyComponent<A extends B, B extends C, C extends String> {
  A stringBound;
}

main() {
  var x = new MyComponent();
  x.stringBound = 'hello';
  print(x.stringBound);
}

$ dartanalyzer --strong ~/scratch/test.dart
Analyzing [test.dart]...
[error] The literal ''hello'' with type 'String' isn't of expected type 'B'. (test.dart, line 7, col 19)
1 error found.

this is definitely a strong mode bug, whatever we do it shouldn't end up as B. I'll see if we have one filed already, I believe we do. There was assumption that bounds on type parameters would only refer to earlier ones in the list, not later ones like this example does.

[jmesserly]

Update: if I reverse the order it works in strong mode:

class MyComponent<C extends String, B extends C, A extends B> {
  A stringBound;
}
main() {
  var x = new MyComponent();
  x.stringBound = 'hello';
  print(x.stringBound);
}

[jmesserly]

this is covered by dart-lang/sdk#27072

[MichaelRFairhurst]

Woot, glad I was trying so hard to break my own analysis code that I broke strong mode! I'm going to grab myself a cookie.

So I should be able to call typeSystem.instantiateToBounds(returntype) to avoid having to have the classElement in scope when I resolve these?

[jmesserly]

yeah it's a great test case! 👍

I think that's right... you'll want to typeSystem.instantiateToBounds on the MyComponent type, however you're getting that. Conceptually it's what happens when you write MyComponent c; ... there's an implicit instantiation. In non-strong mode it will make MyComponent<dynamic, dynamic, dynamic> but in strong mode that would be illegal, because dynamic is not a subtype of String, so we try and do better, or we need to issue an error if we can't.

BTW feel free to leave a note if that bug is blocking you, we can increase the priority.

[MichaelRFairhurst]

Hmm, that sounds great and not great.

I really wanted there to be lower bounds type checking for generic components, just from the perspective of trying to make useful angular errors. Given that the typing of templates isn't truly specified anywhere in the angular spec or in the dart spec...and we are generating warnings rather than compile errors...I'd like to still use lower bounds rather than dynamic for the template errors...even if they aren't in strong mode, ideally. This is me trying to take all the liberties I can.

On the other hand, instantiateToBounds() seems what I was looking for when I used substitute2. Seems more complete and the right method for the job. I could see my code breaking in the future because substitute2 is not exactly made for what I'm doing.

Also worth noting, we are traversing the ast of the class to build up a model of a directive when this happens, so I am going from a Node to a PropertyInducingElement to a type. Looks like I'll have to go from a Node to PropertyInducingElement to ClassElement to Type to instantiateToBounds to lookupGetter based on the PropertyInducingElement to get the type! A bit of a headache, but certainly doable.

I guess as long as strong mode is turned on it will do the right thing so that's great.

On the other hand I can leave it as it is now until the bug in the sdk is fixed..The difference between the two right now is that when using substitute2 there are no working orders (A extends B extends String, A extends String extends B...none). But I think this is an edge case that I can leave unsolved without fear of users hitting it often or at all at first. And this way users without strong mode will get bounds-related errors in templates when they don't have generic constraint chains...

[jmesserly]

Also worth noting, we are traversing the ast of the class to build up a model of a directive when this happens, so I am going from a Node to a PropertyInducingElement to a type. Looks like I'll have to go from a Node to PropertyInducingElement to ClassElement to Type to instantiateToBounds to lookupGetter based on the PropertyInducingElement to get the type! A bit of a headache, but certainly doable

if you're traversing the class AST, could you get the ClassElement from the ClassDeclaration node? .element will give you the ClassElement.

[jmesserly]

yeah I think you definitely will want to use instantiateToBounds over substitute2. The latter is for something very specific, it's best thought of as a low level primitive that we use to implement other things (such as TypeSystem.instantiateToBounds and FunctionType/InterfaceType instantiate).

[MichaelRFairhurst]

I think I solved it without using instantiateToBounds which leaves us with the philosophical choice of whether I'm overstepping my bounds by doing the strong mode thing regardless of strong mode being on.

  DartType _deparameterizeType(
      ClassElement classElement, DartType parameterizedType) {
    if (parameterizedType == null) {
      return null;
    }

    // See #91 for discussion about bugs related to bounds
    var getBound = (p) {
      var type = p.bound;
      while (type != null && type is TypeParameterType) {
        type = type.bound;
      }
      return type ?? typeProvider.dynamicType;
    };

    var getType = (p) => p.type;
    var bounds = new List<DartType>()
      ..addAll(classElement.typeParameters.map(getBound));
    var parameters = new List<DartType>()
      ..addAll(classElement.typeParameters.map(getType));
    return parameterizedType.substitute2(bounds, parameters);
  }

Maybe that is useful to you.

Happy to take discussion about whether I'm doing too much or not.

[jmesserly]

review suggestion ... your "getBound" is just type.resolveToBound(typeProvider.dynamicType)

[jmesserly]

another review suggestion: instead of new List<DartType>() ..addAll(classElement.typeParameters.map(getBound));, use new List<DartType>.from(...) or .toList()

but yeah I would definitely recommend instantateToBounds, if you don't want non-strong mode behavior, you could always create a StrongTypeSystemImpl to use it for instantateToBounds, that way you get consistent results and you'll pick up any bug fixes we do as well :)

[MichaelRFairhurst]

Ah great tips all three! Thanks!

[MichaelRFairhurst]

OK leaving this ticket open until dart-lang/sdk#27072 is solved, to use instantiateToBounds without bugs. Can use strong mode typesystem to ensure strong checking since its not specified what errors we can't report.