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

Treat unknown prototype props as unknown #4428

Merged
merged 3 commits into from Mar 6, 2022

Conversation

lukastaegert
Copy link
Member

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 #4425

Description

Following the argument provided in #4425, this will deoptimize unknown array (and object) properties in the following way:

  • Accessing an unknown property is treated as an "unknown" value instead of undefined.
    const object = {};
    if (object.missing) console.log('will be retained because Object.prototype.missing might be defined');
    There are two exceptions:
    • When we know there is no prototype by using __proto__: null in an object literal
      const object = { __proto__: null };
      if (object.missing) console.log('will be removed');
    • When the property is a number because we do not expect builtins to be numbers. This also helps for array literals:
      const object = {};
      if (object[0]) console.log('will be removed');
      const array = [];
      if (array[0]) console.log('will be removed');
  • Calling an unknown function if the name is not numeric will deoptimize all properties (i.e. we assume it may mutate the object/array):
    const object = { x: false };
    object.unknown();
    if (object.x) console.log('will be retained because calling unknown could have mutated object');
  • Accessing/assigning unknown properties is not expected to have side effects because we do not expect new builtins to be getters/setters with side effects (other than the obvious side effect of assignments to change a value).

This also means that the previous feature of treating unknown object properties as undefined is disabled with this PR.

@github-actions
Copy link

github-actions bot commented Mar 5, 2022

Thank you for your contribution! ❤️

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

npm install rollup/rollup#gh-4425_unknown_prototype_props

or load it into the REPL:
https://rollupjs.org/repl/?pr=4428

@codecov
Copy link

codecov bot commented Mar 5, 2022

Codecov Report

Merging #4428 (9c9c6d3) into master (9c8894e) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4428   +/-   ##
=======================================
  Coverage   98.76%   98.76%           
=======================================
  Files         204      204           
  Lines        7309     7316    +7     
  Branches     2077     2079    +2     
=======================================
+ Hits         7219     7226    +7     
  Misses         33       33           
  Partials       57       57           
Impacted Files Coverage Δ
src/ast/nodes/shared/ObjectPrototype.ts 100.00% <100.00%> (ø)

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 9c8894e...9c9c6d3. Read the comment docs.

@lukastaegert lukastaegert merged commit 84c0ea3 into master Mar 6, 2022
@lukastaegert lukastaegert deleted the gh-4425_unknown_prototype_props branch March 6, 2022 06:35
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.

Dead code detection too aggressive on array prototype mutation
1 participant