assert: move assertion part in extra file #20486
Conversation
nodejs-github-bot
added
assert
BridgeAR
force-pushed
the
improve-assertion-stuff
branch
from
79a1021
to
d9871b3
Compare
This adds concrete expected types to the assert documentation. It also fixes a `changes` entry and improves some minor comments.
This moves the `assert` parts from `internal/errors` into an own file. `internal/errors` got bigger and bigger and it was difficult to keep a good overview of what was going on. While doing so it also removes the `internalAssert` function and just lazy loads `assert`.
This makes sure the functions are actually directly beneat the specification of an error code. That way it is not necessary to jump around when looking at the functionality.
BridgeAR
force-pushed
the
improve-assertion-stuff
branch
from
d9871b3
to
4c9c77d
Compare
|
Rebased due to conflicts. New CI https://ci.nodejs.org/job/node-test-pull-request/14695/ @nodejs/testing @jasnell @joyeecheung PTAL. This is open for five days without getting a single bit of attention... |
BridgeAR
added
the
author ready
| E('ERR_INVALID_ARG_TYPE', invalidArgType, TypeError); | ||
| E('ERR_INVALID_ARG_TYPE', | ||
| (name, expected, actual) => { | ||
| assert(typeof name === 'string', "'name' must be a string"); |
There was a problem hiding this comment.
In general I think it's pretty odd to use assert in internal/errors.js. My impression is that assert in our own code is used to guard against potential user-land monkey-patching gone wrong, but the internal errors are not open to monkey-patching in any way, and most of the internal error code should be simple enough to debug without assertions (even if they are not that simple now, at least they are supposed to be, since internal/errors is required by almost every module). It is especially odd in cases like this where the formatter of ERR_INVALID_ARG_TYPE is effectively guarding against a ERR_INVALID_ARG_TYPE. Even if we want to guard against our own code, we should probably be using DCHECK macros that are only available in debug mode instead of asserts
There was a problem hiding this comment.
I did not add anything new here. Before it was just about using internalAssert instead. In this specific case I do not think it provides a lot of benefit to check this (especially since we do not check input types in any other error). There are a few cases in here that make it much easier to implement the error codes though. I found a few cases that were wrong when validating the passed through arguments and I definitely would like to keep that.
That aside: when I worked on #20567 I also tried to remove assert. As far as I see it is partially used in places where we would now use other errors from in here and partially to prevent malicious values getting through to the C++ layer (you could call it "monkey-patching gone wrong" ;-) ).
Using CHECK and DCHECK for these things would definitely be one way to deal with these. There is just one problem: using e.g., DCHECK means our regular builds while coding will not show any errors and we would only see the issues in the CI. I am not sure if that is fine.
There was a problem hiding this comment.
In general: I guess this discussion is not really about the specific code change?
This adds concrete expected types to the assert documentation. It also fixes a `changes` entry and improves some minor comments. PR-URL: nodejs#20486 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
This moves the `assert` parts from `internal/errors` into an own file. `internal/errors` got bigger and bigger and it was difficult to keep a good overview of what was going on. While doing so it also removes the `internalAssert` function and just lazy loads `assert`. PR-URL: nodejs#20486 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
This makes sure the functions are actually directly beneath the specification of an error code. That way it is not necessary to jump around when looking at the functionality. PR-URL: nodejs#20486 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
|
Landed in cf7be86...5a0e379 |
This adds concrete expected types to the assert documentation. It also fixes a `changes` entry and improves some minor comments. PR-URL: #20486 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
This moves the `assert` parts from `internal/errors` into an own file. `internal/errors` got bigger and bigger and it was difficult to keep a good overview of what was going on. While doing so it also removes the `internalAssert` function and just lazy loads `assert`. PR-URL: #20486 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
This makes sure the functions are actually directly beneath the specification of an error code. That way it is not necessary to jump around when looking at the functionality. PR-URL: #20486 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
This improves the doc a tiny bit by providing the type for the message and some other minor doc fixes.
It also moves the
AssertionErrorfrominternal/errorsinto a new file since the errors begin to become pretty big. Since we changed the way how we create the errors, we also do not have to have the base classes in theerrorsfile anymore.Besides that I moved the functions from some error codes directly to the code, so the code is directly related instead of having to jump around to see what code belongs to what code.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes