breaking-change-request: const.fromEnvironment requires compile-time environment when compiled via kernel and run on the VM

[sigmundch]

Dart exposes a const fromEnvironment constructor in String, int, and bool. These constants return the value of an environment variable.

There is no concrete specification on when the environment is defined today (filed as request at dart-lang/language#304), and as a result different implementations capture the environment variables at different times:

• (a) DDC uses a compile-time environment
• (b) dart2js uses a compile-time environment, but supports a link-time environment
• (c) AOT snapshots use a compile-time environment when snapshots are built
• (d) VM-JIT uses a runtime environment. However because this configuration is compiling the app on launch, it can also be considered a compile-time environment
• (e) VM consuming kernel files and then running on the JIT uses a runtime environment as well.

Planned Change

We will no longer support using a runtime environment in (e). If no environment is provided when creating kernel files, the VM will complain that the input kernel files contain unevaluated constants.

In terms of support for fromEnvironment, this makes the scenario in (e) match what's allowed with AOT in (c). Support for (d) is not changed.

See #36513 for more context

Expected Impact

Most users run the VM directly from sources (scenario (d) above) and will not be affected.

This change is only visible when users:

• compile to kernel separately from running the app
• provide -D flags

We expect this may affect users of build systems like bazel and GN where it is common to build kernel files as intermediate artifacts before running a VM. This may also affect users of build_runner if they are using a configuration with intermediate kernel files.

pub [global] run is not affected because it doesn't use -D flags.

Mitigation

• For users using build systems, provide the -D flags at the time the kernel artifacts are built instead of providing them to the dart command
• For users that need a runtime environment variable (e.g. to enable a debug feature locally), switch to use command-line arguments or Platform.environment instead

@aadilmaan @mit-mit @Hixie @matanlurey

[matanlurey]

LGTM

[sigmundch]

/cc @a-siva @askeksa-google

[Hixie]

sgtm

[sigmundch]

@mit-mit - will ask for your lgtm on behalf of @dgrove (technically I already lgtmed on the other issue filed by @vsmenon)

[mit-mit]

SGTM

But please wait a bit longer before landing; the external announcement was just made, so we need to offer enough time for externals to offer feedback.

[askeksa-google]

Is it the plan to also remove the support for new X.fromEnvironment when running from a Kernel file on the VM? The description sounds like that is the case. With the new implementation as it is (after moving constant evaluation to the CFE) this still works, and replacing const by new is also a valid migration path (similar to using Platform.environment but without the need to replace VM arguments by platform environment variables).

If we want to remove support for new X.fromEnvironment in the VM, we should be aware that this is currently used by the Kernel service in JIT mode to look up const X.fromEnvironment in the JIT'ed Dart program.

[sigmundch]

Is it the plan to also remove the support for new X.fromEnvironment when running from a Kernel file on the VM?

It is a good question, I don't have a final answer. At this time, the answer is no, but long term the value you get back may change. If the resolution of dart-lang/language#304 is that new X.fromEnvironment must give the same value as const X.fromEnvironment, the answer will be yes in the future. At that point we will define the environment at compile-time and pass it through to the runtime.

Even though this is not changing at this time, I didn't suggest it as a mitigation because it has the potential to break again in the future.

[vsmenon]

@aadilmaan - are you tracking what we should do here and when?

[vsmenon]

Do we need anything here beyond a CHANGELOG update?

[aadilmaan]

@vsmenon - The announce email has been sent out. We have the approvals, we can land this change when ready unless folks feel we need to wait a little more for feedback.

[vsmenon]

I believe we landed the change in BE. :-)

It's the behavior of the new const-update flag. If we ship that in 2.3, we're going forward with this change. If we revert (looking unlikely at this point), we're not.

[dgrove]

We will be flipping the flag immediately after D24. Moving to next milestone.

[srawlins]

@vsmenon @aadilmaan the flag has been flipped on. I think this should have been closed?

[sigmundch]

@srawlins - you are correct

[nex3]

I just realized that this change also affected script snapshots generated with dart --snapshot, even though as I understand it they are not JIT compiled. Is this intentional? If so, it wasn't clearly documented, and has caused downstream breakage for Sass users: sass/dart-sass#914

[a-siva]

Yes it is intentional, the change is covered in the line

We will no longer support using a runtime environment in (e). If no environment is provided when creating kernel files, the VM will complain that the input kernel files contain unevaluated constants

script snapshots are nothing but kernel files in Dart 2.