Resolve / announce breaking change on const X.fromEnvironment in VM

[vsmenon]

Currently, with the constant-update-2018 change, the VM no longer supports (or plans to support) const X.fromEnvironment for values defined at runtime in JIT mode.

That would make it consistent (?) with the VM in AOT mode, Dart2JS, and DDC.

If we're agreed on this behavior going forward, we need a breaking change announcement.

@a-siva @leafpetersen @lrhn @kmillikin @sigmundch @aadilmaan

[vsmenon]

Marking as P0 as this is a blocker for 2.3.

[vsmenon]

fyi - @mit-mit on breaking change.

Some more context:

• Spread / control flow depend upon the constant-updates / new CFE constant impl change.
• Per Siva, it'll take the VM team a non-trivial amount of time to implement the old const X.fromEnvironment semantics for user-defined values for VM-JIT - delaying all the above features past our window for 2.3.
• There is some but relatively little usage for const X.fromEnvironment for user-defined values. Neither AOT nor the web build tools support it, so it's limited to tools / scripts running on the JIT.
• Internally (see b/129881700), Siggi has a CL to replace all uses for this (VM/JIT) with Platform.environment instead.

Based on this, I think we should roll forward with the change.

[kevmoo]

Ugh. That's nasty. Let's get the email out ASAP.

This may affect SASS IIRC – @nex3

[mit-mit]

cc @aadilmaan for breaking change management

[davidmorgan]

One thought: internally we will handle this in three steps: 1) produce kernel with defaults hardcoded for these consts; 2) ban -D flags everywhere, so we know the defaults are correct; 3) reimplement anything that needs it using Platform.environment. Note that #3 only touches places where they are used, declarations of such consts that are not used in practice don't need fixing.

A way to provide an equivalent path externally would be to make the CFE always hardcode defaults and make the VM refuse to start up with -D flags. i.e., remove the feature entirely in 2.3, rather than leaving it in a broken state.

[askeksa-google]

Clarification: const X.fromEnvironment works exactly as before when running from Dart source.

It is only the scenario where a Dart program is compiled to a dill file (without specifying an environment) and that dill file is run on the VM (specifying a runtime environment that is supposed to affect the environment constants) which does not work anymore. I would expect that very few external users do this.

[davidmorgan]

Yes. I think the key question is whether any of the build tools we publish (e.g. package:build) use dill files in a way which will be affected.

[vsmenon]

@jakemac53 @natebosch on the build tools question.

[vsmenon]

@a-siva - thoughts on locking down -D? Perhaps it makes sense for the VM to refuse -D arguments if running from dill files instead of source?

[jakemac53]

We have never supported passing any custom arguments to the modular compilers, so neither DDC nor Kernel support this externally (via package:build at least).

We could in theory support it but only as a global option (the only way it makes sense to use it), and we don't have the ability to make global-only options, although we could add that in the future. This would basically be equivalent to supporting it through some --define bazel argument.

However, I don't think that actually produces a desirable workflow (it would invalidate all modular dart actions in both cases), and I don't think its worth supporting without really compelling use cases.

[askeksa-google]

It still makes sense for the VM to accept -D when running from dill, as the -D options also affect the runtime environment, accessible through new X.fromEnvironment.

[vsmenon]

@askeksa-google - the concern (see discussion in b/129881700) is that, when running from dill, users will now see a different value depending on whether they use new or const.

So, either we should explain that very clearly or disallow it.

Because of that difference, the internal code is being migrated to use Platform.environment instead.

[lrhn]

I'm not sure having two different environments makes sense. I'd prefer if the environment is locked down at one particular point in time, usually compilation time, and after that you cannot change it. The .dill file should contain the entire environment so that new String.fromEnvironment gives the same result as const String.fromEnvironment. So, I'd say it's a good idea to not allow -D in the VM when running from .dill file, and make sure that the defines used when creating the .dill file are still available (if there is any access to new ...fromEnvironment at all, otherwise it can be garbage collected/tree shaken).

[davidmorgan]

@jakemac53 The question is not whether we have allowed people to pass -D, it's whether external users are using the CFE in a way that, with 2.3, will cause 'unknown constants' to be emitted in the dill. These will then cause the VM to crash. Or, if we work around it by hardcoding to default, it will cause the VM to use incorrect values.

[sigmundch]

To summarize where we are:

• We have a lot of inconsistencies across our tools, so it might really help to clearly specify fromEnvironment for the future (filed this language issue)

• Running from sources continue to work as before (the most common use case)

• Configurations that didn't allow a runtime environment (e.g. AOT snapshots) also continue to work

• environment variables will break whenever compilation and running is split. Only known cases is modular build systems (e.g. build_runner), but technically other tools that split build and running could be affected:

  • @jonasfj - pub global activate creates snapshots ahead of time, do you however allow to pass VM environment variables, if not, pub-run should not be affected?

• what can be done to avoid breaking these?

  • support unevaluated constants in the VM so we can use the runtime environment again
  • write a link-tool that adds the environment and finishes const evaluation
    • this tool can help, but wont fix the breaking change transparently: eventually users need to take action. For example pub run can invoke the linker, but build_runner provides the bytes but doesn't execute them, so it requires user's to update their build steps.
    • to be fully transparent, the linking would be done within the VM (e.g. in a service similar to the CFE).
  • modular tools can switch to run from sources whenever environment flags are provided. This preserves functionality but will regress in performance.

@kevmoo - what's your take? The general consensus seems to be that the breaking change here has low impact.

[nex3]

As long as Dart Sass can specify the constant at either snapshot-generation time or runtime, our use-case is satisfied.

[aadilmaan]

@Hixie @matanlurey for approval for this breaking change request.

[matanlurey]

I'm actually going to defer to @srawlins for google3, as I don't know think any of our tools interact with this API. My assumption is LGTM if Sam and/or @keertip are already are already for it.

[matanlurey]

That being said, I'd love to see this be an explicit error (or stop being a const API) instead of just silently not working or something like that. It's not clear what the plan is here.

[sigmundch]

@matanlurey - see also more details about internal uses are in b/129881700

[matanlurey]

I'm satisfied with the process taking place there, thanks!

[jonasfj]

A quick look through pub global activate suggests that it doesn't allow snapshotting with dart -D. It doesn't seems like any variant of pub [global] run supports passing these either.

Is there any documentation of -D? are there other ways to set this environment configuration?

[sigmundch]

@jonasfj - thanks for checking. The only way to define these constants is via the -D flag. If there is now way to provide VM flags when running pub [global] run, then there should be no problems there.

@a-siva - I believe this doesn't affect downstream users in flutter/fuschia, but please confirm when you have details.

Assuming there are no surprises I also vote in favor of this change (voting in lieu of @dgrove who is OOO).

@mit-mit, here as well, since @dgrove is OOO, could you
in addition provide your opinion or suggest anyone else that should approve this?

[Hixie]

I don't understand what is being proposed here. const X.fromEnvironment() works in AOT mode as far as I can tell. Why would it not work if the code was compiled in JIT mode?

[Hixie]

by "works" I mean in Flutter, specifically, the kReleaseMode constant seems to work, and is defined as:

const bool kReleaseMode = bool.fromEnvironment('dart.vm.product', defaultValue: false);

[Hixie]

We also use Platform.environment.containsKey('FLUTTER_TEST'). Performance doesn't much matter for that case, but I suppose it'd be nice if we could make that be based on const too, and that would be relevant in the JIT mode if I'm not mistaken.

[matanlurey]

@Hixie dart.vm.product is not a user-defined value (it comes from the embedder/compiler). What the Dart team is changing is being able to, for example, write dart -Dflag=1 main.dart and then:

void main() {
  const int.fromEnvironment('flag');
}

[askeksa-google]

Correction: const X.fromEnvironment still works when running directly from Dart source (as in @matanlurey's example).

The changes affect the situation where a Dart program is compiled to a dill file without specifying an environment, and then an environment is specified on the VM commandline when running from that dill file. In that scenario, new X.fromEnvironment works, but const X.fromEnvironment does not.

If an environment is specified to the front end when compiling to dill, and that dill file is then run on the VM, const X.fromEnvironment will use the environment given to the front end, and new X.fromEnvironment will use the VM runtime environment.

[sigmundch]

There seems to be a lot of confusion due to the long discussion and format of this bug, so I filled a new issue using a more structured breaking change proposal template in #36579

Hope this helps!

[sigmundch]

closing this in favor of #36579