jasnell / proposal-istypes

TC-39 Proposal for additional is{Type} APIs
201 stars 7 forks source link

Concerns around `Builtin.is` #34

Open caridy opened 7 years ago

caridy commented 7 years ago

At first glance Builtin.is is confusing, it is also very different from Object.is.

When looking at the README:

The Builtin.is() function returns true if both of the given values have a @@builtin own property function that each returns values that, after coercion to a string, are strictly equal to one another. Otherwise, return false.

It is not clear to me what's the motivation behind this API. It doesn't seem to be related to the identity discontinuity phenomenon that Builtin.typeOf() is trying to solve.

caridy commented 7 years ago

In fact, the more I re-read the proposal, the more I think Builtin.is doesn't add much to it. And knowing that adding a new global is going to be a problem for many, I rather will like to recommend to narrow this proposal down to a new method of Reflect, (e.g.: Reflect.typeOf(x)), and we will have a better chance to get this to be considered. If a user wants to implement something equivalent to Builtin.is then should be able to do so by relying on the descriptor for the @@builtin property.

jasnell commented 7 years ago

Builtin.is() addresses the problem of determining if two cross-realm things represent the same intrinsic thing. For instance, in Node.js

const vm = require('vm');
console.log(Date === vm.runInNewContext('Date')) // false

// whereas...

console.log(Builtins.is(Date, vm.runInNewContext('Date'))) // true

This will be far more important for intrinsic functions than it will be for objects, e.g.

Builtins.typeOf(vm.runInNewContext('eval'))  // function ...
Builtins.typeOf(vm.runInNewContext('function eval() {}')) // function ...
Builtins.is(vm.runInNewContext('eval'), eval) // true
Builtins.is(vm.runInNewContext('function eval() {}'), eval) // false
ljharb commented 7 years ago

It's definitely a very important use case - the name indeed could be better (#26)

jasnell commented 7 years ago

I honestly can't stand the name :-) ... couldn't think of anything better!

caridy commented 7 years ago

Ok, I'm even more confused now, what does Builtins.is(vm.runInNewContext('eval'), eval) === true mean? When do you need to know that? Same question for Date constructor, why would you care about the constructors be the same? What if I patch the constructor and still return the proper Date instance?

Normally, you don't care about the constructors, you care about the instances. As for the Date instances:

Builtins.typeOf(new Date()) === Builtins.typeOf(vm.runInNewContext('new Date()'));

It yields true since both return the string value for the "Date".

Btw, the reason of all these questions is because we are working on exposing the intrinsics as part of the Realm proposal, so probably there will be some overlapping with this proposal.

/cc @erights

jasnell commented 7 years ago

Builtins.is(vm.runInNewContext('eval'), eval) === true means that the two instances of the eval function represent the same intrinsic eval function, even tho they are not strictly equal to one another. Knowing this, the assumption would be that the two are interchangeable. I admit that it is an edge case, but it's one that I have seen before in debugging across realms.

jasnell commented 7 years ago

FWIW, if there's not enough justification for Builtins.is(), I'm not opposed to dropping it if doing so allows us to advance Builtins.typeOf().

ljharb commented 7 years ago

I think that what's really important is a function for "get own builtin value". I find Builtin.typeof less useful, because it uses the unreliable constructor property.

If we want to reduce the proposal, I'd prefer a single method that returns the own builtin value (with associated typechecks), and ideally has a fallback to brand checks.

caridy commented 7 years ago

I find Builtin.typeof less useful, because it uses the unreliable constructor property.

@ljharb can you elaborate more? I'm curious what problem are you referencing to!

I'd prefer a single method that returns the own builtin value

What exactly does this function should return? a reference to the intrinsic object? How is that useful across realms?

ljharb commented 7 years ago

@caridy the constructor property can be deleted, or overridden, so it provides me zero value when trying to robustly brand-check a thing.

What I mean is (where "foo" is my single method):

function foo(x) {
  if (x == null) { return typeof x; }
  var builtin = x[Symbol.builtin];
  if (typeof builtin === 'function') {
    return String(builtin(x));
  }
  if (builtin !== undefined) {
    throw new TypeError(…);
  }
  return FallbackBrandCheckingLogic(x);
}

Then if I want to check foo(x.constructor) i can, but i'm not forced to rely on a mutable property.

caridy commented 7 years ago

@ljharb I understand now, thanks! It is very low level API, which is not a bad thing, but we loose a little bit on the ergonomics IMO.

ljharb commented 7 years ago

Agreed, which is why Builtins.is is nice, because generally i'm doing two "foo"s and ===ing the result :-)