Dart2: Collection literals in templates cause type cast failure

[leonsenft]

Currently in DDC, binding a collection literal in a template where a typed collection is expected

<div [ngStyle]="{'color': divColor}">

generates

Cast fail warning: Ignoring cast fail from IdentityMap<String, dynamic> to Map<String, String>

In Dart 2, this will become an error.

The culprit is the use of a pureProxyN function to change detect the bound values in the collection and build a new collection each time one changes.

https://github.com/dart-lang/angular/blob/0f6db9823667f13bd772425caa89321d5ff268d5/angular/lib/src/core/linker/app_view_utils.dart#L325-L337

While this function does have generic type parameters, the generated code doesn't use it in a context where they can be inferred, nor do we have the information to populate them manually.

This is an example of what the code generated for the above template looks like, with comments explaining the issue.

class ViewAppComponent0 {
  NgStyle _NgStyle_0_4;
  var _map_0;
  var _expr_0;

  ComponentRef<AppComponent> build() {
    ...
    // There is no type from `_map_0`, nor the closure parameter, to flow
    // through the type parameters of `pureProxy1`, thus the type returned by
    // `_map_0` will be a `Map<String, dynamic>`.
    _map_0 = pureProxy1((p0) {
      return {'color': p0};
    });
    ...
  }

  void detectChangesInternal() {
    ...
    final currVal_0 = _map_0(_ctx.divColor);
    if (!identical(_expr_0, currVal_0)) {
      // The cast warning failure occurs here because `currVal_0` of type
      // `Map<String, dynamic> is assigned to an input of type
      // `Map<String, String>`.
      _NgStyle_0_4.rawStyle = currVal_0;
      _expr_0 = currVal_0;
    }
    ...
  }
}

Type annotating the _map_0 field isn't always feasible, since it requires analyzing arbitrary expressions in the template. Likewise, determining the types bound in the collection pose a similar challenge.

[matanlurey]

Just for clarification, this would be "simpler" if we removed proxying literals, right @leonsenft?

(Albeit, with potential performance consequences)

[leonsenft]

Can you elaborate more on what exactly you have in mind if we removed proxy literals?

A few ideas come to mind (mostly terrible).

1. Drop support altogether for literals in templates

   We simply remove collection literals from the template grammar.

   • Breaks users relying on this feature. 👎
   • Simplifies template parser, intermediate ASTs, and code generation.

2. Rebuild every cycle

   Forego any concept of performance and just rebuild and bind the collection every cycle.

   void detectChangesInternal() {
     ...
     _NgStyle_0_4.rawStyle = {'color': _ctx.divColor};
   }

   • No lost types! Any type mismatch here would be a legitimate error.
   • Performance. 📉 😭
   • Likely still breaks users relying on Angular's change detection contract.

3. Inline change detection

   Check each bound value in the collection rather than the collection itself.

   class ViewAppComponent0 {
     var _p0;
     var _p1;
     void detectChangesInternal() {
       ...
       if (!identical(_p0, _ctx.color) ||
           !identical(_p1, _ctx.background)) {
         _NgStyle_0_4.rawStyle = {
           'color': _ctx.color,
           'background': _ctx.background,
         };
         _p0 = _ctx.color;
         _p1 = _ctx.background;
       }
     }
   }

   • Again, no lost types. Any mismatch would be a legitimate error.
   • Code size 🐳

4. Expose proxies for users

   Similar to solution 1., we drop support for literals and expose the pureProxyN functions to the users. This effectively pushes the generated _map_0 field to the user code.

   @Component(
     ...
     template: '<div [ngStyle]="getStyles(color)"></div>',
   )
   class AppComponent {
     String color = 'blue';
   
     Map<String, String> Function(String) getStyles = pureProxy1((color) {
       return {'color': color};
     });
   }

   • No type information is dropped; solves the cast error!
   • Less code bloat from users having to implement this functionality themselves in each component.
   • Feels super clunky, especially without varargs.
   • Still breaks users, but at least provides a fix for them.

5. Generate computed properties

   Similiar to 4., but instead of directly exposing the pureProxyN functions, we provide a new mechanism for users to mark a method as the source of a computed property. We then use this method as the input to a proxy to centralize change detection.

   @Component(
     ...
     template: '<div [ngStyle]="styles"></div>',
   )
   class AppComponent {
     String divColor = 'blue'; 
   
     // The parameter here is the name of this property accessible in the template.
     @Computed('styles')
     // There would be details to work out how the parameters are provided.
     //  1. Require the parameter name to be a field (as shown here)?
     //  2. Add a parameter to the above annotation in which to list the names of fields
     //      that are passed in order as the parameters?
     Map<String, String> computeStyles(String divColor) => {'color': divColor};
   }

   This would produce code very similar to what is produced today.

   class ViewAppComponent0 {
     // I'm not sure if we'd need to type this or not here? But we could if needed
     // since it would be the type of the computed property in the component.
     var _compute_styles;
   
     ComponentRef<AppComponent> build() {
       ...
       _compute_styles = pureProxy1(computeStyles);
     }
   
     void detectChangesInternal() {
       ...
       final currVal_0 = _compute_styles(_ctx.divColor);
       if (!identical(_expr_0, currVal_0)) {
         _NgStyle_0_4.rawStyle = currVal_0;
         _expr_0 = currVal_0;
       }
     }
   }

   • No type information is dropped; solves the cast error.
   • Less code bloat from users having to implement this functionality themselves in each component.
   • Overall less clunky than the previous solution, but still kind of awkward.
   • Increases complexity of understanding the framework, in addition to keeping track of which symbols are in scope in a template.
   • Similar concepts exist in other frameworks, but this doesn't feel very Angular-y. @yjbanov had some opinions about this solution if he cares to share them.
   • Still breaks users, but at least provides a fix for them.

[matanlurey]

Too many choices! I have choice paralysis!

1. Drop support altogether for literals in templates

We should gather a bit of data for what the impact of this would be, in case we run out of options and need to go down this route. I don't think it's the best option today, but depending on the date of Dart2 we might need to at least consider it.

2. Rebuild every cycle

Yeah, seems difficult to land, and I'd be worried about usage.

That being said, again, it depends on the amount of usage today.

3. Inline change detection

This doesn't seem terrible, especially since it more or less keeps the same contract, and we could always optimize again in the future when we aren't under a deadline.

4. Expose proxies for users

This seems better than 1, FWIW.

5. Generate computed properties

I'm not a fan of the suggestion above, specifically creating "pseudo" properties that only the template compiler can see, but something like this has been requested by users in the past. What about a sort of combination of this and 4:

class Comp {
  String color = 'blue';

  @Computed.from(const [#color])
  Map<String, String> get styles => {'color': color};
}

... with the following definition ...

/// An annotation that defines a getter or method as dependent on external fields.
///
/// The AngularDart compiler may, where able, reduce the amount of change detection
/// checks of the annotated getter or method to when the provided fields change. For
/// example:
/// ```
/// class Comp {
///   String color = 'blue';
///
///   @Computed.from(const [#color])
///   Map<String, String> get styles => {'color': color};
/// }
/// ```
abstract class Computed {
  const factory Computed.from(List<Object> properties) = ...
}

This does require more thought, for example if you could have computed fields that rely on other computed fields, how would cycles work, etc, but could be an nice "carrot" to offer if we end up having to remove literal support in the templates.

FWIW, this overlaps a bit with:

• Optimizing for immutability with AngularDart #797
• User requests for a @checkOnce

/cc @jonahwilliams @hterkelsen @ferhatb

[matanlurey]

Also @leonsenft, nice use of emojis 👏

[leonsenft]

We should gather a bit of data for what the impact of this would be, in case we run out of options and need to go down this route. I don't think it's the best option today, but depending on the date of Dart2 we might need to at least consider it.

Agreed.

Yeah, seems difficult to land, and I'd be worried about usage.

That being said, again, it depends on the amount of usage today.

In hindsight I think this (2.) is by far the worst option. Better to temporarily bloat code size (3.), than to fundamentally break Angular's change detection contract.

This doesn't seem terrible, especially since it more or less keeps the same contract, and we could always optimize again in the future when we aren't under a deadline.

This seems better than 1, FWIW.

Good point, and agreed.

Agree with you across the board on (5.). Your solution is much more elegant than mine! What advantage does using a Symbol have over a string? In my experience the analyzer doesn't validate that such a symbol is in scope. Am I missing something?

If we're going to seriously consider this option we should file a new issue to discuss it, but I think most of your concerns are addressable. For example, a computed property could rely on another, but we'd need to produce a compile error when we detect a cycle (like we already do for dependency injection!).

[matanlurey]

@leonsenft is investigating the cost of more bloated code on our large internal clients.

[leonsenft]

There are two additional options not yet outlined.

6. Inference

   I mentioned that this approach wasn't always feasible, but we may need to implement a hybrid approach. If that were the case, this approach would be the first choice, and only if we fail to infer the correct type would we fall back to another approach.

   This approach is very simple if the collection literal is the root of the bound expression. Consider the original case:

   <div [ngStyle]="{'color': color}"></div>

   Rather than inferring the expression itself, we can instead use the input's type. This works since the expression will have to be assignable to this type anyways. In this case we know [ngStyle] is a Map<String, String>, so we can use this information to type the proxy:

   class ViewAppComponent0 {
     Map<String, String> Function(String) _map_0;
     ...
   }

   This gets more challenging if the literal isn't the root of the expression:

   <div [ngStyle]="transformStyles({'color': color})"></div>

   We now have two options.

   • Infer the type to which the collection literal is assigned/passed in the expression and use that to type the proxy, similar to the above example.
   • Infer types of all bound expressions in the collection literal, and use them to type the proxy closure itself:

   class ViewAppComponent0 {
     var _list_0;
     
     ComponentRef<AppComponent> build() {
       _list_0 = pureProxy2((double x, int y) {
         return [x, y]; // Inferred as List<num>.
       });
     }

7. Collection literal type parameters

   In places where the compiler is unable to determine the type of a literal collection, we could issue a compile error informing users they must explicitly type their collection literal. This would like like type arguments on a collection literal in Dart today:

   <input #input />
   <div [ngStyle]="<String, String>{'color': input.value} | filterInput">...</div>

   We'd then use this type in a similar manner to type the proxy field.

[jonahwilliams]

Just my 2c

6. Inference

Inference based on the Input type seems pretty reasonable - it's not like it will work at runtime if the types are wrong anyway.

I've never in two years of Angular Dart seen someone pass a literal in a template through a method or closure. Though that doesn't mean people aren't using it of course, but it doesn't seem like an edge people would hit.

7. Collection literal type parameters

This seems like a can of worms, possibly the same can of worms as passing generics to templates/components themselves.

[leonsenft]

Inference based on the Input type seems pretty reasonable - it's not like it will work at runtime if the types are wrong anyway.

I've never in two years of Angular Dart seen someone pass a literal in a template through a method or closure. Though that doesn't mean people aren't using it of course, but it doesn't seem like an edge people would hit.

Yeah, ideally this ends up being the overwhelmingly common case, which gives us flexibility in choosing a rarely generated fallback option.

[yjbanov]

I probably don't have enough context to contribute anything meaningful to this discussion, but I'm failing to see how types affect performance at all. I'm assuming you are making a copy of the collection as a snapshot for a future change detection cycle. AFAICT, change detection does not call any methods on the elements of the collection, but merely compares them using identical (and maybe ==?). Hence there is no dynamic method dispatch going on. If so, why do you need generic types?

[yjbanov]

To clarify, by "not needing generic types" I mean that Map (i.e. Map<dynamic, dynamic) should work just fine (this should also be true for change detection inside ng-for, where List<dynamic> should suffice for keeping snapshots of previous values).

[leonsenft]

@yjbanov Typing isn't about performance, but rather making templates sound. Left as they are, they'll cease to work in Dart 2. Any concern over performance is in regards to the solution, since some have performance implications. For example we can always fallback to local inference in the absence of knowing the type annotation to generate (solution 3.), but this would seriously bloat the generated code.

[yjbanov]

Gotcha. If I may throw in an option, maybe another take on inference can work:

8. Inferred collection type

class ViewAppComponent0 {
  // Compiler already supports map literals natively, so
  // it can create specialized code for maps.
  Map<String, String> _map_0 = {};
  ...

  ComponentRef<AppComponent> build() {
    // no extra code here
  }

  void detectChangesInternal() {
    ...
    _map_0['color'] = _ctx.color;
    _map_0['background'] = _ctx.background;
    if (!compareMaps(_map_0, _NgStyle_0_4.rawStyle)) {
      _NgStyle_0_4.rawStyle = _map_0;
      _map_0 = {};
    }
    ...
  }
}

[leonsenft]

The issue is we don't always have a way of easily determining the type of the map. Your solution is effectively equivalent to 6 in terms of the analysis required on the compiler's behalf.

[matanlurey]

Worth noting, this is a test case that fails in DDC with errors enabled:

https://github.com/dart-lang/angular/blob/f55fea736cb25040be640e75541acfbcd34e0329/_tests/test/common/directives/ng_style_test.dart#L15-L20

... I'll skip: ... it for now.

[leonsenft]

We plan to move forward with a partial implementation of option 6.

The type of the input to which a collection literal is bound will be used to type annotate the generated code. This has two important implications that may be breaking changes for users.

1. The use of collection literals in templates where the collection is not the root expression of the binding will no longer be officially supported. This will be a breaking change in Dart 2 when the type cast warning becomes an error.

   ✅ For example, this will still be allowed. ✅

   <div [ngStyle]="{'color': divColor}">

   🚫 These will break. 🚫

   <div [ngStyle]="filterStyles({'color': divColor})">

   <input (ngModelChange)="handleEvents([$event])" />

   Since we know this will break, we should probably warn users about this pattern when we generate code for it, if not just disallow it entirely at compile-time.

2. Any input to which a collection is bound can't contain a private type. This is because we need to use this type to annotate the generated code.

   For example, the following code will no longer be allowed.

   @Component(
     selector: 'has-collection',
     template: '',
   )
   class HasCollectionWithPrivateType {
     @Input()
     List<_Item> items;
   }
   
   @Component(
     selector: 'binds-collection',
     template: '<has-collection [items]="[item]"></has-collection>',
     directives: const [HasCollectionWithPrivateType],
   )
   class BindsCollectionWithPrivateType {
     _Item item;
   }

   This would cause the compiler to use the _Item type in the generated code which would not be permitted since it's private.

The reasoning for this decision is based on two factors.

1. Any solution that retained full support for collection literals was either infeasible or costly to implement (full inference), or we weren't happy with the performance drawbacks (in-lining all the change detection code).

2. We found collection literals in templates to be quite rare, with only 2.5% of cases where the collection was not the root of the expression.

If anyone anticipates this to be a large setback for them, please let us know here or file another issue requesting full support be reinstated through other means.

[fmatuszewski]

I still have this issue in angular 5.0.0
Dart VM version: 2.1.0-dev.0.0 (Thu Aug 9 10:17:40 2018 +0200) on "windows_x64"

EXCEPTION: Type 'IdentityMap<String, dynamic>' should be 'IdentityMap<String, String>' to implement expected type 'Map<String, String>'.
STACKTRACE:
packages/$sdk/dev_compiler/amd/dart_sdk.js 4834:29 throw
packages/$sdk/dev_compiler/amd/dart_sdk.js 4461:15 castError
packages/$sdk/dev_compiler/amd/dart_sdk.js 4750:17 as
packages/$sdk/dev_compiler/amd/dart_sdk.js 3988:19 check_C
packages/platnosci/platnosci.template.ddc.js 2430:103 detectChangesInternal
packages/angular/src/bootstrap/modules.ddc.js 1721:16 detectCrash
packages/angular/src/bootstrap/modules.ddc.js 1709:16 detectChanges
packages/angular/src/bootstrap/modules.ddc.js 873:32 detectChangesInNestedViews
packages/platnosci/platnosci.template.ddc.js 2163:23 detectChangesInternal
packages/angular/src/bootstrap/modules.ddc.js 1721:16 detectCrash
packages/angular/src/bootstrap/modules.ddc.js 1709:16 detectChanges
packages/angular/src/bootstrap/modules.ddc.js 873:32 detectChangesInNestedViews
packages/platnosci/platnosci.template.ddc.js 1835:23 detectChangesInternal
packages/angular/src/bootstrap/modules.ddc.js 1721:16 detectCrash
packages/angular/src/bootstrap/modules.ddc.js 1709:16 detectChanges
packages/wnioski/src/routes.ddc.js 297:25 detectChangesInternal
packages/angular/src/bootstrap/modules.ddc.js 1721:16 detectCrash
packages/angular/src/bootstrap/modules.ddc.js 1709:16 detectChanges
packages/wnioski/src/routes.ddc.js 1447:25 detectChangesIntern

[matanlurey]

You will need to file another bug/repro case, it's not possible for us to monitor closed bugs to help.

[leonsenft]

@fmatuszewski Yes, please file another issue, and include the template HTML where this error is occurring; it's not possible for us to tell what's causing this issue without seeing the source.