Migrate to null safety (#76)

* Auto-migration prep

* Update pubspec with null-safe versions

* Rerun automated migration

Some dependencies' APIs changed, so this needed a few tweaks beyond what
Natalie did in the last fixit

* Post-migration cleanup

* Add missing encoding parameter to testing_test

* Try changing collections of Object to dynamic

* TestFailure had a breaking change

* Switch another Object to dynamic

* Fix chocolatey (hopefully?)

* Eliminate dependency on mustache package

* Make version non-nullable

This was inconsistently enforced earlier, but it's much simpler to
just assume that the version is always available.

* Update node-interop ref.

* Update changelog and pubspec

Co-authored-by: Natalie Weizenbaum <[email protected]>
Co-authored-by: awjin <[email protected]>

Author: 3 people

CHANGELOG.md

@@ -1,3 +1,7 @@
+# 1.3.0
+
+* Migrate to null-safety.
+
 # 1.2.0
 
 * Add a `pkg.githubBearerToken` to make it easier to integrate with GitHub

lib/src/chocolatey.dart

@@ -130,16 +130,16 @@ final chocolateyNuspec = InternalConfigVariable.fn<String>(() {
 
 /// Returns the XML-decoded contents of [chocolateyNuspecText], with a
 /// `"version"` field and a dependency on the Dart SDK automatically added.
-XmlDocument get _nuspec {
-  if (__nuspec != null) return __nuspec;
+late final XmlDocument _nuspec = () {
+  XmlDocument nuspec;
 
   try {
-    __nuspec = XmlDocument.parse(chocolateyNuspec.value);
+    nuspec = XmlDocument.parse(chocolateyNuspec.value);
   } on XmlParserException catch (error) {
     fail("Invalid nuspec: $error");
   }
 
-  var metadata = _nuspecMetadata;
+  var metadata = _findElement(nuspec.rootElement, "metadata");
   if (metadata.findElements("version").isNotEmpty) {
     fail("The nuspec must not have a package > metadata > version element. One "
         "will be added automatically.");
@@ -148,7 +148,7 @@ XmlDocument get _nuspec {
   metadata.children
       .add(XmlElement(XmlName("version"), [], [XmlText(_chocolateyVersion)]));
 
-  var dependencies = _findElement(metadata, "dependencies", allowNone: true);
+  var dependencies = _findElementAllowNone(metadata, "dependencies");
   if (dependencies == null) {
     dependencies = XmlElement(XmlName("dependencies"));
     metadata.children.add(dependencies);
@@ -162,10 +162,8 @@ XmlDocument get _nuspec {
     XmlAttribute(XmlName("version"), "[$chocolateyDartVersion]")
   ]));
 
-  return __nuspec;
-}
-
-XmlDocument __nuspec;
+  return nuspec;
+}();
 
 /// The `metadata` element in [_nuspec].
 XmlElement get _nuspecMetadata => _findElement(_nuspec.rootElement, "metadata");
@@ -288,22 +286,35 @@ Future<void> _deploy() async {
 }
 
 /// Returns the single child of [parent] named [name], or throws an error.
-///
-/// If [allowNone] is `true`, this returns `null` if there are no children of
-/// [parent] named [name]. Otherwise, it throws an error.
-XmlElement _findElement(XmlNode parent, String name, {bool allowNone = false}) {
+XmlElement _findElement(XmlNode parent, String name) {
+  var elements = parent.findElements(name);
+  if (elements.length == 1) return elements.single;
+
+  var path = _pathToElement(parent, name);
+  fail(elements.isEmpty
+      ? "The nuspec must have a $path element."
+      : "The nuspec may not have multiple $path elements.");
+}
+
+/// Like [findElement], but returns `null` if there are no children of [parent]
+/// named [name].
+XmlElement? _findElementAllowNone(XmlNode parent, String name) {
   var elements = parent.findElements(name);
   if (elements.length == 1) return elements.single;
-  if (allowNone && elements.isEmpty) return null;
+  if (elements.isEmpty) return null;
+
+  var path = _pathToElement(parent, name);
+  fail("The nuspec may not have multiple $path elements.");
+}
 
+/// Returns a human-readable CSS-formatted path to the element named [name]
+/// within [parent].
+String _pathToElement(XmlNode? parent, String name) {
   var nesting = [name];
   while (parent is XmlElement) {
-    nesting.add((parent as XmlElement).name.qualified);
+    nesting.add(parent.name.qualified);
     parent = parent.parent;
   }
 
-  var path = nesting.reversed.join(" > ");
-  fail(elements.isEmpty
-      ? "The nuspec must have a $path element."
-      : "The nuspec may not have multiple $path elements.");
+  return nesting.reversed.join(" > ");
 }

lib/src/config_variable.dart

@@ -19,10 +19,12 @@
 /// `tool/grind.dart`, before any `add*Tasks()` functions are called.
 class ConfigVariable<T> {
   /// The cached value.
-  T _value;
+  T? _value;
 
   /// The variable's original value,
-  T _defaultValue;
+  ///
+  /// This is always set if `_defaultCallback` is null.
+  T? _defaultValue;
 
   /// Whether [_value] has been cached yet or not.
   ///
@@ -31,25 +33,32 @@ class ConfigVariable<T> {
   var _cached = false;
 
   /// A function that generates [_value].
-  T Function() _callback;
+  ///
+  /// This can only be null if [_value] has been set explicitly and [_cached] is
+  /// `true`.
+  T Function()? _callback;
 
   /// The original callback for generating [_value].
-  T Function() _defaultCallback;
+  T Function()? _defaultCallback;
 
   /// Whether this variable has been frozen and can no longer be modified by the
   /// user.
   var _frozen = false;
 
   /// A function to call to make [_value] unmodifiable.
-  final T Function(T) _freeze;
+  final T Function(T)? _freeze;
 
   /// The variable's value.
   T get value {
     if (!_cached) {
-      _value = _callback();
+      _value = _callback!();
       _cached = true;
     }
-    return _value;
+
+    // We can't use `!` here because `T` might itself be a nullable type. We
+    // don't necessarily know that `_value` isn't null, we just know that it's
+    // the value returned by `_callback`.
+    return _value as T;
   }
 
   set value(T value) {
@@ -66,8 +75,13 @@ class ConfigVariable<T> {
 
   /// Returns the default value for this variable, even if its value has since
   /// been overridden.
-  T get defaultValue =>
-      _defaultCallback == null ? _defaultValue : _defaultCallback();
+  T get defaultValue {
+    var defaultCallback = _defaultCallback;
+    // We can't use `!` for `_defaultValue` because `T` might itself be a
+    // nullable type. We don't necessarily know that `_value` isn't null, we
+    // just know that it's the value returned by `_callback`.
+    return defaultCallback == null ? _defaultValue as T : defaultCallback();
+  }
 
   /// Sets the variable's value to the result of calling [callback].
   ///
@@ -85,11 +99,11 @@ class ConfigVariable<T> {
     _cached = false;
   }
 
-  ConfigVariable._fn(this._callback, {T Function(T) freeze})
+  ConfigVariable._fn(this._callback, {T Function(T)? freeze})
       : _defaultCallback = _callback,
         _freeze = freeze;
 
-  ConfigVariable._value(this._value, {T Function(T) freeze})
+  ConfigVariable._value(this._value, {T Function(T)? freeze})
       : _defaultValue = _value,
         _cached = true,
         _freeze = freeze;
@@ -104,26 +118,31 @@ extension InternalConfigVariable<T> on ConfigVariable<T> {
   ///
   /// If [freeze] is passed, it's called when the variable is frozen to make the
   /// value unmodifiable as well.
-  static ConfigVariable<T> fn<T>(T callback(), {T Function(T) freeze}) =>
+  static ConfigVariable<T> fn<T>(T callback(), {T Function(T)? freeze}) =>
       ConfigVariable._fn(callback, freeze: freeze);
 
   /// Creates a configuration variable with the given [value].
   ///
   /// If [freeze] is passed, it's called when the variable is frozen to make the
   /// value unmodifiable as well.
-  static ConfigVariable<T> value<T>(T value, {T Function(T) freeze}) =>
+  static ConfigVariable<T> value<T>(T value, {T Function(T)? freeze}) =>
       ConfigVariable._value(value, freeze: freeze);
 
   /// Marks the variable as unmodifiable.
   void freeze() {
     if (_frozen) return;
     _frozen = true;
-    if (_freeze != null) {
+
+    var freeze = _freeze;
+    if (freeze != null) {
       if (_cached) {
-        _value = _freeze(_value);
+        // We can't use `!` for `_value` because `T` might itself be a nullable
+        // type. We don't necessarily know that `_value` isn't null, we just
+        // know that it's the value returned by `_callback`.
+        _value = freeze(_value as T);
       } else {
-        var oldCallback = _callback;
-        _callback = () => _freeze(oldCallback());
+        var oldCallback = _callback!;
+        _callback = () => freeze(oldCallback());
       }
     }
   }

lib/src/github.dart

@@ -33,12 +33,12 @@ import 'utils.dart';
 /// Git URL, this must be set explicitly.
 final githubRepo = InternalConfigVariable.fn<String>(() =>
     _repoFromOrigin() ??
-    _repoFromPubspec() ??
+    _parseHttp(pubspec.homepage ?? '') ??
     fail("pkg.githubRepo must be set to deploy to GitHub."));
 
 /// Returns the GitHub repo name from the Git configuration's
 /// `remote.origin.url` field.
-String _repoFromOrigin() {
+String? _repoFromOrigin() {
   try {
     var result = Process.runSync("git", ["config", "remote.origin.url"]);
     if (result.exitCode != 0) return null;
@@ -49,14 +49,10 @@ String _repoFromOrigin() {
   }
 }
 
-/// Returns the GitHub repo name from the pubspec's `homepage` field.
-String _repoFromPubspec() =>
-    pubspec.homepage == null ? null : _parseHttp(pubspec.homepage);
-
 /// Parses a GitHub repo name from an SSH reference or a `git://` URL.
 ///
 /// Returns `null` if it couldn't be parsed.
-String _parseGit(String url) {
+String? _parseGit(String url) {
   var match = RegExp(r"^(git@github\.com:|git://github\.com/)"
           r"(?<repo>[^/]+/[^/]+?)(\.git)?$")
       .firstMatch(url);
@@ -66,7 +62,7 @@ String _parseGit(String url) {
 /// Parses a GitHub repo name from an HTTP or HTTPS URL.
 ///
 /// Returns `null` if it couldn't be parsed.
-String _parseHttp(String url) {
+String? _parseHttp(String url) {
   var match = RegExp(r"^https?://github\.com/([^/]+/[^/]+?)(\.git)?($|/)")
       .firstMatch(url);
   return match == null ? null : match[1];
@@ -126,7 +122,7 @@ final githubPassword = InternalConfigVariable.fn<String>(() =>
 /// By default, this comes from the `GITHUB_BEARER_TOKEN` environment variable.
 /// If it's set, it's used in preference to [githubUser] and [githubPassword].
 /// To override this behavior, set its value to `null`.
-final githubBearerToken = InternalConfigVariable.value<String>(
+final githubBearerToken = InternalConfigVariable.value<String?>(
     Platform.environment["GITHUB_BEARER_TOKEN"]);
 
 /// Returns the HTTP basic authentication Authorization header from the
@@ -147,7 +143,7 @@ String get _authorization {
 ///
 /// If this is set to `null`, or by default if no `CHANGELOG.md` exists, no
 /// release notes will be added to the GitHub release.
-final githubReleaseNotes = InternalConfigVariable.fn<String>(() {
+final githubReleaseNotes = InternalConfigVariable.fn<String?>(() {
   if (!File("CHANGELOG.md").existsSync()) return null;
 
   return _lastChangelogSection() +
@@ -302,7 +298,7 @@ Future<void> _uploadExecutables(String os) async {
   ].map((architecture) async {
     var format = os == "windows" ? "zip" : "tar.gz";
     var package = "$standaloneName-$version-$os-$architecture.$format";
-    var response = await client.post("$uploadUrl?name=$package",
+    var response = await client.post(Uri.parse("$uploadUrl?name=$package"),
         headers: {
           "content-type":
               os == "windows" ? "application/zip" : "application/gzip",

lib/src/homebrew.dart

@@ -28,14 +28,14 @@ final homebrewRepo = InternalConfigVariable.fn<String>(
 /// If this isn't set, the task will default to looking for a single `.rb` file
 /// at the root of the repo without an `@` in its filename and modifying that.
 /// If there isn't exactly one such file, the task will fail.
-final homebrewFormula = InternalConfigVariable.value<String>(null);
+final homebrewFormula = InternalConfigVariable.value<String?>(null);
 
 /// Whether to update [homebrewFormula] in-place or copy it to a new
 /// `@`-versioned formula file for the current version number.
 ///
 /// By default, this is `true` if and only if [version] is a prerelease version.
 final homebrewCreateVersionedFormula =
-    InternalConfigVariable.fn<bool>(() => version.isPreRelease);
+    InternalConfigVariable.fn(() => version.isPreRelease);
 
 /// Whether [addHomebrewTasks] has been called yet.
 var _addedHomebrewTasks = false;
@@ -196,5 +196,5 @@ Future<String> _originHead(String repo) async {
 String _classify(Version version) => version
     .toString()
     .replaceAllMapped(
-        RegExp(r'[-_.]([a-zA-Z0-9])'), (match) => match[1].toUpperCase())
+        RegExp(r'[-_.]([a-zA-Z0-9])'), (match) => match[1]!.toUpperCase())
     .replaceAll('+', 'x');

lib/src/info.dart

@@ -14,21 +14,28 @@
 
 import 'dart:io';
 
+import 'package:grinder/grinder.dart';
 import 'package:path/path.dart' as p;
+import 'package:pub_semver/pub_semver.dart';
 import 'package:pubspec_parse/pubspec_parse.dart';
 
 import 'config_variable.dart';
 import 'utils.dart';
 
 /// The parsed pubspec for the CLI package.
 final pubspec = Pubspec.parse(File('pubspec.yaml').readAsStringSync(),
-    sourceUrl: 'pubspec.yaml');
+    sourceUrl: Uri(path: 'pubspec.yaml'));
 
 /// The name of the package, as specified in the pubspec.
 final dartName = pubspec.name;
 
 /// The package's version, as specified in the pubspec.
-final version = pubspec.version;
+final Version version = () {
+  var version = pubspec.version;
+  if (version != null) return version;
+
+  fail("The pubspec must declare a version number.");
+}();
 
 /// The default name of the package on package managers other than pub.
 ///
@@ -61,7 +68,7 @@ final botEmail = InternalConfigVariable.fn<String>(() => "cli_pkg@none");
 /// may be modified, but the values must be paths to executable files in the
 /// package.
 final executables = InternalConfigVariable.fn<Map<String, String>>(() {
-  var executables = rawPubspec['executables'] as Map<Object, Object>;
+  var executables = rawPubspec['executables'] as Map<dynamic, dynamic>?;
 
   return {
     for (var entry in (executables ?? {}).entries)