fix(shaker): avoid collision between function name and its param (#1081)

* fix(shaker): avoid collision between function name and its param (fixes #1055)

* fix(shaker): incorrect identifier type detection for void expressions

.changeset/cuddly-laws-drum.md

@@ -0,0 +1,6 @@
+---
+'@linaria/shaker': patch
+'@linaria/utils': patch
+---
+
+Fix an incorrect dead-code detection when a function has a parameter with the same name as the function itself. Fixes #1055

.changeset/poor-socks-check.md

@@ -0,0 +1,6 @@
+---
+'@linaria/shaker': patch
+'@linaria/utils': patch
+---
+
+Fix rare use case when `void`-expression causes too aggressive tree-shaking. Fixes #1055.

packages/babel/src/index.ts

@@ -13,6 +13,7 @@ import loadLinariaOptions from './transform-stages/helpers/loadLinariaOptions';
 
 export { slugify } from '@linaria/utils';
 
+export { default as preeval } from './plugins/preeval';
 export * from './utils/collectTemplateDependencies';
 export { default as collectTemplateDependencies } from './utils/collectTemplateDependencies';
 export { default as withLinariaMetadata } from './utils/withLinariaMetadata';

packages/shaker/src/plugins/__tests__/__snapshots__/shaker-plugin.test.ts.snap

@@ -20,6 +20,16 @@ exports[`shaker should process array patterns 1`] = `
 export { c };"
 `;
 
+exports[`shaker should process identifiers in void expressions as references 1`] = `
+"let _;
+
+function b(b) {
+  void _;
+}
+
+exports.b = b;"
+`;
+
 exports[`shaker should process object patterns 1`] = `
 "const {
   b: b1

packages/shaker/src/plugins/__tests__/shaker-plugin.test.ts

@@ -161,4 +161,22 @@ describe('shaker', () => {
     expect(code).toMatchSnapshot();
     expect(metadata.imports.size).toBe(0);
   });
+
+  it('should process identifiers in void expressions as references', () => {
+    const { code, metadata } = keep(['b'])`
+      let _;
+
+      const a = void _;
+
+      function b(b) {
+        void _;
+      }
+
+      exports.a = a;
+      exports.b = b;
+    `;
+
+    expect(code).toMatchSnapshot();
+    expect(metadata.imports.size).toBe(0);
+  });
 });

packages/testkit/src/__snapshots__/preeval.test.ts.snap

@@ -0,0 +1,21 @@
+// Jest Snapshot v1, https://goo.gl/fbAQLP
+
+exports[`preeval should keep getGlobal but remove window-related code 1`] = `
+"function getGlobal() {
+  if (typeof globalThis !== \\"undefined\\") {
+    return globalThis;
+  }
+
+  if (typeof window !== \\"undefined\\") {}
+
+  if (typeof global !== \\"undefined\\") {
+    return global;
+  }
+
+  if (typeof self !== \\"undefined\\") {
+    return self;
+  }
+
+  return mockGlobal;
+}"
+`;

packages/testkit/src/preeval.test.ts

@@ -0,0 +1,62 @@
+import { join } from 'path';
+
+import { transformSync } from '@babel/core';
+import dedent from 'dedent';
+
+import { preeval } from '@linaria/babel-preset';
+
+const run = (code: TemplateStringsArray) => {
+  const filename = join(__dirname, 'source.js');
+  const formattedCode = dedent(code);
+
+  const transformed = transformSync(formattedCode, {
+    babelrc: false,
+    configFile: false,
+    filename,
+    plugins: [
+      [
+        preeval,
+        {
+          evaluate: true,
+        },
+      ],
+    ],
+  });
+
+  if (!transformed) {
+    throw new Error(`Something went wrong with ${filename}`);
+  }
+
+  return {
+    code: transformed.code,
+    // metadata: transformed.metadata.__linariaShaker,
+  };
+};
+
+describe('preeval', () => {
+  it('should keep getGlobal but remove window-related code', () => {
+    const { code } = run`
+      function getGlobal() {
+        if (typeof globalThis !== "undefined") {
+          return globalThis;
+        }
+
+        if (typeof window !== "undefined") {
+          return window;
+        }
+
+        if (typeof global !== "undefined") {
+          return global;
+        }
+
+        if (typeof self !== "undefined") {
+          return self;
+        }
+
+        return mockGlobal;
+      }
+    `;
+
+    expect(code).toMatchSnapshot();
+  });
+});

packages/utils/src/collectExportsAndImports.ts

@@ -27,6 +27,7 @@ import type {
 
 import { warn } from '@linaria/logger';
 
+import { getScope } from './getScope';
 import isExports from './isExports';
 import isNotNull from './isNotNull';
 import isRequire from './isRequire';
@@ -637,7 +638,7 @@ function unfoldNamespaceImport(
     return [importItem];
   }
 
-  const binding = local.scope.getBinding(local.node.name);
+  const binding = getScope(local).getBinding(local.node.name);
   if (!binding?.referenced) {
     // Imported namespace is not referenced and probably not used,
     // but it can have side effects, so we should keep it as is

packages/utils/src/findIdentifiers.ts

@@ -1,12 +1,29 @@
 import type { NodePath } from '@babel/traverse';
 import type { Node, Identifier, JSXIdentifier } from '@babel/types';
 
+import { getScope } from './getScope';
+
 type FindType = 'binding' | 'both' | 'referenced';
 
+function isInVoid(path: NodePath): boolean {
+  return path.parentPath?.isUnaryExpression({ operator: 'void' }) ?? false;
+}
+
+function isBindingIdentifier(path: NodePath): path is NodePath<Identifier> {
+  return path.isBindingIdentifier() && !isInVoid(path);
+}
+
+function isReferencedIdentifier(
+  path: NodePath
+): path is NodePath<Identifier | JSXIdentifier> {
+  return path.isReferencedIdentifier() || isInVoid(path);
+}
+
+// For some reasons, `isBindingIdentifier` returns true for identifiers inside `void` expressions.
 const checkers: Record<FindType, (ex: NodePath) => boolean> = {
-  binding: (ex) => ex.isBindingIdentifier(),
-  both: (ex) => ex.isBindingIdentifier() || ex.isReferencedIdentifier(),
-  referenced: (ex) => ex.isReferencedIdentifier(),
+  binding: (ex) => isBindingIdentifier(ex),
+  both: (ex) => isBindingIdentifier(ex) || isReferencedIdentifier(ex),
+  referenced: (ex) => isReferencedIdentifier(ex),
 };
 
 export function nonType(path: NodePath): boolean {
@@ -38,7 +55,7 @@ export default function findIdentifiers(
 
       // TODO: Is there a better way to check that it's a local variable?
 
-      const binding = path.scope.getBinding(path.node.name);
+      const binding = getScope(path).getBinding(path.node.name);
       if (!binding) {
         return;
       }

packages/utils/src/getScope.ts

@@ -0,0 +1,9 @@
+import type { NodePath } from '@babel/traverse';
+
+export function getScope(path: NodePath) {
+  // In some nodes (like FunctionDeclaration) `scope` for `id` returns
+  // local function scope instead of a scope where function is declared.
+  return path.key === 'id' && path.parent === path.scope.block
+    ? path.scope.parent
+    : path.scope;
+}

packages/utils/src/isExports.ts

@@ -1,5 +1,7 @@
 import type { NodePath } from '@babel/traverse';
 
+import { getScope } from './getScope';
+
 /**
  * Checks that specified Identifier is a global `exports`
  * @param id
@@ -9,8 +11,9 @@ export default function isExports(id: NodePath | null | undefined) {
     return false;
   }
 
+  const scope = getScope(id);
+
   return (
-    id.scope.getBinding('exports') === undefined &&
-    id.scope.hasGlobal('exports')
+    scope.getBinding('exports') === undefined && scope.hasGlobal('exports')
   );
 }

packages/utils/src/isRequire.ts

@@ -1,5 +1,7 @@
 import type { NodePath } from '@babel/traverse';
 
+import { getScope } from './getScope';
+
 /**
  * Checks that specified Identifier is a global `require`
  * @param id
@@ -9,8 +11,9 @@ export default function isRequire(id: NodePath | null | undefined) {
     return false;
   }
 
+  const scope = getScope(id);
+
   return (
-    id.scope.getBinding('require') === undefined &&
-    id.scope.hasGlobal('require')
+    scope.getBinding('require') === undefined && scope.hasGlobal('require')
   );
 }

packages/utils/src/isUnnecessaryReactCall.ts

@@ -3,6 +3,7 @@ import type { CallExpression } from '@babel/types';
 
 import type { IImport, ISideEffectImport } from './collectExportsAndImports';
 import collectExportsAndImports from './collectExportsAndImports';
+import { getScope } from './getScope';
 
 function getCallee(p: NodePath<CallExpression>) {
   const callee = p.get('callee');
@@ -66,7 +67,7 @@ function isClassicReactRuntime(
   if (reactImports.length === 0) return false;
   const callee = getCallee(p);
   if (callee.isIdentifier() && isHookOrCreateElement(callee.node.name)) {
-    const bindingPath = callee.scope.getBinding(callee.node.name)?.path;
+    const bindingPath = getScope(callee).getBinding(callee.node.name)?.path;
     return reactImports.some((i) => bindingPath?.isAncestor(i.local));
   }
 
@@ -89,7 +90,7 @@ function isClassicReactRuntime(
       return false;
     }
 
-    const bindingPath = object.scope.getBinding(object.node.name)?.path;
+    const bindingPath = getScope(object).getBinding(object.node.name)?.path;
     return bindingPath?.isAncestor(defaultImport.local) ?? false;
   }
 

packages/utils/src/scopeHelpers.ts

@@ -5,11 +5,12 @@ import type { Binding, NodePath } from '@babel/traverse';
 import type { Identifier, JSXIdentifier, Program } from '@babel/types';
 
 import findIdentifiers, { nonType } from './findIdentifiers';
+import { getScope } from './getScope';
 import isNotNull from './isNotNull';
 import isRemoved from './isRemoved';
 
 function getBinding(path: NodePath<Identifier | JSXIdentifier>) {
-  const binding = path.scope.getBinding(path.node.name);
+  const binding = getScope(path).getBinding(path.node.name);
   if (!binding) {
     return undefined;
   }
@@ -275,7 +276,7 @@ function removeUnreferenced(items: NodePath<Identifier | JSXIdentifier>[]) {
   const referenced = new Set<NodePath<Identifier | JSXIdentifier>>();
   items.forEach((item) => {
     if (!item.node || isRemoved(item)) return;
-    const binding = item.scope.getBinding(item.node.name);
+    const binding = getScope(item).getBinding(item.node.name);
     if (!binding) return;
     const hasReferences =
       binding.referencePaths.filter((i) => !isRemoved(i)).length > 0;
@@ -306,7 +307,8 @@ function removeUnreferenced(items: NodePath<Identifier | JSXIdentifier>[]) {
 function removeWithRelated(paths: NodePath[]) {
   if (paths.length === 0) return;
 
-  const rootPath = paths[0].scope.getProgramParent().path as NodePath<Program>;
+  const rootPath = getScope(paths[0]).getProgramParent()
+    .path as NodePath<Program>;
 
   if (!fixed.has(rootPath)) {
     // Some libraries don't care about bindings, references, and other staff
@@ -328,7 +330,7 @@ function removeWithRelated(paths: NodePath[]) {
     );
     declared.push(
       ...findIdentifiers([deletingPath], 'binding').map(
-        (i) => i.scope.getBinding(i.node.name)!
+        (i) => getScope(i).getBinding(i.node.name)!
       )
     );
 

packages/utils/src/visitors/JSXElementsRemover.ts

@@ -6,6 +6,7 @@ import type {
   JSX,
 } from '@babel/types';
 
+import { getScope } from '../getScope';
 import { mutate } from '../scopeHelpers';
 
 function getFunctionName(path: NodePath<FunctionNode>): string | null {
@@ -24,7 +25,7 @@ export default function JSXElementsRemover(
 
   // We can do even more
   // If that JSX is a result of a function, we can replace the function body.
-  const functionScope = path.scope.getFunctionParent();
+  const functionScope = getScope(path).getFunctionParent();
   const scopePath = functionScope?.path;
   if (scopePath?.isFunction()) {
     const emptyBody = t.blockStatement([t.returnStatement(nullLiteral)]);