src: start annotating native code side effect by TimothyGu · Pull Request #21458 · nodejs/node

TimothyGu · 2018-06-22T05:46:15Z

Allows eager evaluation to take place for many builtin modules.

Refs: #20977

/cc @nodejs/v8-inspector @benjamingr

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Refs: nodejs#20977

nodejs-github-bot · 2018-06-22T05:46:17Z

@TimothyGu build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/117/pipeline

TimothyGu · 2018-06-22T06:00:45Z

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

bnoordhuis · 2018-06-22T07:45:26Z

src/env-inl.h

+      NewFunctionTemplate(callback,
+                          v8::Local<v8::Signature>(),
+                          // TODO(TimothyGu): Investigate if SetMethod is ever
+                          // used for constructors.


That's easy to check, isn't it? Or do you suspect make test will miss instances?

I'd rather leave that to a separate PR is what I meant.

bnoordhuis · 2018-06-22T08:06:42Z

src/node_crypto.cc

-  env->SetMethod(target, "getHashes", GetHashes);
-  env->SetMethod(target, "getCurves", GetCurves);
+  env->SetMethod(target, "randomBytes", RandomBytes,
+                 SideEffectType::kHasNoSideEffect);


Is there a reason you annotated randomBytes but not pbkdf2 and scrypt? They all use the thread pool.

Hmm, I didn't realize RandomBytes was async. I'll remove this part then.

crypto

bnoordhuis · 2018-06-22T19:13:44Z

src/node_crypto.cc

-                            Signature::New(env->isolate(), t));
+                            Signature::New(env->isolate(), t),
+                            // TODO(TimothyGu): should be deny
+                            ConstructorBehavior::kAllow,


Should be /* length */ 0, ConstructorBehavior::kAllow,. Likewise on line 3968.

Oops, good catch. Fixed.

jasnell · 2018-06-22T20:56:42Z

src/node.cc

-  env->SetMethod(process, "getgid", GetGid);
-  env->SetMethod(process, "getegid", GetEGid);
-  env->SetMethod(process, "getgroups", GetGroups);
+  env->SetMethod(process, "getuid", GetUid,


Just a thought... but a env->SetSafeMethod(process, ...) variant that sets SideEfectType::kHasNoSideEffect automatically may be worthwhile.

TimothyGu · 2018-06-23T02:33:07Z

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

bnoordhuis · 2018-06-23T09:00:03Z

SetSafeMethod() is a bad name. Safe how? f you want something short, use SetMethodNSE() - a little cryptic but on point.

We could then later add SetMethodNSFW() for methods you wouldn't want to be seen calling at the office.

jasnell · 2018-06-23T14:29:17Z

I disagree on Safe being bad here... It's a fairly well known idea that "safe" methods have no side effects and NSE conveys no useful information. I think the code comments are enough to convey what Safe means here.

bnoordhuis · 2018-06-23T16:33:37Z

It's a fairly well known idea that "safe" methods have no side effects

We have functions such as SafeGetenv() and SafeX509ExtPrint() where the Safe means they go the extra mile to stay secure. It doesn't denote anything about side effects.

jasnell · 2018-06-23T16:47:24Z

I'm not even remotely interested in debating it so whatever method name folks think is appropriate is fine with me. Having the utility method is what I wanted so I'm good with this.

addaleax · 2018-06-23T23:49:19Z

and NSE conveys no useful information

@TimothyGu I’d suggest just spelling it out then as NoSideEffect, if that’s alright with you? I agree with Ben that Safe is not a great name, it doesn’t convey much information either.

TimothyGu · 2018-06-24T04:00:01Z

@addaleax Sure, I'll fix that later.

benjamingr · 2018-06-24T07:06:40Z

For what it's worth Pure is typically used to indicate a function has no side effects - at least a lot more often than Safe or NSE. https://www.sitepoint.com/functional-programming-pure-functions/

bnoordhuis · 2018-06-24T10:34:23Z

"Pure" suggests it's safe to collapse multiple calls because the return value depends only on the inputs. That clearly isn't true for functions such as process.uptime().

Compare gcc's __attribute__((pure)).

benjamingr

Still looks good after rename

TimothyGu · 2018-06-26T01:57:16Z

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

TimothyGu · 2018-06-26T03:16:22Z

Landed in 65f6173.

PR-URL: #21458 Refs: #20977 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

targos · 2018-06-26T06:43:00Z

Depends on #21105 to land on v10.x

PR-URL: #21458 Refs: #20977 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

src: start annotating native code side effect

7132853

Refs: nodejs#20977

TimothyGu added c++ Issues and PRs that require attention from people who are familiar with C++. v8 engine Issues and PRs related to the V8 dependency. inspector Issues and PRs related to the V8 inspector protocol labels Jun 22, 2018

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Jun 22, 2018

addaleax approved these changes Jun 22, 2018

View reviewed changes

devsnek approved these changes Jun 22, 2018

View reviewed changes

benjamingr approved these changes Jun 22, 2018

View reviewed changes

bnoordhuis approved these changes Jun 22, 2018

View reviewed changes

cjihrig approved these changes Jun 22, 2018

View reviewed changes

fixup! src: start annotating native code side effect

c5f4cb5

crypto

joyeecheung approved these changes Jun 22, 2018

View reviewed changes

bnoordhuis reviewed Jun 22, 2018

View reviewed changes

jasnell reviewed Jun 22, 2018

View reviewed changes

TimothyGu added 2 commits June 22, 2018 21:43

fixup! fixup! src: start annotating native code side effect

d418bb8

fixup! src: start annotating native code side effect

a129a6f

jasnell approved these changes Jun 23, 2018

View reviewed changes

fixup! fixup! src: start annotating native code side effect

c43658d

benjamingr approved these changes Jun 24, 2018

View reviewed changes

bnoordhuis approved these changes Jun 24, 2018

View reviewed changes

jasnell approved these changes Jun 24, 2018

View reviewed changes

TimothyGu closed this Jun 26, 2018

TimothyGu deleted the side-effects branch June 26, 2018 03:16

targos added the backport-requested-v10.x label Jun 26, 2018

targos mentioned this pull request Jul 14, 2018

src: add HandleWrap::AddWrapMethods #21769

Closed

2 tasks

targos removed the backport-requested-v10.x label Jul 14, 2018

targos mentioned this pull request Jul 17, 2018

v10.7.0 proposal #21851

Merged

Uh oh!

Conversation

TimothyGu commented Jun 22, 2018

Checklist

Uh oh!

nodejs-github-bot commented Jun 22, 2018

Uh oh!

TimothyGu commented Jun 22, 2018

Uh oh!

bnoordhuis Jun 22, 2018

Choose a reason for hiding this comment

Uh oh!

TimothyGu Jun 22, 2018

Choose a reason for hiding this comment

Uh oh!

bnoordhuis Jun 22, 2018

Choose a reason for hiding this comment

Uh oh!

TimothyGu Jun 22, 2018

Choose a reason for hiding this comment

Uh oh!

bnoordhuis Jun 22, 2018

Choose a reason for hiding this comment

Uh oh!

TimothyGu Jun 22, 2018

Choose a reason for hiding this comment

Uh oh!

jasnell Jun 22, 2018

Choose a reason for hiding this comment

Uh oh!

TimothyGu commented Jun 23, 2018

Uh oh!

bnoordhuis commented Jun 23, 2018

Uh oh!

jasnell commented Jun 23, 2018

Uh oh!

bnoordhuis commented Jun 23, 2018

Uh oh!

jasnell commented Jun 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax commented Jun 23, 2018

Uh oh!

TimothyGu commented Jun 24, 2018

Uh oh!

benjamingr commented Jun 24, 2018

Uh oh!

bnoordhuis commented Jun 24, 2018

Uh oh!

benjamingr left a comment

Choose a reason for hiding this comment

Uh oh!

TimothyGu commented Jun 26, 2018

Uh oh!

TimothyGu commented Jun 26, 2018

Uh oh!

targos commented Jun 26, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

jasnell commented Jun 23, 2018 •

edited

Loading