diff --git a/laravel-lsp/src/main.rs b/laravel-lsp/src/main.rs index e3f76977..090c71a4 100644 --- a/laravel-lsp/src/main.rs +++ b/laravel-lsp/src/main.rs @@ -18140,7 +18140,9 @@ async fn classify_with_decl_fallback( // subsequent invocations don't re-parse the file. if !is_in_routes_dir(root, file_path) { // A Folio page's `name('...')` helper declares its route name, but the - // page lives outside `routes/` and the bare helper isn't tagged by + // page isn't a conventional route file — it lives outside the + // project-root `routes/`, or nested deeper inside it (a mount under + // `routes/pages/`, issue #105) — and the bare helper isn't tagged by // php.scm. If the cursor sits on that call, resolve it to the page's // (mount-prefixed) route name — read from the already-built in-memory // route index, never a fresh filesystem walk — so find-references / @@ -18221,36 +18223,46 @@ async fn decl_range_at( None } -/// Is this file a conventional route file — i.e. does it live directly under -/// the project root's `routes/` directory? Used to gate the declaration-fallback +/// Is this file a conventional route file — i.e. is its immediate parent the +/// project root's `routes/` directory? Used to gate the declaration-fallback /// walk so we don't pay the parse cost on every PHP file. /// -/// Only the project-root `routes/` qualifies. A `routes` component nested -/// deeper — a package's `vendor/.../routes/`, or a Folio mount that happens to -/// carry a `routes` segment (issue #98) — must NOT be treated as the -/// conventional routes dir, or find-references from such a file would be routed -/// into the decl walk and never reach its real resolver. When the project root -/// is unknown (`None`), default to `false` so the walk never triggers without a -/// root to anchor against. +/// Only files sitting *directly* in the project-root `routes/` qualify +/// (`routes/web.php`, `routes/api.php`). Two kinds of `routes` path component +/// must NOT trigger the gate: +/// - a `routes` segment nested elsewhere — a package's `vendor/.../routes/`, +/// or a sibling dir that merely shares the name (issue #98); +/// - a file nested *deeper* inside `routes/` — e.g. a Folio mount under +/// `routes/pages/about.php` (issue #105). Such a page has its own resolver +/// (the Folio branch in `classify_with_decl_fallback`); routing it into the +/// decl walk would bypass that branch and yield no references. Matching the +/// immediate parent — not a `starts_with` prefix — is what keeps the Folio +/// page out of the gate. /// -/// Both `path` and the joined `routes/` dir are canonicalized before the -/// component-wise prefix check (issue #122), mirroring the sibling helper +/// When the project root is unknown (`None`), default to `false` so the walk +/// never triggers without a root to anchor against. +/// +/// Both the file's parent and the joined `routes/` dir are canonicalized before +/// the equality check (issue #122), mirroring the sibling helper /// `path_within_root`. `root` is stored once (from an earlier `did_open`) while /// `path` arrives per-request, so the two can resolve through different symlink -/// states — e.g. macOS `/tmp` → `/private/tmp`. A raw textual `starts_with` -/// would then return a false negative and silently skip the decl-fallback walk -/// for a real conventional route file. When either side can't be canonicalized -/// (the file doesn't exist yet, a permission error) we fall back to the textual -/// prefix check rather than guess — no panic, and the same behaviour as before +/// states — e.g. macOS `/tmp` → `/private/tmp`. A raw textual comparison would +/// then return a false negative and silently skip the decl-fallback walk for a +/// real conventional route file. When either side can't be canonicalized (the +/// file doesn't exist yet, a permission error) we fall back to comparing the +/// uncanonicalized paths rather than guess — no panic, and the same behaviour /// for in-memory paths that aren't on disk. fn is_in_routes_dir(root: Option<&Path>, path: &Path) -> bool { let Some(r) = root else { return false; }; let routes_dir = r.join("routes"); - match (path.canonicalize(), routes_dir.canonicalize()) { - (Ok(real_path), Ok(real_routes)) => real_path.starts_with(&real_routes), - _ => path.starts_with(&routes_dir), + let Some(parent) = path.parent() else { + return false; + }; + match (parent.canonicalize(), routes_dir.canonicalize()) { + (Ok(real_parent), Ok(real_routes)) => real_parent == real_routes, + _ => parent == routes_dir, } } diff --git a/laravel-lsp/src/tests/routes_dir_gate.rs b/laravel-lsp/src/tests/routes_dir_gate.rs index b56e9619..a97de1d2 100644 --- a/laravel-lsp/src/tests/routes_dir_gate.rs +++ b/laravel-lsp/src/tests/routes_dir_gate.rs @@ -1,18 +1,40 @@ //! Unit tests for `is_in_routes_dir` — the gate deciding whether the -//! declaration-fallback route walk runs for a given file. Only the project -//! root's own `routes/` directory qualifies; a `routes` component nested -//! deeper (a package's `vendor/.../routes/`, or a Folio mount) must not be -//! mistaken for it (issue #98). +//! declaration-fallback route walk runs for a given file. Only a file whose +//! immediate parent is the project root's own `routes/` directory qualifies. +//! A `routes` component nested elsewhere (a package's `vendor/.../routes/`, a +//! sibling that shares the name) must not be mistaken for it (issue #98), and +//! neither must a file nested *deeper* inside `routes/` — e.g. a Folio mount +//! under `routes/pages/` (issue #105), which has its own resolver. use crate::is_in_routes_dir; use std::path::Path; #[test] fn matches_project_root_routes_dir() { + // Direct children of the project-root `routes/` are conventional route + // files (AC: both web.php and api.php resolve correctly). assert!(is_in_routes_dir( Some(Path::new("/project")), Path::new("/project/routes/web.php") )); + assert!(is_in_routes_dir( + Some(Path::new("/project")), + Path::new("/project/routes/api.php") + )); +} + +#[test] +fn rejects_folio_page_nested_under_routes() { + // A Folio mount under `routes/pages/` lives *inside* routes/, but deeper + // than its immediate child level (issue #105). It must NOT be gated as a + // conventional route file — its `name('...')` is resolved by the Folio + // branch in `classify_with_decl_fallback`, and routing it into the decl + // walk would bypass that branch and yield no references. The immediate- + // parent check (not a `starts_with` prefix) is what excludes it. + assert!(!is_in_routes_dir( + Some(Path::new("/project")), + Path::new("/project/routes/pages/about.php") + )); } #[test] @@ -46,6 +68,16 @@ fn rejects_false_prefix_sibling() { )); } +#[test] +fn rejects_routes_dir_outside_project_root() { + // A `routes/` directory that belongs to some *other* tree, not the + // configured project root, must not qualify (AC: root-known case). + assert!(!is_in_routes_dir( + Some(Path::new("/project")), + Path::new("/elsewhere/routes/web.php") + )); +} + #[test] fn rejects_when_root_unknown() { // No project root to anchor against → never trigger the fallback walk. @@ -95,24 +127,31 @@ fn rejects_real_path_outside_routes_dir() { #[test] fn falls_back_to_textual_when_path_missing() { - // A path that doesn't exist can't be canonicalized; the gate must fall back - // to the textual component check rather than panic (issue #122). Create a - // real `routes/` dir on disk so `routes_dir` canonicalizes but the missing - // `path` does not — genuinely driving the `(Err, Ok)` arm. This is the - // realistic case the production doc comment calls out: a brand-new route - // file still in the editor buffer, not yet saved to disk, sitting inside a - // real `routes/` directory. + // The gate compares the file's *parent* dir against `/routes`. When a + // path can't be canonicalized it must fall back to a direct (uncanonical) + // comparison rather than panic (issue #122). This is the realistic case the + // production doc comment calls out: a brand-new route file still in the + // editor buffer, not yet saved to disk, sitting inside a real `routes/` dir. let tmp = tempfile::tempdir().unwrap(); let root = tmp.path().to_path_buf(); std::fs::create_dir_all(root.join("routes")).unwrap(); let missing = root.join("routes").join("does_not_exist.php"); - // No panic, and the textual prefix still matches the project routes/ dir. + // The parent `/routes` exists, so this resolves via the canonical arm + // — no panic on the unsaved file, and the parent matches the routes/ dir. assert!(is_in_routes_dir(Some(&root), &missing)); - // False-negative side: a missing path *outside* routes/ must still return - // false through the same textual fallback (routes_dir canonicalizes, the - // path does not — again the `(Err, Ok)` arm). + // False-negative side: a missing path *outside* routes/ has a parent + // (`/app`) that doesn't exist either, so its parent can't be + // canonicalized — driving the `(Err, Ok)` textual-fallback arm. It must + // still return false. let missing_outside = root.join("app").join("does_not_exist.php"); assert!(!is_in_routes_dir(Some(&root), &missing_outside)); + + // And a route file *and* its parent both missing from disk (neither + // canonicalizes) must still match through the textual fallback — the parent + // `/routes` equals the joined routes dir by direct comparison. + let unsaved_root = tmp.path().join("never_created"); + let unsaved_file = unsaved_root.join("routes").join("web.php"); + assert!(is_in_routes_dir(Some(&unsaved_root), &unsaved_file)); }