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

Deoptimize ObjectExpression when a __proto__ property is present #4019

Merged
merged 5 commits into from Mar 29, 2021

Conversation

marijnh
Copy link
Contributor

@marijnh marijnh commented Mar 26, 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: #4014

Description

Works around issues with effect tracking in object literals with a prototype (specified with a __proto__ or "__proto__" property) by completely deoptimizing nodes with such a property.

@codecov
Copy link

codecov bot commented Mar 26, 2021

Codecov Report

Merging #4019 (d3e7b1f) into master (56b3eb8) will increase coverage by 0.00%.
The diff coverage is 71.42%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4019   +/-   ##
=======================================
  Coverage   97.47%   97.47%           
=======================================
  Files         192      192           
  Lines        6771     6773    +2     
  Branches     1983     1984    +1     
=======================================
+ Hits         6600     6602    +2     
  Misses         84       84           
  Partials       87       87           
Impacted Files Coverage Δ
src/ast/nodes/ObjectExpression.ts 94.55% <71.42%> (+0.07%) ⬆️

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 56b3eb8...d3e7b1f. Read the comment docs.

Comment on lines 56 to 63
this.getPropertyMap();
if (
!this.hasUnknownDeoptimizedProperty &&
this.properties.some(prop => {
if (prop instanceof SpreadElement || prop.computed) return false;
if (prop.key instanceof Identifier) return prop.key.name == "__proto__";
return String((prop.key as Literal).value) == "__proto__";
})
Copy link
Member

Choose a reason for hiding this comment

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

This could actually be simplified by using the property map:

Suggested change
this.getPropertyMap();
if (
!this.hasUnknownDeoptimizedProperty &&
this.properties.some(prop => {
if (prop instanceof SpreadElement || prop.computed) return false;
if (prop.key instanceof Identifier) return prop.key.name == "__proto__";
return String((prop.key as Literal).value) == "__proto__";
})
const propertyMap = this.getPropertyMap();
if (
!this.hasUnknownDeoptimizedProperty &&
propertyMap.__proto__?.exactMatchRead

that means we check if there is at least one property in the object that can be explicitly determined to be __proto__ (and is not a setter). That would also automatically cover cases like this:

var foo = {['__proto__']: obj}

const getProto () => '__proto__' ;
var foo = {[getProto()]: obj}

One thing that is not covered, though is an unknown computed property that resolves to __proto__. If we would want to include that as well, then it gets kind of complicated.

Another alternative here would be to integrate this into getPropertyMap. Because in the end, __proto__ does not deoptimize everything, it just changes the prototype so that there may be additional "hidden" properties. So it would be equivalent to the very first property being an "unknown computed" property: All properties below are still valid.
Simplifying it further, one could just treat __proto__ like an unknown computed property when building the property map. And no explicit property deoptimization would need to take place. Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Computed properties don't work for this feature (they create a regular property named __proto__, don't set the prototype), so that's not an issue.

This could definitely be smarter, this patch mostly tries to crudely patch the soundness hole. I could switch it over to using an unknown computed property, if you think that works. I'll look into it next week.

Copy link
Member

Choose a reason for hiding this comment

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

TIL: __proto__ needs to be a written string in object expressions. Ouch. Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've pushed a patch that moves this to getPropertyMap as suggested.

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.

Looks good! I took the liberty to fix the formatting as apparently the commit hooks is not running for you (maybe a problem with the setup?). In any case, I guess we should move to ESLint with an included prettier config eventually...

@github-actions
Copy link

Thank you for your contribution! ❤️

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

npm install marijnh/rollup#deoptimize-proto

Once a build has completed, you can load it into the REPL by inserting the CircleCI build number of the ci/circleci: node-v12-latest build into the link below (unfortunately, we cannot auto-update this comment for PRs from forks at this time):

https://rollupjs.org/repl/?circleci=<build_number>

@lukastaegert lukastaegert merged commit 85a3724 into rollup:master Mar 29, 2021
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.

None yet

2 participants