tools: add prefer-proto rule

[ljharb]

If this lands, I'd be happy to refactor this rule to a prefer-syntax rule, which also checks for things like ObjectAssign with a literal as the first argument, use of Boolean(x) instead of !!x, etc.

[nodejs-github-bot]

Review requested:

• @nodejs/http
• @nodejs/http2
• @nodejs/loaders
• @nodejs/modules
• @nodejs/net
• @nodejs/startup
• @nodejs/test_runner

[ljharb]

cc @aduh95

[cjihrig]

I'm in favor of having more consistency here. I have a couple of questions:

1. Can we remove all ObjectCreate() occurrences from the codebase? I noticed the instance from tools: add prefer-proto rule #46083 (review) is no longer part of this diff because it has more than one argument.
2. If autofixing with the second argument is off the table (current state of this PR), could we still show a lint error?

[ljharb]

The alternative form of ObjectCreate(null, x) is ObjectDefineProperties({ __proto__: null, x) which i'm not sure is better. There's also at least one ObjectCreate(constructor, x) form.

I am more than happy to do a more thorough audit once there's general agreement on the lint rule's philosophy (prefer syntax over API)

[cjihrig]

LGTM, but I'm probably less opinionated about this than some other folks 😄

[GeoffreyBooth]

👍 with the change suggested in https://github.com/nodejs/node/pull/46083/files#r1060980774

[anonrig]

lgtm when the tests succeed

[RafaelGSS]

@ljharb This commit didn't land cleanly on v19.x because some branches contain the ObjectCreate. I'll open a backport for you.

[RafaelGSS]

@ljharb here you go #46239.

[Qard]

Is there some explanation for why this change happened? Nothing against it, just could use some explanation in the PR description so people know why we prefer the syntax form over the function call form.

[ljharb]

@Qard sure, i'd phrase it as:

Syntax is always preferred over equivalent non-syntax forms, modulo readability concerns, and __proto__ syntax is simpler, faster, and most importantly, more idiomatic than a primordial call. Implied here is also that a safe idiomatic approach would always be preferred over an equivalent primordial approach.

Does that suffice?

[Qard]

Sounds reasonable, thanks! 🙂

[WebReflection]

@ljharb about this:

and __proto__ syntax is simpler, faster

I have very mixed results across gjs, bun, or node ... not the best benchmark ever, but __proto__ is not always the best option. What happened to Null class? how come node doesn't have one in core? 'cause I remember this was already tested and benchmarked as the best option.

Here a test example:

const {assign, create, setPrototypeOf} = Object;

class Test {
  constructor(i) {
    this.i = i;
  }
}

class CEWValue {
  constructor(value) {
    this.value = value;
  }
  get configurable() { return true }
  get enumerable() { return true }
  get writable() { return true }
}

class Null {}
setPrototypeOf(
  Null.prototype,
  null
);

const bench = {
  ["__proto__"](times, withField) {
    const instances = [];
    for (let {prototype} = Test, i = 0; i < times; i++)
      instances[i] = withField ?
        {__proto__: prototype, i: Math.random()} :
        {__proto__: prototype};
    return instances;
  },
  create(times, withField) {
    const instances = [];
    for (let {prototype} = Test, i = 0; i < times; i++)
      instances[i] = withField ?
        create(prototype, {"i": new CEWValue(Math.random())}) :
        create(prototype);
    return instances;
  },
  new(times, withField) {
    const instances = [];
    for (let i = 0; i < times; i++)
      instances[i] = withField ?
        new Test(Math.random()) :
        new Test;
    return instances;
  },
  Null(times, withField) {
    const instances = [];
    for (let i = 0; i < times; i++)
      instances[i] = withField ?
        assign(new Null, {i: Math.random()}) :
        new Null;
    return instances;
  }
};

function test(name, times, Class = Test) {
  if (typeof process === 'object')
    console.log(`\x1b[1m${name}\x1b[0m`);
  else
    console.log(`%c${name}`, 'font-weight:bold');
  let instances = [new Class(0)];
  console.time('cold - no fields');
  instances = bench[name](times, false);
  console.timeEnd('cold - no fields');
  for (let i = 0; i < 5; i++)
    instances = bench[name](times, false);
  console.time('hot - no fields');
  instances = bench[name](times, false);
  console.timeEnd('hot - no fields');
  console.time('cold ? fields');
  instances = bench[name](times, true);
  console.timeEnd('cold ? fields');
  for (let i = 0; i < 5; i++)
    instances = bench[name](times, true);
  console.time('hot - fields');
  instances = bench[name](times, true);
  console.timeEnd('hot - fields');
  verify(name, instances, Class);
  console.log('');
}

function verify(name, instances, Class) {
  console.assert(
    instances.every(o => o instanceof Class && typeof o.i === 'number'),
    `${name} failed`
  );
}

function benchAll(times) {
  test("new", times);
  test("Null", times, Null);
  test("create", times);
  test("__proto__", times);
}

benchAll(10_000);

[ljharb]

@WebReflection if { __proto__: Null } is faster than { __proto__: null } then i'm sure we'd all be interested in that! My claim was that { __proto__: null } was faster than ObjectCreate(null).

[WebReflection]

@ljharb it's new Null vs {__proto__: null} but definitively everything is faster than Object.create

[juanarbol]

This does not land cleanly on v18.x

[ljharb]

@juanarbol thanks, does #46239 land cleanly? if not, ill try to open a backport PR.

[maximelkin]

Patch to benchmark:

-        create(prototype, {"i": new CEWValue(Math.random())})
+        assign(create(prototype), {"i": Math.random()}) :

after patch

const {assign, create, setPrototypeOf} = Object;

class Test {
  constructor(i) {
    this.i = i;
  }
}

class CEWValue {
  constructor(value) {
    this.value = value;
  }
  get configurable() { return true }
  get enumerable() { return true }
  get writable() { return true }
}

class Null {}
setPrototypeOf(
  Null.prototype,
  null
);

const bench = {
  ["__proto__"](times, withField) {
    const instances = [];
    for (let {prototype} = Test, i = 0; i < times; i++)
      instances[i] = withField ?
        {__proto__: prototype, i: Math.random()} :
        {__proto__: prototype};
    return instances;
  },
  create(times, withField) {
    const instances = [];
    for (let {prototype} = Test, i = 0; i < times; i++)
      instances[i] = withField ?
        assign(create(prototype), {"i": Math.random()}) :
        create(prototype);
    return instances;
  },
  new(times, withField) {
    const instances = [];
    for (let i = 0; i < times; i++)
      instances[i] = withField ?
        new Test(Math.random()) :
        new Test;
    return instances;
  },
  Null(times, withField) {
    const instances = [];
    for (let i = 0; i < times; i++)
      instances[i] = withField ?
        assign(new Null, {i: Math.random()}) :
        new Null;
    return instances;
  }
};

function test(name, times, Class = Test) {
  if (typeof process === 'object')
    console.log(`\x1b[1m${name}\x1b[0m`);
  else
    console.log(`%c${name}`, 'font-weight:bold');
  let instances = [new Class(0)];
  console.time('cold - no fields');
  instances = bench[name](times, false);
  console.timeEnd('cold - no fields');
  for (let i = 0; i < 5; i++)
    instances = bench[name](times, false);
  console.time('hot - no fields');
  instances = bench[name](times, false);
  console.timeEnd('hot - no fields');
  console.time('cold ? fields');
  instances = bench[name](times, true);
  console.timeEnd('cold ? fields');
  for (let i = 0; i < 5; i++)
    instances = bench[name](times, true);
  console.time('hot - fields');
  instances = bench[name](times, true);
  console.timeEnd('hot - fields');
  verify(name, instances, Class);
  console.log('');
}

function verify(name, instances, Class) {
  console.assert(
    instances.every(o => o instanceof Class && typeof o.i === 'number'),
    `${name} failed`
  );
}

function benchAll(times) {
  test("new", times);
  test("Null", times, Null);
  test("create", times);
  test("__proto__", times);
}

benchAll(10_000);

node v19.5.0, ubuntu 20.04

new
cold - no fields: 1.156ms
hot - no fields: 0.326ms
cold ? fields: 1.486ms
hot - fields: 1.193ms

Null
cold - no fields: 0.821ms
hot - no fields: 0.059ms
cold ? fields: 1.275ms
hot - fields: 0.987ms

create
cold - no fields: 0.798ms
hot - no fields: 0.262ms
cold ? fields: 1.744ms
hot - fields: 0.548ms

__proto__
cold - no fields: 1.124ms
hot - no fields: 0.529ms
cold ? fields: 2.146ms
hot - fields: 1.545ms