fix(compiler-vapor): do not cache expressions with globally allowed identifiers (#14562)

close #14560

packages/compiler-vapor/__tests__/transforms/__snapshots__/expression.spec.ts.snap

@@ -89,13 +89,12 @@ export function render(_ctx) {
   const n0 = _child(n1)
   const x1 = _txt(n1)
   _renderEffect(() => {
-    const _String = String
     const _foo = _ctx.foo
-    _setProp(n1, "id", _String(_foo.id++))
+    _setProp(n1, "id", String(_foo.id++))
     _setProp(n1, "foo", _foo)
     _setProp(n1, "bar", _ctx.bar++)
-    _setText(n0, _toDisplayString(_String(_foo.id++)) + " " + _toDisplayString(_foo) + " " + _toDisplayString(_ctx.bar))
-    _setText(x1, _toDisplayString(_String(_foo.id++)) + " " + _toDisplayString(_foo) + " " + _toDisplayString(_ctx.bar))
+    _setText(n0, _toDisplayString(String(_foo.id++)) + " " + _toDisplayString(_foo) + " " + _toDisplayString(_ctx.bar))
+    _setText(x1, _toDisplayString(String(_foo.id++)) + " " + _toDisplayString(_foo) + " " + _toDisplayString(_ctx.bar))
   })
   return n1
 }"

packages/compiler-vapor/__tests__/transforms/__snapshots__/vBind.spec.ts.snap

@@ -302,6 +302,66 @@ export function render(_ctx) {
 }"
 `;
 
+exports[`cache multiple access > should not cache Date.now() call expressions 1`] = `
+"import { setProp as _setProp, renderEffect as _renderEffect, template as _template } from 'vue';
+const t0 = _template("<div>")
+
+export function render(_ctx) {
+  const n0 = t0()
+  const n1 = t0()
+  _renderEffect(() => {
+    _setProp(n0, "id", Date.now())
+    _setProp(n1, "id", Date.now())
+  })
+  return [n0, n1]
+}"
+`;
+
+exports[`cache multiple access > should not cache globally allowed identifier as variable 1`] = `
+"import { setProp as _setProp, renderEffect as _renderEffect, template as _template } from 'vue';
+const t0 = _template("<div>")
+
+export function render(_ctx) {
+  const n0 = t0()
+  const n1 = t0()
+  _renderEffect(() => {
+    _setProp(n0, "id", String(_ctx.foo))
+    _setProp(n1, "id", String(_ctx.bar))
+  })
+  return [n0, n1]
+}"
+`;
+
+exports[`cache multiple access > should not cache globally allowed identifier call expressions 1`] = `
+"import { setProp as _setProp, renderEffect as _renderEffect, template as _template } from 'vue';
+const t0 = _template("<div>")
+
+export function render(_ctx) {
+  const n0 = t0()
+  const n1 = t0()
+  _renderEffect(() => {
+    _setProp(n0, "id", Math.random())
+    _setProp(n1, "id", Math.random())
+  })
+  return [n0, n1]
+}"
+`;
+
+exports[`cache multiple access > should not cache member expression containing globally allowed call 1`] = `
+"import { setProp as _setProp, renderEffect as _renderEffect, template as _template } from 'vue';
+const t0 = _template("<div>")
+
+export function render(_ctx) {
+  const n0 = t0()
+  const n1 = t0()
+  _renderEffect(() => {
+    _setProp(n0, "id", _ctx.obj[Math.random()])
+    _setProp(n1, "id", _ctx.obj[Math.random()])
+  })
+  return [n0, n1]
+}"
+`;
+
 exports[`cache multiple access > should not cache method call with different arguments 1`] = `
 "import { setProp as _setProp, renderEffect as _renderEffect, template as _template } from 'vue';
 const t0 = _template("<div>")
@@ -318,6 +378,21 @@ export function render(_ctx) {
 }"
 `;
 
+exports[`cache multiple access > should not cache mixed expression with globally allowed call 1`] = `
+"import { setProp as _setProp, renderEffect as _renderEffect, template as _template } from 'vue';
+const t0 = _template("<div>")
+
+export function render(_ctx) {
+  const n0 = t0()
+  const n1 = t0()
+  _renderEffect(() => {
+    _setProp(n0, "id", Math.random() + _ctx.foo)
+    _setProp(n1, "id", Math.random() + _ctx.foo)
+  })
+  return [n0, n1]
+}"
+`;
+
 exports[`cache multiple access > variable name substring edge cases 1`] = `
 "import { setProp as _setProp, renderEffect as _renderEffect, template as _template } from 'vue';
 const t0 = _template("<div>", true)

packages/compiler-vapor/__tests__/transforms/expression.spec.ts

@@ -45,7 +45,7 @@ describe('compiler: expression', () => {
       </div>
     `)
     expect(code).toMatchSnapshot()
-    expect(code).contains(`_String(_foo.id++)`)
+    expect(code).contains(`String(_foo.id++)`)
   })
 
   test('empty interpolation', () => {

packages/compiler-vapor/__tests__/transforms/vBind.spec.ts

@@ -992,4 +992,55 @@ describe('cache multiple access', () => {
     expect(code).matchSnapshot()
     expect(code).contains(`const _obj_foo_bar = _ctx.obj[_ctx.foo?.(_ctx.bar)]`)
   })
+
+  test('should not cache globally allowed identifier call expressions', () => {
+    const { code } = compileWithVBind(`
+      <div :id="Math.random()"></div>
+      <div :id="Math.random()"></div>
+    `)
+    expect(code).matchSnapshot()
+    // Math.random() should NOT be cached because it has side effects
+    expect(code).not.contains('const _Math')
+    expect(code).contains('Math.random()')
+  })
+
+  test('should not cache Date.now() call expressions', () => {
+    const { code } = compileWithVBind(`
+      <div :id="Date.now()"></div>
+      <div :id="Date.now()"></div>
+    `)
+    expect(code).matchSnapshot()
+    expect(code).not.contains('const _Date')
+    expect(code).contains('Date.now()')
+  })
+
+  test('should not cache mixed expression with globally allowed call', () => {
+    const { code } = compileWithVBind(`
+      <div :id="Math.random() + foo"></div>
+      <div :id="Math.random() + foo"></div>
+    `)
+    expect(code).matchSnapshot()
+    // The whole expression should NOT be cached because Math.random() has side effects
+    expect(code).not.contains('const _Math_random')
+    expect(code).contains('Math.random()')
+  })
+
+  test('should not cache globally allowed identifier as variable', () => {
+    const { code } = compileWithVBind(`
+      <div :id="String(foo)"></div>
+      <div :id="String(bar)"></div>
+    `)
+    expect(code).matchSnapshot()
+    expect(code).not.contains('const _String = String')
+  })
+
+  test('should not cache member expression containing globally allowed call', () => {
+    const { code } = compileWithVBind(`
+      <div :id="obj[Math.random()]"></div>
+      <div :id="obj[Math.random()]"></div>
+    `)
+    expect(code).matchSnapshot()
+    expect(code).not.contains('const _obj_Math_random')
+    expect(code).contains('Math.random()')
+  })
 })

packages/compiler-vapor/src/generators/expression.ts

@@ -331,11 +331,13 @@ function analyzeExpressions(expressions: SimpleExpressionNode[]) {
     walkIdentifiers(exp.ast, (currentNode, parent, parentStack) => {
       if (parent && isMemberExpression(parent) && !seenParents.has(parent)) {
         seenParents.add(parent)
+        let hasGlobalIdentifier = false
         const memberExp = extractMemberExpression(parent, id => {
           registerVariable(id.name, exp, true, {
             start: id.start!,
             end: id.end!,
           })
+          if (isGloballyAllowed(id.name)) hasGlobalIdentifier = true
         })
 
         const parentOfMemberExp = parentStack[parentStack.length - 2]
@@ -343,6 +345,10 @@ function analyzeExpressions(expressions: SimpleExpressionNode[]) {
           return
         }
 
+        // skip member expressions containing globally allowed identifiers
+        // e.g. obj[Math.random()] - the call may have side effects
+        if (hasGlobalIdentifier) return
+
         registerVariable(
           memberExp,
           exp,
@@ -393,6 +399,9 @@ function processRepeatedVariables(
 
   for (const [name, exps] of variableToExpMap) {
     if (updatedVariable.has(name)) continue
+    // skip globally allowed identifiers - they are not reactive and
+    // their method calls (e.g. Math.random()) may have side effects
+    if (isGloballyAllowed(name)) continue
     if (seenVariable[name] > 1 && exps.size > 0) {
       const isIdentifier = seenIdentifier.has(name)
       const varName = isIdentifier ? name : genVarName(name)
@@ -529,7 +538,11 @@ function processRepeatedExpressions(
       if (
         exp.ast &&
         exp.ast.type !== 'Identifier' &&
-        !(variables && variables.some(v => updatedVariable.has(v)))
+        !(variables && variables.some(v => updatedVariable.has(v))) &&
+        // skip expressions containing globally allowed identifiers
+        // (e.g. Math.random(), Date.now() + foo) - they are not reactive
+        // and may involve impure calls with side effects
+        !variables.some(v => isGloballyAllowed(v))
       ) {
         acc[exp.content] = (acc[exp.content] || 0) + 1
       }