Sdk: inferred types not treated the same as explicit type by deferred loading

[sigmundch]

According to the spec it is an error to use a deferred type in an expression or type: you can't use them in variable or function declarations, is/as/catch clauses, and cannot use them in generic type arguments of any form.

This is checked and reported as an error for explicit types in dartanalyzer and CFE, but it is today ignored for inferred types. Unfortunately there is code out there where this occurs. For example, some applications show cases like these:

import 'b.dart' deferred as d;
main() {
   ...
   // inferred return-type in closures:
   d.B f1() => new d.B(); // detected as an error today
   var f2 = () => new d.B(); // no error, but f1 has inferred type: () -> d.B

   // inferred type-arguments
   d.list = <d.B>[]; // detected as an error today, can't use the type parameter
   d.list = []; // no error, but type parameter was injected here
   d.list = d.eList.map((x) => x.bValue).toList(); // no error, type parameter inferred on closure and map<T>.
}

When dart2js splits a program with these types, it ignores the error cases and allows those deferred types to be deferred. This is unsound in the presence of reified types.

To address performance problems in our current implementation of deferred loading, we are revisiting the algorithm and our changes are likely going to affect this behavior.

4 options so far:

1. Make this a proper error: change the CFE/analyzer, announce a breaking change, and ignore the issue in dart2js.
2. Make deferred loading more flexible: the errors above exist because dart2js represents types and classes as one thing. If we split the runtime type representation in dart2js, we could remove this limitation of deferred loading.
3. Make this a warning and do a sound code split by assigning more types to the main output unit.
4. Hide the error and try to mimic the current unsound behavior (either by ignoring certain cases or by using the "any" type we have for js-interop)

(2) is our long term goal, but it will take time (at least a few months), (1) seems unfortunate if we are aiming to do (2). (4) could work because today's behavior is working for our users, however we are not sure if it's feasible to do after we change the algorithm implementation (in particular, some patterns we want to ignore are hard to distinguish from problematic patterns like #33046).

[sigmundch]

My proposal right now is to go with (3)

Turns out that there was also a bug in the old implementation that prevented constructor elements from being treated as deferred when accessed directly via a deferred import.

Going with a sound approach that fixes this bug seems to have a positive effect on code-split for some large customer apps (0.5% better!). Using an unsound approach, for example ignoring return types in closures, improves the result in that app by maybe 0.05%, so it doesn't appear to be worthwhile.

[sigmundch]

fixed by 70e1517d985e234ffcb1df637dc650eb1cd1386c

[leonsenft]

@matanlurey @ferhatb

I'm happy to see that (2) is the long term goal. To confirm, this would allow using a deferred type in a type signature, before the deferred library is used correct?

import 'foo.dart' deferred as foo;

class FooLoader {
  Future<foo.Foo> load() async {
    await foo.loadLibrary();
    return foo.Foo();
  }
}

void main() await {
  final loader = FooLoader();
  final foo = await loader.load();
  foo.bar(); // Invoke methods on `Foo` (not dynamic dispatch).
}

I can't find the old issue, but this is aligned with the Angular team's requests for deferred loading and would make the feature much more ergonomic.

I understand the desire to fix the soundness issues with the temporary approach, but our team has some concerns with the implementation (enabled by --new-deferred-split and checked by --report-invalid-deferred-types).

My understanding is that even if the closure is assigned to a type which disregards the return value, the inferred type still can't contain a deferred type because the closure is reified? This will be an ergonomic problem for Angular as a common pattern for defining deferred routes is

import 'my_component.template.dart' deferred as my_component;

RouteDefinition(
  loader: () async {
    await my_component.loadLibrary();
    return my_component.MyComponentNgFactory;
  },
)

where MyComponentNgFactory has type ComponentFactory<MyComponent>.

Is there anything we can do as the API authors to alleviate our users from having to insert an as dynamic cast on the return? The loader property already disregards the ComponentFactory type argument.

Furthermore, I tried the suggested solution of returning dynamic

return my_component.MyComponentNgFactory as dynamic

and it actually increased code size in our example app.

However, I figured rather than cast as dynamic, it may be better to cast as a non-deferred supertype

// ignore: unnecessary_cast
return my_component.MyComponentNgFactory as ComponentFactory; // implicitly a ComponentFactory<dynamic> now.

Fortunately this improves (reduces) code size! Unfortunately the analyzer treats this as an unnecessary cast, which is a build error for our clients, making it necessary to also add the // ignore comment.

[sigmundch]

Thanks @leonsenft - we were also spending some time looking at the different use cases and are still debating what the right course should be. More and more I'm inclined to wait until we have (2) before we make the new algorithm the default and remove the soundness bug, but if by construction angular is setup to avoid this pitfall, we may be in a position to do the switch sooner.

The most common place where I see this come up is the exact pattern that you showed above.

To confirm, this would allow using a deferred type in a type signature, before the deferred library is used correct?

Correct once (2) is done, you'll be able to use deferred types explicitly or implicitly in variable and function declarations, even before the type is loaded. We may also reconsider to allow them in is and as expressions.

I tried the suggested solution of returning dynamic ... and it actually increased code size in our example app.

Yikes, I honestly thought that using as dynamic would have no impact on code-size, so I appreciate you letting us know that you have seen an effect from it. We should look more closely to ensure we aren't missing something important.

Is there anything we can do as the API authors to alleviate our users from having to insert an as dynamic cast on the return? The loader property already disregards the ComponentFactory type argument.

The main thing to prevent inference from picking up the deferred type. One option here is changing the angular code generator to use a weaker type for the factory getter. So instead of:

ComponentFactory<MyComponent> get MyComponentNgFactory => ...

Would it work to remove the type parameter?

ComponentFactory get MyComponentNgFactory => ...

[leonsenft]

Yikes, I honestly thought that using as dynamic would have no impact on code-size, so I appreciate you letting us know that you have seen an effect from it. We should look more closely to ensure we aren't missing something important.

To be fair, it was a rather negligible code size increase, but I'm happy to see the code decrease from using a non-deferred supertype.

Would it work to remove the type parameter?

Unfortunately no. While the type argument doesn't matter for loading a route, it does matter for runApp and imperative component loading.

[leonsenft]

We revisited removing exports from our generated code and hit this issue again (we forgot about it).

@sigmundch I believe dart2js shipped a whole new type system since we lasted discussed this. Any chance this is addressable now?

[vsmenon]

@rakudrama re new rti impact on this.

[sigmundch]

@leonsenft - correct! This is now unblocked with the new-rti. We are still wrapping up several aspects of null-safety in dart2js, but after that, @joshualitt is planning on taking a look at this.

[matanlurey]

Just checking in, do we expect to potentially try to tackle/investigate this in the next few weeks?

[sigmundch]

Not quite yet - we still have a few more NNBD tasks left (including our test migration work).

[matanlurey]

Hi @sigmundch, checking in again.

We've been talking about @jakemac53 for our own null-safety migration, and one thing that came up is we might be able to _always_ emit null-safe annotated code, relying on the mixed-mode runtime to "do the right thing" (if a user is not migrated, they will largely ignore our annotations, and if they are, they will be enforced to some extent).

This probably reduces the amount of work we need to do possibly by 1/3 or more, because we would only need to focus on emitting null-safe code, and not forking 35K+ lines of our compiler, or adding lots of if statements. It doesn't need to be done _now_, but it could be a huge benefit towards our overall strategy.

... however, that whole strategy is blocked because we currently export{user_code}.dart` in our generated code, and we can't remove that feature because of this very issue.

[sigmundch]

@matanlurey thanks for checking in.

@joshualitt is actively working on this. woohoo!

Large part of the algorithm is in progress (see cl/154580). There is a bit more polish work needed and some validation to ensure that we are properly slicing the code, so we plan to start with this under a flag for a short period before we enable it internally.

I didn't fully grasp how reexports are at play with this problem, though. I'm happy to chat offline if you think it could help explore other ideas that may let you avoid being blocked on this issue.

[matanlurey]

@sigmundch I can try and create an external reproduction case for your team. I'll reach out again shortly.

[sigmundch]

@matanlurey feel free to wait on it, though. I forgot about the internal bug, so I can dive in more into it there if needed.

[matanlurey]

@sigmundch I am going to follow-up first, since your team has made a lot of RTI changes since.

[leonsenft]

@joshualitt Can this be closed now? At least the code size aspect of this issue has been resolved (not sure if there are other, unresolved aspects of it).

[joshualitt]

Yes, I think this can be closed. Thanks!