NNBD const evaluation error in String.fromEnvironment

[liamappelbe]

lib_2/isolate/string_from_environment_default_value_test.dart is failing in the NNBD SDK. Both expectations fail:

Expect.isNull(const String.fromEnvironment('NOT_FOUND'));
Expect.equals('x', const String.fromEnvironment('NOT_FOUND', defaultValue: 'x'));

The failures are because the API was changed to default defaultValue to an empty string rather than null:

const factory String.fromEnvironment(String name, {String defaultValue = ""})

So the first expect is easily fixed by updating it to expect an empty string rather than null. The second failure is more interesting, and that's what this bug is about.

The issue is that const String.fromEnvironment('NOT_FOUND', defaultValue: 'x') is returning an empty string even though defaultValue is given. Some other interesting data points:

• If we remove the const modifier, the test passes. I think this means its a const eval bug, and the VM implementation works.
• If we remove = "" from the method signature, reverting it back to a null default, then the call returns "x" and the test passes.
• If we change = "" to something else, like "hi", then this string is returned from the call in the test. So that part of the plumbing is working.

My guess is that there's something in the const evaluator that is checking whether the defaultValue is provided by comparing it to null, so the default value of empty string looks like a provided value.

[lrhn]

That looks like a bug in the implementation of String.fromEnvironment. It should return the default value when there is no declaration.

[liamappelbe]

This also affects language_2/string/const_interpolation2_test

[alexmarkov]

Front-end relies on environmentDefines map to do the constant evaluation of String.fromEnvironment:

sdk/pkg/front_end/lib/src/fasta/kernel/constant_evaluator.dart

Lines 1755 to 1787 in 3a8264c

	String value = environmentDefines[name.value];
	Constant defaultValue = named["defaultValue"];
	if (target.enclosingClass == coreTypes.boolClass) {
	Constant boolConstant = value == "true"
	? trueConstant
	: value == "false"
	? falseConstant
	: defaultValue is BoolConstant
	? makeBoolConstant(defaultValue.value)
	: defaultValue is NullConstant
	? nullConstant
	: falseConstant;
	return boolConstant;
	} else if (target.enclosingClass == coreTypes.intClass) {
	int intValue = value != null ? int.tryParse(value) : null;
	Constant intConstant;
	if (intValue != null) {
	bool negated = value.startsWith('-');
	intConstant =
	intFolder.makeIntConstant(intValue, unsigned: !negated);
	} else if (intFolder.isInt(defaultValue)) {
	intConstant = defaultValue;
	} else {
	intConstant = nullConstant;
	}
	return canonicalize(intConstant);
	} else if (target.enclosingClass == coreTypes.stringClass) {
	value ??=
	defaultValue is StringConstant ? defaultValue.value : null;
	if (value == null) return nullConstant;
	return canonicalize(new StringConstant(value));
	}

It is using default value only if environmentDefines[name.value] is null.
environmentDefines map is passed to the front-end from the VM.
VM implements environmentDefines in the following way to forward current environment defines to the front-end:

sdk/pkg/vm/bin/kernel_service.dart

Lines 241 to 258 in 3a8264c

	// Environment map which looks up environment defines in the VM environment
	// at runtime.
	// TODO(askesc): This is a temporary hack to get hold of the environment during
	// JIT compilation. We use a lazy map accessing the VM runtime environment using
	// new String.fromEnvironment, since the VM currently does not support providing
	// the full (isolate specific) environment as a finite, static map.
	class EnvironmentMap extends UnmodifiableMapBase<String, String> {
	@override
	String operator [](Object key) {
	// The fromEnvironment constructor is specified to throw when called using
	// new. However, the VM implementation actually looks up the given name in
	// the environment.
	return new String.fromEnvironment(key);
	}
	@override
	get keys => throw "Environment map iteration not supported";
	}

So this map is calling String.fromEnvironment(key) to get the environment variable. With NNBD API breaking change String.fromEnvironment now returns empty string, so environmentDefines[] now returns an empty string and default value is not used.

We can probably change EnvironmentMap in kernel service to return null if environment doesn't have a key. Testing String.fromEnvironment(key, "1") == String.fromEnvironment(key, "2") should do the trick, but it looks a little bit hacky.
@lrhn Is there a recommended way to figure out if environment define is specified?

/cc @askeksa-google

[askeksa-google]

Alternatively, we can change the way environment defines are provided to the front end into an EnvironmentDefines interface where the default value is provided by the constant evaluator with each lookup. Those users of the front end which currently pass a proper map could wrap that map to return the default value when the key is not found (the EnvironmentDefines interface could provide a factory constructor for wrapping a Map like this), and the kernel service could just pass on the default value to the runtime String.fromEnvironment.

We have talked about making this change in any case, as it is cleaner than having the kernel service masquerade behind the Map interface.

[lrhn]

There is no added functionality which would allow you to distinguish an unset environment from one set to the default value from user code. I think the String.fromEnvironment(key, "1") == String.fromEnvironment(key, "2") hack is the best I've come up with too.

Is it possible for the VM to go behind the abstraction in its implementation of the environmentDefines? Since the VM itself is implementing String.fromEnvironment run as non-const in the patch file (it's native), it could just make the native function return null, and add the ?? defaultValue in Dart code, then base the map on the native function.

[vsmenon]

Is this a blocker for unforking? ( @leafpetersen @franklinyow )

[leafpetersen]

I think this is a blocker - if I understand correctly, it's a regression in the existing behavior of the constructor.

[alexmarkov]

There are two problems described in this issue:

1. Breaking change in String.fromEnvironment (returning an empty string instead of null).
2. A bug in implementation of that breaking change: String.fromEnvironment with specified defaultValue still returns an empty string.

If we decide to go ahead with the breaking change we need to fix problem 2 (probably when that breaking change is back-ported to the default core libraries).

[johnniwinther]

Further work on this issue is awaiting the SDK implementation of #40678