Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
sigmundch opened this issue Mar 1, 2023 · 8 comments
Labels
feature Proposed language feature that solves one or more problems question Further information is requested

Comments

@sigmundch
Copy link
Member

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.

@sigmundch sigmundch added question Further information is requested feature Proposed language feature that solves one or more problems labels Mar 1, 2023
@eernstg
Copy link
Member

eernstg commented Mar 1, 2023

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, #304 (comment).

@alexmarkov
Copy link

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
Copy link
Member

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 #2807.

We can go in two different directions here:

  • We start parametrizing dart->kernel compilations with various aspects (assertions enabled/disabled, operating system, target architecture, abi, from-environment defines, ...). This will lead to us needing to change various infrastructure (Dart SDK, Flutter SDK, g3 build rules) to be able to compile N kernels where we currently only compile only 1. From this comment - which says:

    The restriction of having a known set of values means we can only use this for well known use cases with well known values, and we'd be frugal in using this for very few cases

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

  • We instead fold the related code away in AOT "link time step" / "global step" by a kernel transformer, so the code will be tree shaken before it reaches other global analysis. In development / modular / JIT mode we evaluate the conditions at runtime (instead of "link-time" in AOT).

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
Copy link
Member Author

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
Copy link
Member

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
Copy link

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
Copy link
Member

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
Copy link
Member

lrhn commented Oct 26, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Proposed language feature that solves one or more problems question Further information is requested
Projects
None yet
Development

No branches or pull requests

8 participants