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

What Are Idiomatic Brand Checks for Built-ins? #13

Closed
sebmarkbage opened this issue Jul 4, 2017 · 11 comments
Closed

What Are Idiomatic Brand Checks for Built-ins? #13

sebmarkbage opened this issue Jul 4, 2017 · 11 comments

Comments

@sebmarkbage
Copy link

sebmarkbage commented Jul 4, 2017

The different types of brand checks I've identified in JS code follow these patterns:

x instanceof y
y in x
x.hasOwnProperty(y)
!!x.y
typeof x === y
isArray(x)
IsPromise(x) // (abstract operation not yet exposed)
x.nodeType === y

It would be ideal if these were unified by pattern matching so that it is not necessary to use other mechanisms. Currently we tend to mix these in the same place which looks pretty messy. If we were to continue using the same brand checks we'd have to bail out of the pattern matching syntax to do so. That in turn means that whatever the default matchers are basically say what the best practice is.

One interpretation is that the proposal only defines the x instanceof y and y in x. This means that this matching strategy is what we define as the best practice or common use case. However, this is not the cow path often used by libraries.

For duck typing properties it is fairly uncommon to use in. It's common to use hasOwnProperty to protect against prototype chains being tainted or expanded. Although it is also common to check for truthiness.

For primitives it is more common to check typeof x === "string" than x instanceof String to ensure that you have a true primitive string rather than the object form. An instanceof check mostly works when you only use primitives but it lets through cross-realm objects which can be bad since most code relies on the prototype methods being the same. It doesn't compose well when other parts of the system use typeof. It might be ok to try to push through instanceof as the defacto way everyone should do it going forward, if we assume that the normal use case is to use primitive values in those cases.

To be resilient against multiple realms, it is common to use isArray to allow arrays to pass between realms. Similarly, iframes have multiple documents and each of those have different Node and Element prototypes. Meaning that instanceof Element is not safe across frames. node.nodeType === Node.ELEMENT_NODE is used for such brand checks. Matching on Array or Element won't catch those in the current proposal. However, similarly to allowing cross-realm strings, it might also be confusing that they do match when the prototype may have different methods on it.

Promise polyfills typically have a variant of IsPromise (which is not directly exposed by the standard library) to check for Promises for the same reason. Should matching a Promise do instanceof or IsPromise?

@ljharb
Copy link
Member

ljharb commented Jul 4, 2017

This might work well in combination with https://github.com/jasnell/proposal-istypes, which attempts to answer this question.

@Jamesernator
Copy link

In the library I wrote for pattern matching I used Object.is for all primitive types so:

match(undefined, undefined) // true
match(true, true) // true
match(3, 12) // false
match(0, -0) // false
match(NaN, NaN) // true

I found that to be generally useful, other builtin types I did more complicated things though:

For functions I found the most useful default was that a function would
// do a boolean check if it didn't otherwise have a matchSymbol property

const positive = x => x > 0

match(positive, 10) // true

function Positive() {
    this.electrode = 'positive'
}

Positive[matchSymbol] = function() { return false }
match(Positive, 3) // false, Positive not even called

For arrays I automatically converted them into an array-pattern check thing:

match([1,2,3], [1,2,3]) // true
match([1,2], [1,3]) // false
match([[1], [2]], [[1], [2]]) // true

match([1, rest(arrayOf(number))], [1,2, 'cats', 3, 7])
    // false, rest is just a special value I used to indicate that
    // it should collect the rest then match against it

For plain objects (e.g. with no prototype or Object.prototype prototype) I just did key value matching:

const number = {
    [matchSymbol](x) {
        return typeof x === 'number' 
    }
}

match({ x: number, y: number }, { x: 10, y: 20 }) // true
match({ x: number }, { __proto__: { x: 10 } }) // true
match({ __proto__: { x: number } }, { x: 'Banana' } }) // false, only own enumerable keys used
match({ __proto__: null, x: 10, y: 20 }, { x: 10, y: 20 }) // true, but it was sufficiently rare a function would've been fine to do it

In my ideal pattern matching it'd be fairly similar with objects/functions/arrays getting resolved automatically and having some way to pattern match rest of array:

function matches(pattern, value) {
    return match (value) {
        pattern: true,
        else: false
    }
}

const Cheese = or('Brie', 'Camembert', 'Cheddar')

const Person = {
    name: string,
    age: number,
    favouriteCheeses: [..Cheese],
    bestFriend: or(null, person => matches(
        Person, /* Using a function as definition is recursive */
        person
    )
}

const person = {
    name: 'Bob',
    age: 42,
    favouriteCheeses: ['Potato']
}
    
matches(Person, person) // false

@domenic
Copy link
Member

domenic commented Jul 6, 2017

I am also concerned about using Number to mean number-the-primitive and not instances of the Number constructor.

In general though I think using constructors should be instanceof or .constructor checks, not brand checks. You're specifically using the identifier whose value equals self.Constructor, which is distinct from otherGlobal.Constructor. If you wanted a realm-agnostic brand check you'd actually put that in the predicate, not use a realm-specific constructor value as some sort of misleading proxy for what you actually mean.

@ljharb
Copy link
Member

ljharb commented Jul 6, 2017

I agree - that's why I don't want "constructors" as a special check. Rather, if you want instanceof semantics, you should put x instanceof Number in your predicate (just like a brand check) - that decision thus is left up to the user.

@Jamesernator
Copy link

It's still a shame there's no mechanism for built-in modules yet as then you could have a module with all the primitive checks (and potentially other useful patterns e.g. callable, thenable, etc) e.g.

import { number, string } from patterns

const message = match (someValue) {
    string: 'Is a string!',
    number: 'Is a number!',
}

It'd be nicer if they were just global but I don't think adding a bunch of a almost certainly commonly used variable names to the global scope would be a great idea.

@aluanhaddad
Copy link

aluanhaddad commented Oct 31, 2017

@Jamesernator they could be meta properties like match.string and match.number. I'm just 🚲🏠ing / spitballing here.

Or perhaps for better syntactic distinction, case.string and case.number.

@allenwb
Copy link
Member

allenwb commented Oct 31, 2017

The techniques described in https://esdiscuss.org/topic/tostringtag-spoofing-for-null-and-undefined#content-59 are probably of interest.

That and subsequent posts in the thread also discuss the hazards of over generalizing the meaningfulness of such brand checks.

@ljharb
Copy link
Member

ljharb commented Oct 31, 2017

They tell you for each of the tested built-ins (well, except for the individual typed arrays) whether the built-ins defined for it will work on the tested object. Nothing more, nothing less.

That's precisely what brand checks are critically necessary for, and "try/catching around an arbitrary-for-each-type prototype method" isn't sufficiently usable or elegant to suffice.

The only way a brand check isn't extremely useful for any object is if it has no internal slots or private state that a function could pivot on - the only builtin object type in JS that's true for is Error, and only that because stacks aren't specified.

@allenwb
Copy link
Member

allenwb commented Oct 31, 2017

That's precisely what brand checks are critically necessary for, and "try/catching around an arbitrary-for-each-type prototype method" isn't sufficiently usable or elegant to suffice.

see: https://esdiscuss.org/topic/tostringtag-spoofing-for-null-and-undefined#content-61

In JS a brand check (however you perform it) of a non-frozen object guarantees very little about the object. It doesn't guarantee what its [[Prototype]] is. It doesn't guarantee what own properties it has. It doesn't guarantee anything about the actual behaviors of its methods. It doesn't tell you whether a Proxy has been used to change any of its essential internal method behaviors. It may be able to guarantee the presence of specific internal slots or private fields but it can't guarantee that they are actually used for anything.

Unless, you are only branding/testing frozen objects it isn't clear what general invariant you think the test is guaranteeing.

@ljharb
Copy link
Member

ljharb commented Oct 31, 2017

Since Proxy doesn't forward internal slots, it absolutely can tell you there's no Proxy involved - Function.prototype.toString.call(new Proxy(() => {}, {})) throws; it only returns a value on a non-proxied function; the same is true for any brand checks in the builtins as well as upcoming private fields.

A brand check of a Map or a Set or a Promise give me those guarantees (using call-bound prototype methods prior to running untrusted code, of course) - Object.freeze has no impact on their behaviors since "freeze" only prevents public properties from changing - there's no way to freeze a Map, for example.

@zkat
Copy link
Collaborator

zkat commented Mar 25, 2018

Hey y'all! #65 has gotten merged, and a lot of issues have become irrelevant or significantly changed in context. Because of the magnitude of changes and subtle differences in things that seem similar, we've decided to just nuke all existing issues so we can start fresh. Thank you so much for the contributions and discussions and feel free to create new issues if something seems to still be relevant, and link to the original, related issue so we can have a paper trail (but have the benefit of that clean slate anyway).

There is some discussion to be had about the default instanceof checks in the current proposal, but they should probably be discussed anew with the new spec in consideration.

@zkat zkat closed this as completed Mar 25, 2018
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

No branches or pull requests

7 participants