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

src: disallow constructor behaviour for native methods #26700

Closed
wants to merge 2 commits into from

Conversation

addaleax
Copy link
Member

Disallow constructor behaviour and setting up prototypes
for native methods that are not constructors (i.e. make
them behave like ES6 class methods).

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

Disallow constructor behaviour and setting up prototypes
for native methods that are not constructors (i.e. make
them behave like ES6 class methods).
@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Mar 16, 2019
@@ -42,3 +42,11 @@ const err = {
};
common.expectsError(function() { process.chdir({}); }, err);
common.expectsError(function() { process.chdir(); }, err);

// Check that our built-in methods do not have a prototype/constructor behaviour
// if they don't need to. This could be tested for any of our C++ methods.
Copy link
Member

Choose a reason for hiding this comment

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

This could be tested for any of our C++ methods.

Maybe it doesn't matter, but the one picked here means this isn't tested in Worker threads.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I’d say it doesn’t matter – these things are pretty independent, and I’ve chosen something that is likely to stay a directly C++-backed method for a long time. I’m happy to take other suggestions, though.

Copy link
Member

Choose a reason for hiding this comment

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

Seems like this conflicted with #27224 😄

@Hakerh400
Copy link
Contributor

Hakerh400 commented Mar 16, 2019

Nice, but what about @joyeecheung's assertion:

I don't think we can simply replace that wholesale though, that could break a lot of hidden use cases.

That's why I didn't do it in #26096, I felt like it should be firstly investigated if some modules rely on some methods/functions being constructors.

@addaleax
Copy link
Member Author

I don't think we can simply replace that wholesale though, that could break a lot of hidden use cases.

That's why I didn't do it in #26096, I felt like it should be firstly investigated if some modules rely on some methods/functions being constructors.

@Hakerh400 We can run our ecosystem checking tool on this, but generally, this should be safe to do. None of the methods affected by this are likely to be used as constructors, and I’m not concerned that there could be significant ecosystem usage of this. Plus, this won’t be released until Node 12.

@addaleax
Copy link
Member Author

@addaleax
Copy link
Member Author

@devsnek @jdalton Okay, simplified the check to verify only the bare essentials.

@BridgeAR
Copy link
Member

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 17, 2019
@addaleax
Copy link
Member Author

Landed in 1935625

@addaleax addaleax closed this Mar 21, 2019
@addaleax addaleax deleted the kthrow branch March 21, 2019 11:16
addaleax added a commit that referenced this pull request Mar 21, 2019
Disallow constructor behaviour and setting up prototypes
for native methods that are not constructors (i.e. make
them behave like ES6 class methods).

PR-URL: #26700
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++.
Projects
None yet
Development

Successfully merging this pull request may close these issues.