search

dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.22k stars 1.57k forks source link

Incorrect documentation in fromEnvironment constructors #36482

Open a-siva opened 5 years ago

a-siva commented 5 years ago

sdk/lib/core/bool.dart has the following documentation which is not quite true when running code on the VM. // The .fromEnvironment() constructors are special in that we do not want // users to call them using "new". We prohibit that by giving them bodies // that throw, even though const constructors are not allowed to have bodies. // Disable those static errors. //ignore: const_constructor_with_body //ignore: const_factory external const factory bool.fromEnvironment(String name, {bool defaultValue = false});

Similar comments are in other files like sdk/lib/core/string.dart sdk/lib/core/int.dart

This needs to be clarified as it has implications on how we want users to use these fromEnvironment constructors to control runtime configuration.

lrhn commented 5 years ago

I have no idea what the comment means, but it appears to have been put there by the DDC developers while developing DDC (@munificent). It's not a doc-comment, so users should not see it unless they read the code. I'd say the entire comment should be in the DDC patch file, not in the main library file, since it is DDC specific. That is, you should ignore it, and so should users, and we should move it away to make that obvious.

The design for fromEnvironment has not changed. They are still intended to be callable both as const and as new. The values should agree, which means that ahead-of-time compilation needs to inline the compile-time environment in the deliverable, or delay constant evaluation until the environment becomes available (compile-time, link-time, run-time).

A program should never have two different environments that matter to its execution, so the moment an environment has been applied, that environment is the one that the program uses for everything, even run-time new String.fromEnvironment(computedString) lookups. Obviously, if there are no new invocations, that environment can be tree-shaken away at run-time.

a-siva commented 5 years ago

based on the comment above, I am going to change the area to DDC

a-siva commented 5 years ago

@lrhn can you also clarify what should happen if compilation happens in a modular fashion, that is we have multiple modules one compiled with an environment provided at compile time and another without an environment at compile time, now let us say we concat these module and run them with an environment at runtime one could potentially see different values in the two modules for the same environment variable.

sigmundch commented 5 years ago

We may need to revisit this in more detail.

Since the first day fromEnvironment constructors were added, they were only intended to be used for consts and not as a regular constructor. You can see this even in the error message in the dart2js patch files on the very first commit that introduced the change back in '13 (https://github.com/dart-lang/sdk/commit/36a67fa046803bf0d8b83ae604e8b6a69675320c#diff-d2b30585615e8246f87aeed72c2844ecR108)

I'm not opposed to allowing it, but it is not supported today in dart2js and will take some work on our end.

lrhn commented 5 years ago

For what it's worth, I don't remember that new String.fromEnvironment was not intended to work. There is #16846 requesting that dart2js makes it work too.

It is clearly a feature which was not designed with modular compilation in mind. There was no such thing at the time - the VM was JIT from source and dart2js was full-program compilation. Not supporting new String.fromEnvironment allwed dart2js to no omit the compilation environment from the compiled program.

As for modular compilation seeing two different environments, I'd probably say that the compiled output of each should contain the necessary environment, and if the two environments are not compatible, then linking should fail (maybe just a hash of the environment would be sufficient to ensure compatibility, then only code using new String.fromEnvironment would need to carry the actual environment.

munificent commented 5 years ago

it appears to have been put there by the DDC developers while developing DDC (@munificent).

I added those comments when I was fixing all of the static warnings/errors produced when DDC compiled the SDK.

These declarations are weird. Analyzer (and maybe CFE?) reports a compile error that you have a const constructor with a body. It treats "external" as as "body". The spec is a little vague on whether that should be the case or not, but it seems reasonable to me.

How would a tool like analyzer or the CFE execute an external constructor body when doing const evaluation compile time? Currently, the answer seems to be "they don't", but I don't see any implication anywhere that that should be permitted. As far as I can tell, these functions are just punching a hole in the language semantics.

Worse, they apparently do so in an implementation-specific way. The external bodies of these in DDC throw exceptions, because DDC wants to prohibit calling fromEnvironment() with new. The external bodies in the VM I guess allows that to work?

The comment I added incorrectly describes only DDC's behavior. At the time, I don't think I realized other platforms (deliberately?) behaved differently. We could "fix" the comment, but only to the degree that it would then describe behavior that is incompatible across implementations.

lrhn commented 5 years ago

The fromEnvironment and Symbol constructors have always been defined in ways that preclude being implemented in pure Dart. They require compiler cooperation. For fromEnvironment, it requires interaction with the compilation and run-time environment, for Symbol it requires doing input validation in a const evaluation.

This design was deliberate, we knew the compilers needed to handle the constructors specially, almost like language features. So, whether an external const factory constructor makes sense or not, it exists with a documented behavior, and compilers should "just" implement that by special-casing.

However, compilers have not agreed on how, or if, to support the documented behavior, and we haven't made the necessary effort to make behavior consistent. The VM allowed new String.fromEnvironment(...) because they didn't use to differentiate between compile-time and run-time. Until they did with precompilation, then things got weird. DDC and dart2js never supported new for fromEnvironment. (The VM also never did the proper validation in the Symbol constructor, and even allowed creating private symbols using it).

The comment in the current library is wrong. Other platforms did, deliberately, behave differently, and the comment does not represent an actual language design choice, just a restriction on DDC. The comment should be removed.

Then we should sit down and decide what behavior we actually want, and then do the work of getting the platforms to agree. We will do that knowing that we now have an ahead-of-time compiled language, unlike what we thought when the feature was designed. The result of that may be a breaking change for some platforms.

jmesserly commented 5 years ago

Tagging this as area-library since it sounds like all we want is to remove the comment.

In terms of accepting new it sounds like both DDC and dart2js would have to do some work; presumably storing the declared variables so we can recover them at runtime. (Also related: https://github.com/dart-lang/sdk/issues/36579)

aadilmaan commented 5 years ago

@lrhn @munificent It appears we are awaiting from understanding and agreement between you folks on what to do here.