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

Fix transform of unary expression in Babel plugin #17596

Merged
merged 1 commit into from
Jan 29, 2024

Conversation

nicolo-ribaudo
Copy link
Contributor

The generated code in build/.../viewer.js is now

  async getNimbusExperimentData() {
    return null;
  }

and the generated code in build/.../viewer-geckoview.js is

  async getNimbusExperimentData() {
    const nimbusData = await FirefoxCom.requestAsync("getNimbusExperimentData", null);
    return nimbusData && JSON.parse(nimbusData);
  }

Fixes #17589

Copy link
Collaborator

@Snuffleupagus Snuffleupagus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To (easily) fix the linting errors locally, you can run gulp lint --fix.

Please add tests in this file, e.g. as follows, since the lack of testing is what "caused" the regression.

var k = !PDFJSDev.test('TRUE');
var l = !PDFJSDev.test('FALSE');

All of our static evaluation & dead-code elimination transforms need to
happen in post-order, transforming inner nodes first. This is so that
in complex nested cases all transforms see the simplified version of
their inner nodes.

For example:
  async getNimbusExperimentData() {
    if (!PDFJSDev.test("GECKOVIEW")) { return null; }
    // other code
  }
-> [evaluation of PDFJSDev.*]
  async getNimbusExperimentData() {
    if (!false) { return null; }
    // other code
  }
-> [!false -> true]
  async getNimbusExperimentData() {
    if (true) { return null; }
    // other code
  }
-> [if (true) -> replace with the if branch]
  async getNimbusExperimentData() {
    return null;
    // other code
  }
-> [early return -> remove dead code]
  async getNimbusExperimentData() {
    return null;
    // other code
  }

This was done correctly in all cases except for our `UnaryExpression`
transform, which was happening in pre-order.
@nicolo-ribaudo
Copy link
Contributor Author

Done!

external/builder/fixtures_esprima/evals.js Dismissed Show dismissed Hide dismissed
external/builder/fixtures_esprima/evals.js Dismissed Show dismissed Hide dismissed
external/builder/fixtures_esprima/evals-expected.js Dismissed Show dismissed Hide dismissed
external/builder/fixtures_esprima/evals-expected.js Dismissed Show dismissed Hide dismissed
@Snuffleupagus
Copy link
Collaborator

/botio test

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

Command cmd_test from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.241.84.105:8877/fc9fee8528a7fd2/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Received

Command cmd_test from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.193.163.58:8877/c801288436756a0/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Failed

Full output at http://54.241.84.105:8877/fc9fee8528a7fd2/output.txt

Total script time: 24.82 mins

  • Unit tests: Passed
  • Integration Tests: Passed
  • Regression tests: FAILED
  different ref/snapshot: 16
  different first/second rendering: 3

Image differences available at: http://54.241.84.105:8877/fc9fee8528a7fd2/reftest-analyzer.html#web=eq.log

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Failed

Full output at http://54.193.163.58:8877/c801288436756a0/output.txt

Total script time: 39.71 mins

  • Unit tests: Passed
  • Integration Tests: FAILED
  • Regression tests: FAILED
  different ref/snapshot: 4

Image differences available at: http://54.193.163.58:8877/c801288436756a0/reftest-analyzer.html#web=eq.log

Copy link
Collaborator

@Snuffleupagus Snuffleupagus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me, thank you for the patch!

@Snuffleupagus Snuffleupagus merged commit 56dabe9 into mozilla:master Jan 29, 2024
7 checks passed
@nicolo-ribaudo nicolo-ribaudo deleted the fix-babel-plugin branch January 29, 2024 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deadcode elimination during the build no longer working correctly in all cases; PR 17563 regression
3 participants