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 and Flutter CFE targets should only support strong mode #34468

Closed
mraleph opened this issue Sep 14, 2018 · 17 comments

Comments

Projects
None yet
6 participants
@mraleph
Copy link
Contributor

commented Sep 14, 2018

It turns out we still sometimes use legacy mode - which is unexpected.

We should add errors when trying to use VmTarget with strongMode set to false.

/cc @alexmarkov @a-siva

@mraleph mraleph added the area-vm label Sep 14, 2018

@mraleph mraleph added this to the Dart2.1 milestone Sep 14, 2018

dart-bot pushed a commit that referenced this issue Sep 15, 2018

[vm/kernel] In async transformation check if strongMode is on.
It seems we are still using non-strong mode targets in few
places (e.g. when training front-end server we by accident
train it in legacy mode).

A separate bug is filed to clean that up #34468

For now to unbreak the build we simply check if we are in the
legacy mode and then avoid using getStaticType in async
transformation.

This is followup to eec96f9.

Bug: #34463
Change-Id: Ib693fddfb9abbf89599ae646cb408d4a9c93f1b6
Reviewed-on: https://dart-review.googlesource.com/75061
Commit-Queue: Vyacheslav Egorov <vegorov@google.com>
Reviewed-by: Alexander Markov <alexmarkov@google.com>
@mit-mit

This comment has been minimized.

Copy link
Member

commented Sep 21, 2018

dart seems to also still accept the old --checked flag

@mit-mit mit-mit referenced this issue Sep 21, 2018

Closed

Changes for Dart 2.1 #1164

9 of 9 tasks complete
@dgrove

This comment has been minimized.

Copy link
Member

commented Oct 1, 2018

Any further updates, @a-siva @mraleph ?

@a-siva

This comment has been minimized.

Copy link
Contributor

commented Oct 1, 2018

Yes we still accept '--checked' flag. This essentially equivalent to --enable-asserts.

Did we make a decision on whether we want to deprecate the '--checked' flag ?

@mit-mit

This comment has been minimized.

Copy link
Member

commented Oct 3, 2018

This is being discussed over in #32442

@mit-mit

This comment has been minimized.

Copy link
Member

commented Oct 3, 2018

Opened #34660 to track removing --checked from the VM. With that @mraleph I think we can close this issue as the other cleanup was completed, right?

@mraleph

This comment has been minimized.

Copy link
Contributor Author

commented Oct 3, 2018

This bug is actually about strongMode flag that is used pervasively through front-end and parts of the VM code.

VM also has a FLAG_strong which also needs to be removed.

@mraleph mraleph removed their assignment Oct 3, 2018

@mit-mit

This comment has been minimized.

Copy link
Member

commented Oct 3, 2018

OK, who's removing that?

@kmillikin

This comment has been minimized.

Copy link
Member

commented Oct 4, 2018

We will remove it from the CFE.

@a-siva

This comment has been minimized.

Copy link
Contributor

commented Oct 9, 2018

This CL https://dart-review.googlesource.com/c/sdk/+/78660 to remove the non strong version of vm_platform is blocked on some front end work.

@dgrove

This comment has been minimized.

Copy link
Member

commented Oct 11, 2018

@peter-ahe-google

This comment has been minimized.

Copy link
Contributor

commented Oct 11, 2018

I suggest that we push this to the next release. I'm actively working on this, but there's a lot of places that are still (mostly likely unintentionally) relying on strongMode = false. This is my plan:

  1. Convert all strongMode flags in package:kernel and package:front_end to legacyMode.
  2. When I don't see legacyMode: true, I'll completely remove the flag.
  3. Rename "vm_platform_strong.dill" to "vm_platform.dill" (I think this can be done in parallel).
  4. Audit all the places that use legacyMode: true and either convert them to Dart 2, or delete them (I can ask for help with this).

It's hard form me to estimate the amount of work required before I have a burn-down list. That's because strongMode: false is the default in most cases, and not explicit in code. This is why I'm converting all strongMode options to explicitly named arguments so I can search for legacyMode: true.

@dgrove

This comment has been minimized.

Copy link
Member

commented Oct 11, 2018

@a-siva

This comment has been minimized.

Copy link
Contributor

commented Oct 11, 2018

I guess I am fine with some internal tests using the legacy mode but we need to make sure that it is not possible for command line Dart users or Flutter/Fuchsia to run the VM in non strong mode.
We are in the process of creating a CL that will remove the following flags from the VM
--strong
--reify_generic_functions
--error_on_bad_override
--enable_type_checks
--error_on_bad_type

@dgrove dgrove modified the milestones: Dart2.1, Dart2.2 Oct 11, 2018

@peter-ahe-google

This comment has been minimized.

Copy link
Contributor

commented Oct 12, 2018

I've created a CL that removes the non-strong VM platform.dill files from the SDK. This should help prevent command-line users from running the VM in non-strong mode.

@peter-ahe-google

This comment has been minimized.

Copy link
Contributor

commented Oct 12, 2018

@a-siva

This comment has been minimized.

Copy link
Contributor

commented Nov 19, 2018

When https://dart-review.googlesource.com/c/sdk/+/79431 lands there will be no way to run the VM in non strong mode.

@a-siva

This comment has been minimized.

Copy link
Contributor

commented Dec 10, 2018

The VM and Flutter only run in strong mode now, it is not possible to use non strong mode.

@a-siva a-siva closed this Dec 10, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.