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

Rollup removes correct code #3264

Closed
lpatiny opened this issue Nov 28, 2019 · 10 comments · Fixed by #3267
Closed

Rollup removes correct code #3264

lpatiny opened this issue Nov 28, 2019 · 10 comments · Fixed by #3267

Comments

@lpatiny
Copy link

lpatiny commented Nov 28, 2019

  • Rollup Version: 1.27.5
  • Operating System (or Browser):
  • Node Version: 12

How Do We Reproduce?

Can be reproduce on repl

Demo

rollup with remove the condition (axis.x === undefined || axis.x.unit !== 'V') but will not remove it if you uncomment the line console.log(axis)

export function fromSIV(content) {
    let axis = {};

    // removing the following line corrupt the build ???!!!
    //console.log(axis);

    if (axis.x === undefined || axis.x.unit !== 'V') {
      // eslint-disable-next-line no-console
      console.log('Unknown X axis:', axis.kind, axis.unit);
    }

}

Expected Behavior

Should convert to

    if (axis.x === undefined || axis.x.unit !== 'V') {
      // eslint-disable-next-line no-console
      console.log('Unknown X axis:', axis.kind, axis.unit);
    }

Actual Behavior

Returns

    {
      // eslint-disable-next-line no-console
      console.log('Unknown X axis:', axis.kind, axis.unit);
    }

The condition has disappeared only if you comment the console.log(axis);

@lukastaegert
Copy link
Member

Rollup handles console.log as an unknown global function, thus it assumes that console.log could possibly mutate its argument. For instance it could change the value of axis.x. As Rollup has no way of knowing this, it decides to be conservative about it and keeps the condition.

@lukastaegert
Copy link
Member

Actually this is what Rollup does for ALL function calls at the moment. The reason is performance, as previous attempts at this turned out to have a very negative effect. This might change in the future, but probably not for console.log.

@lukastaegert
Copy link
Member

Sorry, I read this the wrong way around. The reason it is removed is of course that the condition is always true.

@lpatiny
Copy link
Author

lpatiny commented Nov 28, 2019

Actually my code is more complex that this and it seems impossible to me to predict the axis because it depends of the content. And even in this case the condition is removed.

When testing the code I don't have the same result if I add the console.log or remove it. My code is only working if I leave this console.log inside.

export function fromSIV(content) {
  let allLines = content.split(/[\r\n]+/);
  let sampleMeta = parseS(allLines.filter((line) => line.match(/X S_/)));
  let instrumentMeta = parseV(allLines.filter((line) => line.match(/X V_/)));
  let date = parseDate(allLines.filter((line) => line.match(/X d_t/))[0]);

  let parts = content.split('WAVES\t');
  let spectra = [];

  for (let part of parts) {
    let lines = part.split(/[\r\n]+/);
    let ys = lines
      .filter((line) => line.match(/^[\t 0-9.eE-]+$/))
      .map((line) => Number(line));
    if (ys.length < 10) continue;

    let kind = lines[0].trim();
    let metaLines = lines
      .filter((line) => line.match(/^X /))
      .map((line) => line.substring(2));

    let axis = parseScale(metaLines[0], ys.length);

    // removing the following line corrupt the build ???!!!
    // console.log(axis);

    if (axis.x === undefined || axis.x.unit !== 'V') {
      // eslint-disable-next-line no-console
      console.log('Unknown X axis:', axis.kind, axis.unit);
      continue;
    }
    if (axis.y === undefined || axis.y.unit !== 'A') {
      // eslint-disable-next-line no-console
      console.log('Unknown Y axis:', axis.kind, axis.unit);
      continue;
    }
    // let note = parseNote(metaLines[1]);
    let xs = axis.x.values;
    let data = {
      x: xs,
      y: ys,
    };

    let meta = {
      ...sampleMeta,
      date,
      experiment: kind,
      ...instrumentMeta,
    };
    console.log({ meta });
    spectra.push(new Spectrum(data.x, data.y, spectra.length + 1, { meta }));
  }
  return spectra;
}

function parseDate(line) {
  let dateString = line
    .replace('X d_t=', '')
    .trim()
    .replace(/"/g, '');
  let date = new Date(dateString);
  return date;
}

function parseScale(line, nbValues) {
  let result = {};
  line = line.replace(/ ([xy]) /g, ',$1,');
  let parts = line.split('; ');

  for (let part of parts) {
    let parsedPart = parseScalePart(part, nbValues);
    result[parsedPart.axis] = parsedPart;
  }
  return result;
}

function parseS(lines) {
  let result = {};
  for (let line of lines) {
    let key = line.replace(/X ._([^=]*)=(.*)/, '$1').trim();
    key = getFieldName(key);
    let value = line.replace(/X ._([^=]*)=(.*)/, '$2').trim();
    value = value.replace(/^"(.*)"$/, '$1');
    if (!isNaN(value)) value = Number(value);
    result[key] = value;
  }
  return result;
}

function parseV(lines) {
  let result = {};
  for (let line of lines) {
    let key = line.replace(/X ._([^=]*)=(.*)/, '$1').trim();
    key = getFieldName(key);
    let value = line.replace(/X ._([^=]*)=(.*)/, '$2').trim();
    value = value.replace(/^"(.*)"$/, '$1');
    if (!isNaN(value)) value = Number(value);
    result[key] = value;
  }
  return result;
}

// eslint-disable-next-line no-unused-vars
function parseNote(line) {
  line = line.replace(/"/g, '').replace(/\\r/g, ';');
  let parts = line.split(/ *[;,] */);
  let result = {};
  for (let part of parts) {
    let semiColumn = part.indexOf(':');
    let key = part.substring(0, semiColumn);
    key = getFieldName(key);
    let value = part.substring(semiColumn + 1).trim();
    value = value.replace(/^"(.*)"$/, '$1');
    if (!isNaN(value)) value = Number(value);
    if (!key) continue;
    result[key] = value;
  }
}

function parseScalePart(scale, nbValues) {
  let parts = scale.split(',');
  let result = {};
  result.axis = parts[1];
  result.kind = parts[0];
  result.unit = parts[4].replace(/"/g, '');
  if (result.kind === 'SetScale/P') {
    let from = Number(parts[2]);
    let step = Number(parts[3]);
    let values = [];
    for (let i = 0; i < nbValues; i++) {
      values.push(from);
      from += step;
      result.values = values;
    }
  }
  return result;
}

function getFieldName(key) {
  const mapping = {
    CE: 'counterElectrodeType',
    Calibrationfile: 'calibrationFile',
    Username: 'username',
    WE: 'workingElectrodeGlass',
    cellname: 'cellname',
    electrolyte: 'electrolyteZ960',
    layer: 'semicondutorLayer',
    specification: 'remarks',
    temp: 'workingTemperature',
    type: 'typeOfCell',
    AR: 'cellActiveArea',
    IT: 'powerIn',
  };
  return mapping[key] || key;
}

@lpatiny
Copy link
Author

lpatiny commented Nov 28, 2019

Here is a code that you can actually test and run in node:

If you run the code there will be written IT WORKS in the console.

If you remove console.log(axis); all the code after the condition is removed (and as well IT WORKS)

fromSIV(`BEGIN
	0.0022139272
	0.0023900089
	0.002451069
	3.2340083e-08
	2.5559748e-08
	1.623679e-08
	4.3712021e-09
END
X SetScale/P x 1.19,-0.01,"V", DarkCurrent; SetScale y 0,0,"A", DarkCurrent
X Note DarkCurrent, "IT:0.101656;AR:0.158;FT:7e-06;CM:0.002;ST:0.1;SV:1.19;ET:0"

`);

function fromSIV(content) {
  let allLines = content.split(/[\r\n]+/);

  let sampleMeta = parseS(allLines.filter((line) => line.match(/X S_/)));
  let instrumentMeta = parseV(allLines.filter((line) => line.match(/X V_/)));

  let parts = content.split('WAVES\t');
  let spectra = [];

  for (let part of parts) {
    let lines = part.split(/[\r\n]+/);
    let ys = lines
      .filter((line) => line.match(/^[\t 0-9.eE-]+$/))
      .map((line) => Number(line));

    let kind = lines[0].trim();
    let metaLines = lines
      .filter((line) => line.match(/^X /))
      .map((line) => line.substring(2));

    console.log({ metaLines });
    let axis = parseScale(metaLines[0], ys.length);

    // removing the following line corrupt the build ???!!!
    console.log(axis);

    if (axis.x === undefined || axis.x.unit !== 'V') {
      // eslint-disable-next-line no-console
      console.log('Unknown X axis:', axis.kind, axis.unit);
      continue;
    }
    if (axis.y === undefined || axis.y.unit !== 'A') {
      // eslint-disable-next-line no-console
      console.log('Unknown Y axis:', axis.kind, axis.unit);
      continue;
    }

    console.log('IT WORKS');

    // let note = parseNote(metaLines[1]);
    let xs = axis.x.values;
    let data = {
      x: xs,
      y: ys,
    };

    let meta = {
      ...sampleMeta,
      experiment: kind,
      ...instrumentMeta,
    };
  }
  return spectra;
}

function parseScale(line, nbValues) {
  console.log(line);
  let result = {};
  line = line.replace(/ ([xy]) /g, ',$1,');
  let parts = line.split('; ');

  for (let part of parts) {
    let parsedPart = parseScalePart(part, nbValues);
    result[parsedPart.axis] = parsedPart;
  }
  return result;
}

function parseS(lines) {
  let result = {};
  for (let line of lines) {
    let key = line.replace(/X ._([^=]*)=(.*)/, '$1').trim();
    key = getFieldName(key);
    let value = line.replace(/X ._([^=]*)=(.*)/, '$2').trim();
    value = value.replace(/^"(.*)"$/, '$1');
    if (!isNaN(value)) value = Number(value);
    result[key] = value;
  }
  return result;
}

function parseV(lines) {
  let result = {};
  for (let line of lines) {
    let key = line.replace(/X ._([^=]*)=(.*)/, '$1').trim();
    key = getFieldName(key);
    let value = line.replace(/X ._([^=]*)=(.*)/, '$2').trim();
    value = value.replace(/^"(.*)"$/, '$1');
    if (!isNaN(value)) value = Number(value);
    result[key] = value;
  }
  return result;
}

// eslint-disable-next-line no-unused-vars
function parseNote(line) {
  line = line.replace(/"/g, '').replace(/\\r/g, ';');
  let parts = line.split(/ *[;,] */);
  let result = {};
  for (let part of parts) {
    let semiColumn = part.indexOf(':');
    let key = part.substring(0, semiColumn);
    key = getFieldName(key);
    let value = part.substring(semiColumn + 1).trim();
    value = value.replace(/^"(.*)"$/, '$1');
    if (!isNaN(value)) value = Number(value);
    if (!key) continue;
    result[key] = value;
  }
}

function parseScalePart(scale, nbValues) {
  let parts = scale.split(',');
  let result = {};
  result.axis = parts[1];
  result.kind = parts[0];
  result.unit = parts[4].replace(/"/g, '');
  if (result.kind === 'SetScale/P') {
    let from = Number(parts[2]);
    let step = Number(parts[3]);
    let values = [];
    for (let i = 0; i < nbValues; i++) {
      values.push(from);
      from += step;
      result.values = values;
    }
  }
  return result;
}

function getFieldName(key) {
  const mapping = {
    CE: 'counterElectrodeType',
    Calibrationfile: 'calibrationFile',
    Username: 'username',
    WE: 'workingElectrodeGlass',
    cellname: 'cellname',
    electrolyte: 'electrolyteZ960',
    layer: 'semicondutorLayer',
    specification: 'remarks',
    temp: 'workingTemperature',
    type: 'typeOfCell',
    AR: 'cellActiveArea',
    IT: 'powerIn',
  };
  return mapping[key] || key;
}

@lpatiny
Copy link
Author

lpatiny commented Nov 28, 2019

This bug was introduced between version 1.9.2 and 1.9.3. Commit: v1.9.2...v1.9.3

@lukastaegert
Copy link
Member

Definitely a bug, thanks for spotting. I will see if I can reduce it further.

@lukastaegert
Copy link
Member

Here is a very simplified version. It is indeed a tricky bug:

https://rollupjs.org/repl/?version=1.27.5&shareable=JTdCJTIybW9kdWxlcyUyMiUzQSU1QiU3QiUyMm5hbWUlMjIlM0ElMjJtYWluLmpzJTIyJTJDJTIyY29kZSUyMiUzQSUyMmNvbnN0JTIwYXhpcyUyMCUzRCUyMCU3QiU3RCUzQiU1Q25heGlzJTVCZ2V0UmVzdWx0KCkuYXhpcyU1RCUyMCUzRCUyMDElM0IlNUNuJTVDbmlmJTIwKGF4aXMueCklMjBjb25zb2xlLmxvZygnV09SS0lORycpJTNCJTVDbmVsc2UlMjBjb25zb2xlLmxvZygnQlVHJyklM0IlNUNuJTVDbmZ1bmN0aW9uJTIwZ2V0UmVzdWx0KCklMjAlN0IlNUNuJTIwJTIwY29uc3QlMjByZXN1bHQlMjAlM0QlMjAlN0IlN0QlM0IlNUNuJTIwJTIwcmVzdWx0LmF4aXMlMjAlM0QlMjAneCclM0IlNUNuJTIwJTIwcmV0dXJuJTIwcmVzdWx0JTNCJTVDbiU3RCU1Q24lMjIlMkMlMjJpc0VudHJ5JTIyJTNBdHJ1ZSU3RCU1RCUyQyUyMm9wdGlvbnMlMjIlM0ElN0IlMjJmb3JtYXQlMjIlM0ElMjJlc20lMjIlMkMlMjJuYW1lJTIyJTNBJTIybXlCdW5kbGUlMjIlMkMlMjJhbWQlMjIlM0ElN0IlMjJpZCUyMiUzQSUyMiUyMiU3RCUyQyUyMmdsb2JhbHMlMjIlM0ElN0IlN0QlN0QlMkMlMjJleGFtcGxlJTIyJTNBbnVsbCU3RA==

Note that when the function is moved to the top, the bug is not triggered:

https://rollupjs.org/repl/?version=1.27.5&shareable=JTdCJTIybW9kdWxlcyUyMiUzQSU1QiU3QiUyMm5hbWUlMjIlM0ElMjJtYWluLmpzJTIyJTJDJTIyY29kZSUyMiUzQSUyMmZ1bmN0aW9uJTIwZ2V0UmVzdWx0KCklMjAlN0IlNUNuJTIwJTIwY29uc3QlMjByZXN1bHQlMjAlM0QlMjAlN0IlN0QlM0IlNUNuJTIwJTIwcmVzdWx0LmF4aXMlMjAlM0QlMjAneCclM0IlNUNuJTIwJTIwcmV0dXJuJTIwcmVzdWx0JTNCJTVDbiU3RCU1Q24lNUNuY29uc3QlMjBheGlzJTIwJTNEJTIwJTdCJTdEJTNCJTVDbmF4aXMlNUJnZXRSZXN1bHQoKS5heGlzJTVEJTIwJTNEJTIwMSUzQiU1Q24lNUNuaWYlMjAoYXhpcy54KSUyMGNvbnNvbGUubG9nKCdXT1JLSU5HJyklM0IlNUNuZWxzZSUyMGNvbnNvbGUubG9nKCdCVUcnKSUzQiUyMiUyQyUyMmlzRW50cnklMjIlM0F0cnVlJTdEJTVEJTJDJTIyb3B0aW9ucyUyMiUzQSU3QiUyMmZvcm1hdCUyMiUzQSUyMmVzbSUyMiUyQyUyMm5hbWUlMjIlM0ElMjJteUJ1bmRsZSUyMiUyQyUyMmFtZCUyMiUzQSU3QiUyMmlkJTIyJTNBJTIyJTIyJTdEJTJDJTIyZ2xvYmFscyUyMiUzQSU3QiU3RCU3RCUyQyUyMmV4YW1wbGUlMjIlM0FudWxsJTdE

I hope I find some time soon to look into this further.

@kzc
Copy link
Contributor

kzc commented Nov 30, 2019

@lpatiny Please test against PR #3266 and report your findings.

@lpatiny
Copy link
Author

lpatiny commented Dec 1, 2019

I just tested it and I can confirm it works correctly using this PR
Thanks !

lukastaegert pushed a commit that referenced this issue Dec 2, 2019
* Workaround for various object literal mutation bugs.

Some necessary minor regressions related to previously
unsafe object literal optimizations.

fixes #2345
fixes #2473
fixes #3027
fixes #3093
fixes #3192
fixes #3264

* Remove special logic to handle deoptimizing properties, always deopt `this` by default
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants