Migrate to nwn_script_comp

[cgtudor]

This is built on top of the existing PR and it adds support for the new compiler by first having modified the compiler to accept the -n flag in order to allow it to parse include files (no entry points) without trying to produce an .ncs out of them, just for validation.

The commit history is a bit messy as I first tried a hacky way of doing it, which turned out to be quite problematic. This, however, I've tested locally to great success.

Caveat: While I did make a PR to neverwinter.nim with my addition, it's not yet merged to the main repo. You can see the changes I made to it here, though.

[cgtudor]

Note about the last commit. An improvement to the compiler to get parity (hopefully) with nwnsc. The code changes are here.

[PhilippeChab]

⚠️ Cross-Platform Compatibility Issue: PowerShell

I've identified a critical cross-platform bug in this PR that will break diagnostics on Linux and macOS.

The Problem

Line 151 in DiagnosticsProvider.ts:

const child = spawn(join(__dirname, this.getExecutablePath(os)), args, { shell: 'powershell.exe' });

This change replaces PR #62's { shell: true } (which uses the system's default shell) with a Windows-specific shell.

Impact

Platform	Status	Issue
Windows	✅ Works	powershell.exe exists
Linux	❌ BROKEN	powershell.exe doesn't exist
macOS	❌ BROKEN	powershell.exe doesn't exist

This will completely break the diagnostics feature on non-Windows platforms.

Why This Matters

• The project provides separate compiler binaries for Linux/macOS (lines 56-65)
• The code already has OS detection logic
• Many NWN modders use Linux servers

Recommended Fix

Option 1 (simplest): Revert to { shell: true }

const child = spawn(join(__dirname, this.getExecutablePath(os)), args, { shell: true });

Option 2: Use OS-specific shells

const shell = type() === OS.windows ? 'powershell.exe' : true;
const child = spawn(join(__dirname, this.getExecutablePath(os)), args, { shell });

Option 3 (best): Fix argument escaping without relying on PowerShell-specific behavior

If you encountered quoting issues on Windows, we should solve that without breaking other platforms.

[PhilippeChab]

Additional Issues Found

After deeper review, I've identified several more issues beyond the PowerShell problem:

---

🔴 CRITICAL: Linux/macOS Binaries Not Updated

Problem: Only the Windows binary was updated in this PR, but the code uses new flags (-n and -E) that require the updated compiler.

Evidence:

• Windows binary: Updated from 4.9MB → 5.5MB ✅
• Linux binary: Still at neverwinter.nim v1.6.4 (unchanged) ❌
• macOS binary: Unchanged ❌

Impact: The diagnostics will fail on Linux/macOS because the compiler won't recognize the -n and -E flags:

const args = ["-y", "-s", "-n", "-E"];  // Lines 110

The comment even states: // Note: -E requires compiler binary with ABI v2+

Fix Required: Update Linux and macOS binaries to match the Windows version, or wait for neverwinter.nim PR #152 to be merged and released.

---

⚠️ Potential Crash: Unsafe Regex Access

Lines 33 & 40 in DiagnosticsProvider.ts:

const linePosition = Number(lineNumber.exec(line)\![1]) - 1;
// ...
message: lineMessage.exec(line)\![2].trim(),

Problem: Uses non-null assertion operator (\!) without checking if regex matched first.

Risk: If the compiler outputs a malformed error line (e.g., error without line number), this will throw:

Cannot read property '1' of null

Recommended Fix:

const lineNumberMatch = lineNumber.exec(line);
const lineMessageMatch = lineMessage.exec(line);
if (lineNumberMatch && lineMessageMatch) {
  const linePosition = Number(lineNumberMatch[1]) - 1;
  // ... use lineMessageMatch[2]
}

---

⚠️ WorkspaceFilesSystem: Non-Deterministic File Resolution

Lines 23-26 in WorkspaceFilesSystem.ts:

const filteredPaths = paths.find((p) => {
  return isQueueSrc ? p.includes("queue_src") : \!p.includes("queue_src");
});

Problem: If multiple files exist with the same name in different directories (outside queue_src), .find() returns whichever glob finds first - order may not be deterministic.

Example Scenario:

src/utils.nss
lib/utils.nss

When including utils, which one gets picked? Depends on glob order.

Potential Fix: Use directory proximity or depth-first search from parent directory, or warn about ambiguous includes.

---

📝 Minor: Inconsistent Variable Naming

Line 26: const path = filteredPaths ? filteredPaths : null;

This is just const path = filteredPaths || null; or more clearly filteredPaths ?? null. Minor style issue.

---

Summary

Issue	Severity	Platforms Affected
PowerShell hardcoded	🔴 Critical	Linux, macOS
Missing binary updates	🔴 Critical	Linux, macOS
Unsafe regex access	⚠️ High	All (if malformed compiler output)
Non-deterministic file resolution	⚠️ Medium	All (if duplicate filenames)

Recommendation: Address the two critical cross-platform issues before merging.

[cgtudor]

Everything addressed! One comment was on a bit that's a hack for TDN and so I removed it, but I will make a new PR with a version of that which is generalized and behind an option because we need it and maybe others do as well.

[PhilippeChab]

Are we sure this doesn't cause performance issues?

[cgtudor]

It's obviously less performant, but the hit is moderate.
Even with hundreds of scripts, even >1k, we are talking about an O(n) iteration over them on each file save and each queued document. In practice so far, for me, it's not been noticeable. The start-up indexing period takes the biggest hit but it was already slow and the difference is not that big.

If we wanted to optimize this, I could do some caching on ServerManager and invalidate it only when documents are added/removed so we don't have to recompute on every publish call.

However, there is a potential performance hit here that's bigger, and that's the amount of directories we pass to "--dirs", it might affect compilation time if the dirs list is very large. I had to do this to get the new compiler to work, but there might be a better way. I'll have a think.

[cgtudor]

I'll try and see if I can walk the tree of children to minimize it and not mess up compilation

[cgtudor]

Yea, we can't as I remembered.

We can't narrow this to just transitive children from getChildren() because the tokenizer's include tracking may be incomplete compared to what the compiler actually resolves (e.g. deep/nested transitive includes, conditional includes, or files the tokenizer hasn't fully parsed).

[PhilippeChab]

Let's try it this way then, thank you for validating.

[PhilippeChab]

Alright, once the upstream compiler changes are merged, we can go forward.