specify behavior of fromEnvironment

[sigmundch]

We'd like to have a source of truth to define what is required for fromEnvironment constructors and what is the expected behavior we will support.

Since modular compilation was not even in the picture at the time the feature was introduced, we are now finding a lot of inconsistencies across the tools. The recent changes that move constant evaluation to the CFE also highlight the need to define a coherent story going forward.

Some questions that have recently come up:

• are both const and new supported?
• when is evaluation supposed to take place (compile-time, link-time, runtime)?
• if evaluation is at compile-time
  • how is constant consistency maintained across modular builds (are environment definitions stored so they are always consistent at the app level, or are they substituted eagerly and then forgotten?)

[eernstg]

Makes sense! I'll label this issue as 'specification' at first. There might be some elements which would justify a language design perspective, but I suspect (and hope) that we can consider it to be more like a clarification. The decision may end up being part of the language specification or the core library documentation, but that can be decided independently of the actual contents, so nothing is implied at this point.

[leafpetersen]

We'd like to get closure on this. My proposal is the following:

• are both const and new supported?

Yes

• when is evaluation supposed to take place (compile-time, link-time, runtime)?

compile-time

• if evaluation is at compile-time

  • how is constant consistency maintained across modular builds (are environment definitions stored so they are always consistent at the app level, or are they substituted eagerly and then forgotten?)

Consistency is checked at link time. That is, if two separately compiled modules are linked together (whether before runtime, or at runtime) there is a check that they have consistent definitions for fromEnvironment).

@lrhn @munificent @eernstg Are we in agreement on this? Issues I'm not seeing?

[eernstg]

@leafpetersen wrote:

[evaluation occurs at] compile-time ..
Consistency is checked at link time

Making the choice at compile-time and then checking consistency at link time is what I've suggested we should do whenever this came up, for simplicity. So in that sense I agree.

But it is my impression that we are currently using a more complex approach, in particular in the common front end: I believe it has a notion of constant expressions that are unevaluated at compile time. Such expressions are evaluated to such an extent that there is no need for a regular binding environment (so we do not need to look up the declaration of an identifier in order to finish the evaluation), the evaluation can be completed based on nothing more than the values of constant expressions of the form e.fromEnvironment(...).

Presumably, this machinery would not be needed if our tools were able to rely on having the values of objects fromEnvironment at compile-time, which means that we have been aiming at a more flexible model than the one you mention.

@askeksa-google, can you provide more detail on this?

[lrhn]

• Both const and new supported: I'd be fine with only allowing const, but it is a breaking change.
• When does evaluation happen: If we are completely consistent, then it should not matter. Our constant expressions are exactly expressions where it doesn't matter when they are evaluated. Since some constant evaluation happens at compile-time, and conditional imports are based on the same environment and definitely happens at compile-time, that would mean that we need to retain the compile-time environment at run-time (at least if there is any new ...fromEnvironment invocation with a dynamic argument). So, in short, I think the answer to this should be decided by the tool needs, not the language/library team. There is no "link time" in the language (and the definition of "compile time" is really just that a program containing a compile-time error should not start running, there is no requirement that constant expressions are evaluated at compile time apart from that, so if a constant expression can provably not fail, it can be evaluated at any time and retain the specified semantics).
• Consistency: We do need consistency, as if there is only one environment which is used both during compile-time checks and conditional imports, and at run-time. If we retain compile-time constants in the intermediate formats, then I'd be fine with linking checking whether there are conflicts and breaking if there is. It might be sufficient to retain the values that were actually used (no use, no conflict), and only the full environment if a new .. invocation is in the compilation unit. We should not run code that doesn't correspond to a single full program with a single available environment.

Retaining the full environment will increase the files. It will not work well for DDC where you compile files individually and only link them by loading the JS code into the same JS VM. We don't want to do environment checking there. Perhaps they can just retain a sufficiently large hash of the environment and check that (again as long as there is no new operations).

If we disallow new, then I think a lot of the complications get smaller. A hash might be sufficient in all cases, and still only store that in compilation units that actually depend on the environment.

[askeksa-google]

Some thoughts about supporting new:

• It is currently not supported in dart2js and DDC, and it seems not obvious how to supply a run-time environment in either case. We could specify it at compile-time (though retaining that in the compilation has complications / overhead), but then const could just as well be used.
• In JIT mode, const and new are equivalent.
• Our API documentation states (since dart-lang/sdk@7f7c681) that only const is supported.
• The VM currently allows the embedder to supply a callback for specifying environment values. Thus, the VM does not have a fixed (and not necessarily finite) environment.
• The Isolate.spawnUri method has an environment parameter to specify the environment for the new isolate. This feature is currently unimplemented. All isolates in the same VM use the same environment (based on commandline options and the embedder callback).

It seems new is only really relevant for the compile-to-kernel + run-in-vm case. Here, it seems mainly useful if the run-time values can be different from the compile-time values, but this goes against the desire for consistency.

[leafpetersen]

@lrhn I don't really understand your reply, I think maybe we're talking past each other?

The key questions as I understand it are the following. Assume the compiler is run with -DPARAM=0 in module A, and the VM is run on the resulting kernel file with -DPARAM=2

0. Should it be made legal to call the fromEnvironment constructors with new?
1. Given a String.fromEnvironment call in module A with "PARAM":
   • is it guaranteed to produce 0
   • guaranteed to produce 2
   • does the result depend on whether you call it with "new" vs "const"?
   • It the result tool defined?
2. Given a second module B, compiled with -DPARAM=1 and linked with module A
   • Is this an error?
   • If not, do invocations in the different modules see different results?

My answers are as follow:

0. I'm agnostic, but don't see a need for the breaking change on the VM
1. I think this should produce 0, always (new or const)
2. I think this should be an error

What are your answers?

Retaining the full environment will increase the files. It will not work well for DDC where you compile files individually and only link them by loading the JS code into the same JS VM. We don't want to do environment checking there.

I'm not sure I see this. Why is the environment large? Most invocations don't pass any -D arguments at all?

@vsmenon Do you have DDC implementation concerns here?

[lrhn]

@leafpetersen
My answers are:
0. I'm agnostic as well. I believe some tools may get an advantage from "no".

1. A const invocation should produce 0. If we do allow passing flags at run-time, they should be honored and produce 2 if invoked using new. I'd prefer to not allow it and produce 0 for new as well. Not a strong use-case, so I'm not particularly attached to either.
2. If module B uses the value, I'll make it a compile-time error, precisely so that invocations in different modules do not see different values. If the value isn't used, the program is indistinguishable from one which was provided a consistent environment.

For modular compilation, any environment will be in each module. I also expect dart.library.xyz variables to be represented somehow. Maybe we'll just encode that as a "platform", but I think that's not a future safe design (it requires us to enumerate platforms in the format somehow).

[leafpetersen]

OK, as far as I can tell we have consensus.

• Using new produces the same result as using const, always
• We may not initially have support for using new on all platforms, that's ok
• Using new or const uses the value that was provided at the point that the source code was processed (that is, compile time if using AOT/snapshot, etc, or runtime if running directly from source)
• When multiple compilation artifacts are processed ("linked") it is an error if they have inconsistent environments.
  • There is a small question here to be resolved as to exactly what it means to be inconsistent

[leafpetersen]

2. If module B uses the value, I'll make it a compile-time error, precisely so that invocations in different modules do not see different values. If the value isn't used, the program is indistinguishable from one which was provided a consistent environment.

I'm not sure I think this is a good idea. What does it mean for a value to be used? Mentioned? Evaluated? If the latter, do you mean even at runtime (using "new")? What about short-circuiting in "const"? If the former, can we always tell which ones are mentioned? Certainly doesn't seem like that for "new".

Even ignoring the issue of defining this well, this feels fragile to me. I have a library that's working "fine" in all of my tests (because coverage doesn't hit the path that evaluates a particular fromEnvironment), but as soon as someone uses something that hits the code path with the fromEnvironment they start getting link time errors. I'd rather know up front that something has inconsistent defines.

[lrhn]

It's true that if we allow new, then we can't possibly know which environment declarations are used. We'll have to consider every value used if the code contains a new String.fromEnvironment (including every unspecified name).
For a non-taken branch of a potentially constant evaluation, we know at evaluation point whether the value of the declaration affects run-time semantics. If the branch isn't taken, then it doesn't matter what the value would have been.

I guess I'm saying that I only want to make a declaration conflict into a compile-time error if the conflicting value actually makes a (potential) difference.
Maybe that's a bad idea, and failing early on any compilation discrepancy is the way to go, but dart.library.io is such an environment declaration, so pre-linking artifacts would not be shareable between platforms, even if they are entirely platform independent.

If a compilation artifact only remembers the values that it has actually depended on, then anything that is entirely compilation environment independent can be reused freely (and it doesn't need to keep the extra data around).

[kmillikin]

• Using new or const uses the value that was provided at the point that the source code was processed (that is, compile time if using AOT/snapshot, etc, or runtime if running directly from source)

I would amend this to say that "using new or const takes the value that was provided at the point that a value was provided". We can leave it up to the tools to decide how an environment is provided and to make their own performance and usability tradeoffs.

If that formulation is too vague we could try something like "using new or const takes the value that was provided at the point that the code was processed for the purpose of evaluating calls to const {bool, int, String}.fromEnvironment". (Please don't require processing the source code for this purpose!)