errors: make sure all Node.js errors show their properties

[BridgeAR]

This improves Node.js errors by always showing the attached properties
when inspecting such an error. This applies especially to SystemError.
It did often not show any properties but now all properties will be
visible.
This is done in a mainly backwards compatible way. Instead of using
prototype getters and setters, the property is now set directly on the
error.

Note: the custom inspect function is necessary for the SystemError to keep the old getter/setter behavior intact .

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/25653/

[joyeecheung]

One question regarding err.code: do we still need to show this after we put it into the message line?

[BridgeAR]

I personally think we should, since it's an actual property and the user would otherwise not know about that. It's possible to check for the code in that case.

It is of course somewhat redundant but I think for now we should just show both.

[Trott]

@nodejs/collaborators This could use some reviews.

[mcollina]

LGTM

[BridgeAR]

@mcollina PTAL (I added a TODO comment as suggested by @jasnell#29677 (comment)).

[mcollina]

Still LGTM

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/25806/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/25810/

[Trott]

Wondering if this might be more breaking than we realize. Out of caution, here's a CITGM (with debian-8 unchecked for $REASONS): https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2027/ (queued)

[boneskull]

why did this test in particular need to change? is there something unusual about how this test works?

[BridgeAR]

The difference is that the code property became enumerable. That seems important so that users become aware that it's an actual property, not only part of .stack.

Mostly we just do not check what properties exist but only verify that specific properties contain specific values.

[boneskull]

In other words, because no other tests inspect .stack this way?

[BridgeAR]

It's not the stack that is changed here. It's the output when inspecting the error (e.g., using console.log or util.inspect). The reason is that only enumerable properties are inspected by default and others are ignored.

We rarely inspect errors in tests. Mostly we just check for the properties existence and their values. That also works with non-enumerable properties.

[Trott]

CITGM looks good.

[Trott]

@nodejs/tsc What's the semver-ity of this? Patch? Major? Minor even?

[BridgeAR]

@Trott I would say it's a patch, since no property was added or removed and they are also writable and configurable as before. We might call it semver-minor due to the change in enumerable properties but it seems more like a fix to me. We do enumerable properties on other system errors and also on other error types. Just the code property was an exception to that rule (but it is the most common property).

[Trott]

Going to land as patch. semver-* label can always be added after landing if anyone disagrees.

[Trott]

Landed in 500720f