Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate to null safety #183

Merged
merged 7 commits into from
Apr 28, 2021
Merged

Migrate to null safety #183

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d2ecb16
Upgrade dependencies and fix analysis errors
jathak Apr 22, 2021
62357d5
Prep work before automated migration
jathak Apr 24, 2021
e4c6527
Automated migration + fixes
jathak Apr 24, 2021
57d645d
Update node_io dep to master branch
jathak Apr 24, 2021
4b99ae1
Use published version of node_io
jathak Apr 26, 2021
d834e0e
Code review
jathak Apr 28, 2021
4b03025
Code review + changelog
jathak Apr 28, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion bin/sass_migrator.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

MigratorRunner().execute(args.cast<String>());
}
21 changes: 11 additions & 10 deletions lib/src/migration_visitor.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import 'package:sass/src/visitor/recursive_ast.dart';

import 'exception.dart';
import 'patch.dart';
import 'utils.dart';

/// A visitor that migrates a stylesheet.
///
Expand Down Expand Up @@ -50,21 +51,21 @@ abstract class MigrationVisitor extends RecursiveAstVisitor {

/// The patches to be applied to the stylesheet being migrated.
@protected
List<Patch> get patches => UnmodifiableListView(_patches);
List<Patch> _patches;
List<Patch> get patches => UnmodifiableListView(assertNotNull(_patches));
List<Patch>? _patches;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done


/// URL of the stylesheet currently being migrated.
@protected
Uri get currentUrl => _currentUrl;
Uri _currentUrl;
Uri get currentUrl => assertNotNull(_currentUrl);
Uri? _currentUrl;

/// The importer that's being used to resolve relative imports.
///
/// If this is `null`, relative imports aren't supported in the current
/// stylesheet.
@protected
Importer get importer => _importer;
Importer _importer;
late Importer _importer;

MigrationVisitor(this.importCache, this.migrateDependencies);

Expand All @@ -87,7 +88,7 @@ abstract class MigrationVisitor extends RecursiveAstVisitor {
var oldPatches = _patches;
var oldUrl = _currentUrl;
_patches = [];
_currentUrl = node.span.sourceUrl;
_currentUrl = node.span.sourceUrl!;
super.visitStylesheet(node);
beforePatch(node);
var results = patches.isNotEmpty
Expand All @@ -102,7 +103,7 @@ abstract class MigrationVisitor extends RecursiveAstVisitor {
"it's loaded.");
}

_migrated[_currentUrl] = results;
_migrated[currentUrl] = results;
}

_patches = oldPatches;
Expand Down Expand Up @@ -139,7 +140,7 @@ abstract class MigrationVisitor extends RecursiveAstVisitor {
_importer = oldImporter;
} else {
_missingDependencies.putIfAbsent(
context.sourceUrl.resolveUri(dependency), () => context);
context.sourceUrl!.resolveUri(dependency), () => context);
}
}

Expand All @@ -151,9 +152,9 @@ abstract class MigrationVisitor extends RecursiveAstVisitor {
@protected
void addPatch(Patch patch, {bool beforeExisting = false}) {
if (beforeExisting) {
_patches.insert(0, patch);
_patches!.insert(0, patch);
} else {
_patches.add(patch);
_patches!.add(patch);
}
}

Expand Down
34 changes: 17 additions & 17 deletions lib/src/migrator.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import 'package:sass/src/import_cache.dart';

import 'package:args/command_runner.dart';
import 'package:glob/glob.dart';
import 'package:glob/list_local_fs.dart';
import 'package:meta/meta.dart';
import 'package:path/path.dart' as p;
import 'package:sass_migrator/src/util/node_modules_importer.dart';
Expand All @@ -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 >> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

String get invocation => super
.invocation
.replaceFirst("[arguments]", "[options] <entrypoints.scss...>");
Expand All @@ -45,7 +46,7 @@ abstract class Migrator extends Command<Map<Uri, String>> {
"See also https://sass-lang.com/documentation/cli/migrator#$name";

/// If true, dependencies will be migrated in addition to the entrypoints.
bool get migrateDependencies => globalResults['migrate-deps'] as bool;
bool get migrateDependencies => globalResults!['migrate-deps'] as bool;

/// Map of missing dependency URLs to the spans that import/use them.
///
Expand All @@ -61,7 +62,7 @@ abstract class Migrator extends Command<Map<Uri, String>> {
/// Files that did not require any changes, even if touched by the migrator,
/// should not be included map of results.
@protected
Map<Uri, String> migrateFile(
Map<Uri, String > migrateFile(
ImportCache importCache, Stylesheet stylesheet, Importer importer);

/// Runs this migrator.
Expand All @@ -72,14 +73,15 @@ abstract class Migrator extends Command<Map<Uri, String>> {
///
/// Entrypoints and dependencies that did not require any changes will not be
/// included in the results.
Map<Uri, String> run() {
Map<Uri, String > run() {
var allMigrated = <Uri, String>{};
var importer = FilesystemImporter('.');
var importCache = ImportCache([NodeModulesImporter()],
loadPaths: globalResults['load-path']);
var importCache = ImportCache(
importers: [NodeModulesImporter()],
loadPaths: globalResults!['load-path']);

var entrypoints = [
for (var argument in argResults.rest)
for (var argument in argResults!.rest)
for (var entry in Glob(argument).listSync())
if (entry is File) entry.path
];
Expand All @@ -91,15 +93,14 @@ abstract class Migrator extends Command<Map<Uri, String>> {
}

var migrated = migrateFile(importCache, tuple.item2, tuple.item1);
for (var file in migrated.keys) {
if (allMigrated.containsKey(file) &&
migrated[file] != allMigrated[file]) {
migrated.forEach((file, contents) {
if (allMigrated.containsKey(file) && contents != allMigrated[file]) {
throw MigrationException(
"The migrator has found multiple possible migrations for $file, "
"depending on the context in which it's loaded.");
}
allMigrated[file] = migrated[file];
}
allMigrated[file] = contents;
});
}

if (missingDependencies.isNotEmpty) _warnForMissingDependencies();
Expand All @@ -114,7 +115,7 @@ abstract class Migrator extends Command<Map<Uri, String>> {
/// In verbose mode, this instead prints a full warning with the source span
/// for each missing dependency.
void _warnForMissingDependencies() {
if (globalResults['verbose'] as bool) {
if (globalResults!['verbose'] as bool) {
for (var uri in missingDependencies.keys) {
emitWarning("Could not find Sass file at '${p.prettyUri(uri)}'.",
missingDependencies[uri]);
Expand All @@ -123,11 +124,10 @@ abstract class Migrator extends Command<Map<Uri, String>> {
var count = missingDependencies.length;
emitWarning(
"$count dependenc${count == 1 ? 'y' : 'ies'} could not be found.");
for (var uri in missingDependencies.keys) {
var context = missingDependencies[uri];
printStderr(' ${p.prettyUri(uri)} '
missingDependencies.forEach((url, context) {
printStderr(' ${p.prettyUri(url)} '
'@${p.prettyUri(context.sourceUrl)}:${context.start.line + 1}');
}
});
}
}
}
14 changes: 7 additions & 7 deletions lib/src/migrators/division.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ More info: https://sass-lang.com/d/slash-div""";
help: "Only migrate / expressions that are unambiguously division.",
negatable: false);

bool get isPessimistic => argResults['pessimistic'] as bool;
bool get isPessimistic => argResults!['pessimistic'] as bool;

@override
Map<Uri, String> migrateFile(
Expand Down Expand Up @@ -143,16 +143,16 @@ class _DivisionMigrationVisitor extends MigrationVisitor {
return false;
}

ListExpression channels;
ListExpression? channels;
if (node.arguments.positional.length == 1 &&
node.arguments.named.isEmpty &&
node.arguments.positional.first is ListExpression) {
channels = node.arguments.positional.first;
channels = node.arguments.positional.first as ListExpression?;
} else if (node.arguments.positional.isEmpty &&
node.arguments.named.containsKey(r'$channels') &&
node.arguments.named.length == 1 &&
node.arguments.named[r'$channels'] is ListExpression) {
channels = node.arguments.named[r'$channels'];
channels = node.arguments.named[r'$channels'] as ListExpression?;
}
if (channels == null ||
channels.hasBrackets ||
Expand All @@ -170,7 +170,7 @@ class _DivisionMigrationVisitor extends MigrationVisitor {
_patchOperatorToComma(last);
}
_withContext(() {
channels.contents[0].accept(this);
channels!.contents[0].accept(this);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

channels.contents[1].accept(this);
last.left.accept(this);
}, isDivisionAllowed: true);
Expand Down Expand Up @@ -326,7 +326,7 @@ class _DivisionMigrationVisitor extends MigrationVisitor {
/// ParenthesizedExpression.
void _patchParensIfAny(SassNode node) {
if (node is! ParenthesizedExpression) return;
var expression = (node as ParenthesizedExpression).expression;
var expression = node.expression;
if (expression is BinaryOperationExpression &&
expression.operator == BinaryOperator.dividedBy) {
return;
Expand All @@ -337,7 +337,7 @@ class _DivisionMigrationVisitor extends MigrationVisitor {

/// Runs [operation] with the given context.
void _withContext(void operation(),
{bool isDivisionAllowed, bool expectsNumericResult}) {
{bool? isDivisionAllowed, bool? expectsNumericResult}) {
var previousDivisionAllowed = _isDivisionAllowed;
var previousNumericResult = _expectsNumericResult;
if (isDivisionAllowed != null) _isDivisionAllowed = isDivisionAllowed;
Expand Down
Loading