Migrate to null safety #183

jathak · 2021-04-24T00:42:25Z

No description provided.

jathak · 2021-04-26T16:26:02Z

Windows tests are failing due to an issue cloning a git dependency

nex3 · 2021-04-26T20:19:05Z

bin/sass_migrator.dart

@@ -12,6 +12,6 @@ import 'package:sass_migrator/src/runner.dart';
 // We can't declare args as a List<String> or Iterable<String> beacause of
 // dart-lang/sdk#36627.
 main(Iterable args) {
-  if (process.argv != null) args = process.argv.skip(2);
+  if (process.argv != null) args = process.argv!.skip(2);


Suggested change

if (process.argv != null) args = process.argv!.skip(2);

var argv = process.argv;

if (argv != null) args = argv.skip(2);

I think it's generally better to do this slightly awkward two-step check than to include runtime non-null assertions where possible. Even though it's more verbose, it helps preserve the signal that every ! indicates something weird going on and needs extra attention and care.

nex3 · 2021-04-26T20:21:32Z

lib/src/migration_visitor.dart

+  List<Patch> get patches => UnmodifiableListView(assertNotNull(_patches));
+  List<Patch>? _patches;


Since you do sometimes need to access _patches directly to get the modifiable version of this list, I'd split this up even further to minimize the number of necessary non-null assertions:

Suggested change

List<Patch> get patches => UnmodifiableListView(assertNotNull(_patches));

List<Patch>? _patches;

List<Patch> get patches => UnmodifiableListView(_patches);

List<Patch> get _patches => assertNotNull(__patches);

List<Patch>? __patches;

nex3 · 2021-04-26T20:22:30Z

lib/src/migrator.dart

@@ -36,7 +37,7 @@ import 'utils.dart';
 ///
 /// Most migrators will want to create a subclass of [MigrationVisitor] and
 /// implement [migrateFile] with `MyMigrationVisitor(this, entrypoint).run()`.
-abstract class Migrator extends Command<Map<Uri, String>> {
+abstract class Migrator extends Command<Map<Uri, String >> {


There are a couple places here where a space is added after String in a generic type. Did you forget to reformat? Is this missing a formatter check in CI? Or is the formatter adding bogus spaces?

Whoops. My editor auto-formats on save, so I didn't think to run it manually (and I guess the migrator doesn't format after running).

It looks like the formatter check got dropped from the CI for both this repo and Dart Sass during the switch to GitHub Actions. Added it to the CI config.

nex3 · 2021-04-26T20:26:14Z

lib/src/migrators/division.dart

@@ -170,7 +170,7 @@ class _DivisionMigrationVisitor extends MigrationVisitor {
      _patchOperatorToComma(last);
    }
    _withContext(() {
-      channels.contents[0].accept(this);
+      channels!.contents[0].accept(this);


Add a comment referencing https://github.com/dart-lang/sdk/issues/45348 so it's clear why this non-null assertion is necessary.

nex3 · 2021-04-26T21:01:38Z

lib/src/utils.dart

@@ -17,6 +17,9 @@ import 'package:sass/src/ast/node.dart';
 import 'io.dart';
 import 'patch.dart';

+T assertNotNull<T>(T? object) =>
+    object ?? (throw AssertionError('Unexpected null value.'));


Part of the value of writing our own non-null assertion function is that it can produce a more relevant error message that explains why the value shouldn't be null, but putting it in a utils file kind of undermines that. I'd write a separate method for each class that needs to use it whose error message can explain why the variable should be initialized (e.g. https://github.com/sass/dart-sass/blob/295a11696240eca4f76f542fdb277b4b01bb7db6/lib/src/visitor/async_evaluate.dart#L528-L536).

Done.

All but one use of this was for a variable that's initialized in visitStylesheet of a MigrationVisitor, so I added a method there. _configuredVariables is initialized when a dependency is visited, but since it's the only case like that I just threw the error inline.

nex3 · 2021-04-26T23:10:19Z

lib/src/renamer.dart

@@ -183,7 +190,7 @@ class Renamer<T> {
          buffer.writeCharCode(next);
        }
      } else {
-        buffer.writeCharCode(char);
+        buffer.writeCharCode(char!);


If you make the if ({...}.contains(char)) check above more explicit you won't need this assertion.

nex3 · 2021-04-26T23:11:02Z

lib/src/util/bidirectional_map.dart

    return value;
  }

  /// Finds the keys associated with a given value.
  Iterable<K> keysForValue(V value) sync* {
-    if (_keysForValue.containsKey(value)) yield* _keysForValue[value];
+    if (_keysForValue.containsKey(value)) yield* _keysForValue[value]!;


Intermediate variable

nex3 · 2021-04-26T23:11:19Z

lib/src/utils.dart

@@ -135,11 +138,11 @@ bool isWhitespace(int character) =>
 /// `$` at the start of variable names.
 FileSpan nameSpan(SassNode node) {
  if (node is VariableDeclaration) {
-    var start = node.namespace == null ? 1 : node.namespace.length + 2;
+    var start = node.namespace == null ? 1 : node.namespace!.length + 2;


Intermediate variable, also below

nex3 · 2021-04-26T23:11:45Z

test/utils.dart

@@ -63,7 +65,7 @@ Future<void> _testHrx(File hrxFile, String migrator) async {

  if (files.expectedLog != null) {
    expect(process.stdout,
-        emitsInOrder(files.expectedLog.trimRight().split("\n")));
+        emitsInOrder(files.expectedLog!.trimRight().split("\n")));


Intermediate variable

nex3 · 2021-04-26T23:12:42Z

tool/grind.dart

@@ -43,7 +43,7 @@ sanityCheckBeforeRelease() {
 /// Returns the environment variable named [name], or throws an exception if it
 /// can't be found.
 String environment(String name) {
-  var value = Platform.environment[name];
+  String? value = Platform.environment[name];


I think this type annotation is unnecessary

Done (I added it because the migrator was getting confused without it, but I forgot to remove it afterwards)

nex3 · 2021-04-28T22:23:37Z

lib/src/migrators/module.dart

      var tuple = importCache.canonicalize(Uri.parse(ruleUrl),
          baseImporter: importer, baseUrl: currentUrl, forImport: true);
      var canonicalImport = tuple?.item2;
      if (references.orphanImportOnlyFiles.containsKey(canonicalImport)) {
        ruleUrl = null;
-        var url = references.orphanImportOnlyFiles[canonicalImport]?.url;
+        var url = references.orphanImportOnlyFiles[canonicalImport!]?.url;


I meant canonicalImport, not the result of Map.[].

nex3 · 2021-04-28T22:32:53Z

lib/src/migrators/module.dart

-    if (libraryUrls != null) {
-      url = minBy(libraryUrls, (url) => url.pathSegments.length);
+    if (libraryUrls != null && libraryUrls.isNotEmpty) {
+      Uri? minUrl = minBy(libraryUrls, (url) => url.pathSegments.length);


Generally I think it's cleaner to try to keep all type stuff on the RHS if possible, so if inference isn't working here I'd write:

Suggested change

Uri? minUrl = minBy(libraryUrls, (url) => url.pathSegments.length);

var minUrl = minBy<Uri>(libraryUrls, (url) => url.pathSegments.length);

Same below.

minBy has two type parameters, so is this still cleaner as minBy<Uri, int>?

Yeah, I think so.

jathak added 4 commits April 22, 2021 14:57

Upgrade dependencies and fix analysis errors

d2ecb16

Prep work before automated migration

62357d5

Automated migration + fixes

e4c6527

Update node_io dep to master branch

57d645d

jathak requested a review from nex3 April 26, 2021 16:25

Use published version of node_io

4b99ae1

nex3 requested changes Apr 26, 2021

View reviewed changes

Code review

d834e0e

jathak requested a review from nex3 April 28, 2021 17:14

nex3 approved these changes Apr 28, 2021

View reviewed changes

Code review + changelog

4b03025

jathak merged commit b4fc3d4 into master Apr 28, 2021

jathak deleted the null-safety branch April 28, 2021 23:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate to null safety #183

Migrate to null safety #183

jathak commented Apr 24, 2021 •

edited

Loading

jathak commented Apr 26, 2021

nex3 Apr 26, 2021

jathak Apr 27, 2021

nex3 Apr 26, 2021

jathak Apr 27, 2021

nex3 Apr 26, 2021

jathak Apr 27, 2021

nex3 Apr 26, 2021

jathak Apr 27, 2021

nex3 Apr 26, 2021

jathak Apr 28, 2021

nex3 Apr 26, 2021

jathak Apr 28, 2021

nex3 Apr 26, 2021

jathak Apr 28, 2021

nex3 Apr 26, 2021

jathak Apr 28, 2021

nex3 Apr 26, 2021

jathak Apr 28, 2021

nex3 Apr 26, 2021

jathak Apr 28, 2021

nex3 Apr 28, 2021

nex3 Apr 28, 2021

jathak Apr 28, 2021

nex3 Apr 28, 2021

	if (process.argv != null) args = process.argv!.skip(2);
	var argv = process.argv;
	if (argv != null) args = argv.skip(2);

		List<Patch> get patches => UnmodifiableListView(assertNotNull(_patches));
		List<Patch>? _patches;

-  List<Patch> get patches => UnmodifiableListView(assertNotNull(_patches));
-  List<Patch>? _patches;
+  List<Patch> get patches => UnmodifiableListView(_patches);
+  List<Patch> get _patches => assertNotNull(__patches);
+  List<Patch>? __patches;

	Uri? minUrl = minBy(libraryUrls, (url) => url.pathSegments.length);
	var minUrl = minBy<Uri>(libraryUrls, (url) => url.pathSegments.length);

Migrate to null safety #183

Migrate to null safety #183

Conversation

jathak commented Apr 24, 2021 • edited Loading

jathak commented Apr 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jathak commented Apr 24, 2021 •

edited

Loading