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

Wrap simplified assignments if necessary #3951

Merged
merged 1 commit into from Feb 5, 2021

Conversation

@lukastaegert
Copy link
Member

@lukastaegert lukastaegert commented Feb 5, 2021

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

List any relevant issue numbers:
Resolves #3950

Description

See #3950
When simplifying assignments, it was possible that invalid code was generated. https://rollupjs.org/repl/?version=2.38.4&shareable=JTdCJTIybW9kdWxlcyUyMiUzQSU1QiU3QiUyMm5hbWUlMjIlM0ElMjJtYWluLmpzJTIyJTJDJTIyY29kZSUyMiUzQSUyMmNvbnN0JTIwdW51c2VkMSUyMCUzRCUyMGZ1bmN0aW9uKCklN0IlN0Quc29tZUZ1bmN0aW9uKCklM0IlNUNuY29uc3QlMjB1bnVzZWQyJTIwJTNEJTIwY2xhc3MlN0IlN0Quc29tZUZ1bmN0aW9uKCklM0IlNUNuY29uc3QlMjB1bnVzZWQzJTIwJTNEJTIwJTdCJTdELnNvbWVGdW5jdGlvbigpJTNCJTIyJTJDJTIyaXNFbnRyeSUyMiUzQXRydWUlN0QlNUQlMkMlMjJvcHRpb25zJTIyJTNBJTdCJTIyZm9ybWF0JTIyJTNBJTIyZXMlMjIlMkMlMjJuYW1lJTIyJTNBJTIybXlCdW5kbGUlMjIlMkMlMjJhbWQlMjIlM0ElN0IlMjJpZCUyMiUzQSUyMiUyMiU3RCUyQyUyMmdsb2JhbHMlMjIlM0ElN0IlN0QlN0QlMkMlMjJleGFtcGxlJTIyJTNBbnVsbCU3RA==

This is fixed in this PR by wrapping expressions in parentheses if necessary:
https://rollupjs.org/repl/?circleci=14200&shareable=JTdCJTIybW9kdWxlcyUyMiUzQSU1QiU3QiUyMm5hbWUlMjIlM0ElMjJtYWluLmpzJTIyJTJDJTIyY29kZSUyMiUzQSUyMmNvbnN0JTIwdW51c2VkMSUyMCUzRCUyMGZ1bmN0aW9uKCklN0IlN0Quc29tZUZ1bmN0aW9uKCklM0IlNUNuY29uc3QlMjB1bnVzZWQyJTIwJTNEJTIwY2xhc3MlN0IlN0Quc29tZUZ1bmN0aW9uKCklM0IlNUNuY29uc3QlMjB1bnVzZWQzJTIwJTNEJTIwJTdCJTdELnNvbWVGdW5jdGlvbigpJTNCJTIyJTJDJTIyaXNFbnRyeSUyMiUzQXRydWUlN0QlNUQlMkMlMjJvcHRpb25zJTIyJTNBJTdCJTIyZm9ybWF0JTIyJTNBJTIyZXMlMjIlMkMlMjJuYW1lJTIyJTNBJTIybXlCdW5kbGUlMjIlMkMlMjJhbWQlMjIlM0ElN0IlMjJpZCUyMiUzQSUyMiUyMiU3RCUyQyUyMmdsb2JhbHMlMjIlM0ElN0IlN0QlN0QlMkMlMjJleGFtcGxlJTIyJTNBbnVsbCU3RA==

Additionally this PR slightly improves white-space rendering when simplifying certain expressions and also prevents an ASI issue when simplifying assignments:
https://rollupjs.org/repl/?version=2.38.4&shareable=JTdCJTIybW9kdWxlcyUyMiUzQSU1QiU3QiUyMm5hbWUlMjIlM0ElMjJtYWluLmpzJTIyJTJDJTIyY29kZSUyMiUzQSUyMmZ1bmN0aW9uJTIwdGVzdCgpJTIwJTdCJTVDbiUyMCUyMHZhciUyMHglM0IlNUNuJTVDdHJldHVybiUyMHglMjAlM0QlMjAlMkYqJTIwY29tbWVudCUyMColMkYlNUNuJTVDdCU1Q3Q0MiUzQiU1Q24lN0QlNUNuY29uc29sZS5sb2codGVzdCgpKSUzQiUyMiUyQyUyMmlzRW50cnklMjIlM0F0cnVlJTdEJTVEJTJDJTIyb3B0aW9ucyUyMiUzQSU3QiUyMmZvcm1hdCUyMiUzQSUyMmVzJTIyJTJDJTIybmFtZSUyMiUzQSUyMm15QnVuZGxlJTIyJTJDJTIyYW1kJTIyJTNBJTdCJTIyaWQlMjIlM0ElMjIlMjIlN0QlMkMlMjJnbG9iYWxzJTIyJTNBJTdCJTdEJTdEJTJDJTIyZXhhbXBsZSUyMiUzQW51bGwlN0Q=
Fixed:
https://rollupjs.org/repl/?circleci=14200&shareable=JTdCJTIybW9kdWxlcyUyMiUzQSU1QiU3QiUyMm5hbWUlMjIlM0ElMjJtYWluLmpzJTIyJTJDJTIyY29kZSUyMiUzQSUyMmZ1bmN0aW9uJTIwdGVzdCgpJTIwJTdCJTVDbiUyMCUyMHZhciUyMHglM0IlNUNuJTVDdHJldHVybiUyMHglMjAlM0QlMjAlMkYqJTIwY29tbWVudCUyMColMkYlNUNuJTVDdCU1Q3Q0MiUzQiU1Q24lN0QlNUNuY29uc29sZS5sb2codGVzdCgpKSUzQiUyMiUyQyUyMmlzRW50cnklMjIlM0F0cnVlJTdEJTVEJTJDJTIyb3B0aW9ucyUyMiUzQSU3QiUyMmZvcm1hdCUyMiUzQSUyMmVzJTIyJTJDJTIybmFtZSUyMiUzQSUyMm15QnVuZGxlJTIyJTJDJTIyYW1kJTIyJTNBJTdCJTIyaWQlMjIlM0ElMjIlMjIlN0QlMkMlMjJnbG9iYWxzJTIyJTNBJTdCJTdEJTdEJTJDJTIyZXhhbXBsZSUyMiUzQW51bGwlN0Q=

@rollup-bot
Copy link
Collaborator

@rollup-bot rollup-bot commented Feb 5, 2021

Thank you for your contribution! ❤️

You can try out this pull request locally by installing Rollup via

npm install rollup/rollup#gh-3950-wrap-simplified-assignments

or load it into the REPL:
https://rollupjs.org/repl/?circleci=14200

@codecov
Copy link

@codecov codecov bot commented Feb 5, 2021

Codecov Report

Merging #3951 (4dd0049) into master (991bb98) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3951   +/-   ##
=======================================
  Coverage   97.18%   97.19%           
=======================================
  Files         188      190    +2     
  Lines        6678     6694   +16     
  Branches     1948     1958   +10     
=======================================
+ Hits         6490     6506   +16     
  Misses         99       99           
  Partials       89       89           
Impacted Files Coverage Δ
src/ast/nodes/LabeledStatement.ts 100.00% <ø> (ø)
src/utils/renderHelpers.ts 100.00% <ø> (ø)
src/ast/nodes/AssignmentExpression.ts 100.00% <100.00%> (ø)
src/ast/nodes/CallExpression.ts 95.41% <100.00%> (-0.09%) ⬇️
src/ast/nodes/ClassExpression.ts 100.00% <100.00%> (ø)
src/ast/nodes/ConditionalExpression.ts 96.34% <100.00%> (ø)
src/ast/nodes/FunctionExpression.ts 100.00% <100.00%> (ø)
src/ast/nodes/LogicalExpression.ts 98.78% <100.00%> (+0.01%) ⬆️
src/ast/nodes/MemberExpression.ts 98.36% <100.00%> (+0.04%) ⬆️
src/ast/nodes/ObjectExpression.ts 92.85% <100.00%> (+0.05%) ⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 991bb98...4dd0049. Read the comment docs.

@lukastaegert lukastaegert merged commit 29aed19 into master Feb 5, 2021
9 checks passed
@lukastaegert lukastaegert deleted the gh-3950-wrap-simplified-assignments branch Feb 5, 2021
{}.propertyIsEnumerable( 'toString' );
{}.propertyIsEnumerable( 'toString' ).valueOf();
({}).propertyIsEnumerable( 'toString' );
({}).propertyIsEnumerable( 'toString' ).valueOf();
Copy link
Contributor

@kzc kzc Feb 5, 2021

@lukastaegert If all test output is reparsed with acorn in the test framework then issues like this one could be caught earlier in the future. Please ignore if this PR already implemented this suggestion, but I did not see it.

For example, using the previous version of this file:

$ rollup test/form/samples/builtin-prototypes/object-expression/_expected.js --silent
[!] Error: Unexpected token
test/form/samples/builtin-prototypes/object-expression/_expected.js (3:2)
1: const object = {};
2: object.propertyIsEnumerable( 'toString' );
3: {}.propertyIsEnumerable( 'toString' );
     ^
4: {}.propertyIsEnumerable( 'toString' ).valueOf();
Error: Unexpected token

Copy link
Contributor

@kzc kzc Feb 5, 2021

With 574050b:

$ rollup --silent < test/form/samples/system-export-compact/_expected.js
[!] Error: Unexpected token (Note that you need plugins to import files that are not JavaScript)
- (3:8)
1: System.register([],function(exports){'use strict';return{execute:function(){exports({a:void 0,b:void 0,c:void 0});let a, b;
2: 
3: function(v){return exports({a:a}),v}([{ b: a = (exports('a',a-1),a--) }] = { b: function(v){return exports({b:v,c:v}),v}(--b) });}}});
           ^
$ rollup --silent < test/form/samples/system-multiple-export-bindings/_expected.js 
[!] Error: Unexpected token (Note that you need plugins to import files that are not JavaScript)
- (47:12)
45: 
46:       // Update Expression
47:       function (v) { return exports({ a: v, a2: v }), v; }(++a), (exports('b', b + 1), b++), (++c, exports({ c: c, c2: c }), c);
                   ^
$ npx terser < test/form/samples/no-external-live-bindings-compact/_expected/system.js
Parse error at 0:1,265
odule.external1);},function(module){var _setter={};for(var _$pinmodule){if(!_sta
                                                                      ^
ERROR: Unexpected token punc «)», expected punc «;»

@kzc
Copy link
Contributor

@kzc kzc commented Feb 6, 2021

The following patch finds the above errors in test/form by running each test's output through acorn. It does not significantly slow down npm test. Something similar could be done to catch possible ill-formed expected output in test/chunking-form/index.js and test/cli/index.js.

diff --git a/test/form/index.js b/test/form/index.js
index 4e329c4..2734cfc 100644
--- a/test/form/index.js
+++ b/test/form/index.js
@@ -13,34 +13,48 @@ runTestSuiteWithSamples('form', path.resolve(__dirname, 'samples'), (dir, config
     path.basename(dir) + ': ' + config.description,
     () => {
       let bundle;
+      let parse;
       const runRollupTest = async (inputFile, bundleFile, defaultFormat) => {
         if (config.before) config.before();
         try {
           process.chdir(dir);
-          bundle =
-            bundle ||
-            (await rollup.rollup(
-              Object.assign(
-                {
-                  input: dir + '/main.js',
-                  onwarn: warning => {
-                    if (
-                      !(
-                        config.expectedWarnings &&
-                        config.expectedWarnings.indexOf(warning.code) >= 0
-                      )
-                    ) {
-                      throw new Error(
-                        `Unexpected warnings (${warning.code}): ${warning.message}\n` +
-                          'If you expect warnings, list their codes in config.expectedWarnings'
-                      );
-                    }
-                  },
-                  strictDeprecations: true
-                },
-                config.options || {}
-              )
-            ));
+          let options = Object.assign(
+            {
+              input: dir + '/main.js',
+              onwarn: warning => {
+                if (
+                  !(
+                    config.expectedWarnings &&
+                    config.expectedWarnings.indexOf(warning.code) >= 0
+                  )
+                ) {
+                  throw new Error(
+                    `Unexpected warnings (${warning.code}): ${warning.message}\n` +
+                      'If you expect warnings, list their codes in config.expectedWarnings'
+                  );
+                }
+              },
+              strictDeprecations: true,
+            },
+            config.options || {}
+          );
+
+          // Get the test specific acorn parser.
+          // Add a buildStart hook without destroying possible test plugins.
+          if (options.plugins) {
+            if (!Array.isArray(options.plugins)) {
+              options.plugins = [ options.plugins ];
+            }
+          } else {
+            options.plugins = [];
+          }
+          options.plugins.push({
+            buildStart() {
+              parse = this.parse;
+            }
+          });
+
+          bundle = bundle || await rollup.rollup(options);
           await generateAndTestBundle(
             bundle,
             Object.assign(
@@ -52,7 +66,8 @@ runTestSuiteWithSamples('form', path.resolve(__dirname, 'samples'), (dir, config
               (config.options || {}).output || {}
             ),
             bundleFile,
-            config
+            config,
+            parse
           );
         } finally {
           if (config.after) config.after();
@@ -76,7 +91,7 @@ runTestSuiteWithSamples('form', path.resolve(__dirname, 'samples'), (dir, config
   );
 });
 
-async function generateAndTestBundle(bundle, outputOptions, expectedFile, { show }) {
+async function generateAndTestBundle(bundle, outputOptions, expectedFile, { show }, parse) {
   await bundle.write(outputOptions);
   const actualCode = normaliseOutput(sander.readFileSync(outputOptions.file));
   let expectedCode;
@@ -89,6 +104,13 @@ async function generateAndTestBundle(bundle, outputOptions, expectedFile, { show
     expectedCode = 'missing file';
   }
 
+  // verify that the expected output is valid
+  try {
+    parse(expectedCode);
+  } catch (err) {
+    expectedCode = `expected code is ill-formed: ${outputOptions.file}: ${err}`;
+  }
+
   try {
     actualMap = JSON.parse(sander.readFileSync(outputOptions.file + '.map').toString());
     actualMap.sourcesContent = actualMap.sourcesContent

@kzc
Copy link
Contributor

@kzc kzc commented Feb 6, 2021

Ignore the patch above. A more comprehensive solution to output verification is #3952.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants