process: mark process.env as side-effect-free

Read-only access to `process.env` does not have side effects. Refs: #27523 PR-URL: #27684 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Aleksei Koziatinskii <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Yongsheng Zhang <[email protected]>
nodejs · May 18, 2019 · abe8211 · abe8211
1 parent 2a24aa2
commit abe8211
Show file tree

Hide file tree

Showing 3 changed files with 29 additions and 3 deletions.
diff --git a/src/node_env_var.cc b/src/node_env_var.cc
@@ -28,6 +28,7 @@ using v8::Nothing;
 using v8::Object;
 using v8::ObjectTemplate;
 using v8::PropertyCallbackInfo;
+using v8::PropertyHandlerFlags;
 using v8::String;
 using v8::Value;
 
@@ -377,7 +378,8 @@ MaybeLocal<Object> CreateEnvVarProxy(Local<Context> context,
   EscapableHandleScope scope(isolate);
   Local<ObjectTemplate> env_proxy_template = ObjectTemplate::New(isolate);
   env_proxy_template->SetHandler(NamedPropertyHandlerConfiguration(
-      EnvGetter, EnvSetter, EnvQuery, EnvDeleter, EnvEnumerator, data));
+      EnvGetter, EnvSetter, EnvQuery, EnvDeleter, EnvEnumerator, data,
+      PropertyHandlerFlags::kHasNoSideEffect));
   return scope.EscapeMaybe(env_proxy_template->NewInstance(context));
 }
 }  // namespace node
diff --git a/test/parallel/test-inspector-vm-global-accessors-sideeffects.js b/test/parallel/test-inspector-vm-global-accessors-sideeffects.js
@@ -19,7 +19,7 @@ session.post('Runtime.evaluate', {
   expression: 'a',
   throwOnSideEffect: true,
   contextId: 2 // context's id
-}, (error, res) => {
+}, common.mustCall((error, res) => {
   assert.ifError(error),
   assert.deepStrictEqual(res, {
     result: {
@@ -28,4 +28,4 @@ session.post('Runtime.evaluate', {
       description: '100'
     }
   });
-});
+}));
diff --git a/test/parallel/test-process-env-sideeffects.js b/test/parallel/test-process-env-sideeffects.js
@@ -0,0 +1,24 @@
+'use strict';
+const common = require('../common');
+common.skipIfInspectorDisabled();
+
+// Test that read-only process.env access is considered to have no
+// side-effects by the inspector.
+
+const assert = require('assert');
+const inspector = require('inspector');
+
+const session = new inspector.Session();
+session.connect();
+
+process.env.TESTVAR = 'foobar';
+
+session.post('Runtime.evaluate', {
+  expression: 'process.env.TESTVAR',
+  throwOnSideEffect: true
+}, (error, res) => {
+  assert.ifError(error);
+  assert.deepStrictEqual(res, {
+    result: { type: 'string', value: 'foobar' }
+  });
+});