From 7dfdcb7c73c9b0f09111f3ac8d8a942c80e57c78 Mon Sep 17 00:00:00 2001 From: dave caruso Date: Tue, 30 Jan 2024 12:57:16 -0800 Subject: [PATCH 1/6] yay!!!!!! --- CMakeLists.txt | 2 +- src/bun.js/ConsoleObject.zig | 56 ++++++++++--------- .../bindings/JSEnvironmentVariableMap.cpp | 47 +++++++++++++++- src/js/builtins/ProcessObjectInternals.ts | 44 +++++++++++++++ test/js/node/env-windows.test.ts | 24 ++++++++ 5 files changed, 142 insertions(+), 31 deletions(-) create mode 100644 test/js/node/env-windows.test.ts diff --git a/CMakeLists.txt b/CMakeLists.txt index 147cec1fcf7f8..7b3fb3d6dce68 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -2,7 +2,7 @@ cmake_minimum_required(VERSION 3.22) cmake_policy(SET CMP0091 NEW) cmake_policy(SET CMP0067 NEW) -set(Bun_VERSION "1.0.26") +set(Bun_VERSION "1.1.0") set(WEBKIT_TAG 9c501b9aa712b7959f80dc99491e8758c151c20e) set(BUN_WORKDIR "${CMAKE_CURRENT_BINARY_DIR}") diff --git a/src/bun.js/ConsoleObject.zig b/src/bun.js/ConsoleObject.zig index a332cd3bc506b..840b05d350040 100644 --- a/src/bun.js/ConsoleObject.zig +++ b/src/bun.js/ConsoleObject.zig @@ -1107,22 +1107,24 @@ pub const Formatter = struct { return .{ .tag = switch (js_type) { - JSValue.JSType.ErrorInstance => .Error, - JSValue.JSType.NumberObject => .Double, - JSValue.JSType.DerivedArray, JSValue.JSType.Array => .Array, - JSValue.JSType.DerivedStringObject, JSValue.JSType.String, JSValue.JSType.StringObject => .String, - JSValue.JSType.RegExpObject => .String, - JSValue.JSType.Symbol => .Symbol, - JSValue.JSType.BooleanObject => .Boolean, - JSValue.JSType.JSFunction => .Function, - JSValue.JSType.JSWeakMap, JSValue.JSType.JSMap => .Map, - JSValue.JSType.JSMapIterator => .MapIterator, - JSValue.JSType.JSSetIterator => .SetIterator, - JSValue.JSType.JSWeakSet, JSValue.JSType.JSSet => .Set, - JSValue.JSType.JSDate => .JSON, - JSValue.JSType.JSPromise => .Promise, - JSValue.JSType.Object, - JSValue.JSType.FinalObject, + .ErrorInstance => .Error, + .NumberObject => .Double, + .DerivedArray, JSValue.JSType.Array => .Array, + .DerivedStringObject, JSValue.JSType.String, JSValue.JSType.StringObject => .String, + .RegExpObject => .String, + .Symbol => .Symbol, + .BooleanObject => .Boolean, + .JSFunction => .Function, + .JSWeakMap, JSValue.JSType.JSMap => .Map, + .JSMapIterator => .MapIterator, + .JSSetIterator => .SetIterator, + .JSWeakSet, JSValue.JSType.JSSet => .Set, + .JSDate => .JSON, + .JSPromise => .Promise, + + .Object, + .FinalObject, + .ProxyObject, .ModuleNamespaceObject, => .Object, @@ -1132,17 +1134,17 @@ pub const Formatter = struct { .GlobalObject, .ArrayBuffer, - JSValue.JSType.Int8Array, - JSValue.JSType.Uint8Array, - JSValue.JSType.Uint8ClampedArray, - JSValue.JSType.Int16Array, - JSValue.JSType.Uint16Array, - JSValue.JSType.Int32Array, - JSValue.JSType.Uint32Array, - JSValue.JSType.Float32Array, - JSValue.JSType.Float64Array, - JSValue.JSType.BigInt64Array, - JSValue.JSType.BigUint64Array, + .Int8Array, + .Uint8Array, + .Uint8ClampedArray, + .Int16Array, + .Uint16Array, + .Int32Array, + .Uint32Array, + .Float32Array, + .Float64Array, + .BigInt64Array, + .BigUint64Array, .DataView, => .TypedArray, diff --git a/src/bun.js/bindings/JSEnvironmentVariableMap.cpp b/src/bun.js/bindings/JSEnvironmentVariableMap.cpp index b23eac87d0301..effea288f6dd0 100644 --- a/src/bun.js/bindings/JSEnvironmentVariableMap.cpp +++ b/src/bun.js/bindings/JSEnvironmentVariableMap.cpp @@ -5,7 +5,13 @@ #include #include +#include +#include +#include +#include + #include "BunClientData.h" + using namespace JSC; extern "C" size_t Bun__getEnvCount(JSGlobalObject* globalObject, void** list_ptr); @@ -48,7 +54,11 @@ JSC_DEFINE_CUSTOM_SETTER(jsSetterEnvironmentVariable, (JSGlobalObject * globalOb if (!object) return false; - object->putDirect(vm, propertyName, JSValue::decode(value), 0); + auto string = JSValue::decode(value).toString(globalObject); + if (UNLIKELY(string)) + return false; + + object->putDirect(vm, propertyName, string, 0); return true; } @@ -125,19 +135,30 @@ JSValue createEnvironmentVariablesMap(Zig::GlobalObject* globalObject) object = constructEmptyObject(globalObject, globalObject->objectPrototype()); } +#if OS(WINDOWS) + JSArray* keyArray = constructEmptyArray(globalObject, nullptr, count); +#endif + static NeverDestroyed TZ = MAKE_STATIC_STRING_IMPL("TZ"); bool hasTZ = false; for (size_t i = 0; i < count; i++) { unsigned char* chars; size_t len = Bun__getEnvKey(list, i, &chars); auto name = String::fromUTF8(chars, len); +#if OS(WINDOWS) + keyArray->putByIndexInline(globalObject, (unsigned)i, jsString(vm, name), false); +#endif if (name == TZ) { hasTZ = true; continue; } ASSERT(len > 0); - - Identifier identifier = Identifier::fromString(vm, name); +#if OS(WINDOWS) + String idName = name.convertToASCIIUppercase(); +#else + String idName = name; +#endif + Identifier identifier = Identifier::fromString(vm, idName); // CustomGetterSetter doesn't support indexed properties yet. // This causes strange issues when the environment variable name is an integer. @@ -165,6 +186,26 @@ JSValue createEnvironmentVariablesMap(Zig::GlobalObject* globalObject) vm, Identifier::fromString(vm, TZ), JSC::CustomGetterSetter::create(vm, jsTimeZoneEnvironmentVariableGetter, jsTimeZoneEnvironmentVariableSetter), TZAttrs); +#if OS(WINDOWS) + JSC::JSFunction* getSourceEvent = JSC::JSFunction::create(vm, processObjectInternalsWindowsEnvCodeGenerator(vm), globalObject); + RETURN_IF_EXCEPTION(scope, {}); + JSC::MarkedArgumentBuffer args; + args.append(object); + args.append(keyArray); + auto clientData = WebCore::clientData(vm); + JSC::CallData callData = JSC::getCallData(getSourceEvent); + NakedPtr returnedException = nullptr; + auto result = JSC::call(globalObject, getSourceEvent, callData, globalObject->globalThis(), args, returnedException); + RETURN_IF_EXCEPTION(scope, {}); + + if (returnedException) { + throwException(globalObject, scope, returnedException.get()); + return jsUndefined(); + } + + RELEASE_AND_RETURN(scope, result); +#else return object; +#endif } } diff --git a/src/js/builtins/ProcessObjectInternals.ts b/src/js/builtins/ProcessObjectInternals.ts index 48495efdeb2d5..9c870bd73e4c8 100644 --- a/src/js/builtins/ProcessObjectInternals.ts +++ b/src/js/builtins/ProcessObjectInternals.ts @@ -344,3 +344,47 @@ export function setMainModule(value) { $putByIdDirectPrivate(this, "main", value); return true; } + +type InternalEnvMap = Record; + +export function windowsEnv(internalEnv: InternalEnvMap, envMapList: Array) { + // The use of String(key) here is intentional because Node.js as of v21.5.0 will throw + // on symbol keys as it seems they assume the user uses string keys: + // + // it throws "Cannot convert a Symbol value to a string" + + return new Proxy(internalEnv, { + get(_, p) { + return typeof p === "string" ? Reflect.get(internalEnv, p.toUpperCase()) : undefined; + }, + set(_, p, value) { + var k = String(p).toUpperCase(); + $assert(typeof p === "string"); // proxy is only string and symbol. the symbol would have thrown by now + if (!Reflect.has(internalEnv, k)) { + envMapList.push(p); + } + return Reflect.set(internalEnv, k, String(value)); + }, + has(_, p) { + return typeof p === "string" ? Reflect.has(internalEnv, p.toUpperCase()) : false; + }, + deleteProperty(_, p) { + return typeof p === "string" ? Reflect.deleteProperty(internalEnv, p.toUpperCase()) : true; + }, + defineProperty(_, p, attributes) { + var k = String(p).toUpperCase(); + $assert(typeof p === "string"); // proxy is only string and symbol. the symbol would have thrown by now + if (!Reflect.has(internalEnv, k)) { + envMapList.push(p); + } + return Reflect.defineProperty(internalEnv, k, attributes); + }, + getOwnPropertyDescriptor(target, p) { + return typeof p === "string" ? Reflect.getOwnPropertyDescriptor(target, p.toUpperCase()) : undefined; + }, + ownKeys() { + // .slice() because paranoia that there is a way to call this without the engine cloning it for us + return envMapList.slice(); + }, + }) +} diff --git a/test/js/node/env-windows.test.ts b/test/js/node/env-windows.test.ts new file mode 100644 index 0000000000000..fe2fe82d70234 --- /dev/null +++ b/test/js/node/env-windows.test.ts @@ -0,0 +1,24 @@ +import { test,expect} from 'bun:test'; + +test.if(process.platform === 'win32')('process.env is case insensitive on windows', () => { + const keys = Object.keys(process.env); + // this should have at least one character that is lowercase + // it is likely that PATH will be 'Path', and also stuff like 'WindowsLibPath' and so on. + // but not guaranteed, so we just check that there is at least one of each case + expect(keys.join('').split('').some(c => c.toUpperCase() !== c)).toBe(true); + expect(keys.join('').split('').some(c => c.toLowerCase() !== c)).toBe(true); + expect(process.env.path).toBe(process.env.PATH!); + expect(process.env.pAtH).toBe(process.env.PATH!); + + expect(process.env.doesntexistahahahahaha).toBeUndefined(); + // @ts-expect-error + process.env.doesntExistAHaHaHaHaHa = true; + expect(process.env.doesntexistahahahahaha).toBe('true'); + expect(process.env.doesntexistahahahahaha).toBe('true'); + expect(process.env.doesnteXISTahahahahaha).toBe('true'); + expect(Object.keys(process.env).pop()).toBe('doesntExistAHaHaHaHaHa'); + delete process.env.DOESNTEXISTAHAHAHAHAHA; + expect(process.env.doesntexistahahahahaha).toBeUndefined(); + expect(Object.keys(process.env)).not.toInclude('doesntExistAHaHaHaHaHa'); + +}); From 393da71ab121d22f2b579468fadf9c87a843a285 Mon Sep 17 00:00:00 2001 From: "autofix-ci[bot]" <114827586+autofix-ci[bot]@users.noreply.github.com> Date: Tue, 30 Jan 2024 21:01:13 +0000 Subject: [PATCH 2/6] [autofix.ci] apply automated fixes --- src/js/builtins/ProcessObjectInternals.ts | 4 +- test/js/node/env-windows.test.ts | 51 +++++++++++++---------- 2 files changed, 32 insertions(+), 23 deletions(-) diff --git a/src/js/builtins/ProcessObjectInternals.ts b/src/js/builtins/ProcessObjectInternals.ts index 9c870bd73e4c8..2bff5b66e5be2 100644 --- a/src/js/builtins/ProcessObjectInternals.ts +++ b/src/js/builtins/ProcessObjectInternals.ts @@ -356,7 +356,7 @@ export function windowsEnv(internalEnv: InternalEnvMap, envMapList: Array { - const keys = Object.keys(process.env); - // this should have at least one character that is lowercase - // it is likely that PATH will be 'Path', and also stuff like 'WindowsLibPath' and so on. - // but not guaranteed, so we just check that there is at least one of each case - expect(keys.join('').split('').some(c => c.toUpperCase() !== c)).toBe(true); - expect(keys.join('').split('').some(c => c.toLowerCase() !== c)).toBe(true); - expect(process.env.path).toBe(process.env.PATH!); - expect(process.env.pAtH).toBe(process.env.PATH!); - - expect(process.env.doesntexistahahahahaha).toBeUndefined(); - // @ts-expect-error - process.env.doesntExistAHaHaHaHaHa = true; - expect(process.env.doesntexistahahahahaha).toBe('true'); - expect(process.env.doesntexistahahahahaha).toBe('true'); - expect(process.env.doesnteXISTahahahahaha).toBe('true'); - expect(Object.keys(process.env).pop()).toBe('doesntExistAHaHaHaHaHa'); - delete process.env.DOESNTEXISTAHAHAHAHAHA; - expect(process.env.doesntexistahahahahaha).toBeUndefined(); - expect(Object.keys(process.env)).not.toInclude('doesntExistAHaHaHaHaHa'); +test.if(process.platform === "win32")("process.env is case insensitive on windows", () => { + const keys = Object.keys(process.env); + // this should have at least one character that is lowercase + // it is likely that PATH will be 'Path', and also stuff like 'WindowsLibPath' and so on. + // but not guaranteed, so we just check that there is at least one of each case + expect( + keys + .join("") + .split("") + .some(c => c.toUpperCase() !== c), + ).toBe(true); + expect( + keys + .join("") + .split("") + .some(c => c.toLowerCase() !== c), + ).toBe(true); + expect(process.env.path).toBe(process.env.PATH!); + expect(process.env.pAtH).toBe(process.env.PATH!); + expect(process.env.doesntexistahahahahaha).toBeUndefined(); + // @ts-expect-error + process.env.doesntExistAHaHaHaHaHa = true; + expect(process.env.doesntexistahahahahaha).toBe("true"); + expect(process.env.doesntexistahahahahaha).toBe("true"); + expect(process.env.doesnteXISTahahahahaha).toBe("true"); + expect(Object.keys(process.env).pop()).toBe("doesntExistAHaHaHaHaHa"); + delete process.env.DOESNTEXISTAHAHAHAHAHA; + expect(process.env.doesntexistahahahahaha).toBeUndefined(); + expect(Object.keys(process.env)).not.toInclude("doesntExistAHaHaHaHaHa"); }); From 6f9e0f5438a4e314e0094870f953e2c7c788fea3 Mon Sep 17 00:00:00 2001 From: dave caruso Date: Tue, 30 Jan 2024 13:16:45 -0800 Subject: [PATCH 3/6] ok --- test/cli/run/env.test.ts | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/test/cli/run/env.test.ts b/test/cli/run/env.test.ts index 645b41cf672a5..25ac6f480505b 100644 --- a/test/cli/run/env.test.ts +++ b/test/cli/run/env.test.ts @@ -706,3 +706,21 @@ test("NODE_ENV default is not propogated in bun run", () => { }); expect(bunRunAsScript(tmp, "show-env", {}).stdout).toBe(""); }); + + +const todoOnPosix = process.platform !== "win32" ? test.todo : test; +todoOnPosix('setting process.env coerces the value to a string', () => { + // @ts-expect-error + process.env.SET_TO_TRUE = true; + let did_call = 0; + // @ts-expect-error + process.env.SET_TO_BUN = { + toString() { + did_call++; + return 'bun!'; + } + }; + expect(process.env.SET_TO_TRUE).toBe('true'); + expect(process.env.SET_TO_BUN).toBe('bun!'); + expect(did_call).toBe(1); +}); From e4d3a295e2a236cc1c6707d55b8765aabad309db Mon Sep 17 00:00:00 2001 From: dave caruso Date: Tue, 30 Jan 2024 13:53:28 -0800 Subject: [PATCH 4/6] do not use reflect here --- src/bun.js/bindings/JSEnvironmentVariableMap.cpp | 2 +- src/js/builtins/ProcessObjectInternals.ts | 3 ++- src/string_immutable.zig | 3 ++- 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/bun.js/bindings/JSEnvironmentVariableMap.cpp b/src/bun.js/bindings/JSEnvironmentVariableMap.cpp index effea288f6dd0..d0514ae781935 100644 --- a/src/bun.js/bindings/JSEnvironmentVariableMap.cpp +++ b/src/bun.js/bindings/JSEnvironmentVariableMap.cpp @@ -55,7 +55,7 @@ JSC_DEFINE_CUSTOM_SETTER(jsSetterEnvironmentVariable, (JSGlobalObject * globalOb return false; auto string = JSValue::decode(value).toString(globalObject); - if (UNLIKELY(string)) + if (UNLIKELY(!string)) return false; object->putDirect(vm, propertyName, string, 0); diff --git a/src/js/builtins/ProcessObjectInternals.ts b/src/js/builtins/ProcessObjectInternals.ts index 2bff5b66e5be2..453301bd1f276 100644 --- a/src/js/builtins/ProcessObjectInternals.ts +++ b/src/js/builtins/ProcessObjectInternals.ts @@ -363,7 +363,8 @@ export function windowsEnv(internalEnv: InternalEnvMap, envMapList: Array return buf[0..result.count], From 2f02eadd3867039bbb99c94433254c0d80fd2ce7 Mon Sep 17 00:00:00 2001 From: dave caruso Date: Tue, 30 Jan 2024 14:22:43 -0800 Subject: [PATCH 5/6] ok --- src/js/builtins/ProcessObjectInternals.ts | 3 +-- test/cli/run/env.test.ts | 5 ++++- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/js/builtins/ProcessObjectInternals.ts b/src/js/builtins/ProcessObjectInternals.ts index 453301bd1f276..2bff5b66e5be2 100644 --- a/src/js/builtins/ProcessObjectInternals.ts +++ b/src/js/builtins/ProcessObjectInternals.ts @@ -363,8 +363,7 @@ export function windowsEnv(internalEnv: InternalEnvMap, envMapList: Array { + const getenv = process.platform !== 'win32' + ? 'env | grep NODE_ENV && exit 1 || true' + : "node -e if(process.env.NODE_ENV)throw(1)"; const tmp = tempDirWithFiles("default-node-env", { - "package.json": '{"scripts":{"show-env":"env | grep NODE_ENV && exit 1 || true"}}', + "package.json": '{"scripts":{"show-env":' + JSON.stringify(getenv) + '}}', }); expect(bunRunAsScript(tmp, "show-env", {}).stdout).toBe(""); }); From ef5174777da682f8790031931f44b447a4332019 Mon Sep 17 00:00:00 2001 From: "autofix-ci[bot]" <114827586+autofix-ci[bot]@users.noreply.github.com> Date: Tue, 30 Jan 2024 22:35:14 +0000 Subject: [PATCH 6/6] [autofix.ci] apply automated fixes --- test/cli/run/env.test.ts | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/test/cli/run/env.test.ts b/test/cli/run/env.test.ts index fb61822ed7dab..6fcacd6ab9017 100644 --- a/test/cli/run/env.test.ts +++ b/test/cli/run/env.test.ts @@ -701,18 +701,16 @@ console.log(dynamic().NODE_ENV); }); test("NODE_ENV default is not propogated in bun run", () => { - const getenv = process.platform !== 'win32' - ? 'env | grep NODE_ENV && exit 1 || true' - : "node -e if(process.env.NODE_ENV)throw(1)"; + const getenv = + process.platform !== "win32" ? "env | grep NODE_ENV && exit 1 || true" : "node -e if(process.env.NODE_ENV)throw(1)"; const tmp = tempDirWithFiles("default-node-env", { - "package.json": '{"scripts":{"show-env":' + JSON.stringify(getenv) + '}}', + "package.json": '{"scripts":{"show-env":' + JSON.stringify(getenv) + "}}", }); expect(bunRunAsScript(tmp, "show-env", {}).stdout).toBe(""); }); - const todoOnPosix = process.platform !== "win32" ? test.todo : test; -todoOnPosix('setting process.env coerces the value to a string', () => { +todoOnPosix("setting process.env coerces the value to a string", () => { // @ts-expect-error process.env.SET_TO_TRUE = true; let did_call = 0; @@ -720,10 +718,10 @@ todoOnPosix('setting process.env coerces the value to a string', () => { process.env.SET_TO_BUN = { toString() { did_call++; - return 'bun!'; - } + return "bun!"; + }, }; - expect(process.env.SET_TO_TRUE).toBe('true'); - expect(process.env.SET_TO_BUN).toBe('bun!'); + expect(process.env.SET_TO_TRUE).toBe("true"); + expect(process.env.SET_TO_BUN).toBe("bun!"); expect(did_call).toBe(1); });