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

Incorrect polyfill removal when type checking is off #3171

Closed
shicks opened this issue Dec 11, 2018 · 2 comments
Closed

Incorrect polyfill removal when type checking is off #3171

shicks opened this issue Dec 11, 2018 · 2 comments
Labels
triage-done Has been reviewed by someone on triage rotation.

Comments

@shicks
Copy link
Member

shicks commented Dec 11, 2018

In integrating RemoveUnusedPolyfills into RemoveUnusedCode, we're making some changes to how polyfills are removed. The upshot is that if type checking is on, the polyfill removal should be a lot more accurate. When type checking is off, the compiler does its best to be accurate, but will have both false positives and false negatives.

For global polyfills (e.g. Promise, Map):

  • When type checking, look for either global names (note that locals have all been normalized at this point) or same-named properties on objects that could be the global object (e.g. global.Map).
  • Without type checking, it is impossible to tell if the owner is the global object, so we just assume it could be and consider it a reference.

For static polyfills (e.g. Array.from, Math.fround):

  • When type checking, ensure that the owner has the correct constructor (or namespace) type.
  • Without type checking, match the name of the owner, allowing to catch window.Array.from. This will incorrectly remove polyfills accessed via window.SubArray.from and incorrectly retain polyfills accessed via properties with the same name but different types.

For instance polyfills (e.g. String.repeat, Array.includes):

  • When type checking, a GETPROP is a reference only if the owner's type can be cast to the correct owner type.
  • Without type checking, all property accesses count as references to all matching polyfills (e.g. x.repeat regardless of where x came from).

This change should not break any existing usages, since we still use the old version of RewritePolyfills, which won't insert the polyfill in the first place in the problematic cases.

@EatingW
Copy link
Contributor

EatingW commented Dec 11, 2018

Created Google internal issue b/120848264

@EatingW EatingW added the triage-done Has been reviewed by someone on triage rotation. label Dec 11, 2018
@brad4d
Copy link
Contributor

brad4d commented Jul 1, 2020

Using type information within RemoveUnusedCode has turned out to be problematic with the way we want to move forward with type checking and optimizations, so we have removed it.

@brad4d brad4d closed this as completed Jul 1, 2020
copybara-service bot pushed a commit that referenced this issue Jul 1, 2020
Related to http://go/gh//issues/3171

PiperOrigin-RevId: 319313792
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
triage-done Has been reviewed by someone on triage rotation.
Projects
None yet
Development

No branches or pull requests

3 participants