ljharb / es-abstract

ECMAScript spec abstract operations.
MIT License
114 stars 30 forks source link

[New] `GetIntrinsic`: Cache accessed intrinsics #98

Closed ExE-Boss closed 4 years ago

ExE-Boss commented 4 years ago

This makes it so that getBaseIntrinsic takes an array of intrinsic names in ascending priority, e.g.: ['Object', 'ObjectPrototype', 'ObjProto_valueOf'] and iterates over them backwards by checking to see if INTRINSICS has the relevant intrinsic.

This was changed so that getBaseIntrinsic first consults the LEGACY_ALIASES object, which is used to expand legacy aliases into property paths.

After that, the rest of the path is walked normally, caching all own properties up to the constructor property, which would result in recursion.


Do note that this code will need to be revised once the Iterator Helpers intrinsics get added to GetIntrinsic.js, which will be the first time a constructor was added after its prototype.

ExE-Boss commented 4 years ago

If the intent is to make sure that, once looked up, a fully-dotted intrinsic remains available, is there a reason not to merely keep an object whose keys are in the form of%a.b.c%, to store them there once found, before returning, and before doing the walk, check to see if they're already in the cache?

The first part of this makes it so that if you have an intrinsic like %ObjProto_toString%, then GetIntrinsic("%Object.prototype.toString%") returns the same value as GetIntrinsic("%ObjProto_toString%"), because if we only cache by the name parameter, then the following would fail for all but the last two entries:

const original = Object.prototype.toString;
forEach([
    "%Object.prototype.toString%",
    "Object.prototype.toString",
    "%ObjectPrototype.toString%",
    "ObjectPrototype.toString",
    "%ObjProto_toString%",
    "ObjProto_toString",
], name => {
    Object.prototype.toString = function toString() {
        return original.apply(this, arguments);
    };
    console.assert(GetIntrinsic(name) === original);
});
ljharb commented 4 years ago

hmm, i feel like i'd rather manually specify/hardcode those values in that case, rather than trying to derive that they're the same?

In all cases we should be normalizing to have the % (or to not have them), so that list of 6 items is really just 3.

ljharb commented 4 years ago

iow i think a hardcoded lookup for all the non-dotted ones to directly map to dotted ones, and then falling into the normal dotted lookup, seems good to me.

codecov[bot] commented 4 years ago

Codecov Report

Merging #98 into master will decrease coverage by 0.02%. The diff coverage is 95.45%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #98      +/-   ##
==========================================
- Coverage   90.89%   90.87%   -0.03%     
==========================================
  Files         670      670              
  Lines        9447     9448       +1     
  Branches     2191     2169      -22     
==========================================
- Hits         8587     8586       -1     
- Misses        860      862       +2     
Impacted Files Coverage Δ
GetIntrinsic.js 93.91% <95.45%> (-1.71%) :arrow_down:

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 1f8ad9b...5999619. Read the comment docs.