From 04e02986cd24281d662ebe43e2cd2be0af3f7e40 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Matej=20=C4=8Cern=C3=BD?=
 <112722215+cernymatej@users.noreply.github.com>
Date: Thu, 9 Jan 2025 16:11:44 +0100
Subject: [PATCH] fix(nuxt): don't hoist identifiers declared locally in
 `definePageMeta` when extracting page metadata (#30490)

---
 packages/nuxt/src/core/utils/parse.ts        |  82 ++++++--
 packages/nuxt/src/pages/plugins/page-meta.ts |  20 +-
 packages/nuxt/test/page-metadata.test.ts     | 196 ++++++++++++++++++-
 packages/nuxt/test/parse.test.ts             |  85 +++++++-
 4 files changed, 360 insertions(+), 23 deletions(-)

diff --git a/packages/nuxt/src/core/utils/parse.ts b/packages/nuxt/src/core/utils/parse.ts
index c063dbcdfa..0f6f6d8b5e 100644
--- a/packages/nuxt/src/core/utils/parse.ts
+++ b/packages/nuxt/src/core/utils/parse.ts
@@ -53,12 +53,30 @@ export function withLocations<T> (node: T): WithLocations<T> {
   return node as WithLocations<T>
 }
 
+/**
+ * A function to check whether scope A is a child of scope B.
+ * @example
+ * ```ts
+ * isChildScope('0-1-2', '0-1') // true
+ * isChildScope('0-1', '0-1') // false
+ * ```
+ *
+ * @param a the child scope
+ * @param b the parent scope
+ * @returns true if scope A is a child of scope B, false otherwise (also when they are the same)
+ */
+function isChildScope (a: string, b: string) {
+  return a.startsWith(b) && a.length > b.length
+}
+
 abstract class BaseNode<T extends Node = Node> {
   abstract type: string
+  readonly scope: string
   node: WithLocations<T>
 
-  constructor (node: WithLocations<T>) {
+  constructor (node: WithLocations<T>, scope: string) {
     this.node = node
+    this.scope = scope
   }
 
   /**
@@ -72,6 +90,14 @@ abstract class BaseNode<T extends Node = Node> {
    * For instance, for a function parameter, this would be the end of the function declaration.
    */
   abstract get end (): number
+
+  /**
+   * Check if the node is defined under a specific scope.
+   * @param scope
+   */
+  isUnderScope (scope: string) {
+    return isChildScope(this.scope, scope)
+  }
 }
 
 class IdentifierNode extends BaseNode<Identifier> {
@@ -90,8 +116,8 @@ class FunctionParamNode extends BaseNode {
   type = 'FunctionParam' as const
   fnNode: WithLocations<FunctionDeclaration | FunctionExpression | ArrowFunctionExpression>
 
-  constructor (node: WithLocations<Node>, fnNode: WithLocations<FunctionDeclaration | FunctionExpression | ArrowFunctionExpression>) {
-    super(node)
+  constructor (node: WithLocations<Node>, scope: string, fnNode: WithLocations<FunctionDeclaration | FunctionExpression | ArrowFunctionExpression>) {
+    super(node, scope)
     this.fnNode = fnNode
   }
 
@@ -120,8 +146,8 @@ class VariableNode extends BaseNode<Identifier> {
   type = 'Variable' as const
   variableNode: WithLocations<VariableDeclaration>
 
-  constructor (node: WithLocations<Identifier>, variableNode: WithLocations<VariableDeclaration>) {
-    super(node)
+  constructor (node: WithLocations<Identifier>, scope: string, variableNode: WithLocations<VariableDeclaration>) {
+    super(node, scope)
     this.variableNode = variableNode
   }
 
@@ -138,8 +164,8 @@ class ImportNode extends BaseNode<ImportSpecifier | ImportDefaultSpecifier | Imp
   type = 'Import' as const
   importNode: WithLocations<Node>
 
-  constructor (node: WithLocations<ImportSpecifier | ImportDefaultSpecifier | ImportNamespaceSpecifier>, importNode: WithLocations<Node>) {
-    super(node)
+  constructor (node: WithLocations<ImportSpecifier | ImportDefaultSpecifier | ImportNamespaceSpecifier>, scope: string, importNode: WithLocations<Node>) {
+    super(node, scope)
     this.importNode = importNode
   }
 
@@ -156,8 +182,8 @@ class CatchParamNode extends BaseNode {
   type = 'CatchParam' as const
   catchNode: WithLocations<CatchClause>
 
-  constructor (node: WithLocations<Node>, catchNode: WithLocations<CatchClause>) {
-    super(node)
+  constructor (node: WithLocations<Node>, scope: string, catchNode: WithLocations<CatchClause>) {
+    super(node, scope)
     this.catchNode = catchNode
   }
 
@@ -264,7 +290,7 @@ export class ScopeTracker {
 
     const identifiers = getPatternIdentifiers(param)
     for (const identifier of identifiers) {
-      this.declareIdentifier(identifier.name, new FunctionParamNode(identifier, fn))
+      this.declareIdentifier(identifier.name, new FunctionParamNode(identifier, this.scopeIndexKey, fn))
     }
   }
 
@@ -276,10 +302,10 @@ export class ScopeTracker {
       this.declareIdentifier(
         identifier.name,
         parent.type === 'VariableDeclaration'
-          ? new VariableNode(identifier, parent)
+          ? new VariableNode(identifier, this.scopeIndexKey, parent)
           : parent.type === 'CatchClause'
-            ? new CatchParamNode(identifier, parent)
-            : new FunctionParamNode(identifier, parent),
+            ? new CatchParamNode(identifier, this.scopeIndexKey, parent)
+            : new FunctionParamNode(identifier, this.scopeIndexKey, parent),
       )
     }
   }
@@ -295,7 +321,7 @@ export class ScopeTracker {
       case 'FunctionDeclaration':
         // declare function name for named functions, skip for `export default`
         if (node.id?.name) {
-          this.declareIdentifier(node.id.name, new FunctionNode(node))
+          this.declareIdentifier(node.id.name, new FunctionNode(node, this.scopeIndexKey))
         }
         this.pushScope()
         for (const param of node.params) {
@@ -309,7 +335,7 @@ export class ScopeTracker {
         this.pushScope()
         // can be undefined, for example in class method definitions
         if (node.id?.name) {
-          this.declareIdentifier(node.id.name, new FunctionNode(node))
+          this.declareIdentifier(node.id.name, new FunctionNode(node, this.scopeIndexKey))
         }
 
         this.pushScope()
@@ -333,7 +359,7 @@ export class ScopeTracker {
       case 'ClassDeclaration':
         // declare class name for named classes, skip for `export default`
         if (node.id?.name) {
-          this.declareIdentifier(node.id.name, new IdentifierNode(withLocations(node.id)))
+          this.declareIdentifier(node.id.name, new IdentifierNode(withLocations(node.id), this.scopeIndexKey))
         }
         break
 
@@ -342,13 +368,13 @@ export class ScopeTracker {
         // e.g. const MyClass = class InternalClassName { // InternalClassName is only available within the class body
         this.pushScope()
         if (node.id?.name) {
-          this.declareIdentifier(node.id.name, new IdentifierNode(withLocations(node.id)))
+          this.declareIdentifier(node.id.name, new IdentifierNode(withLocations(node.id), this.scopeIndexKey))
         }
         break
 
       case 'ImportDeclaration':
         for (const specifier of node.specifiers) {
-          this.declareIdentifier(specifier.local.name, new ImportNode(withLocations(specifier), node))
+          this.declareIdentifier(specifier.local.name, new ImportNode(withLocations(specifier), this.scopeIndexKey, node))
         }
         break
 
@@ -429,6 +455,26 @@ export class ScopeTracker {
     return null
   }
 
+  getCurrentScope () {
+    return this.scopeIndexKey
+  }
+
+  /**
+   * Check if the current scope is a child of a specific scope.
+   * @example
+   * ```ts
+   * // current scope is 0-1
+   * isCurrentScopeUnder('0') // true
+   * isCurrentScopeUnder('0-1') // false
+   * ```
+   *
+   * @param scope the parent scope
+   * @returns `true` if the current scope is a child of the specified scope, `false` otherwise (also when they are the same)
+   */
+  isCurrentScopeUnder (scope: string) {
+    return isChildScope(this.scopeIndexKey, scope)
+  }
+
   /**
    * Freezes the scope tracker, preventing further declarations.
    * It also resets the scope index stack to its initial state, so that the scope tracker can be reused.
diff --git a/packages/nuxt/src/pages/plugins/page-meta.ts b/packages/nuxt/src/pages/plugins/page-meta.ts
index a5972f4e44..bde07df6a3 100644
--- a/packages/nuxt/src/pages/plugins/page-meta.ts
+++ b/packages/nuxt/src/pages/plugins/page-meta.ts
@@ -228,6 +228,8 @@ export const PageMetaPlugin = (options: PageMetaPluginOptions = {}) => createUnp
 
           if (!meta) { return }
 
+          const definePageMetaScope = scopeTracker.getCurrentScope()
+
           walk(meta, {
             scopeTracker,
             enter (node, parent) {
@@ -236,10 +238,24 @@ export const PageMetaPlugin = (options: PageMetaPluginOptions = {}) => createUnp
                 || node.type !== 'Identifier' // checking for `node.type` to narrow down the type
               ) { return }
 
+              const declaration = scopeTracker.getDeclaration(node.name)
+              if (declaration) {
+                // check if the declaration was made inside `definePageMeta` and if so, do not process it
+                // (ensures that we don't hoist local variables in inline middleware, for example)
+                if (
+                  declaration.isUnderScope(definePageMetaScope)
+                  // ensures that we compare the correct declaration to the reference
+                  // (when in the same scope, the declaration must come before the reference, otherwise it must be in a parent scope)
+                  && (scopeTracker.isCurrentScopeUnder(declaration.scope) || declaration.start < node.start)
+                ) {
+                  return
+                }
+              }
+
               if (isStaticIdentifier(node.name)) {
                 addImport(node.name)
-              } else {
-                processDeclaration(scopeTracker.getDeclaration(node.name))
+              } else if (declaration) {
+                processDeclaration(declaration)
               }
             },
           })
diff --git a/packages/nuxt/test/page-metadata.test.ts b/packages/nuxt/test/page-metadata.test.ts
index 60247fd4e1..ab1c44b46a 100644
--- a/packages/nuxt/test/page-metadata.test.ts
+++ b/packages/nuxt/test/page-metadata.test.ts
@@ -393,6 +393,188 @@ definePageMeta({
     `)
   })
 
+  it('should not import static identifiers when shadowed in the same scope', () => {
+    const sfc = `
+<script setup lang="ts">
+import { useState } from '#app/composables/state'
+
+definePageMeta({
+  middleware: () => {
+    const useState = (key) => ({ value: { isLoggedIn: false } })
+    const auth = useState('auth')
+    if (!auth.value.isLoggedIn) {
+      return navigateTo('/login')
+    }
+  },
+})
+</script>
+      `
+    const res = compileScript(parse(sfc).descriptor, { id: 'component.vue' })
+    expect(transformPlugin.transform.call({
+      parse: (code: string, opts: any = {}) => Parser.parse(code, {
+        sourceType: 'module',
+        ecmaVersion: 'latest',
+        locations: true,
+        ...opts,
+      }),
+    }, res.content, 'component.vue?macro=true')?.code).toMatchInlineSnapshot(`
+      "const __nuxt_page_meta = {
+        middleware: () => {
+          const useState = (key) => ({ value: { isLoggedIn: false } })
+          const auth = useState('auth')
+          if (!auth.value.isLoggedIn) {
+            return navigateTo('/login')
+          }
+        },
+      }
+      export default __nuxt_page_meta"
+    `)
+  })
+
+  it('should not import static identifiers when shadowed in parent scope', () => {
+    const sfc = `
+<script setup lang="ts">
+import { useState } from '#app/composables/state'
+
+definePageMeta({
+  middleware: () => {
+    function isLoggedIn() {
+      const auth = useState('auth')
+      return auth.value.isLoggedIn
+    }
+
+    const useState = (key) => ({ value: { isLoggedIn: false } })
+    if (!isLoggedIn()) {
+      return navigateTo('/login')
+    }
+  },
+})
+</script>
+      `
+    const res = compileScript(parse(sfc).descriptor, { id: 'component.vue' })
+    expect(transformPlugin.transform.call({
+      parse: (code: string, opts: any = {}) => Parser.parse(code, {
+        sourceType: 'module',
+        ecmaVersion: 'latest',
+        locations: true,
+        ...opts,
+      }),
+    }, res.content, 'component.vue?macro=true')?.code).toMatchInlineSnapshot(`
+      "const __nuxt_page_meta = {
+        middleware: () => {
+          function isLoggedIn() {
+            const auth = useState('auth')
+            return auth.value.isLoggedIn
+          }
+
+          const useState = (key) => ({ value: { isLoggedIn: false } })
+          if (!isLoggedIn()) {
+            return navigateTo('/login')
+          }
+        },
+      }
+      export default __nuxt_page_meta"
+    `)
+  })
+
+  it('should import static identifiers when a shadowed and a non-shadowed one is used', () => {
+    const sfc = `
+<script setup lang="ts">
+import { useState } from '#app/composables/state'
+
+definePageMeta({
+  middleware: [
+    () => {
+      const useState = (key) => ({ value: { isLoggedIn: false } })
+      const auth = useState('auth')
+      if (!auth.value.isLoggedIn) {
+        return navigateTo('/login')
+      }
+    },
+    () => {
+      const auth = useState('auth')
+      if (!auth.value.isLoggedIn) {
+        return navigateTo('/login')
+      }
+    }
+  ]
+})
+</script>
+      `
+    const res = compileScript(parse(sfc).descriptor, { id: 'component.vue' })
+    expect(transformPlugin.transform.call({
+      parse: (code: string, opts: any = {}) => Parser.parse(code, {
+        sourceType: 'module',
+        ecmaVersion: 'latest',
+        locations: true,
+        ...opts,
+      }),
+    }, res.content, 'component.vue?macro=true')?.code).toMatchInlineSnapshot(`
+      "import { useState } from '#app/composables/state'
+
+      const __nuxt_page_meta = {
+        middleware: [
+          () => {
+            const useState = (key) => ({ value: { isLoggedIn: false } })
+            const auth = useState('auth')
+            if (!auth.value.isLoggedIn) {
+              return navigateTo('/login')
+            }
+          },
+          () => {
+            const auth = useState('auth')
+            if (!auth.value.isLoggedIn) {
+              return navigateTo('/login')
+            }
+          }
+        ]
+      }
+      export default __nuxt_page_meta"
+    `)
+  })
+
+  it('should import static identifiers when a shadowed and a non-shadowed one is used in the same scope', () => {
+    const sfc = `
+<script setup lang="ts">
+import { useState } from '#app/composables/state'
+
+definePageMeta({
+  middleware: () => {
+    const auth1 = useState('auth')
+    const useState = (key) => ({ value: { isLoggedIn: false } })
+    const auth2 = useState('auth')
+    if (!auth1.value.isLoggedIn || !auth2.value.isLoggedIn) {
+      return navigateTo('/login')
+    }
+  },
+})
+</script>
+      `
+    const res = compileScript(parse(sfc).descriptor, { id: 'component.vue' })
+    expect(transformPlugin.transform.call({
+      parse: (code: string, opts: any = {}) => Parser.parse(code, {
+        sourceType: 'module',
+        ecmaVersion: 'latest',
+        locations: true,
+        ...opts,
+      }),
+    }, res.content, 'component.vue?macro=true')?.code).toMatchInlineSnapshot(`
+      "import { useState } from '#app/composables/state'
+
+      const __nuxt_page_meta = {
+        middleware: () => {
+          const auth1 = useState('auth')
+          const useState = (key) => ({ value: { isLoggedIn: false } })
+          const auth2 = useState('auth')
+          if (!auth1.value.isLoggedIn || !auth2.value.isLoggedIn) {
+            return navigateTo('/login')
+          }
+        },
+      }
+      export default __nuxt_page_meta"
+    `)
+  })
+
   it('should work with esbuild.keepNames = true', async () => {
     const sfc = `
 <script setup lang="ts">
@@ -516,7 +698,12 @@ definePageMeta({
         test () {}
       }
 
-      console.log(hoisted.value)
+      const someFunction = () => {
+        const someValue = 'someValue'
+        console.log(someValue)
+      }
+
+      console.log(hoisted.value, val)
     },
   ],
   validate: (route) => {
@@ -564,7 +751,12 @@ const hoisted = ref('hoisted')
               test () {}
             }
 
-            console.log(hoisted.value)
+            const someFunction = () => {
+              const someValue = 'someValue'
+              console.log(someValue)
+            }
+
+            console.log(hoisted.value, val)
           },
         ],
         validate: (route) => {
diff --git a/packages/nuxt/test/parse.test.ts b/packages/nuxt/test/parse.test.ts
index a051a57c1d..b2bfa085a5 100644
--- a/packages/nuxt/test/parse.test.ts
+++ b/packages/nuxt/test/parse.test.ts
@@ -1,4 +1,4 @@
-import { describe, expect, it } from 'vitest'
+import { assert, describe, expect, it } from 'vitest'
 import { getUndeclaredIdentifiersInFunction, parseAndWalk } from '../src/core/utils/parse'
 import { TestScopeTracker } from './fixture/scope-tracker'
 
@@ -667,4 +667,87 @@ describe('parsing', () => {
 
     expect(processedFunctions).toBe(5)
   })
+
+  it ('should correctly compare identifiers defined in different scopes', () => {
+    const code = `
+      // ""
+      const a = 1
+
+      // ""
+      const func = () => {
+        // "0-0"
+        const b = 2
+
+        // "0-0"
+        function foo() {
+          // "0-0-0-0"
+          const c = 3
+        }
+      }
+
+      // ""
+      const func2 = () => {
+        // "1-0"
+        const d = 2
+
+        // "1-0"
+        function bar() {
+          // "1-0-0-0"
+          const e = 3
+        }
+      }
+
+      // ""
+      const f = 4
+    `
+
+    const scopeTracker = new TestScopeTracker({
+      keepExitedScopes: true,
+    })
+
+    parseAndWalk(code, filename, {
+      scopeTracker,
+    })
+
+    const a = scopeTracker.getDeclarationFromScope('a', '')
+    const func = scopeTracker.getDeclarationFromScope('func', '')
+    const foo = scopeTracker.getDeclarationFromScope('foo', '0-0')
+    const b = scopeTracker.getDeclarationFromScope('b', '0-0')
+    const c = scopeTracker.getDeclarationFromScope('c', '0-0-0-0')
+    const func2 = scopeTracker.getDeclarationFromScope('func2', '')
+    const bar = scopeTracker.getDeclarationFromScope('bar', '1-0')
+    const d = scopeTracker.getDeclarationFromScope('d', '1-0')
+    const e = scopeTracker.getDeclarationFromScope('e', '1-0-0-0')
+    const f = scopeTracker.getDeclarationFromScope('f', '')
+
+    assert(a && func && foo && b && c && func2 && bar && d && e && f, 'All declarations should be found')
+
+    // identifiers in the same scope should be equal
+    expect(f.isUnderScope(a.scope)).toBe(false)
+    expect(func.isUnderScope(a.scope)).toBe(false)
+    expect(d.isUnderScope(bar.scope)).toBe(false)
+
+    // identifiers in deeper scopes should be under the scope of the parent scope
+    expect(b.isUnderScope(a.scope)).toBe(true)
+    expect(b.isUnderScope(func.scope)).toBe(true)
+    expect(c.isUnderScope(a.scope)).toBe(true)
+    expect(c.isUnderScope(b.scope)).toBe(true)
+    expect(d.isUnderScope(a.scope)).toBe(true)
+    expect(d.isUnderScope(func2.scope)).toBe(true)
+    expect(e.isUnderScope(a.scope)).toBe(true)
+    expect(e.isUnderScope(d.scope)).toBe(true)
+
+    // identifiers in parent scope should not be under the scope of the children
+    expect(a.isUnderScope(b.scope)).toBe(false)
+    expect(a.isUnderScope(c.scope)).toBe(false)
+    expect(a.isUnderScope(d.scope)).toBe(false)
+    expect(a.isUnderScope(e.scope)).toBe(false)
+    expect(b.isUnderScope(c.scope)).toBe(false)
+
+    // identifiers in parallel scopes should not influence each other
+    expect(d.isUnderScope(b.scope)).toBe(false)
+    expect(e.isUnderScope(b.scope)).toBe(false)
+    expect(b.isUnderScope(d.scope)).toBe(false)
+    expect(c.isUnderScope(e.scope)).toBe(false)
+  })
 })