dart-lang / language

Design of the Dart language
Other
2.65k stars 202 forks source link

Backends will expose whether assertions are enabled via a constant. Question: should this be specified? #2876

Open sigmundch opened 1 year ago

sigmundch commented 1 year ago

Dart2js will expose some compiler flags to the program via environment booleans. Currently, we are just interested in one flag: --enable-asserts. When --enable-asserts is provided, the constant bool.fromEnvironment('dart.web.enable_asserts') will be true. This is consistent with the recent proposal in #2807 - we only expose flags known at compile time.

The thing is: other backends are also interested in doing something similar, and we may want to expose other flags in the future. Once we generalize this to more backends, we want to align on the behavior and naming conventions.

Our question for the language team is: do we want this behavior to be part of the language specification? We are happy to coordinate with one another among the backend teams, but we wanted to make a conscious decision about it.

Context

All backend teams (web, aot, dart2wasm) try to tree-shake inaccessible code. Recently, we've all been pushing for a kernel-level transformation that allows us to delete code at early stages of compilation, before analyzing the program or doing optimizations.

This kind of early tree-shaking is very attractive for configuration specific code that is not guarded by conditional imports. Examples of this include: dev/debug only logic and customizations by kind of renderer in flutter (canvaskit vs html).

Framework specific options like the renderer need to be specified via command-line environment booleans, but some historically were only determined by a runtime value. A clear example of that is dev/debug only logic, where users typically write something of the form:

final bool assertsEnabled = () {
  var b = false;
  assert(b = true);
  return b;
}();

instead of a constant.

We'd like to move this logic to use constants to make it easier to resolve and tree-shake early in the compilation pipeline. In our recent experience implementing this transformation, we were able to prune 7% of user code much earlier in the compilation pipeline in large internal customer code.

eernstg commented 1 year ago

My first take on this question is that it fits well into the documentation of the platform libraries (say, the documentation of fromEnvironment constructors).

There was a similar consideration in Feb 2021, https://github.com/dart-lang/language/issues/304#issuecomment-778101314.

alexmarkov commented 1 year ago

Flutter has kDebugMode, kProfileMode and kReleaseMode constant which are declared using environment defines: https://github.com/flutter/flutter/blob/master/packages/flutter/lib/src/foundation/constants.dart Flutter debug mode has assertions enabled, and kDebugMode is often used to guard assertion code in Flutter. Do we want to unify/specify those environment defines too?

If we would specify a standard environment define for the assertions mode, we should probably make it optional. For example, we should avoid using it in core libraries as platform currently doesn't depend on the assertions mode and we can have one platform dill file serving both enabled and disabled assertions. If we would like to have this define to always present, then we would need to have separate platform dill files for enabled and disabled assertions. As we add more modes and more such defines, we can end up with a combinatorial explosion of different modes and corresponding platform dill files.

mkustermann commented 1 year ago

Backends will expose whether assertions are enabled via a constant.

This makes our kernel files no longer agnostic to asserts (Currently one can compile dart sources to kernel and then later "configure" the kernel file by launching with --enable-asserts or with --no-enable-assertions). This is a somewhat related topic to https://github.com/dart-lang/language/issues/2807.

We can go in two different directions here:

I think the ladder we will do anyway, because we'd like to be able to fold away a richer set of expression/statement language than the current const language (which is a subset of dart) allows.

If all our tools will have this code pruning step in whole-program compilers anyway, I'm wondering whether it's worthwhile introducing e.g. const bool assertsEnabled = const bool.fromEnvironment('dart.enable_asserts') over having a @pragma('evaluate-at-compile-time-in-aot-mode') final bool assertsEnabled = ...;?

sigmundch commented 1 year ago

cc @fishythefish

Great points @mkustermann - I like where you are headed.

This makes our kernel files no longer agnostic to asserts

Yes, but only to the degree that those are used. That is, if we don't need to know about assertions/OS/etc in the SDK, then we can still continue to have a single platform kernel file.

Is your expectation that we will need many of these configurations to be available at the SDK platform level?

it seems it may not work for arbitrary user-supplied const-from-environment key/value pairs anyway. @sigmundch is that right?

Correct!

I think the ladder we will do anyway, because we'd like to be able to fold away a richer set of expression/statement language than the current const language (which is a subset of dart) allows.

This heavily resonates with my preference too!

We've found repeatedly that the the conflation we have between language constants and flags tremendously limits the flexibility we have in our compiler pipeline (especially in running parts of the compiler modularly).

I do like that developers understand the concept of constants easily, and that it is clear for them how to properly to guard configuration specific code with them. We should aim for a design for flags that also assists developers in the process.

I'd like to go back to the drawing board for a bit. I think our current use of const fromEnvironment('dart.web.enable_asserts') is good enough to unblock the use case we have today, but I'm convinced this is not the long term direction we want to take. I'd like to start working on a proposal for how we'd expose user-level link-time flags and see where t take us.

fishythefish commented 1 year ago

Although early tree-shaking was the original motivation for bool.fromEnvironment('dart.web.assertions_enabled'), I think there are other potential use cases we should keep in mind. For example, while performing 1P null-safety migrations, I discovered that it would be helpful to be able to write something like

@GenerateInjector([
  // ...
  if (assertionsEnabled) ValueProvider.forToken(...)
  // ...
])

Unlike the tree-shaking case, this is a const context, so if we expose the flag as a non-const value, we limit what our users can express. IMO, if there's anything we can make const, we should try to do so. We can, of course, continue folding non-const expressions as well, but having a richer const subset of the language is valuable.

rrousselGit commented 1 year ago

A constant for whether asserts are enabled would certainly be lovely.

The assert((){}()) trick has its flaws & limitations. It has a very poor readability/formatting and clash with the linter (the "specify assert message" lint).
But more importantly, it cannot be used in scenarios where an assert isn't usage

Namely things like:

if (assertsEnabled && someCheck)  return;

[ 
  if (assertsEnabled) value
]

for (...)
  if (assertsEnabled && condition) break;

and more.

And relying on a non-constant variable for this breaks tree-shaking.

While Flutter exposes consts like kDebugMode & co, these aren't necessarily respected in non-Flutter apps.
A command line wouldn't be able to rely on kDebugMode to perform the above effects.

leafpetersen commented 1 year ago

Our question for the language team is: do we want this behavior to be part of the language specification? We are happy to coordinate with one another among the backend teams, but we wanted to make a conscious decision about it.

My recollection of the last time we discussed this issue was that we explicitly left the question of what fromEnvironment flags were supported up to the implementations. We discussed (but I believe never followed up on) having a separate document which specified the implementation specific behaviors which we proposed that all of our implementations would choose to agree on. I'm still open to moving forward with that latter.

lrhn commented 10 months ago

I think we should at least division the dart. namespace so that it's clear which parts each tool is in control of, and have some guilde-lines for naming and format. (Perhaps: dot-separated lower-snake-case identifiers only.)

Leaving it to each compiler to do what they need isn't necessarily good for users, if it gives us multiple incoherent ways to do the same thing. If more tools do the same thing, they should coordinate about, at least, naming. (Goes for pragmas too!) A global dart.mode.asserts_enabled, or whereever it would fit, is more generally useful, and more convenient, than having to check both dart.web.enable_asserts and hypothetical later dart.vm.assertsEnabled and dart.tool.ddc.asserts.

If something like that is generally available, we can also consider making it available as a Dart constant in dart:core, somewhere. (Naming is hard.)

I'll try to write up a proposal for a namespace division and naming scheme.