Fix some bugs that cause minor annoyances…

.github/workflows/ci.yml

@@ -1,7 +1,12 @@
 name: ci
 on:
-  - pull_request
-  - push
+  push:
+    branches: [master]
+  pull_request:
+
+concurrency:
+  group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }}
+  cancel-in-progress: true
 
 jobs:
   Test:
@@ -13,26 +18,26 @@ jobs:
         os:
           - ubuntu-latest
           - macos-latest
-          - windows-latest
+          - windows-2022
         node_version:
-          - 16
           - 18
           - 20
+          - 22
     name: Node ${{ matrix.node_version }} on ${{ matrix.os }}
 
     steps:
-      - uses: actions/checkout@v4
+      - uses: actions/checkout@v5
         with:
           submodules: true
       - name: Cache
-        uses: actions/cache@v3
+        uses: actions/cache@v4
         with:
           path: |
             node_modules
           key: ${{ runner.os }}-${{ matrix.node_version }}-${{ hashFiles('package.json') }}
 
       - name: Setup node
-        uses: actions/setup-node@v4
+        uses: actions/setup-node@v5
         with:
           node-version: ${{ matrix.node_version }}
 
@@ -67,10 +72,16 @@ jobs:
       - name: Lint
         run: npm run standard
 
-      - name: Run tests
-        run: |
-          npm run test:node
-          npm run test:native
+      # These are deliberately separate steps. On Windows the default shell is
+      # `pwsh`, where a multi-line `run:` block neither halts on a failed native
+      # command nor reports anything but the last command's exit code — so a
+      # crash in `test:node` was silently discarded by `test:native` running
+      # after it.
+      - name: Run JS tests
+        run: npm run test:node
+
+      - name: Run native tests
+        run: npm run test:native
 
   Skip:
     if: contains(github.event.head_commit.message, '[skip ci]')

.github/workflows/publish.yml

@@ -6,7 +6,7 @@ on:
   workflow_dispatch:
 
 env:
-  NODE_VERSION: 16
+  NODE_VERSION: 20
   NODE_AUTH_TOKEN: ${{ secrets.NPM_PUBLISH_TOKEN }}
 
 jobs:

.npmignore

@@ -9,6 +9,7 @@
 !src/bindings/*.cc
 
 !script/fetch-libiconv-61.sh
+!script/copy-libiconv.sh
 
 !vendor/libcxx/*
 

binding.gyp

@@ -28,27 +28,24 @@
             "conditions": [
                 ['OS=="mac"', {
                     "postbuilds": [
+                        {
+                            'postbuild_name': 'Copy vendored libiconv next to the binding',
+                            'action': [
+                                'bash',
+                                '<(module_root_dir)/script/copy-libiconv.sh',
+                                '<(module_root_dir)/ext/lib/libiconv.2.dylib',
+                                '<(PRODUCT_DIR)/libiconv.2.dylib'
+                            ]
+                        },
                         {
                             'postbuild_name': 'Adjust vendored libiconv install name',
                             'action': [
                                 'install_name_tool',
                                 "-change",
                                 "libiconv.2.dylib",
-                                "@loader_path/../../ext/lib/libiconv.2.dylib",
+                                "@loader_path/libiconv.2.dylib",
                                 "<(PRODUCT_DIR)/superstring.node"
                             ]
-
-                            # NOTE: This version of the post-build action
-                            # should be used if we find it necessary to avoid
-                            # changing the `dylib`’s install name in an earlier
-                            # step.
-                            #
-                            # 'action': [
-                            #     'bash',
-                            #     '<(module_root_dir)/script/adjust-install-name.sh',
-                            #     '<(PRODUCT_DIR)'
-                            # ]
-
                         }
                     ]
                 }]
@@ -120,30 +117,14 @@
                             "action_name": "Run script",
                             "message": "Building GNU libiconv...",
                             "inputs": [],
-                            "outputs": ["ext"],
+                            "outputs": ["<(module_root_dir)/ext/lib/libiconv.2.dylib"],
                             "action": [
                                 "bash",
                                 "script/fetch-libiconv-61.sh"
                             ]
                         }
                     ]
                 }
-                # {
-                #     "target_name": "find_libiconv",
-                #     "target_type": "none",
-                #     "actions": [
-                #         {
-                #             "action_name": "Run script",
-                #             "message": "Locating GNU libiconv...",
-                #             "inputs": [],
-                #             "outputs": ["vendor/libiconv/lib/libiconv.2.dylib"],
-                #             "action": [
-                #                 "bash",
-                #                 "script/find-gnu-libiconv.sh"
-                #             ]
-                #         }
-                #     ]
-                # }
             ]
         }],
 
@@ -194,26 +175,24 @@
                             'MACOSX_DEPLOYMENT_TARGET': '10.12',
                         },
                         "postbuilds": [
+                            {
+                                'postbuild_name': 'Copy vendored libiconv next to the binding',
+                                'action': [
+                                    'bash',
+                                    '<(module_root_dir)/script/copy-libiconv.sh',
+                                    '<(module_root_dir)/ext/lib/libiconv.2.dylib',
+                                    '<(PRODUCT_DIR)/libiconv.2.dylib'
+                                ]
+                            },
                             {
                                 'postbuild_name': 'Adjust vendored libiconv install name',
                                 'action': [
-                                  'install_name_tool',
-                                  "-change",
-                                  "libiconv.2.dylib",
-                                  "@executable_path/../../ext/lib/libiconv.2.dylib",
-                                  "<(PRODUCT_DIR)/tests"
+                                    'install_name_tool',
+                                    "-change",
+                                    "libiconv.2.dylib",
+                                    "@executable_path/libiconv.2.dylib",
+                                    "<(PRODUCT_DIR)/tests"
                                 ]
-
-                                # NOTE: This version of the post-build action
-                                # should be used if we find it necessary to avoid
-                                # changing the `dylib`’s install name in an earlier
-                                # step.
-                                #
-                                # 'action': [
-                                #     'bash',
-                                #     '<(module_root_dir)/script/adjust-install-name.sh',
-                                #     '<(PRODUCT_DIR)'
-                                # ]
                             }
                         ]
                     }]
@@ -242,6 +221,15 @@
                 "defines": [
                     "NOMINMAX"
                 ],
+                # These sources are UTF-8. Absent this flag — and absent a BOM,
+                # which none of them have — MSVC decodes them using the system
+                # ANSI codepage, which mangles non-ASCII characters in string
+                # literals without any diagnostic.
+                "msvs_settings": {
+                    "VCCLCompilerTool": {
+                        "AdditionalOptions": ["/utf-8"]
+                    }
+                },
             }]
         ]
     }

package.json

@@ -25,7 +25,7 @@
     "data-structure"
   ],
   "engines": {
-    "node": ">=16"
+    "node": ">=18"
   },
   "author": "Nathan Sobo <nathan@github.com>",
   "license": "MIT",

script/copy-libiconv.sh

@@ -0,0 +1,24 @@
+#!/bin/bash
+set -euo pipefail
+
+# Copy the vendored libiconv next to the built product. Resolves the symlink
+# (`ext/lib/libiconv.2.dylib` points at the versioned dylib) and installs the
+# copy atomically, so two targets postbuilding in parallel can't observe a
+# half-written file.
+
+src="$1"
+dest="$2"
+
+tmp="$(mktemp "${dest}.XXXXXX")"
+trap 'rm -f "$tmp"' EXIT
+
+# `-L` because `ext/lib/libiconv.2.dylib` is a symlink to the versioned dylib.
+# We want the real file here — a link would still point outside of `build/`,
+# which is what we're getting away from.
+cp -L "$src" "$tmp"
+
+# We _must_ get the permissions right here; a `.dylib` with 0600 would work for
+# the user who built it but fail for anyone else.
+chmod 755 "$tmp"
+
+mv -f "$tmp" "$dest"

script/fetch-libiconv-61.sh

@@ -1,4 +1,5 @@
 #!/bin/bash
+set -euo pipefail
 
 # When compiling `superstring` on macOS, we used to be able to rely on the
 # builtin version of `libiconv`. But newer versions of macOS include FreeBSD
@@ -12,16 +13,16 @@
 # `libiconv.2.dylib`. For now, letting the user compile their own `libiconv`
 # has the advantage of very likely matching the system's architecture.
 
-echoerr() { echo "$@\n" >&2; }
+echoerr() { printf '%s\n\n' "$*" >&2; }
 
 create-if-missing() {
-  if [ -z "$1" ]; then
+  if [ -f "$1" ]; then
     echoerr "Error: $1 is a file."
     usage
     exit 1
   fi
   if [ ! -d "$1" ]; then
-    mkdir "$1"
+    mkdir -p "$1"
   fi
 }
 
@@ -50,9 +51,9 @@ dylib_path="$EXT/lib/libiconv.2.dylib"
 
 # If this path already exists, we'll assume libiconv has already been fetched
 # and compiled. Otherwise we'll do it now.
-if [ ! -L "$dylib_path" ]; then
+if [ ! -e "$dylib_path" ]; then
   echo "Path $dylib_path is missing; fetching and installing libiconv."
-  cd $SCRATCH
+  cd "$SCRATCH"
   # TODO: Instead of downloading this each time, we can check this into source
   # control via git subtree. That would allow someone to build this without
   # needing internet connectivity. But we'd still need to do a `make install` —
@@ -64,7 +65,7 @@ if [ ! -L "$dylib_path" ]; then
   make
   make install
 
-  if [ ! -L "$dylib_path" ]; then
+  if [ ! -e "$dylib_path" ]; then
     echoerr "Error: expected $dylib_path to be present, but it was not. Installation of libiconv failed. Cannot proceed."
     usage
     exit 1
@@ -81,10 +82,9 @@ else
   echo "Path $dylib_path is already present; skipping installation of libiconv."
 fi
 
-cd $ROOT
+cd "$ROOT"
 
-# We expect this path to exist and be a symbolic link that points to a file.
-if [ ! -L "$dylib_path" ]; then
+if [ ! -e "$dylib_path" ]; then
   echoerr "Error: expected $dylib_path to be present, but it was not. Cannot proceed."
   usage
   exit 1

script/test-native.js

@@ -4,14 +4,36 @@ const fs = require('fs')
 const path = require('path')
 const {spawnSync} = require('child_process')
 
-const testsPath = path.resolve(__dirname, '..', 'build', 'Debug', 'tests')
+const isWindows = process.platform === 'win32'
+
+// MSVC's Debug configuration enables checked iterators
+// (`_ITERATOR_DEBUG_LEVEL=2`) and `/RTC1` runtime checks. For a suite this
+// STL-heavy that is orders of magnitude slower — tens of minutes on CI, versus
+// under two seconds elsewhere. Clang's `-O0` does none of that, so Debug stays
+// the default on other platforms, where it costs nothing and keeps the binary
+// friendly to `lldb`.
+//
+// This does not weaken what CI checks: node-gyp never defines `NDEBUG`, so the
+// `assert()` calls throughout `src/core` stay live in Release too.
+//
+// Set SUPERSTRING_TEST_CONFIG to 'Debug' or 'Release' to override — you want
+// 'Debug' if you're about to attach a debugger on Windows.
+const configuration = process.env.SUPERSTRING_TEST_CONFIG ||
+  (isWindows ? 'Release' : 'Debug')
+const isDebug = configuration === 'Debug'
+
+const testsPath = path.resolve(
+  __dirname, '..', 'build', configuration, isWindows ? 'tests.exe' : 'tests'
+)
 const dotPath = path.resolve(__dirname, '..', 'build', 'debug.dot')
 const htmlPath = path.join(__dirname, '..', 'build', 'debug.html')
 
 if (fs.existsSync(testsPath)) {
-  run('node-gyp', ['build'])
+  run('node-gyp', isDebug ? ['build', '--debug'] : ['build'])
 } else {
-  run('node-gyp', ['rebuild', '--debug', '--tests'])
+  run('node-gyp', isDebug
+    ? ['rebuild', '--debug', '--tests']
+    : ['rebuild', '--tests'])
 }
 
 const args = process.argv.slice(2)
@@ -52,6 +74,16 @@ switch (args[0]) {
 }
 
 function run(command, args = [], options = {stdio: 'inherit'}) {
-  const {status} = spawnSync(command, args, options)
-  if (status !== 0) process.exit(status)
+  // `shell` is needed on Windows so that `node-gyp` resolves to `node-gyp.cmd`;
+  // CreateProcess cannot launch a batch file directly.
+  const {status, error} = spawnSync(command, args, {shell: isWindows, ...options})
+
+  // A failure to spawn at all reports `status: null`, which is not `0` — so the
+  // old check handed it to `process.exit`, where Node coerced it to 0 and the
+  // run was reported as a success.
+  if (error) {
+    console.error(`Failed to run ${command}: ${error.message}`)
+    process.exit(1)
+  }
+  if (status !== 0) process.exit(status === null ? 1 : status)
 }

src/core/marker-index.cc

@@ -464,8 +464,21 @@ void MarkerIndex::set_exclusive(MarkerId id, bool exclusive) {
 }
 
 void MarkerIndex::remove(MarkerId id) {
-  Node *start_node = start_nodes_by_id.find(id)->second;
-  Node *end_node = end_nodes_by_id.find(id)->second;
+  // Both maps are populated and cleared together, but check each one anyway:
+  // `splice` reassigns them independently, so treat either miss as "we don't
+  // hold this marker." Dereferencing an `end()` iterator here is undefined —
+  // libc++ and libstdc++ represent it as a null node and crash immediately,
+  // while MSVC represents it as a live list sentinel whose value was never
+  // constructed, quietly yielding garbage that we then write through and free.
+  auto start_entry = start_nodes_by_id.find(id);
+  auto end_entry = end_nodes_by_id.find(id);
+  if (start_entry == start_nodes_by_id.end() ||
+      end_entry == end_nodes_by_id.end()) {
+    return;
+  }
+
+  Node *start_node = start_entry->second;
+  Node *end_node = end_entry->second;
 
   Node *node = start_node;
   while (node) {