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

New rule: no-prototype-builtins #539

Closed
feross opened this issue Jun 2, 2016 · 18 comments

Comments

@feross
Copy link
Member

commented Jun 2, 2016

Disallow use of Object.prototypes builtins directly

http://eslint.org/docs/rules/no-prototype-builtins
eslint/eslint#2693

@feross feross added the enhancement label Jun 2, 2016

@yoshuawuyts

This comment has been minimized.

Copy link
Contributor

commented Jun 2, 2016

foo.hasOwnProperty("bar")

Seems like would be in reasonably widespread use - do we have a % breakage on this?

@feross

This comment has been minimized.

Copy link
Member Author

commented Jun 2, 2016

Seems like would be in reasonably widespread use

Perhaps. Just wanted to start a discussion, since I noticed this is in the latest eslint version. Seems like a legit potential bug in lots of programs, since it's an intricacy of the language that few understand.

@yoshuawuyts

This comment has been minimized.

Copy link
Contributor

commented Jun 2, 2016

Seems like a legit potential bug in lots of programs, since it's an intricacy of the language that few understand.

Yeah, we should def try and catch as many bugs as possible - to clarify: not opposed to enforcing this as it will catch errors (hadn't thought about this particular interaction until this issue).

@feross

This comment has been minimized.

Copy link
Member Author

commented Jun 2, 2016

do we have a % breakage on this?

# tests 427
# pass  401
# fail  26

3 of those failures are false positives due to a bug in the newest eslint. Still, this is a decent-sized breakage.

Next step would be to look through the 23 cases where it fails and see which are actual potential sources of crashes. If a decent number of them (half?) are indeed real issues, then it probably makes sense to enable this in the next major version, even though some will need to change their code.

@qzb

This comment has been minimized.

Copy link

commented Jun 3, 2016

Honestly I'm surprised that % breakage is that small. Anyway, I'm not a fan of hasOwnProperty, but using it is strongly recommended for for in loops, and I've never seen such recommendation using this monstrosity:

for (var bar in foo) {
  if ({}.isPrototypeOf.call(foo, bar)) {
    // ...
  }
}

BTW, if someone needs map, should use Map or prototype-less object, not a plain object.

@LinusU

This comment has been minimized.

Copy link
Member

commented Jun 3, 2016

How is this

for (var bar in foo) {
  if ({}.hasOwnProperty.call(foo, bar)) {
    // ...
  }
}

much worse than this?

for (var bar in foo) {
  if (foo.hasOwnProperty(bar)) {
    // ...
  }
}

I've seen a lot of recommendations for using the former instead of the latter.

Also, I totally agree that you should use a Map, but sadly not all runtimes has that available...

@Flet

This comment has been minimized.

Copy link
Member

commented Jun 3, 2016

Tiny modules to the rescue? :)

module.exports = hasOwnProperty

function hasOwnProperty (obj, prop) {
  return {}.hasOwnProperty.call(obj, prop)
}

// example
var hasOwnProperty = require('has-own-property')
// ...
for (var bar in foo) {
  if (hasOwnProperty(foo, bar)) {
    // ...
  }
}

..this one looks the best 😁

@LinusU

This comment has been minimized.

Copy link
Member

commented Jun 3, 2016

@Flet

This comment has been minimized.

Copy link
Member

commented Jun 3, 2016

LOL you beat me barely! :)

npm ERR! you do not have permission to publish "has-own-property". Are you logged in as the correct user? : has-own-property
@LinusU

This comment has been minimized.

Copy link
Member

commented Jun 3, 2016

@Flet Just saw that you created your own repo, sorry I thought that you where handing out an idea 😳

I'd be happy to transfer ownership on npm to you

@Flet

This comment has been minimized.

Copy link
Member

commented Jun 3, 2016

No worries @LinusU! Honestly I thought sindre would already have this built 😁

@LinusU

This comment has been minimized.

Copy link
Member

commented Jun 3, 2016

Haha, yes, I was quite surprised when I saw that the name was free 😆

I'm adding you as a maintainer at least :) that is, if you want to?

@Flet

This comment has been minimized.

Copy link
Member

commented Jun 3, 2016

Cool! :)

@Flet

This comment has been minimized.

Copy link
Member

commented Jun 3, 2016

@LinusU my problem was it took me over 2 minutes to figure out a cute emoji for the title 😜

@LinusU

This comment has been minimized.

Copy link
Member

commented Jun 3, 2016

Hehehe, yeah, I saw that you where very fast with the project structure. Do you use any tool for that? I literally typed out the entire module with cat > file 😆

@Flet

This comment has been minimized.

Copy link
Member

commented Jun 3, 2016

I use @ungoldman's module-init to get the structure up quickly.

@monolithed

This comment has been minimized.

Copy link

commented Jun 26, 2016

What about own properties?

let object = {
    hasOwnProperty () {
      return false;
    }
};

for (let key in object) {
    console.log(Object.prototype.hasOwnProperty.call(object, key)); //true
}

for (let key in object) {
    console.log(object.hasOwnProperty(key)); // false
}

It can be fixed:

Object.prototype.length = 1;

let object = {};

for (let key in object) {
    console.log(key); //length
}

for (let key in Object.entries(object)) {
   // nothing
}

But what about legacy code?

What about type detections?

Object.prototype.toString.call({}); // [object Object]

Parsing code program (popular usage):

let types = {
   '[object Object]' () {
      // ... do some work
   },

   '[object Array]' () {
      // ... do some work
   },

  // ... 
};

let type  = Object.prototype.toString.call(some_entity);

types[type](); //

What about polyfills?

It seems you need more experience before making an issue like this

@feross

This comment has been minimized.

Copy link
Member Author

commented Jul 13, 2016

I think we'll pass on this for now. Upon further investigation, the error is a bit too confusing and yields mostly false positives. We can always re-examine this later.

@feross feross closed this Jul 13, 2016

@lock lock bot locked as resolved and limited conversation to collaborators May 10, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
6 participants
You can’t perform that action at this time.