Navigation Menu

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

Jump to bottom

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
Labels
area-vm Use area-vm for VM related issues, including code coverage, FFI, and the AOT and JIT backends. dart-1-removal P3 A lower priority bug or feature request

Comments

@matanlurey
Copy link
Contributor

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

... 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.

@keertip
Copy link
Contributor

keertip commented May 14, 2018

/cc @a-siva , could you clarify?

@a-siva
Copy link
Contributor

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.

@matanlurey
Copy link
Contributor Author

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

@a-siva
Copy link
Contributor

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.

@dgrove dgrove added this to the Dart2Stable milestone May 15, 2018
@matanlurey matanlurey added P2 A bug or feature request we're likely to work on area-vm Use area-vm for VM related issues, including code coverage, FFI, and the AOT and JIT backends. dart-1-removal labels May 15, 2018
@a-siva
Copy link
Contributor

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.

@dgrove
Copy link
Contributor

dgrove commented Jun 14, 2018

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

@matanlurey
Copy link
Contributor Author

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?

@matanlurey
Copy link
Contributor Author

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.

@dgrove
Copy link
Contributor

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.

@sigmundch
Copy link
Member

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?

@kwalrath
Copy link
Contributor

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

@sigmundch
Copy link
Member

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?

@a-siva
Copy link
Contributor

a-siva commented Jun 21, 2018

is this feature required for Dart2 stable?

@dgrove
Copy link
Contributor

dgrove commented Jun 21, 2018 via email

@sigmundch sigmundch mentioned this issue Jun 21, 2018
Closed
@sigmundch
Copy link
Member

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

@sigmundch sigmundch modified the milestones: Dart2Stable, Dart2.1 Jun 21, 2018
@natebosch
Copy link
Member

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?

@matanlurey
Copy link
Contributor Author

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().

@sigmundch
Copy link
Member

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.

@kwalrath
Copy link
Contributor

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.

@kwalrath
Copy link
Contributor

PR: dart-lang/site-www#955

@bkonyi
Copy link
Contributor

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?

@matanlurey
Copy link
Contributor Author

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.

@bkonyi bkonyi mentioned this issue Aug 20, 2018
Closed
@bkonyi
Copy link
Contributor

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.

@matanlurey
Copy link
Contributor Author

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)

@dgrove
Copy link
Contributor

dgrove commented Aug 24, 2018

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

@bkonyi
Copy link
Contributor

bkonyi commented Sep 4, 2018

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

@bkonyi
Copy link
Contributor

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.

@bkonyi bkonyi removed this from the Dart2.1 milestone Sep 4, 2018
@bkonyi bkonyi added P3 A lower priority bug or feature request and removed P2 A bug or feature request we're likely to work on labels Sep 4, 2018
@kwalrath
Copy link
Contributor

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.

@kwalrath kwalrath mentioned this issue Sep 4, 2018
Merged
@a-siva a-siva mentioned this issue Mar 1, 2019
Closed
@bkonyi bkonyi removed their assignment Mar 13, 2019
@eernstg eernstg mentioned this issue Mar 25, 2019
Closed
@rmacnak-google
Copy link
Contributor

62893f9

@elliette elliette mentioned this issue May 12, 2022
Closed
@a-siva a-siva mentioned this issue Aug 10, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
area-vm Use area-vm for VM related issues, including code coverage, FFI, and the AOT and JIT backends. dart-1-removal P3 A lower priority bug or feature request
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
9 participants
@matanlurey @sigmundch @dgrove @kwalrath @keertip @natebosch @rmacnak-google @a-siva @bkonyi