Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

VM does not support deferred libraries in Dart2 #33118

Closed
matanlurey opened this issue May 14, 2018 · 30 comments
Closed

VM does not support deferred libraries in Dart2 #33118

matanlurey opened this issue May 14, 2018 · 30 comments

Comments

@matanlurey
Copy link
Contributor

@matanlurey matanlurey commented May 14, 2018

I ran into this while debugging test failures internally that expect this functionality.

Here is a simple reproduction case:

// deferred_lib.dart
final aNumber = 1234;
// deferred_lib_test.dart
import 'package:test/test.dart';

import 'deferred_lib.dart' deferred as deferred_lib;

void main() {
  test('should throw on access to defererd library', () async {
    // In Dart1 this test passes as-is.
    // With --preview-dart-2 + Dart VM version: 2.0.0-dev.55.0 this fails:
    //
    //   Expected: throws anything
    //     Actual: <Closure: () => int>
    //     Which: returned <1234>
    expect(() => deferred_lib.aNumber, throwsA(anything));
    await deferred_lib.loadLibrary();
    expect(deferred_lib.aNumber, 1234);
  });
}

I realize there might not be a convincing reason to "really" support deferred loading in the command-line VM in Dart 2 (since it isn't really supportable in Flutter, and I don't think anyone else downloads Dart over the network i.e. Dartium-style).

@srawlins pointed me to:

... which makes me think that this was (intentionally?) never implemented.

/cc @keertip as this is blocking --preview-dart-2 internally, unless I disable the test.

@matanlurey
Copy link
Contributor Author

@matanlurey matanlurey commented May 14, 2018

... it turns out this code (and test) is unused, so I can safely delete it. This is NOT a P1.

We should clarify whether or not deferred loading is supported though, and hopefully fail better.

Loading

@keertip
Copy link
Contributor

@keertip keertip commented May 14, 2018

/cc @a-siva , could you clarify?

Loading

@a-siva
Copy link
Contributor

@a-siva a-siva commented May 15, 2018

Currently the front end loads all libraries eagerly and hence the first check does not throw. It is not clear if there are plans for the front end to support deferred libraries, maybe @kmillikin could comment on that.
I agree that the front end should produce a better error message and fail without executing any of the code if we choose to not support deferred libraries.

Loading

@matanlurey
Copy link
Contributor Author

@matanlurey matanlurey commented May 15, 2018

Another option is what DDC does, which is eagerly load everything, but prevent access to the deferred library until .loadLibrary() is called. This gives you the option of implementing it in the future without changing the semantics.

/cc @vsmenon

Loading

@a-siva
Copy link
Contributor

@a-siva a-siva commented May 15, 2018

This is how the Dart1 VM worked, does DDK support this behavior because it would require support from the FE for that too.

Loading

@a-siva
Copy link
Contributor

@a-siva a-siva commented May 16, 2018

@kmillikin will update the issue with details on the kind of support provided in kernel for recognizing libraries that are deferred loaded, the VM can then flag these libraries appropriately and mark the libraries as loaded only after LoadLibrary succeeds.

Loading

@dgrove
Copy link
Member

@dgrove dgrove commented Jun 14, 2018

I don't believe this needs to be fixed for Dart2Stable. @a-siva @matanlurey what are your thoughts?

Loading

@matanlurey
Copy link
Contributor Author

@matanlurey matanlurey commented Jun 14, 2018

If deferred loading no longer works in the Dart VM, I think just documentation as such is probably fine:

Deferred loading is currently only enabled for Dart4Web products (DDC, Dart2JS).

When used in the standalone VM, calling loadLibrary() is optional and has no effect on the runtime (the code is not actually deferred loading). We may explore re-adding this functionality in the future.

... and enough to close this issue. Thoughts?

Loading

@matanlurey
Copy link
Contributor Author

@matanlurey matanlurey commented Jun 14, 2018

The other option is the VM failing to compile or issuing a warning is deferred loading is used. I'd prefer that, but I'd understand if its not feasible.

Loading

@dgrove
Copy link
Member

@dgrove dgrove commented Jun 14, 2018

Let's go with documentation for Dart2Stable. /cc @kwalrath Can you add this documentation? Once that's done, we'll move the actual changes to the VM out of Dart2Stable.

Loading

@sigmundch
Copy link
Member

@sigmundch sigmundch commented Jun 15, 2018

To be honest, I'd prefer that we don't go the documentation route: we want consistent semantics, and doing what the Dart1VM seems totally reasonable to me.

I thought that the deferred loading support we added to the FE for dart2js would be practically the same than what the VM needs to implement its checks. In particular, the FE generates expressions for loadLibrary and checkLibraryIsLoaded whenever the code accesses a deferred element. It also handles tearoffs of the loadLibrary method automatically.

@a-siva @kmillikin - is there something else missing that you may need for the VM?

Loading

@kwalrath
Copy link
Member

@kwalrath kwalrath commented Jun 19, 2018

@dgrove & @sigmundch: Should I document this or not?

Loading

@sigmundch
Copy link
Member

@sigmundch sigmundch commented Jun 19, 2018

My preference would be not to document. That being said, this really depends on the remaining work needed to fill the gap in the VM, so I'd wait to hear from @a-siva @kmillikin first before we decide.

@dgrove - do you agree?

Loading

@a-siva
Copy link
Contributor

@a-siva a-siva commented Jun 21, 2018

is this feature required for Dart2 stable?

Loading

@dgrove
Copy link
Member

@dgrove dgrove commented Jun 21, 2018

Loading

@sigmundch
Copy link
Member

@sigmundch sigmundch commented Jun 21, 2018

We discussed this at todays's scrum meeting and decided to do do the following:

  • add the documentation for now to unblock Dart2Stable (@kwalrath I filed #33561 to track that separately)
  • investigate whether the fix in the VM is simple, and if so fix when it is convenient, but this is no longer blocking Dart2Stable

Loading

@sigmundch sigmundch removed this from the Dart2Stable milestone Jun 21, 2018
@sigmundch sigmundch added this to the Dart2.1 milestone Jun 21, 2018
@natebosch
Copy link
Member

@natebosch natebosch commented Jun 21, 2018

if so fix when it is convenient, but this is no longer blocking Dart2Stable

I'm guessing that the fix will mean code that runs in the VM today will stop working after the fix? Will we consider that non-breaking even though it breaks people?

Loading

@matanlurey
Copy link
Contributor Author

@matanlurey matanlurey commented Jun 21, 2018

I'm guessing that the fix will mean code that runs in the VM today will stop working after the fix?

Yes, you can "silently" start using deferred libraries without using .loadLibrary().

Loading

@sigmundch
Copy link
Member

@sigmundch sigmundch commented Jun 21, 2018

correct - that's the main reason I hope the fix is easy and we can avoid the breaking-change. There is a general belief that it is unlikely because almost all users of deferred loading are on the web only.

Loading

@kwalrath
Copy link
Member

@kwalrath kwalrath commented Jun 21, 2018

It sounds like I should say that there's currently a difference but say "don't depend on it" because it's likely to change. And I can point to this bug for details.

Loading

@kwalrath
Copy link
Member

@kwalrath kwalrath commented Jun 22, 2018

Loading

@bkonyi
Copy link
Contributor

@bkonyi bkonyi commented Aug 20, 2018

I'm wondering if this is worth spending cycles on considering Flutter doesn't support deferred libraries and the Dart 1 VM and DDC only "fake" support deferred loading. It makes sense for DDC to do this since dart2js does actually support deferred loading, but I'd argue that treating the deferred keyword and loadLibrary() calls as noops in the VM is sufficient. Thoughts?

Loading

@matanlurey
Copy link
Contributor Author

@matanlurey matanlurey commented Aug 20, 2018

I happen to agree with @bkonyi. The only potential issue is folks writing cross-platform libraries where the implementation of .loadLibrary() varies depending on whether it is running in the web or the VM. Perhaps documentation is enough here, though.

Loading

@bkonyi
Copy link
Contributor

@bkonyi bkonyi commented Aug 23, 2018

Anyone else have an opinion on this? I'll mention @sigmundch explicitly since he had an opinion on this before.

Loading

@matanlurey
Copy link
Contributor Author

@matanlurey matanlurey commented Aug 23, 2018

Maybe reach out to Fuischa potentially or other internal VM partners @bkonyi?

(I don't know if they have any plans to do ephemeral loading in this fashion or not)

Loading

@dgrove
Copy link
Member

@dgrove dgrove commented Aug 24, 2018

@sigmundch is out of the office for quite awhile - we'll need to make the decision without him.

Loading

@bkonyi
Copy link
Contributor

@bkonyi bkonyi commented Sep 4, 2018

@zanderso @rmacnak-google do either of you happen to know if Fuchsia is planning on using deferred loading?

Loading

@bkonyi
Copy link
Contributor

@bkonyi bkonyi commented Sep 4, 2018

Discussed with @zanderso + @rmacnak-google offline and it doesn't sound like Fuchsia currently has any plans to use deferred loading and they seem to be on board with treating deferred loading operations as noops in the VM (feel free to correct me if I'm wrong here). I'm going to remove the Dart 2.1 milestone for this issue as this is low priority without any customers currently needing this in the VM.

Loading

@bkonyi bkonyi removed this from the Dart2.1 milestone Sep 4, 2018
@bkonyi bkonyi added P3 and removed P2 labels Sep 4, 2018
@kwalrath
Copy link
Member

@kwalrath kwalrath commented Sep 4, 2018

I'll update the note at https://www.dartlang.org/guides/language/language-tour#deferred-loading to not say that we expect this bug to be fixed soon.

Loading

@rmacnak-google
Copy link
Contributor

@rmacnak-google rmacnak-google commented Sep 14, 2020

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
9 participants