Fixes: NuGet MSBuild-DOM editing, orphaned-process cleanup, F# type references#133
Merged
Conversation
…#112) Root cause was the shared F# e2e fixture, not the sidecar. Library.fs placed `let area` / `let sumOfSquares` directly in a `namespace`, which is illegal F# (FS0201: namespaces cannot contain values). Those bindings never type-checked, so FCS recorded no `Shape` type use-sites inside them — `textDocument/references` and `textDocument/typeDefinition` on the `Shape` type saw only its declaration, even though rename and go-to-definition (targeted resolution) worked. Make the fixture a valid top-level `module` (line-preserving; every position constant and DisplayName unchanged). This restores full FSAC parity: references now include the `: Shape` annotation use-site and typeDefinition resolves to `type Shape =`. - Add live regression test test_full_stack_fsharp_references_type_use_sites - Tighten test_full_stack_fsharp_navigation: typeDefinition must resolve non-null - Drop obsolete "KNOWN GAP #112" caveats/NOTE in the F# navigation tests - Update FSHARP-FSAC-PARITY-PLAN tracking (references + typeDefinition -> done) Closes #112
Replace the line-oriented `src/nuget/xml_edit.rs` (which corrupted valid project files) with the issue's recommended architecture: PackageReference / PackageVersion edits are delegated to the C# sidecar and mutated through `Microsoft.Build.Construction.ProjectRootElement` (preserveFormatting: true) — the same API MSBuild uses, which preserves whitespace, comments, and attribute order and understands multi-line elements, wrapped attributes, conditional ItemGroups, and CPM. C# sidecar: - PackageEditor.cs: add/upsert/remove via ProjectRootElement. - CSharpSidecar.Packages.cs + Messages.cs: project/addPackage, project/removePackage handlers + DTOs. - csproj: compile-time-only Microsoft.Build reference (runtime resolved by MSBuildLocator; MSBL001), StringTools bumped to match. Rust host: - src/nuget/edit.rs: thin sidecar IPC wrapper (old xml_edit API, sidecar-backed). - handlers.rs (install/uninstall) and consolidate.rs route through the sidecar; main.rs threads the C# sidecar in. - Delete src/nuget/xml_edit.rs; drop the quick-xml dependency. .fsproj edits route to the C# sidecar too: MSBuildLocator (MSBL001) forbids a shared Microsoft.Build reference in the F#/Common projects, and ProjectRootElement is language-agnostic (the issue notes "it's not F#-specific"). Tests: nuget_e2e install/uninstall/consolidate are now full-stack (workspace root + require_dotnet), covering the exact shapes the line editor broke on — multi-line children, wrapped attributes, conditional ItemGroup, comments, CPM, multi-ItemGroup. All green: 33 nuget_e2e, 311 lsp_e2e, 278 lib, 394 C# sidecar. Consolidate write-path coverage moved from unit tests to full-stack e2e. Closes #4
…SIGKILL Issue #8's fix — the custom `makeErrorHandler` in client.ts that returns `CloseAction.Restart` with `handled: true` for up to 5 unexpected closes — landed without a regression test. This adds one. The test SIGKILLs the exact staged sharplsp server binary out from under the live client (reproducing the #8 scenario: transient extension-host restart or `make install-rust` replacing the binary on disk) and asserts the client recovers on its own and serves document symbols again — WITHOUT invoking `sharplsp.restartServer`. Matching the precise resolved binary path (not a bare `sharplsp` basename) guarantees the test only ever kills the test host's own server, never a developer's server running elsewhere. Verified: passes (server auto-recovers in ~50ms after the kill). Closes #8
…t run `compile-tests` (`tsc -p ./`) never prunes orphaned emit, and nothing cleaned `out/` first. When a test source was deleted or renamed, its previously-compiled `out/**/*.test.js` lingered and was still executed by the mocha glob — running a stale test against current source. `out/` is gitignored so CI (fresh checkout) never saw it, but local working trees hit confusing phantom failures. Concretely, a full `make _test-vsix` failed with 7 `acquireDotnet10 is not a function` errors from `out/test/suite/unit-dotnet-runtime.test.js` — a leftover of PR #125, which deleted that source and renamed the export to `acquireDotnet10Sdk`. More broadly, ~200 orphan tests from earlier unit->e2e conversions were still running locally. Fix: add `clean-out.mjs` (cross-platform, zero-dependency) and run it via a new `clean` script before `tsc` in `compile-tests`. After the fix the suite runs exactly the current-source tests that CI runs (536 passing, 0 failing, 94% coverage), and a planted orphan is pruned on the next compile. package.json edited via `npm pkg set` (JSON document model, not hand-splicing). Closes #132
…unaway test processes The `ProfileTarget` test fixture only got cleaned up via Rust `Drop`, which never runs when the test process dies abnormally — nextest SIGKILLs a timed-out test, Ctrl+C, etc. Its CPU-bound worker loops then keep running as an orphan, each holding ~230-316 MB indefinitely (issue #3). Fix: a parent-death watchdog inside the fixture. A dedicated background thread (not the thread pool — the workers are tight CPU-bound spins that would starve an async watchdog) polls `getppid()` and cancels the moment it is reparented (`ppid == 1`, or a changed ppid for Linux subreapers). The `== 1` check also wins the race where the parent dies during our own startup, before the initial ppid is captured. POSIX-only; no-ops on Windows. Regression test `test_profiler_edge_profile_target_dies_when_parent_killed` spawns ProfileTarget under an intermediary, SIGKILLs the intermediary (so Drop cannot run), and asserts the orphan self-terminates. Verified: fails before the fix (orphan survives the full 10s), passes after (dies in <1s), stable across repeat runs with zero leftover processes. Full profiler suite stays green (37/37). The csproj `NoWarn` (adding SYSLIB1054 — LibraryImport would force AllowUnsafeBlocks, not worth it for one getppid) was edited via an XML DOM, not string-splicing. Closes #3
Prettier: reformat context-menus.test.ts and debug-e2e.test.ts to satisfy the `prettier@3 --check` gate. Floating prettier@3 now resolves to 3.9.4, which reflows union-type annotations differently from the older 3.x these were written with. Whitespace-only; no behavior change. C# coverage: add PackageEditorEndToEndTests.cs — coarse in-process e2e tests that drive project/addPackage and project/removePackage through the sidecar socket, covering every branch of PackageEditor and CSharpSidecar.Packages.cs (add/upsert/remove, CPM version-strip, PackageVersion, missing-file error). These were exercised only by the out-of-process Rust nuget e2e, so they scored 0% under coverlet's in-process measurement, dropping the C# sidecar to 91.86% (below the 94% effective gate). Restores both files to 100% and the assembly to 94.3%.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Collection of fixes on this branch. Each ships with a regression test.
Changes
Microsoft.Build.Construction.ProjectRootElementediting in the C# sidecar; preserves whitespace/comments/attribute order, handles multi-line elements, conditional ItemGroups, and CPM..fsprojedits route through the same sidecar.quick-xmldependency dropped.references/typeDefinitionon a type now return all use-sites. Root cause was an invalid F# fixture (letbindings directly under anamespace, FS0201) that never type-checked; fixture is now a valid top-levelmodule.ProfileTargettest fixture self-terminates when orphaned (parent-death watchdog pollinggetppid()), so a SIGKILL'd/timed-out test no longer leaves runaway CPU-bound processes.out/beforecompile-testsso stale/orphaned compiled.test.jsfiles can't run against current source.CI prep (this PR's final commit)
prettier@3 --checkgate (floatingprettier@3now resolves to 3.9.4, which reflows union-type annotations; whitespace-only).PackageEditorEndToEndTests.cs— coarse in-process e2e tests drivingproject/addPackage/removePackagethrough the sidecar socket, covering every branch ofPackageEditor/CSharpSidecar.Packages.cs. These were only exercised by the out-of-process Rust nuget e2e, so they scored 0% under coverlet, dropping the C# sidecar below its coverage gate. Restores both files to 100% (assembly 94.3%).Local verification
-warnaserror, prettier, eslint, tsc)Known pre-existing flake (identical to
main, unrelated to this branch): theCalculator.cs has folding ranges for regionstest can intermittently fail ateditor.foldAlldue to VS Code's async fold-model timing. Passes in warmed runs; may occasionally need a CI re-run.