From 9fe3c661daea5a0a00058f188fa086f19bf66150 Mon Sep 17 00:00:00 2001
From: Jennifer Thakar <jathak@gmail.com>
Date: Thu, 10 Oct 2024 10:41:53 -0700
Subject: [PATCH] Code review

---
 CHANGELOG.md                                  |  2 +-
 lib/src/migrators/module.dart                 | 21 ++++++++++------
 .../{builtin_only => built_in_only}/basic.hrx |  2 +-
 .../built_in_only/get_function.hrx            | 25 +++++++++++++++++++
 .../local_shadow.hrx                          |  2 +-
 .../migrate_deps.hrx                          |  2 +-
 .../module_already_exists.hrx                 |  2 +-
 .../multiple_imports.hrx                      |  2 +-
 .../partially_migrated.hrx                    |  2 +-
 .../shadowed_by_import.hrx                    |  2 +-
 .../shadowed_by_use.hrx                       |  2 +-
 .../module/built_in_functions/namespacing.hrx |  2 ++
 12 files changed, 49 insertions(+), 17 deletions(-)
 rename test/migrators/module/built_in_functions/{builtin_only => built_in_only}/basic.hrx (94%)
 create mode 100644 test/migrators/module/built_in_functions/built_in_only/get_function.hrx
 rename test/migrators/module/built_in_functions/{builtin_only => built_in_only}/local_shadow.hrx (93%)
 rename test/migrators/module/built_in_functions/{builtin_only => built_in_only}/migrate_deps.hrx (92%)
 rename test/migrators/module/built_in_functions/{builtin_only => built_in_only}/module_already_exists.hrx (94%)
 rename test/migrators/module/built_in_functions/{builtin_only => built_in_only}/multiple_imports.hrx (92%)
 rename test/migrators/module/built_in_functions/{builtin_only => built_in_only}/partially_migrated.hrx (93%)
 rename test/migrators/module/built_in_functions/{builtin_only => built_in_only}/shadowed_by_import.hrx (93%)
 rename test/migrators/module/built_in_functions/{builtin_only => built_in_only}/shadowed_by_use.hrx (94%)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 2392b8b..1dacf10 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -2,7 +2,7 @@
 
 ### Module Migrator
 
-* Add a new `--builtin-only` flag, which migrates global functions to their
+* Add a new `--built-in-only` flag, which migrates global functions to their
   `sass:` module equivalents, while leaving `@import` rules unchanged.
 
 * Fix bug where some functions (`opacity`, `is-bracketed`, and
diff --git a/lib/src/migrators/module.dart b/lib/src/migrators/module.dart
index 26eb154..ba623ec 100644
--- a/lib/src/migrators/module.dart
+++ b/lib/src/migrators/module.dart
@@ -32,7 +32,7 @@ class ModuleMigrator extends Migrator {
 
   @override
   final argParser = ArgParser()
-    ..addFlag('builtin-only',
+    ..addFlag('built-in-only',
         help: 'Migrates global functions without migrating @import.')
     ..addMultiOption('remove-prefix',
         abbr: 'p',
@@ -79,12 +79,12 @@ class ModuleMigrator extends Migrator {
   Map<Uri, String> migrateFile(
       ImportCache importCache, Stylesheet stylesheet, Importer importer) {
     var forwards = {for (var arg in argResults!['forward']) ForwardType(arg)};
-    var builtInOnly = argResults!['builtin-only'] as bool;
+    var builtInOnly = argResults!['built-in-only'] as bool;
     if (builtInOnly &&
         (argResults!.wasParsed('forward') ||
             argResults!.wasParsed('remove-prefix'))) {
       throw MigrationException('--forward and --remove-prefix may not be '
-          'passed with --builtin-only.');
+          'passed with --built-in-only.');
     }
     if (forwards.contains(ForwardType.prefixed) &&
         !argResults!.wasParsed('remove-prefix')) {
@@ -630,7 +630,7 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
       });
     }
 
-    if (node.name == "get-function" && !builtInOnly) {
+    if (node.name == "get-function") {
       var declaration = references.getFunctionReferences[node];
       if (declaration != null) {
         _unreferencable.check(declaration, node);
@@ -648,12 +648,15 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
       // Warn for get-function calls without a static name.
       var nameArg =
           node.arguments.named['name'] ?? node.arguments.positional.first;
-      if (nameArg is! StringExpression || nameArg.text.asPlain == null) {
+      if ((nameArg is! StringExpression || nameArg.text.asPlain == null) &&
+          !builtInOnly) {
         emitWarning(
             "get-function call may require \$module parameter", nameArg.span);
         return;
       }
 
+      if (builtInOnly && declaration != null) return;
+
       _patchNamespaceForFunction(node, declaration, (namespace) {
         var beforeParen = node.span.end.offset - 1;
         addPatch(Patch(node.span.file.span(beforeParen, beforeParen),
@@ -794,9 +797,11 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
       }
       if (ruleUrl != null) {
         if (builtInOnly) {
-          _upstreamStylesheets.add(currentUrl);
-          if (migrateDependencies) visitDependency(ruleUrl, import.span);
-          _upstreamStylesheets.remove(currentUrl);
+          if (migrateDependencies) {
+            _upstreamStylesheets.add(currentUrl);
+            visitDependency(ruleUrl, import.span);
+            _upstreamStylesheets.remove(currentUrl);
+          }
         } else if (_useAllowed) {
           migratedRules.addAll(_migrateImportToRules(ruleUrl, import.span));
         } else {
diff --git a/test/migrators/module/built_in_functions/builtin_only/basic.hrx b/test/migrators/module/built_in_functions/built_in_only/basic.hrx
similarity index 94%
rename from test/migrators/module/built_in_functions/builtin_only/basic.hrx
rename to test/migrators/module/built_in_functions/built_in_only/basic.hrx
index f1af697..7ae2ad6 100644
--- a/test/migrators/module/built_in_functions/builtin_only/basic.hrx
+++ b/test/migrators/module/built_in_functions/built_in_only/basic.hrx
@@ -1,5 +1,5 @@
 <==> arguments
---builtin-only
+--built-in-only
 
 <==> input/entrypoint.scss
 @import "library";
diff --git a/test/migrators/module/built_in_functions/built_in_only/get_function.hrx b/test/migrators/module/built_in_functions/built_in_only/get_function.hrx
new file mode 100644
index 0000000..62d9546
--- /dev/null
+++ b/test/migrators/module/built_in_functions/built_in_only/get_function.hrx
@@ -0,0 +1,25 @@
+<==> arguments
+--built-in-only
+
+<==> input/entrypoint.scss
+@import "library";
+
+a {
+  b: call(get-function(mix), red, blue);
+  c: call(get-function(fn), green);
+}
+
+<==> input/_library.scss
+@function fn($x) {
+  @return $x;
+}
+
+<==> output/entrypoint.scss
+@use "sass:color";
+@use "sass:meta";
+@import "library";
+
+a {
+  b: meta.call(meta.get-function(mix, $module: "color"), red, blue);
+  c: meta.call(meta.get-function(fn), green);
+}
diff --git a/test/migrators/module/built_in_functions/builtin_only/local_shadow.hrx b/test/migrators/module/built_in_functions/built_in_only/local_shadow.hrx
similarity index 93%
rename from test/migrators/module/built_in_functions/builtin_only/local_shadow.hrx
rename to test/migrators/module/built_in_functions/built_in_only/local_shadow.hrx
index 7b62b0f..d417c2d 100644
--- a/test/migrators/module/built_in_functions/builtin_only/local_shadow.hrx
+++ b/test/migrators/module/built_in_functions/built_in_only/local_shadow.hrx
@@ -1,5 +1,5 @@
 <==> arguments
---builtin-only
+--built-in-only
 
 <==> input/entrypoint.scss
 @import "library";
diff --git a/test/migrators/module/built_in_functions/builtin_only/migrate_deps.hrx b/test/migrators/module/built_in_functions/built_in_only/migrate_deps.hrx
similarity index 92%
rename from test/migrators/module/built_in_functions/builtin_only/migrate_deps.hrx
rename to test/migrators/module/built_in_functions/built_in_only/migrate_deps.hrx
index 0c2d538..7a53fa2 100644
--- a/test/migrators/module/built_in_functions/builtin_only/migrate_deps.hrx
+++ b/test/migrators/module/built_in_functions/built_in_only/migrate_deps.hrx
@@ -1,5 +1,5 @@
 <==> arguments
---builtin-only --migrate-deps
+--built-in-only --migrate-deps
 
 <==> input/entrypoint.scss
 @import "library";
diff --git a/test/migrators/module/built_in_functions/builtin_only/module_already_exists.hrx b/test/migrators/module/built_in_functions/built_in_only/module_already_exists.hrx
similarity index 94%
rename from test/migrators/module/built_in_functions/builtin_only/module_already_exists.hrx
rename to test/migrators/module/built_in_functions/built_in_only/module_already_exists.hrx
index f08a2fc..c03c437 100644
--- a/test/migrators/module/built_in_functions/builtin_only/module_already_exists.hrx
+++ b/test/migrators/module/built_in_functions/built_in_only/module_already_exists.hrx
@@ -1,5 +1,5 @@
 <==> arguments
---builtin-only
+--built-in-only
 
 <==> input/entrypoint.scss
 @use "sass:color";
diff --git a/test/migrators/module/built_in_functions/builtin_only/multiple_imports.hrx b/test/migrators/module/built_in_functions/built_in_only/multiple_imports.hrx
similarity index 92%
rename from test/migrators/module/built_in_functions/builtin_only/multiple_imports.hrx
rename to test/migrators/module/built_in_functions/built_in_only/multiple_imports.hrx
index 6a68114..49c91a2 100644
--- a/test/migrators/module/built_in_functions/builtin_only/multiple_imports.hrx
+++ b/test/migrators/module/built_in_functions/built_in_only/multiple_imports.hrx
@@ -1,5 +1,5 @@
 <==> arguments
---builtin-only --migrate-deps
+--built-in-only --migrate-deps
 
 <==> input/entrypoint.scss
 @import "a", "b";
diff --git a/test/migrators/module/built_in_functions/builtin_only/partially_migrated.hrx b/test/migrators/module/built_in_functions/built_in_only/partially_migrated.hrx
similarity index 93%
rename from test/migrators/module/built_in_functions/builtin_only/partially_migrated.hrx
rename to test/migrators/module/built_in_functions/built_in_only/partially_migrated.hrx
index 0e60b37..12162c7 100644
--- a/test/migrators/module/built_in_functions/builtin_only/partially_migrated.hrx
+++ b/test/migrators/module/built_in_functions/built_in_only/partially_migrated.hrx
@@ -1,5 +1,5 @@
 <==> arguments
---builtin-only
+--built-in-only
 
 <==> input/entrypoint.scss
 @use "sass:math";
diff --git a/test/migrators/module/built_in_functions/builtin_only/shadowed_by_import.hrx b/test/migrators/module/built_in_functions/built_in_only/shadowed_by_import.hrx
similarity index 93%
rename from test/migrators/module/built_in_functions/builtin_only/shadowed_by_import.hrx
rename to test/migrators/module/built_in_functions/built_in_only/shadowed_by_import.hrx
index 493f55c..266f60a 100644
--- a/test/migrators/module/built_in_functions/builtin_only/shadowed_by_import.hrx
+++ b/test/migrators/module/built_in_functions/built_in_only/shadowed_by_import.hrx
@@ -1,5 +1,5 @@
 <==> arguments
---builtin-only
+--built-in-only
 
 <==> input/entrypoint.scss
 @import "library";
diff --git a/test/migrators/module/built_in_functions/builtin_only/shadowed_by_use.hrx b/test/migrators/module/built_in_functions/built_in_only/shadowed_by_use.hrx
similarity index 94%
rename from test/migrators/module/built_in_functions/builtin_only/shadowed_by_use.hrx
rename to test/migrators/module/built_in_functions/built_in_only/shadowed_by_use.hrx
index 7ff1441..3038439 100644
--- a/test/migrators/module/built_in_functions/builtin_only/shadowed_by_use.hrx
+++ b/test/migrators/module/built_in_functions/built_in_only/shadowed_by_use.hrx
@@ -1,5 +1,5 @@
 <==> arguments
---builtin-only
+--built-in-only
 
 <==> input/entrypoint.scss
 @use "library" as *;
diff --git a/test/migrators/module/built_in_functions/namespacing.hrx b/test/migrators/module/built_in_functions/namespacing.hrx
index 7cf86d4..5904584 100644
--- a/test/migrators/module/built_in_functions/namespacing.hrx
+++ b/test/migrators/module/built_in_functions/namespacing.hrx
@@ -2,6 +2,7 @@
 $a: mix(red, blue);
 $b: str-length("hello");
 $c: scale-color(blue, $lightness: -10%);
+$d: call(get-function(mix), red, blue);
 
 @function fn($args...) {
   @return keywords($args);
@@ -14,6 +15,7 @@ $c: scale-color(blue, $lightness: -10%);
 $a: color.mix(red, blue);
 $b: string.length("hello");
 $c: color.scale(blue, $lightness: -10%);
+$d: meta.call(meta.get-function(mix, $module: "color"), red, blue);
 
 @function fn($args...) {
   @return meta.keywords($args);