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.

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

[Bug] transformer can't tag variable scope correctly in some edge case #9791

Open
GiveMe-A-Name opened this issue Dec 11, 2024 · 9 comments
Open

Comments

@GiveMe-A-Name
Copy link

GiveMe-A-Name commented Dec 11, 2024

Describe the bug

The code after swc transformer, would cause incorrectly behavior.
The code would transform like this:

var keynode = {
  show_type: 1,
  key_list: [
    { show_type: 'a', },
    { show_type: 'b' },
    { show_type: "c" },
  ]
}

var { show_type, key_list } = keynode;

console.log('show_type in loop output:', show_type);

for (var key in key_list) {
  // JS engine hoistings `show_type` variable.
  
  var { show_type } = key_list[key];
  
  console.log("show_type in loop1:", show_type);

}


[1, 2, 3].forEach((i) => {
  // show_type will modifed by loop1.
  console.log("show_type in loop2:", show_type);
})


// show_type in loop output: 1
// show_type in loop1: a
// show_type in loop1: b
// show_type in loop1: c
// show_type in loop2: c
// show_type in loop2: c
// show_type in loop2: c

More detail see: web-infra-dev/rspack#8637 (comment)

Input code

const composeIndicator = (keynode, logList) => {
  if (!keynode || keynode.length < 1) return [];
  for (let i in keynode) {
    const { key_list, relation, name, show_type } = keynode[i] || {};
    if (!key_list || key_list.length < 1) continue;
    const pvArr = [];
    for (let j in key_list) {
      const { abbr, show_type, id, key_id } = key_list[j] || {};
      const { pv, uv } =
        logList.find((i) => {
          if (id && i.id) {
            return i.id === id;
          } else if (key_id && i.key_id) {
            return i.key_id === key_id;
          }
        }) || {};
      pvArr.push({
        pv,
        uv,
        abbr,
        show_type,
      });
    }
    const dataList = [];
    for (let l in pvArr) {
      const { pv, abbr, show_type, uv } = pvArr[l] || {};
      if (Array.isArray(pv)) {
        for (let k in pv) {
          if (!dataList[k]) {
            if (relation.indexOf(abbr) > -1) {
              let copyRelation = relation;
              const data = show_type === 1 ? pv[k] : uv[k];
              const regx = new RegExp(`${abbr}`, "g");
              copyRelation = copyRelation.replace(regx, data);
              dataList.push(copyRelation);
            }
          } else {
            if (dataList[k].indexOf(abbr) > -1) {
              let copyRelation = dataList[k];
              const data = show_type === 1 ? pv[k] : uv[k];
              const regx = new RegExp(`${abbr}`, "g");
              copyRelation = copyRelation.replace(regx, data);
              dataList[k] = copyRelation;
            }
          }
        }
      }
    }
    keynode[i].pv = dataList.map((i) => {
      const calculate = eval(i);
      const val =
        show_type === 1
          ? calculate === Infinity
            ? 0
            : Math.round(calculate)
          : calculate === Infinity
          ? 0
          : Number((calculate * 100).toFixed(2));
      return isNaN(val) ? 0 : val;
    });
    keynode[i].clicktag = name;
  }
  return keynode;
};

Config

{
  "jsc": {
    "parser": {
      "syntax": "ecmascript",
      "jsx": false
    },
    "target": "es5",
    "transform": {
      "react": {
        "runtime": "automatic"
      }
    },
    "loose": false,
    "minify": {
      "compress": false,
      "mangle": false
    }
  },
  "module": {
    "type": "es6"
  },
  "minify": false,
  "isModule": true
}

Playground link (or link to the minimal reproduction)

https://play.swc.rs/?version=1.10.1&code=H4sIAAAAAAAAA91VUW%2BbMBB%2Bz6%2B4VlNlTwwlewxNoz1sUqWtk%2FoaRa0DDnFDAIFJE6X8952NDQa6Tdrj8sLB3X333Xf2JczSUkKYHfKs5PdpJEImswIWQPb8nGYR9yDJ4u%2BilBQWd3CZAIgtkCvjhbc3MKaf8DSWO7iFGYWCy6pIYbUOMGGLgCThEgSI1IZTjQVYWhG4qM9PCZbxMDdhUmSpByk7YP1yl70%2ByXPOoUZeJn0l1qr2pQ40iuWkIQwpbfdYYS0p0ooHTuX8%2BKVQ%2FTZUHbIvhqyGsWw7vmyzKRxuHojI0%2BEisjx15uqlR7QDyI8eVEcVaxxglfa3Io0IEa3i9qe6RPibGxC%2BiGjPB1Zz5YLFYoGEAsdfA09KriEMSw3T2L%2BFMqEKrjF7kK1d036PWlQ%2Fr8od6YCx4dauHFsr2b51ik4sdoNaO0OLmGRKqffmlqi5aQLjoSnNR4NrhtCkrJLBtJRe%2BJ2dfVHqJ8mP1JWrrbtv6tLRxK4s29V%2BPRRa%2Be1593Hq%2FPRzSxRDCnfwaTYMxyPC1W3Nz48mCYnb%2FGAQ2kmFQd0lUrOcwRKpIh%2BYY%2Fv4fD%2B34PEJc1P%2BCo88%2FnrKyfOHi2JXP3twHV%2FTcVqPmfvqFzxPWMiJAvU0rVG6Fao5OW72ILQeH%2Byxro7s%2FyqtA%2FH%2FqKuI9bP%2FIO5kaNXOZex2sZ8fHbn8A8uH%2B6vpOWRJWGFRFAr4kSUY1N%2BL%2BM1ZiANdHWJLFwp99ykuTSHPvU6WMO29z%2BEHkzu%2FyCrcr20%2BnbgRf4Xtg87hoTpseEE6PPgIs%2BmU%2BjL7Jk48Ip9p26Pdq%2BUDeyDYKlVoiIGmWXIm1FE2TES4lyxWRwX%2FD5VfqW%2BgTGAwqYNfz1P1mssHAAA%3D&config=H4sIAAAAAAAAA1WQSw6DMAxE95wCed1tu%2BgdeggrNVUQ%2Bch2pCLE3ZtAQmEXv%2FGM7Sxd38MoBp79kp%2B5iMhCfNSZyOwVv5kAGYdi2EaFW1NHKdKAk9CG1l0BRf6Qbi65Q4OMXobA7pzPhEZPoKDk1ToqbkwaHKo1UOX1MmYKQagtUJmz3g7zeYQJLjKJXBtLK%2FrPRNcDupoOLrzTJtav0TnSftAD%2Fk1t2BEMVl7NqZyoW3%2FQBsGgZgEAAA%3D%3D

SWC Info output

No response

Expected behavior

Transformer correctly.
Like babel transform, the code would be like:

var composeIndicator = function composeIndicator(keynode, logList) {

  /// ...
  var dataList = [];
  for (var l in pvArr) {
    var _ref2 = pvArr[l] || {},
      pv = _ref2.pv,
      abbr = _ref2.abbr,
     ///!!!!!!
      _show_type = _ref2.show_type,
      uv = _ref2.uv;
  }
  keynode[i].pv = dataList.map((i) => {

    var val = show_type === 1 ? calculate === Number.POSITIVE_INFINITY ? 0 : Math.round(calculate) : calculate === Number.POSITIVE_INFINITY ? 0 : Number((calculate * 100).toFixed(2));

  });


  /// ...
};

Actual behavior

No response

Version

1.10.1

Additional context

No response

@GiveMe-A-Name
Copy link
Author

It seems eval function will case swc scope error.
See a example.

If we remove the eval, the transform is correctly

@Austaras
Copy link
Member

Austaras commented Dec 16, 2024

reduced version for anyone interested.

@Austaras
Copy link
Member

Possible duplicate of #6820

@GiveMe-A-Name
Copy link
Author

Possible duplicate of #6820

Yes, I found it's a history problem

@kdy1 kdy1 self-assigned this Dec 26, 2024
@kdy1
Copy link
Member

kdy1 commented Dec 26, 2024

Context

(for future users)

Babel check for existence of conflicting variable names while injecting a variable because they have full control over scope while transpilation. SWC is written in Rust and storing the list of identifiers somewhere else was very hard. So I didn't store it, and as a result, the variable renamer should distinguish the variable written by user and the variable injected by SWC code.

@GiveMe-A-Name
Copy link
Author

GiveMe-A-Name commented Dec 27, 2024

Context

(for future users)

Babel check for existence of conflicting variable names while injecting a variable because they have full control over scope while transpilation. SWC is written in Rust and storing the list of identifiers somewhere else was very hard. So I didn't store it, and as a result, the variable renamer should distinguish the variable written by user and the variable injected by SWC code.

The key point is why swc removes renamer when top-level eval exists, i was noticed babel do not do anything.
Babel don't handle eval will occur unexpected code.

// source
const a = 10;
{
  const a = 20;
  eval("console.log(a);") // 20
}
// after babel transform with @babel/plugin-transform-block-scoping
var a = 10;
{
  var _a = 20;
  eval("console.log("a")"); // 10
}

It will unexpected when user eval rename variable.

It seems swc select another way, sometime it's a good select. But it would occur error in this case:

var data = { 
  name: "xxx",
  names: ["x1", "x2", "x3"], 
}
var { name, names } = data;
for(var name of names) {
  // do something 
}

var callback = () => {
  console.log(name);  // "x3"
  eval('yyy')
}

callback()

@magic-akari
Copy link
Member

const a = 10;
{
  const a = 20;
  eval("console.log(a);"); // 20
}
eval("console.log(a);"); // 10

Downgrading const to var and ensuring the correctness of code containing eval is nearly an impossible task, as we do not analyze the code inside the eval.

@GiveMe-A-Name
Copy link
Author

GiveMe-A-Name commented Dec 27, 2024

const a = 10;
{
  const a = 20;
  eval("console.log(a);"); // 20
}
eval("console.log(a);"); // 10

Downgrading const to var and ensuring the correctness of code containing eval is nearly an impossible task, as we do not analyze the code inside the eval.

So we don't care about eval even though it exists.

@Austaras
Copy link
Member

const a = 10;
{
  const a = 20;
  eval("console.log(a);"); // 20
}
eval("console.log(a);"); // 10

Downgrading const to var and ensuring the correctness of code containing eval is nearly an impossible task, as we do not analyze the code inside the eval.

If that's the case maybe there's nothing we could do with it.

@kdy1 kdy1 added this to the Planned milestone Dec 27, 2024
@kdy1 kdy1 modified the milestones: Planned, Planned (Low priority) Jan 7, 2025
@kdy1 kdy1 removed their assignment Jan 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

4 participants