Merge pull request #868 from Workiva/FED-1720-conditionally-null-safe…

…-builder FED-1720 Conditionally null-safe builder output (tests)
Workiva · Dec 18, 2023 · daa47e1 · daa47e1
2 parents e074bfc + 7ffd41a
commit daa47e1
Show file tree

Hide file tree

Showing 92 changed files with 15,540 additions and 15 deletions.
diff --git a/build.yaml b/build.yaml
@@ -35,6 +35,8 @@ targets:
             # These tests analyze cleanly without a build so the generated files do not need to be checked in.
             - "test/over_react/component_declaration/builder_integration_tests/backwards_compatible/**"
             - "test/over_react/component_declaration/builder_integration_tests/new_boilerplate/**"
+            - "test/over_react/component_declaration/non_null_safe_builder_integration_tests/backwards_compatible/**"
+            - "test/over_react/component_declaration/non_null_safe_builder_integration_tests/new_boilerplate/**"
             # These test un-built base class behavior, and contain code that might look like declarations
             # and should definitely not be built
             - "test/over_react/component_declaration/builder_helpers_test.dart"
@@ -49,6 +51,8 @@ targets:
             # These tests analyze cleanly without a build so the generated files do not need to be checked in.
             - "test/over_react/component_declaration/builder_integration_tests/backwards_compatible/**"
             - "test/over_react/component_declaration/builder_integration_tests/new_boilerplate/**"
+            - "test/over_react/component_declaration/non_null_safe_builder_integration_tests/backwards_compatible/**"
+            - "test/over_react/component_declaration/non_null_safe_builder_integration_tests/new_boilerplate/**"
           exclude:
             - "web/component1/**"
             # The Dart 2 only boilerplate does not analyze cleanly without a build so the generated files need to be

diff --git a/dart_test.yaml b/dart_test.yaml
@@ -16,6 +16,7 @@ presets:
     exclude_tags: no-ddc
     paths:
       - test/over_react_component_declaration_test.dart
+      - test/over_react_component_declaration_non_null_safe_test.dart
       - test/over_react_component_test.dart
       - test/over_react_dom_test.dart
       - test/over_react_prod_test.dart
@@ -26,6 +27,7 @@ presets:
     exclude_tags: ddc
     paths:
       - test/over_react_component_declaration_test.dart
+      - test/over_react_component_declaration_non_null_safe_test.dart
       - test/over_react_component_test.dart
       - test/over_react_dom_test.dart
       - test/over_react_prod_test.dart

diff --git a/lib/src/builder/builder.dart b/lib/src/builder/builder.dart
@@ -13,16 +13,20 @@
 // limitations under the License.
 
 import 'dart:async';
+import 'dart:io';
+import 'dart:isolate';
 
 import 'package:analyzer/dart/analysis/utilities.dart';
 import 'package:analyzer/dart/ast/ast.dart';
 import 'package:build/build.dart';
 import 'package:dart_style/dart_style.dart';
 import 'package:path/path.dart' as p;
+import 'package:package_config/package_config.dart' as pc;
 import 'package:source_span/source_span.dart';
 
 import './util.dart';
 import 'codegen.dart';
+import 'codegen/language_version_util.dart';
 import 'parsing.dart';
 
 Builder overReactBuilder(BuilderOptions? options) => OverReactBuilder();
@@ -58,6 +62,60 @@ class OverReactBuilder extends Builder {
       return;
     }
 
+    String nullSafetyReason;
+    bool nullSafety;
+    {
+      final versionToken = libraryUnit.languageVersionToken;
+      if (versionToken != null) {
+        // If there's a language version comment, honor that.
+        nullSafety = versionToken.supportsNullSafety;
+        nullSafetyReason = {
+          'languageVersion': '${versionToken.major}.${versionToken.minor}',
+          'source': 'libraryVersionComment',
+        }.toString();
+      } else {
+        // Otherwise, read the language version from the package config.
+        //
+        // Normally, builders would do this via (await buildStep.inputLibrary).languageVersion,
+        // but that would trigger resolved analysis which we don't want to do for this builder,
+        // for performance reasons.
+        final packageName = buildStep.inputId.package;
+
+        // Catch any errors coming from our implementation of `$packageConfig`.
+        // We can remove this once we switch to build 2.4.0's `packageConfig`
+        // (see `$packageConfig` doc comment for more info).
+        pc.LanguageVersion? packageConfigLanguageVersion;
+        try {
+          packageConfigLanguageVersion = (await buildStep.$packageConfig)
+              .packages
+              .firstWhereOrNull((p) => p.name == packageName)
+              ?.languageVersion;
+        } catch (e, st) {
+          log.warning('Error reading package config', e, st);
+        }
+
+        if (packageConfigLanguageVersion != null) {
+          nullSafety = packageConfigLanguageVersion.supportsNullSafety;
+          nullSafetyReason = {
+            'languageVersion': packageConfigLanguageVersion,
+            'source': 'packageConfig',
+            'package': packageName,
+          }.toString();
+        } else {
+          // If we don't have any information to go off of, opt out of null safety in 2.x,
+          // and opt in for newer versions (Dart 3+).
+          // It should be pretty unlikely that we don't have access to the package config,
+          // or that the file being generated doesn't have an associated package.
+          final platformOnlySupportsNullSafety = !Platform.version.startsWith('2.');
+          nullSafety = platformOnlySupportsNullSafety;
+          nullSafetyReason = {
+            'languageVersion': 'unknown (no package config or package config entry)',
+            'defaultNullSafetyForCurrentSdk': platformOnlySupportsNullSafety,
+          }.toString();
+        }
+      }
+    }
+
     final outputs = <String>[];
     void generateForFile(String source, AssetId id, CompilationUnit unit) {
       if (!mightContainDeclarations(source)) {
@@ -97,17 +155,6 @@ class OverReactBuilder extends Builder {
       // Validate boilerplate declarations and generate if there aren't any errors.
       //
 
-      // FIXME(null-safety) detect null safety for other packages (FED-1720)
-      bool nullSafety;
-      final languageVersionToken = unit.languageVersionToken;
-      if (languageVersionToken != null) {
-        nullSafety = languageVersionToken.major > 2 ||
-            (languageVersionToken.major == 2 && languageVersionToken.minor >= 12);
-      } else {
-        // During development, only enable null safety by default for over_react.
-        nullSafety = id.package == 'over_react';
-      }
-
       final generator = ImplGenerator(log, sourceFile, nullSafety: nullSafety);
 
       for (final declaration in declarations) {
@@ -170,10 +217,16 @@ class OverReactBuilder extends Builder {
         log.severe('Missing "part \'$expectedPart\';".');
       }
 
+      // Add an extra space, since `false` is one character longer than `true`, so that the next
+      // sentence lines up when grepped across multiple files.
+      final nullSafetyCommentText = 'Using nullSafety: $nullSafety.${nullSafety ? ' ' : ''} $nullSafetyReason';
+
       // Generated part files must have matching language version comments, so copy them over if they exist.
       // TODO use CompilationUnit.languageVersionToken instead of parsing this manually once we're sure we can get on analyzer version 0.39.5 or greater
       final languageVersionCommentMatch = RegExp(r'//\s*@dart\s*=\s*.+').firstMatch(source);
-      await _writePart(buildStep, outputId, outputs, languageVersionComment: languageVersionCommentMatch?.group(0));
+      await _writePart(buildStep, outputId, outputs,
+          nullSafetyCommentText: nullSafetyCommentText,
+          languageVersionComment: languageVersionCommentMatch?.group(0));
     } else {
       if (hasOutputPartDirective()) {
         log.warning('An over_react part directive was found in ${buildStep.inputId.path}, '
@@ -200,7 +253,8 @@ class OverReactBuilder extends Builder {
     return null;
   }
 
-  static FutureOr<void> _writePart(BuildStep buildStep, AssetId outputId, Iterable<String> outputs, {String? languageVersionComment}) async {
+  static FutureOr<void> _writePart(BuildStep buildStep, AssetId outputId, Iterable<String> outputs,
+      {required String nullSafetyCommentText, String? languageVersionComment}) async {
     final partOf = "'${p.basename(buildStep.inputId.uri.toString())}'";
 
     final buffer = StringBuffer();
@@ -214,8 +268,16 @@ class OverReactBuilder extends Builder {
       ..writeln('part of $partOf;')
       ..writeln()
       ..writeln(_headerLine)
-      ..writeln('// OverReactBuilder (package:over_react/src/builder.dart)')
-      ..writeln(_headerLine);
+      ..writeln('// OverReactBuilder (package:over_react/src/builder.dart)');
+    // Omit this for now for filed from over_react so it doesn't show up as a diff in golds and
+    // checked in files in the conditional null safety generation PR.
+    // TODO re-enable after that merges.
+    if (outputId.package != 'over_react') {
+      buffer
+        ..writeln('//')
+        ..write(lineComment(nullSafetyCommentText));
+    }
+    buffer.writeln(_headerLine);
 
     for (final item in outputs) {
       buffer
@@ -233,3 +295,20 @@ class OverReactBuilder extends Builder {
     await buildStep.writeAsString(outputId, output);
   }
 }
+
+extension on BuildStep {
+  // Cache the result so we don't read and parse the package config for every file.
+  //
+  // This value should be safe to reuse globally within an isolate.
+  static pc.PackageConfig? _cachedPackageConfig;
+
+  /// Polyfill for build 2.4.0's BuildStep.packageConfig, which only seems to be resolvable in Dart 3.
+  /// From: https://github.com/dart-lang/build/issues/3492#issuecomment-1533455176
+  ///
+  /// We could name this `packageConfig` and have it get shadowed by the real implementation from
+  /// `BuildStep` when 2.4.0 is resolved to, but that feels unnecessarily risky, and we can just
+  /// follow up later to remove this extension and use the real implementation instead.
+  Future<pc.PackageConfig> get $packageConfig async {
+    return _cachedPackageConfig ??= await pc.loadPackageConfigUri((await Isolate.packageConfig)!);
+  }
+}
diff --git a/lib/src/builder/codegen/language_version_util.dart b/lib/src/builder/codegen/language_version_util.dart
@@ -0,0 +1,13 @@
+import 'package:analyzer/dart/ast/token.dart' show LanguageVersionToken;
+import 'package:package_config/package_config.dart' show LanguageVersion;
+
+bool _languageVersionSupportsNullSafety(int major, int minor) =>
+    major > 2 || (major == 2 && minor >= 12);
+
+extension LanguageVersionToken$NullSafety on LanguageVersionToken {
+  bool get supportsNullSafety => _languageVersionSupportsNullSafety(major, minor);
+}
+
+extension LanguageVersion$NullSafety on LanguageVersion {
+  bool get supportsNullSafety => _languageVersionSupportsNullSafety(major, minor);
+}
diff --git a/lib/src/builder/util.dart b/lib/src/builder/util.dart
@@ -112,3 +112,6 @@ MethodDeclaration? metaMethodOrNull(ClassishDeclaration node) {
   return node.members.whereType<MethodDeclaration>().firstWhereOrNull((member) =>
       member.name.name == 'meta');
 }
+
+String lineComment(String contents) =>
+    contents.split('\n').map((line) => '// $line\n').join('');
diff --git a/pubspec.yaml b/pubspec.yaml
@@ -13,6 +13,7 @@ dependencies:
   js: ^0.6.1+1
   logging: '>=0.11.3+2 <2.0.0'
   meta: ^1.6.0
+  package_config: ^2.1.0
   path: ^1.5.1
   react: ^7.0.0
   redux: '>=5.0.0 <6.0.0'