Constant evaluator won't assume all dart.library.x entries exist.

The existing implementation assumed, hardcoded, that the compilation
environment had an entry for any string of the form `dart.library.`+X,
with a value of either `"true"` or `""`.
This did not match the runtime behavior of the standalone VM,
which allows non-constant access to the compilation environment.

Changed to only have entries with value `"true"` for libraries
which exist.
This changes the value of `bool.hasEnvironment` or a
`String.fromEnvironment` or `bool.fromEnvironment` with a
non-default `defaultValue`.
The existing behavior was that `bool.hasEnvironment` was always true,
and that the other constructors ignored the `defaultValue`,
so most likely such tests or `defaultValues` aren't used anyway.

Fixes #53815

Bug: https://dartbug.com/53815
Change-Id: I995bb34b5ab04b39a8a588d6a59c0027a0fe855c
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/331261
Commit-Queue: Lasse Nielsen <lrn@google.com>
Reviewed-by: Johnni Winther <johnniwinther@google.com>

CHANGELOG.md

@@ -59,6 +59,19 @@ advantage of these improvements, set your package's
 
 [analyzer-completion-correction-issues]: https://github.com/dart-lang/sdk/labels/analyzer-completion-correctness
 
+#### Compilers
+
+- The compilation environment will no longer pretend to contain
+  entries with value `""` for all `dart.library.foo` strings,
+  where `dart:foo` is not an available library.
+  Instead there will only be entries for the available libraries,
+  like `dart.library.core`, where the value was, and still is, `"true"`.
+  This should have no effect on `const bool.fromEnvironment(...)` or
+  `const String.fromEnvironment(...)` without a `defaultValue`
+  argument, an argument which was always ignored previously.
+  It changes the behavior of `const bool.hasEnvironment(...)` on such
+  an input, away from always being `true` and therefore useless.
+
 #### Pub
 
 - Dependency resolution and `dart pub outdated` will now surface if a dependency

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

@@ -2476,7 +2476,8 @@ class ConstantEvaluator implements ExpressionVisitor<Constant> {
     if (_environmentDefines == null && !backend.supportsUnevaluatedConstants) {
       throw new ArgumentError(
           "No 'environmentDefines' passed to the constant evaluator but the "
-          "ConstantsBackend does not support unevaluated constants.");
+              "ConstantsBackend does not support unevaluated constants.",
+          "_environmentDefines");
     }
     intFolder = new ConstantIntFolder.forSemantics(this, numberSemantics);
     pseudoPrimitiveClasses = <Class>{
@@ -2503,35 +2504,30 @@ class ConstantEvaluator implements ExpressionVisitor<Constant> {
 
   Map<String, String>? _supportedLibrariesCache;
 
-  Map<String, String> _computeSupportedLibraries() {
-    Map<String, String> map = {};
-    for (Library library in component.libraries) {
-      if (library.importUri.isScheme('dart')) {
-        map[library.importUri.path] =
-            DartLibrarySupport.getDartLibrarySupportValue(
-                library.importUri.path,
-                libraryExists: true,
-                isSynthetic: library.isSynthetic,
-                isUnsupported: library.isUnsupported,
-                dartLibrarySupport: dartLibrarySupport);
-      }
-    }
-    return map;
-  }
+  Map<String, String> _computeSupportedLibraries() => {
+        for (Library library in component.libraries)
+          if (library.importUri.isScheme('dart') &&
+              DartLibrarySupport.isDartLibrarySupported(
+                  library.importUri.path,
+                  libraryExists: true,
+                  isSynthetic: library.isSynthetic,
+                  isUnsupported: library.isUnsupported,
+                  dartLibrarySupport: dartLibrarySupport))
+            (DartLibrarySupport.dartLibraryPrefix + library.importUri.path):
+                "true"
+      };
 
   String? lookupEnvironment(String key) {
     if (DartLibrarySupport.isDartLibraryQualifier(key)) {
-      String libraryName = DartLibrarySupport.getDartLibraryName(key);
-      String? value = (_supportedLibrariesCache ??=
-          _computeSupportedLibraries())[libraryName];
-      return value ?? "";
+      return (_supportedLibrariesCache ??= _computeSupportedLibraries())[key];
     }
     return _environmentDefines![key];
   }
 
   bool hasEnvironmentKey(String key) {
-    if (key.startsWith(DartLibrarySupport.dartLibraryPrefix)) {
-      return true;
+    if (DartLibrarySupport.isDartLibraryQualifier(key)) {
+      return (_supportedLibrariesCache ??= _computeSupportedLibraries())
+          .containsKey(key);
     }
     return _environmentDefines!.containsKey(key);
   }

pkg/front_end/lib/src/fasta/source/source_loader.dart

@@ -342,11 +342,16 @@ class SourceLoader extends Loader {
   }
 
   /// Return `"true"` if the [dottedName] is a 'dart.library.*' qualifier for a
-  /// supported dart:* library, and `""` otherwise.
+  /// supported dart:* library, and `null` otherwise.
   ///
   /// This is used to determine conditional imports and `bool.fromEnvironment`
   /// constant values for "dart.library.[libraryName]" values.
-  String getLibrarySupportValue(String dottedName) {
+  ///
+  /// The `null` value will not be equal to the tested string value of
+  /// a configurable URI, which is always non-`null`. This prevents
+  /// the configurable URI from matching an absent entry,
+  /// even for an `if (dart.library.nonLibrary == "")` test.
+  String? getLibrarySupportValue(String dottedName) {
     if (!DartLibrarySupport.isDartLibraryQualifier(dottedName)) {
       return "";
     }
@@ -356,11 +361,13 @@ class SourceLoader extends Loader {
     // TODO(johnniwinther): Why is the dill target sometimes not loaded at this
     // point? And does it matter?
     library ??= target.dillTarget.loader.lookupLibraryBuilder(uri);
-    return DartLibrarySupport.getDartLibrarySupportValue(libraryName,
-        libraryExists: library != null,
-        isSynthetic: library?.isSynthetic ?? true,
-        isUnsupported: library?.isUnsupported ?? true,
-        dartLibrarySupport: target.backendTarget.dartLibrarySupport);
+    return DartLibrarySupport.isDartLibrarySupported(libraryName,
+            libraryExists: library != null,
+            isSynthetic: library?.isSynthetic ?? true,
+            isUnsupported: library?.isUnsupported ?? true,
+            dartLibrarySupport: target.backendTarget.dartLibrarySupport)
+        ? "true"
+        : null;
   }
 
   SourceLibraryBuilder _createSourceLibraryBuilder(

pkg/kernel/lib/target/targets.dart

@@ -181,12 +181,13 @@ abstract class DartLibrarySupport {
     return dottedName.substring(dartLibraryPrefix.length);
   }
 
-  /// Returns `"true"` if the "dart:[libraryName]" is supported and `""`
-  /// otherwise.
+  /// Whether the "dart:[libraryName]" is supported.
   ///
   /// This is used to determine conditional imports and `bool.fromEnvironment`
   /// constant values for "dart.library.[libraryName]" values.
-  static String getDartLibrarySupportValue(String libraryName,
+  /// If the value is `false`, no environment entry exists for the library name,
+  /// otherwise an entry with value `"true"` is created.
+  static bool isDartLibrarySupported(String libraryName,
       {required bool libraryExists,
       required bool isSynthetic,
       required bool isUnsupported,
@@ -208,7 +209,7 @@ abstract class DartLibrarySupport {
     bool isSupported = libraryExists && !isSynthetic && !isUnsupported;
     isSupported = dartLibrarySupport.computeDartLibrarySupport(libraryName,
         isSupportedBySpec: isSupported);
-    return isSupported ? "true" : "";
+    return isSupported;
   }
 }
 

tests/standalone/from_env_test.dart

@@ -0,0 +1,116 @@
+// Copyright (c) 2023, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+// SharedOptions=-DtestFlag=testValue -DnumFlag=-0xfeed -DboolFlag=false
+
+import "package:expect/expect.dart";
+
+// Test that the `fromEnvironment`/`hasEnvironment` constructors
+// agree between constant and runtime evaluation,
+// both for platform-library detection entries and for normal entries.
+
+main() {
+  // User string entry, const.
+  Expect.isTrue(const bool.hasEnvironment("testFlag"));
+  Expect.equals("testValue",
+      const String.fromEnvironment("testFlag", defaultValue: "nonce"));
+  // User string entry, runtime.
+  Expect.isTrue(bool.hasEnvironment("testFlag"));
+  Expect.equals(
+      "testValue", String.fromEnvironment("testFlag", defaultValue: "nonce"));
+
+  // User number entry, const.
+  Expect.isTrue(const bool.hasEnvironment("numFlag"));
+  Expect.equals("-0xfeed", const String.fromEnvironment("numFlag"));
+  Expect.equals(-0xfeed, const int.fromEnvironment("numFlag", defaultValue: 4));
+  // User number entry, runtime.
+  Expect.isTrue(bool.hasEnvironment("numFlag"));
+  Expect.equals("-0xfeed", String.fromEnvironment("numFlag"));
+  Expect.equals(-0xfeed, int.fromEnvironment("numFlag", defaultValue: 4));
+
+  // User bool entry, const.
+  Expect.isTrue(const bool.hasEnvironment("boolFlag"));
+  Expect.equals("false", const String.fromEnvironment("boolFlag"));
+  Expect.equals(
+      false, const bool.fromEnvironment("boolFlag", defaultValue: true));
+  // User bool entry, runtime.
+  Expect.isTrue(bool.hasEnvironment("boolFlag"));
+  Expect.equals("false", String.fromEnvironment("boolFlag"));
+  Expect.equals(false, bool.fromEnvironment("boolFlag", defaultValue: true));
+
+  // Missing user entry, const.
+  Expect.isFalse(const bool.hasEnvironment("noEntry"));
+  Expect.equals("", const String.fromEnvironment("noEntry"));
+  Expect.equals(
+      "nonce", const String.fromEnvironment("noEntry", defaultValue: "nonce"));
+  Expect.equals(0, const int.fromEnvironment("noEntry"));
+  Expect.equals(42, const int.fromEnvironment("noEntry", defaultValue: 42));
+  Expect.isFalse(const bool.fromEnvironment("noEntry"));
+  Expect.isTrue(const bool.fromEnvironment("noEntry", defaultValue: true));
+  // Missing user entry, runtime.
+  Expect.isFalse(bool.hasEnvironment("noEntry"));
+  Expect.equals("", String.fromEnvironment("noEntry"));
+  Expect.equals(
+      "nonce", String.fromEnvironment("noEntry", defaultValue: "nonce"));
+  Expect.equals(0, int.fromEnvironment("noEntry"));
+  Expect.equals(42, int.fromEnvironment("noEntry", defaultValue: 42));
+  Expect.isFalse(bool.fromEnvironment("noEntry"));
+  Expect.isTrue(bool.fromEnvironment("noEntry", defaultValue: true));
+
+  // General platform library entry, const.
+  Expect.isTrue(const bool.hasEnvironment("dart.library.core"));
+  Expect.equals("true",
+      const String.fromEnvironment("dart.library.core", defaultValue: "nonce"));
+  Expect.isTrue(
+      const bool.fromEnvironment("dart.library.core", defaultValue: false));
+  // General platform library entry, runtime.
+  Expect.isTrue(bool.hasEnvironment("dart.library.core"));
+  Expect.equals("true",
+      String.fromEnvironment("dart.library.core", defaultValue: "nonce"));
+  Expect.isTrue(bool.fromEnvironment("dart.library.core", defaultValue: false));
+
+  // Standalone VM-specific library, const.
+  Expect.isTrue(const bool.hasEnvironment("dart.library.io"));
+  Expect.equals("true",
+      const String.fromEnvironment("dart.library.io", defaultValue: "nonce"));
+  Expect.isTrue(
+      const bool.fromEnvironment("dart.library.io", defaultValue: false));
+  // Standalone VM-specific library, runtime.
+  Expect.isTrue(bool.hasEnvironment("dart.library.io"));
+  Expect.equals(
+      "true", String.fromEnvironment("dart.library.io", defaultValue: "nonce"));
+  Expect.isTrue(bool.fromEnvironment("dart.library.io", defaultValue: false));
+
+  // Web-specific library, not available here, const.
+  Expect.isFalse(const bool.hasEnvironment("dart.library.html"));
+  Expect.equals("", const String.fromEnvironment("dart.library.html"));
+  Expect.equals("nonce",
+      const String.fromEnvironment("dart.library.html", defaultValue: "nonce"));
+  Expect.isFalse(const bool.fromEnvironment("dart.library.html"));
+  Expect.isTrue(
+      const bool.fromEnvironment("dart.library.html", defaultValue: true));
+  // Web-specific library, not available here, runtime.
+  Expect.isFalse(bool.hasEnvironment("dart.library.html"));
+  Expect.equals("", String.fromEnvironment("dart.library.html"));
+  Expect.equals("nonce",
+      String.fromEnvironment("dart.library.html", defaultValue: "nonce"));
+  Expect.isFalse(bool.fromEnvironment("dart.library.html"));
+  Expect.isTrue(bool.fromEnvironment("dart.library.html", defaultValue: true));
+
+  // Non-existing library, const.
+  Expect.isFalse(const bool.hasEnvironment("dart.library.not"));
+  Expect.equals("", const String.fromEnvironment("dart.library.not"));
+  Expect.equals("nonce",
+      const String.fromEnvironment("dart.library.not", defaultValue: "nonce"));
+  Expect.isFalse(const bool.fromEnvironment("dart.library.not"));
+  Expect.isTrue(
+      const bool.fromEnvironment("dart.library.not", defaultValue: true));
+  // Non-existing library, runtime.
+  Expect.isFalse(bool.hasEnvironment("dart.library.not"));
+  Expect.equals("", String.fromEnvironment("dart.library.not"));
+  Expect.equals("nonce",
+      String.fromEnvironment("dart.library.not", defaultValue: "nonce"));
+  Expect.isFalse(bool.fromEnvironment("dart.library.not"));
+  Expect.isTrue(bool.fromEnvironment("dart.library.not", defaultValue: true));
+}