process: doc-only deprecate non-string env value

PR-URL: #18990
Refs: #15089
Refs: #18158
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

doc/api/deprecations.md

@@ -915,7 +915,6 @@ Type: Runtime
 
 This was never a documented feature.
 
-
 <a id="DEP0101"></a>
 ### DEP0101: --with-lttng
 
@@ -940,6 +939,17 @@ Type: Documentation-only (supports [`--pending-deprecation`][])
 Using `process.binding()` in general should be avoided. The type checking
 methods in particular can be replaced by using [`util.types`][].
 
+<a id="DEP0104"></a>
+### DEP0104: process.env string coercion
+
+Type: Documentation-only (supports [`--pending-deprecation`][])
+
+Currently when assigning a property to [`process.env`][], the assigned value is
+implicitly converted to a string if it is not a string. This behavior is
+deprecated if the assigned value is not a string, boolean, or number. In the
+future, such assignment may result in a thrown error. Please convert the
+property to a string before assigning it to `process.env`.
+
 [`--pending-deprecation`]: cli.html#cli_pending_deprecation
 [`Buffer.allocUnsafeSlow(size)`]: buffer.html#buffer_class_method_buffer_allocunsafeslow_size
 [`Buffer.from(array)`]: buffer.html#buffer_class_method_buffer_from_array
@@ -976,6 +986,7 @@ methods in particular can be replaced by using [`util.types`][].
 [`fs.stat()`]: fs.html#fs_fs_stat_path_callback
 [`os.networkInterfaces`]: os.html#os_os_networkinterfaces
 [`os.tmpdir()`]: os.html#os_os_tmpdir
+[`process.env`]: process.html#process_process_env
 [`punycode`]: punycode.html
 [`require.extensions`]: modules.html#modules_require_extensions
 [`setInterval()`]: timers.html#timers_setinterval_callback_delay_args

doc/api/process.md

@@ -836,6 +836,10 @@ emitMyWarning();
 ## process.env
 <!-- YAML
 added: v0.1.27
+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/18990
+    description: Implicit conversion of variable value to string is deprecated.
 -->
 
 * {Object}
@@ -877,7 +881,8 @@ console.log(process.env.foo);
 ```
 
 Assigning a property on `process.env` will implicitly convert the value
-to a string.
+to a string. **This behavior is deprecated.** Future versions of Node.js may
+throw an error when the value is not a string, number, or boolean.
 
 Example:
 

src/env-inl.h

@@ -330,6 +330,7 @@ inline Environment::Environment(IsolateData* isolate_data,
       trace_sync_io_(false),
       abort_on_uncaught_exception_(false),
       emit_napi_warning_(true),
+      emit_env_nonstring_warning_(true),
       makecallback_cntr_(0),
       should_abort_on_uncaught_toggle_(isolate_, 1),
 #if HAVE_INSPECTOR

src/env.h

@@ -725,6 +725,11 @@ class Environment {
   void AddPromiseHook(promise_hook_func fn, void* arg);
   bool RemovePromiseHook(promise_hook_func fn, void* arg);
   bool EmitNapiWarning();
+  inline bool EmitProcessEnvWarning() {
+    bool current_value = emit_env_nonstring_warning_;
+    emit_env_nonstring_warning_ = false;
+    return current_value;
+  }
 
   typedef void (*native_immediate_callback)(Environment* env, void* data);
   // cb will be called as cb(env, data) on the next event loop iteration.
@@ -779,6 +784,7 @@ class Environment {
   bool trace_sync_io_;
   bool abort_on_uncaught_exception_;
   bool emit_napi_warning_;
+  bool emit_env_nonstring_warning_;
   size_t makecallback_cntr_;
   std::vector<double> destroy_async_id_list_;
 

src/node.cc

@@ -2659,6 +2659,17 @@ static void EnvGetter(Local<Name> property,
 static void EnvSetter(Local<Name> property,
                       Local<Value> value,
                       const PropertyCallbackInfo<Value>& info) {
+  Environment* env = Environment::GetCurrent(info);
+  if (config_pending_deprecation && env->EmitProcessEnvWarning() &&
+      !value->IsString() && !value->IsNumber() && !value->IsBoolean()) {
+    if (ProcessEmitDeprecationWarning(
+          env,
+          "Assigning any value other than a string, number, or boolean to a "
+          "process.env property is deprecated. Please make sure to convert the "
+          "value to a string before setting process.env with it.",
+          "DEP00XX").IsNothing())
+      return;
+  }
 #ifdef __POSIX__
   node::Utf8Value key(info.GetIsolate(), property);
   node::Utf8Value val(info.GetIsolate(), value);

test/parallel/test-process-env-deprecation.js

@@ -0,0 +1,16 @@
+'use strict';
+const common = require('../common');
+const assert = require('assert');
+
+// Flags: --pending-deprecation
+
+common.expectWarning(
+  'DeprecationWarning',
+  'Assigning any value other than a string, number, or boolean to a ' +
+  'process.env property is deprecated. Please make sure to convert the value ' +
+  'to a string before setting process.env with it.',
+  'DEP00XX'
+);
+
+process.env.ABC = undefined;
+assert.strictEqual(process.env.ABC, 'undefined');