Skip to content

lib,doc: enable no-prototype-builtins #18750

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.

Sign up for GitHub

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

Closed
wants to merge 4 commits into from
Closed

lib,doc: enable no-prototype-builtins #18750

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5b1857d
lib,doc: enable no-prototype-builtins
BridgeAR Feb 13, 2018
c75ca3c
fixup
BridgeAR Feb 13, 2018
104dbf3
fixup typo
BridgeAR Feb 13, 2018
af0f40f
fixup
BridgeAR Feb 13, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions doc/.eslintrc.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,3 +12,7 @@ rules:
no-var: error
prefer-const: error
prefer-rest-params: error

# Possible Errors
# http://eslint.org/docs/rules/#possible-errors
no-prototype-builtins: error
4 changes: 3 additions & 1 deletion doc/api/util.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,9 @@ is wrapped in an `Error` with the original value stored in a field named
callbackFunction((err, ret) => {
// When the Promise was rejected with `null` it is wrapped with an Error and
// the original value is stored in `reason`.
err && err.hasOwnProperty('reason') && err.reason === null; // true
console.log(err && Object.prototype.hasOwnProperty.call(err, 'reason') &&
Copy link
Member

@devsnek devsnek Feb 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as long as this is being refactored can you pull hasOwnProperty off at the top like you did with the other files

Copy link
Member Author

@BridgeAR BridgeAR Feb 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went for removing as it is not necessary in the example. It is strictly testing for null and that has to be explicitly set.

err.reason === null);
// Prints: true
});
```

Expand Down
4 changes: 4 additions & 0 deletions lib/.eslintrc.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,3 +7,7 @@ rules:
no-let-in-for-declaration: error
lowercase-name-for-primitive: error
non-ascii-character: error

# Possible Errors
# http://eslint.org/docs/rules/#possible-errors
no-prototype-builtins: error
3 changes: 2 additions & 1 deletion lib/assert.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ const { parseExpressionAt } = require('internal/deps/acorn/dist/acorn');
const { inspect } = require('util');
const { EOL } = require('os');
const nativeModule = require('native_module');
const { isPrototypeOf } = Object.prototype;

// Escape control characters but not \n and \t to keep the line breaks and
// indentation intact.
Expand Down Expand Up @@ -400,7 +401,7 @@ function expectedException(actual, expected, msg) {
if (expected.prototype !== undefined && actual instanceof expected) {
return true;
}
if (Error.isPrototypeOf(expected)) {
if (isPrototypeOf.call(Error, expected)) {
Copy link
Member

@TimothyGu TimothyGu Feb 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reflect.apply is preferred over Function.prototype.call/apply. Ditto everywhere else.

Copy link
Contributor

@apapirovski apapirovski Feb 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have one concern about using that in getOrSetAsyncId since it's somewhat hot path code... means creating an array for the arguments each time.

Copy link
Member

@devsnek devsnek Feb 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in getOrSetAsyncId shouldn't we really be using an undefined check since it appears to be a lot faster

Copy link
Contributor

@apapirovski apapirovski Feb 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@devsnek it still has the problem of potentially inheriting through prototype. I don't know how likely it is though... I'll ping @AndreasMadsen

Copy link
Member

@devsnek devsnek Feb 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reflect.has also seems significantly faster at this point, just as another option

return false;
}
return expected.call({}, actual) === true;
Expand Down
4 changes: 3 additions & 1 deletion lib/internal/async_hooks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,8 @@ const emitDestroyNative = emitHookFactory(destroy_symbol, 'emitDestroyNative');
const emitPromiseResolveNative =
emitHookFactory(promise_resolve_symbol, 'emitPromiseResolveNative');

const { hasOwnProperty } = Object.prototype;

// Setup the callbacks that node::AsyncWrap will call when there are hooks to
// process. They use the same functions as the JS embedder API. These callbacks
// are setup immediately to prevent async_wrap.setupHooks() from being hijacked
Expand Down Expand Up @@ -250,7 +252,7 @@ function newUid() {
}

function getOrSetAsyncId(object) {
if (object.hasOwnProperty(async_id_symbol)) {
if (hasOwnProperty.call(object, async_id_symbol)) {
return object[async_id_symbol];
}

Expand Down
2 changes: 2 additions & 0 deletions lib/internal/bootstrap_node.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
// to the performance of the startup process, many dependencies are invoked
// lazily.

/* eslint-disable no-prototype-builtins */

'use strict';

(function(process) {
Expand Down
9 changes: 5 additions & 4 deletions lib/internal/crypto/sig.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ const {
const { isArrayBufferView } = require('internal/util/types');
const { Writable } = require('stream');
const { inherits } = require('util');
const { hasOwnProperty } = Object.prototype;

function Sign(algorithm, options) {
if (!(this instanceof Sign))
Expand Down Expand Up @@ -55,7 +56,7 @@ Sign.prototype.sign = function sign(options, encoding) {

// Options specific to RSA
var rsaPadding = RSA_PKCS1_PADDING;
if (options.hasOwnProperty('padding')) {
if (hasOwnProperty.call(options, 'padding')) {
if (options.padding === options.padding >> 0) {
rsaPadding = options.padding;
} else {
Expand All @@ -66,7 +67,7 @@ Sign.prototype.sign = function sign(options, encoding) {
}

var pssSaltLength = RSA_PSS_SALTLEN_AUTO;
if (options.hasOwnProperty('saltLength')) {
if (hasOwnProperty.call(options, 'saltLength')) {
if (options.saltLength === options.saltLength >> 0) {
pssSaltLength = options.saltLength;
} else {
Expand Down Expand Up @@ -114,7 +115,7 @@ Verify.prototype.verify = function verify(options, signature, sigEncoding) {

// Options specific to RSA
var rsaPadding = RSA_PKCS1_PADDING;
if (options.hasOwnProperty('padding')) {
if (hasOwnProperty.call(options, 'padding')) {
if (options.padding === options.padding >> 0) {
rsaPadding = options.padding;
} else {
Expand All @@ -125,7 +126,7 @@ Verify.prototype.verify = function verify(options, signature, sigEncoding) {
}

var pssSaltLength = RSA_PSS_SALTLEN_AUTO;
if (options.hasOwnProperty('saltLength')) {
if (hasOwnProperty.call(options, 'saltLength')) {
if (options.saltLength === options.saltLength >> 0) {
pssSaltLength = options.saltLength;
} else {
Expand Down