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.07k stars 1.56k forks source link

Dart-defines should generate a warning when not const #42177

Open pingbird opened 4 years ago

pingbird commented 4 years ago

This issue is related to

Version

Dart VM version: 2.8.3 (stable) (Tue May 26 18:39:38 2020 +0200) on "windows_x64"

Description

When you access dart defines in a non-const expression, e.g.

void main() {
  print('foo: ${String.fromEnvironment('foo')}');
}

This works fine when the script is run directly with dart, e.g. dart -Dfoo=bar bin/main.dart.

But fails silently when run as a Flutter application, e.g. flutter run --dart-define=foo=bar. If I understand correctly this is because Flutter only passes defines to the front-end, so they aren't available at runtime.

The example also crashes on web.

Proposal

The analyzer should give a warning if String.fromEnvironment, bool.fromEnvironment, or int.fromEnvironment are called in a non-const expression.

Documentation should be updated to clarify the importance of using const and whether or not accessing them at runtime is supported at all.

lrhn commented 4 years ago

The "only const" restriction was never intended, but it was implemented that way by dart2js. It does avoid having to carry the entire environment along in the compiled code, which is a reasonable goal for ahead-of-time compilation.

We should either accept the const-only behavior, and then enforce it on the VM too, or make dart2js support run-time lookups (it can still warn that using the constructors as non-const increases the size of the compiled code)..

devoncarew commented 4 years ago

I'm going to triage this into area-web (but this generally may cross over a few areas).

jonahwilliams commented 4 years ago

We cannot reasonably supply non-const values for flutter applications, so it might be a reasonable restriction that we could add to our embedder

lrhn commented 4 years ago

You should not supply different results for non-const invocations of .fromEnvironment than you would have for the same const invocation.

It is supposed to be a compile-time environment that we are retaining at run-time, not a new run-time environment, so one compilation strategy is to embed that environment in the executable (but only if anyone calls .fromEnvironment non-const with a non-const argument).

(It was never actually specified in a way that made sense for AoT compilation, or modular compilation, but now at least the latter has been specced.)

irvine5k commented 3 years ago

Hey, people. Do you think that we can advance with docs upgrade since it is cheaper than creating a warning(it's optimal)?

lrhn commented 3 years ago

The docs update would have to say that the constructor is only guaranteed to work when called as const, but may work with new on some specific platforms, mainly non-AoT-compiled ones.

irvine5k commented 3 years ago

@lrhn Nice, I added this information in my PR https://github.com/dart-lang/sdk/pull/46846

gnprice commented 1 year ago

This seems to be tripping up quite a lot of people using Flutter. See https://github.com/flutter/flutter/issues/55870 and the numerous upvotes and comments there.

It doesn't sound like there's much interest from either Flutter or dart2js in actually supporting non-const usage. (When a user does want to dynamically select from some data provided at build time, there are lots of existing ways they can accomplish that.) So I think a good way forward would be to make non-const usage of String.fromEnvironment and its friends generate a warning, if not an error.