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

workaround for various object literal mutation bugs #3266

Merged
merged 2 commits into from Dec 2, 2019
Merged

workaround for various object literal mutation bugs #3266

merged 2 commits into from Dec 2, 2019

Conversation

kzc
Copy link
Contributor

@kzc kzc commented Nov 30, 2019

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:

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

Description

Workaround for various object literal mutation bugs.

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

Fixes #2345 #2473 #3027 #3093 #3192 #3264

@codecov
Copy link

codecov bot commented Nov 30, 2019

Codecov Report

Merging #3266 into master will decrease coverage by 1.42%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3266      +/-   ##
==========================================
- Coverage   94.19%   92.76%   -1.43%     
==========================================
  Files         170      170              
  Lines        7992     5943    -2049     
  Branches     2870     1796    -1074     
==========================================
- Hits         7528     5513    -2015     
+ Misses        293      225      -68     
- Partials      171      205      +34
Impacted Files Coverage Δ
src/ast/nodes/CallExpression.ts 95.95% <100%> (-0.99%) ⬇️
src/utils/path.ts 66.66% <0%> (-13.34%) ⬇️
src/utils/renderChunk.ts 85% <0%> (-11.16%) ⬇️
src/finalisers/shared/setupNamespace.ts 90% <0%> (-10%) ⬇️
src/ast/scopes/BlockScope.ts 80% <0%> (-5.72%) ⬇️
src/watch/fileWatchers.ts 85.71% <0%> (-5.6%) ⬇️
browser/path.ts 76.92% <0%> (-5.43%) ⬇️
src/ast/nodes/ObjectExpression.ts 86.98% <0%> (-4.98%) ⬇️
src/utils/timers.ts 59.09% <0%> (-4.87%) ⬇️
src/ast/nodes/Property.ts 83.63% <0%> (-4.83%) ⬇️
... and 139 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 c753a7f...59901ec. Read the comment docs.

Copy link
Member

@lukastaegert lukastaegert left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the P!. Even though by now I have a fix for the root cause of #3264 via #3267, I would be interested in merging the other changes related to unsafe this modifications. Obviously, this has been lying around for very long and I have not gotten around to providing the necessary fix myself. If you remove the other change, this can be merged.

One other thing: This does NOT fix the third case mentioned in #2345, so it would be nice to remove #2345 from the list of fixed issues so that it is still tracked.

@kzc
Copy link
Contributor Author

kzc commented Dec 1, 2019

This PR does indeed fix the third test case in #2345 as seen here:

input:

https://github.com/rollup/rollup/pull/3266/files#diff-e327407bfb6a2951b6ffb6e99b6b6dbdR31-R37

output:

https://github.com/rollup/rollup/pull/3266/files#diff-f9e32e571549a0ee3077bed0c7b2d828R21-R27

It's a slight variation of the test case in question, but it also runs correctly on the original test case:

$ cat 2345c.js
const obj = {};

obj.modify = modify;

function modify() {
  this.modified = true;
}

obj.modify();

// This is missing!
if (obj.modified) console.log('MODIFIED');

with this PR:

$ dist/bin/rollup 2345c.js -f es --silent | node
MODIFIED

without this PR:

$ node_modules/.bin/rollup -v
rollup v1.27.6

$ node_modules/.bin/rollup 2345c.js -f es --silent | node
$ # no output

Feel free to modify this PR or ignore. I just wanted to demonstrate a workaround. The CallExpression.ts change relates to invalidating the object for member calls (the this issue), and the MemberExpression.ts change addresses #2345 test case 3 and #3264 which you mentioned was fixed through other means. The latter change can be dropped if it's no longer required.

@lukastaegert
Copy link
Member

My bad, I only copied over the CallExpression change but did not check that it fixes the last case only together with the other change. I will modify this and merge it in, thanks!

kzc and others added 2 commits December 2, 2019 05:46
Some necessary minor regressions related to previously
unsafe object literal optimizations.

fixes #2345
fixes #2473
fixes #3027
fixes #3093
fixes #3192
fixes #3264
@lukastaegert lukastaegert merged commit 3e0aa46 into rollup:master Dec 2, 2019
@@ -66,6 +67,9 @@ export default class CallExpression extends NodeBase implements DeoptimizableEnt
this
);
}
if (this.callee instanceof MemberExpression && !this.callee.variable) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Funny - I had this initially but dismissed it due the higher number of test regressions. But it's the safer route.

Copy link
Member

Choose a reason for hiding this comment

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

Well, if we gonna fix it, we might just as well fix it all the way down. I kept the tests because I REALLY hope to get in a proper solution soon.

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

Successfully merging this pull request may close these issues.

Track object modifications via "this"
2 participants