fix(language): highlight .h headers as the language detection actually chose - #3018
fix(language): highlight .h headers as the language detection actually chose#3018sinelaw wants to merge 3 commits into
Conversation
83574f2 to
e2291ce
Compare
|
Windows CI failure fixed ( The fix is in the test, not the production probe. The probe uses What actually divergedNot the fake tree's lookups,
let resolved_path = if path.is_relative() { base_dir.join(path) } else { path.to_path_buf() };and since the fake filesystem reports a remote connection, Established without a Windows box, two ways: the documented That The changeThe fake root is now constructed rather than written as a literal: take the leading Still meaningfulWith The path resolves correctly there and the label is One pre-existing issue found, deliberately not fixedThat same Caveat: the Windows fix itself is unverified locally — this is a Linux box. What was verified is the mechanism (via the Linux emulation) and that the fix keeps Linux green while keeping the test failing without the production change. That the constructed root makes windows-latest pass rests on CI. Generated by Claude Code |
…y chose Opening `widget.h` next to `widget.cpp` gave a buffer that claimed to be C++ on the LSP side and C everywhere the user can see it: the status bar read `C`, and `namespace`, `class`, `public`, `explicit`, `virtual`, `template` and `typename` all rendered in plain foreground while `std::string` split as `std:` + `:` instead of scoping `::`. The same bytes in `widget.hpp` looked right, which is what made it read as a highlighting bug (#3009). The promotion heuristic was already there and already correct — `services/lsp/manager.rs::detect_language` promotes a `.h` to `cpp` when the surrounding tree smells like C++ (a sibling C++ source, or an ancestor `compile_commands.json` carrying a C++ marker). It simply never reached the screen. Detection has two halves that answered independently: `detect_language` resolves the config/LSP id from `[languages.*]`, while `GrammarRegistry::find_by_path` resolves the grammar from the extension table, where `.h` is unconditionally C. `from_path_with_fallback` applied the first result to `DetectedLanguage::name` only, so the id could say `cpp` while the grammar, the tree-sitter language and the status-bar label all still came from C. Rather than add another per-site patch, the two halves are now reconciled at the single fork that produces every buffer's language — a new `DetectedLanguage::align_with_config_id` re-resolves the grammar through the detected id's `[languages.<id>].grammar` whenever the two disagree. The config id wins, since it is what LSP routing, comment prefixes and tab settings already follow. An id whose grammar the registry doesn't know keeps the path-resolved grammar, so an unfamiliar config key can never downgrade working highlighting, and aliases (`[languages.mylang] grammar = "Rust"`) resolve to the same catalog entry the path lookup found and are unaffected. User config still overrides in both directions: moving `h` into `languages.cpp.extensions` forces C++ unconditionally with no sibling needed, and a config with no `cpp` language leaves headers as C even in a tree full of C++ sources. Both are covered by tests. Cost is one hash lookup per file open — no buffer scan and no extra filesystem access beyond what `detect_language` already performed. Known limitation, inherited unchanged: the tree probe `header_in_cpp_tree` uses `std::fs` directly rather than the `FileSystem` trait (there is a standing NOTE there about that being a cross-cutting refactor shared with `detect_workspace_root`). On a remote/SSH session the probe reads the local filesystem, finds nothing, and the promotion silently no-ops — a `.h` in a remote C++ tree still highlights as C. This commit neither fixes nor worsens that; it only makes the local answer coherent. Tests: unit tests on the primitive in `detected_language.rs` (sibling `.cpp` promotes, pure-C tree does not, both user-config override directions, alias and unknown-grammar safety), plus an e2e pair in `tests/e2e/issue_3009_h_header_cpp.rs` that opens the same header bytes in a C++ tree and in a C tree and asserts only on rendered output — the status bar label and the foreground colour of `namespace` / `virtual` against a token that is a plain identifier in both languages. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014RASTj1HKSpMtmL2ZpjJAN
…++ tree The `.h` → C++ promotion shipped in the previous commit reads the tree around the header to decide whether it is a C or a C++ header. That probe called `std::fs` directly, so it always described the machine running the editor. On an SSH session that is the wrong machine: opening `widget.h` from a remote C++ project listed the *local* directory, found no `.cpp` sibling, declined to promote, and the header rendered as C — status bar `C`, `namespace` and `virtual` unhighlighted, `std::string` splitting at `::`. Locally the same file looked right, which is what made it invisible. Until the previous commit the probe only steered LSP routing so the damage was confined to clangd's mode; now that it drives what is on screen, the blind spot is something the user watches happen. So the probe now asks the filesystem that owns the file. `&dyn FileSystem` is threaded through `detect_language` and `DetectedLanguage::from_path` / `from_path_with_fallback` as a required argument, not an `Option`: every one of the eleven call sites already had a filesystem in hand (each holds the buffer it just loaded, so `buffer.filesystem()` is the answer everywhere), and an optional parameter would have re-created exactly the failure being fixed — a caller that omits it gets a probe that silently answers "no" instead of a compile error. Threading the trait through is not free, and the naive version would have been worse than the bug. `RemoteFileSystem` implements the whole sync `FileSystem` surface with `AgentChannel::request_blocking`: one blocking SSH round trip per call, issued on the single-threaded editor loop. Language detection runs on the file-open path, so a sibling listing plus a ten-deep ancestor walk plus a compile-database read is up to thirteen serialized round trips before a remote header can be displayed, each able to stall the UI for the request timeout on a bad link. `services/remote/ filesystem.rs` already caches `$HOME` and the temp dir for precisely this reason. The probe therefore carries a `ProbeBudget` derived from the filesystem's own `remote_connection_info()`: remote spends exactly one round trip on the decisive signal — the sibling listing, which covers the ordinary `widget.h`/`widget.cpp` layout that motivated #3009 — and skips the `compile_commands.json` walk entirely. A remote header buried under `include/` with its sources elsewhere still reads as C, which is strictly better than the unconditional no-op it gets today and does not trade highlighting for a frozen editor. The budget is data (a directory count) rather than an `if is_remote` inside the loop, so "walk ten ancestors over SSH" is unrepresentable rather than merely unreached. Local behaviour is unchanged: the same eleven directories, the same 1 MiB cap. Reading the compile database moved to `metadata_if_exists` + a clamped `read_range`, which folds the old separate `is_file` check into one op. The clamp is load-bearing rather than cosmetic: `FileSystem::read_range` is `read_exact`-shaped and errors on a short file, so asking for a flat 1 MiB would have made every normal-sized `compile_commands.json` read as "not C++". `detect_workspace_root` is deliberately left alone and the NOTE now says so and why, rather than implying it was covered. It has the same remote blind spot, but its three call sites are inside LSP server spawn and initialize, `LspManager` holds no filesystem to thread, and `resolve_root_uri` walks *host* paths on purpose before applying `path_translation` for devcontainers — so which filesystem it should use is a genuine design question, not the mechanical substitution this commit performs. Fixing it belongs with the LSP lifecycle wiring, not here. Tests: five unit tests in `detected_language.rs` built on a `FakeTree` `FileSystem` whose contents exist at paths `std::fs` cannot open — the promotion fires from a C++ sibling only that fake can see, and the sharper mirror, a real tempdir holding a `.cpp` while the injected filesystem reports only `.c`, stays C. Two more pin the budget from both sides (a remote tree spends exactly one op and skips the ancestor walk; the identical local tree still finds the marker), one covers the read clamp, and one asserts non-`.h` paths touch the filesystem not at all. Plus an e2e that opens a header through a filesystem serving `/remote-cpp-project` — a path with no local existence — and asserts on rendered output. Without the fix its status bar reads `C`; with it, `C++`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014RASTj1HKSpMtmL2ZpjJAN
… literal `test_remote_h_header_beside_cpp_source_highlights_as_cpp` was the one test out of 8561 that failed on windows-latest while ubuntu and macos were green. The production probe was not at fault — the platform-variant primitive was a hard-coded absolute path in the test's own fixture. `RemoteTreeFs` served its fake host tree from the literal `"/remote-cpp-project"`. That string is absolute on Unix, but on Windows `Path::is_absolute` requires a *prefix* (drive letter or UNC share) as well as a root, so `/remote-cpp-project/widget.h` is a relative path there — `components()` yields `RootDir, Normal, Normal` with no `Prefix`. The editor's open path (`Window::open_file_no_focus_inner`) re-anchors relative paths to the session base directory, which for a filesystem reporting `remote_connection_info()` is `home_dir()`. So on Windows the header was opened as `C:\…\remote-cpp-project\widget.h`; `RemoteTreeFs::map` no longer recognised its own prefix, stopped translating, and delegated to `StdFileSystem`, which found nothing. The file "did not exist", the editor created an empty unsaved buffer, and the status-bar assertion failed. Nothing in the sibling scan, `Path::parent`, the extension check or the promotion itself was involved — they were never reached. The mechanism was established without a Windows box by giving the fixture, on Linux, the one property `/remote-cpp-project` has on Windows: a root that is not absolute. With `REMOTE_ROOT = "remote-cpp-project"` the Linux run reproduces the failure exactly, status bar `Opened /root/remote-cpp-project/ widget.h` and no language — the same re-anchor, the same untranslated path. So the root is now constructed rather than written down: take the leading `Prefix`/`RootDir` components of a path the platform itself produced (`std::env::temp_dir()`) and join the project directory name onto them. That yields `/remote-cpp-project` on Unix and `C:\remote-cpp-project` on Windows — absolute under either convention by construction, not by guessing at Windows behaviour. An assertion on `is_absolute()` pins the invariant at the point it matters, so a future regression fails where the cause is rather than as a puzzling status-bar mismatch. `REMOTE_ROOT` becomes a `PathBuf` field and `remote_path` an instance method, since the value is no longer a constant. The test's meaning is unchanged. It still opens a header whose C++ sibling exists only inside the injected filesystem, and a new assertion states that requirement directly: the remote path must not resolve on the local disk. Reverting `crates/fresh-editor/src` to master still makes it fail with status bar `C` on the correctly-resolved `/remote-cpp-project/widget.h`, so it was not weakened into passing. Not fixed here, and worth its own change: the same `path.is_relative()` branch in `open_file_no_focus_inner` means a *real* Windows client attached to a Linux SSH host would re-anchor every absolute remote path it is handed. That is pre-existing behaviour unrelated to #3009 and untouched by this PR; deciding what "absolute" means for a path whose owning host may run a different OS is the same design question the NOTE in `manager.rs` raises about `detect_workspace_root`. Verified on Linux only: the three `issue_3009` e2e tests pass, and fail as expected with the production change reverted. The Windows behaviour this commit targets cannot be executed here and rests on CI. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014RASTj1HKSpMtmL2ZpjJAN
2817749 to
f39241e
Compare
Fixes #3009.
Two commits: the first makes
.hheaders highlight as the language detection already chose, the second makes that detection ask the file's own filesystem so it works over SSH.What was happening
.hwas unconditionally C, with no content or project-layout influence. The same bytes inwidget.handwidget.hppgaveCandC++in the status bar respectively, and in the.hbuffernamespace,class,public,explicit,virtual,template,typenameall rendered in plain foreground instead of the keyword colour —std::stringeven split asstd:+:rather than scoping::.The interesting part is that a C++ promotion heuristic already existed and never reached the screen.
detect_language(services/lsp/manager.rs) promotes.htocppwhen the surrounding tree smells like C++ — sibling C++ sources, or an ancestorcompile_commands.jsonwith a C++ marker. Measured against the unfixed build: a directory holding onlywidget.hand a siblingwidget.cpp— precisely the signal that heuristic looks for — still opened the header as C.DetectedLanguage::from_path_with_fallbackwas the reason. It computedconfig_lang_idfrom that promotingdetect_language, then applied it through anoverride_nameclosure that touchedd.nameand nothing else, whilehighlighter,ts_languageanddisplay_nameall came fromregistry.find_by_path, which resolves.hthrough the extension table (config.rs,languages.c.extensions = ["c", "h"]) and lands on C. So the LSP side could already believe a header was C++ while the grammar, the status-bar label and every colour on screen said C.Commit 1 — let the chosen language id pick the grammar
primitives/detected_language.rs.override_nameis replaced byDetectedLanguage::align_with_config_id: when detection resolved a config id, that id wins and the grammar is re-resolved through[languages.<id>].grammar. If the grammar name resolves to the same catalog entry — aliases, the common case — or to nothing, the path-resolved grammar is left untouched. One hash lookup; no buffer scan and no extra filesystem access.User config still overrides in both directions —
languages.cpp.extensionsincluding"h"continues to force C++ globally, and a config without acpplanguage leaves.has C.Commit 2 — ask the file's own filesystem
The probe deciding that promotion,
header_in_cpp_tree, read the filesystem withstd::fsdirectly — a standing violation of theFileSystemtrait rule, carrying aNOTE(remote-fs)that said the fix needed threading&dyn FileSystemthroughdetect_languageandDetectedLanguage::from_path. Once commit 1 made the probe drive what the user sees, that became user-visible: on an SSH session the probe read the local filesystem, found nothing, and the promotion silently no-opped, so a.hin a remote C++ tree still highlighted as C.The trait is now threaded through. All 11 non-test call sites of
from_path/from_path_with_fallbackwere checked first: every one already holds the buffer it just loaded, sobuffer.filesystem()answers at each and no call site needed an escape hatch. The parameter is a required&dyn FileSystemrather than anOptiondeliberately — an optional one would re-create exactly the failure being fixed, where a caller that omits it gets a silent "no" instead of a compile error.Remote round trips are budgeted, not assumed cheap
RemoteFileSystemimplements the whole syncFileSystemsurface overAgentChannel::request_blocking—read_dir→ls,metadata→stat,read_range→read, one blocking SSH round trip each, on the single-threaded editor loop. That file already documents the hazard for$HOMElookups: a blocking request there "would hang the whole single-threaded UI for the full request timeout."Threading the trait naively would therefore have put up to 13 serialized round trips on the file-open path. Instead the probe carries a
ProbeBudgetderived fromfs.remote_connection_info():widget.h/widget.cpplayout in this issue. Thecompile_commands.jsonancestor walk is skipped.The trade is explicit: a remote header under an
include/-style tree with its sources elsewhere still reads as C. That is strictly better than today's unconditional no-op and does not buy highlighting with a frozen editor. The budget is a directory count rather than anif is_remoteinside the loop, so the expensive case is unrepresentable rather than merely unreached.Two bugs the tests caught
"clang++"does not contain the substring"c++"— the characters before++areg. The first fixture was unrealistic and a failing test said so.FileSystem::read_rangeisread_exact-shaped and errors on a short file, so a flat 1 MiB read would have made every normal-sizedcompile_commands.jsonread as "not C++". The clamp tomin(size, 1 MiB)is load-bearing and has a test pinning it.Not included:
detect_workspace_rootThe old NOTE asked for it in the same pass. It is not a mechanical substitution: its three call sites sit inside LSP spawn/initialize,
LspManagerhas no filesystem to thread, andresolve_root_uriwalks host paths on purpose before applyingpath_translationfor devcontainers — so "which filesystem" is a genuine design question there. The NOTE has been rewritten to state that accurately rather than implying it was covered.Tests
Commit 1: six unit tests in
detected_language.rs(sibling promotion, pure-C project staying C, user config forcinghto C++, config without acpplanguage, an alias resolving to the same entry, an unknown config grammar keeping the path-resolved highlighter) plus two e2e intests/e2e/issue_3009_h_header_cpp.rs.Commit 2, on a
FakeTreefake filesystem (no shared in-memory double existed; it follows the delegation pattern intests/e2e/explorer_bugs.rs):test_h_header_promotion_reads_injected_filesystem_not_local_disk,test_h_header_promotion_ignores_local_disk_when_injected_fs_says_c,test_remote_probe_spends_one_op_and_skips_ancestor_walk,test_local_probe_still_walks_ancestors_for_compile_commands,test_small_compile_commands_is_read_despite_the_one_mib_cap,test_non_header_paths_touch_no_filesystem, plus e2etest_remote_h_header_beside_cpp_source_highlights_as_cpp.Verification status
Commit 1: unit tests verified both directions — 6 passed with the fix; with the re-resolution neutralized,
test_h_header_beside_cpp_sibling_detects_cpp_grammarfailed withleft: "C" / right: "C++"and the other five passed. Its two e2e tests were run only with the fix, so their fails-without direction is unverified.Commit 2:
cargo test -p fresh-editor --lib -- detected_language detect_language jsonc_filenames workspace_root→ 31 passed, 0 failed. Reverting the sibling scan tostd::fsmakes the three key unit tests fail and restoring it makes them pass. The single new e2e test was also run individually (not the suite): it passes, and with the probe reverted tostd::fsit fails with the status bar readingCinstead ofC++.Clean on the committed tree:
cargo fmt --all --check,cargo check -p fresh-editor --all-targets,cargo clippy -p fresh-editor --all-targets(zero warnings in touched files; the rest are pre-existing), andcargo check --all-targets --features gui. Full e2e suites were left to CI. No plugin API or config type changes, so no.d.tsor schema regeneration, and no new i18n keys.🤖 Generated with Claude Code
https://claude.ai/code/session_014RASTj1HKSpMtmL2ZpjJAN