Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -63,10 +63,10 @@ impl GraphFiles {
mut resource_candidates: Vec<PathBuf>,
excluded_indexable: &HashSet<PathBuf>,
) -> Self {
all.sort();
crate::codebase::ts_source::sort_os_str_paths(&mut all);
all.dedup();
let visible = vec![1u8; all.len()];
resource_candidates.sort();
crate::codebase::ts_source::sort_os_str_paths(&mut resource_candidates);
resource_candidates.dedup();
let indexable: Vec<PathBuf> = all
.iter()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,9 @@ impl GraphFiles {
let mut changed = false;
match self
.all
.binary_search_by(|candidate| candidate.as_path().cmp(path.as_path()))
.binary_search_by(|candidate| {
crate::codebase::ts_source::cmp_os_str_paths(candidate, &path)
})
{
Ok(index) => {
if self.visible.get(index).copied() != Some(1) {
Expand All @@ -37,7 +39,7 @@ impl GraphFiles {
if is_indexable(&path) && !self.indexable.contains(&path) {
let indexable = std::sync::Arc::make_mut(&mut self.indexable);
indexable.push(path);
indexable.sort();
crate::codebase::ts_source::sort_os_str_paths(indexable);
changed = true;
}
if changed {
Expand All @@ -53,7 +55,7 @@ impl GraphFiles {

fn visible_index(&self, path: &Path) -> Option<usize> {
self.all
.binary_search_by(|candidate| candidate.as_path().cmp(path))
.binary_search_by(|candidate| crate::codebase::ts_source::cmp_os_str_paths(candidate, path))
.ok()
.filter(|&index| self.visible.get(index).copied() == Some(1))
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,18 +8,26 @@ impl GraphFiles {
visible: impl IntoIterator<Item = PathBuf>,
mut resource_candidates: Vec<PathBuf>,
) -> Self {
all.sort();
crate::codebase::ts_source::sort_os_str_paths(&mut all);
all.dedup();
indexable.sort();
crate::codebase::ts_source::sort_os_str_paths(&mut indexable);
indexable.dedup();
resource_candidates.sort();
crate::codebase::ts_source::sort_os_str_paths(&mut resource_candidates);
resource_candidates.dedup();
let mut visible_paths: Vec<_> = visible.into_iter().collect();
visible_paths.sort();
crate::codebase::ts_source::sort_os_str_paths(&mut visible_paths);
visible_paths.dedup();
let flags = all
.iter()
.map(|path| u8::from(visible_paths.binary_search(path).is_ok()))
.map(|path| {
u8::from(
visible_paths
.binary_search_by(|candidate| {
crate::codebase::ts_source::cmp_os_str_paths(candidate, path)
})
.is_ok(),
)
})
.collect();
Self {
all: std::sync::Arc::new(all),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,10 @@ fn graph_files_visible_does_not_build_a_pathbuf_hashset() {
constructor.contains("vec![1u8; all.len()]"),
"from_files must mark visibility with a parallel bitset, not a cloned path set"
);
assert!(
constructor.contains("sort_os_str_paths") && !constructor.contains("all.sort();"),
"from_files must sort visible paths with OsStr order, not Path::cmp"
);
assert!(
!constructor.contains("visible:")
|| constructor.contains("let visible = vec![1u8; all.len()]"),
Expand All @@ -77,6 +81,16 @@ fn graph_files_visible_does_not_build_a_pathbuf_hashset() {
&& !trait_contains.contains("self.visible_path"),
"VisiblePathLookup::contains_visible must stay exact HashSet-style membership"
);
let visible_index = graph_files_source_function_body(
visible_source,
"fn visible_index(&self, path: &Path) -> Option<usize>",
);
assert!(
visible_index.contains("cmp_os_str_paths")
&& !visible_index.contains("as_path().cmp")
&& !visible_index.contains("canonicalize"),
"visible_index must probe OsStr bytes without Path::cmp or canonicalize"
);
}

#[test]
Expand Down Expand Up @@ -290,6 +304,29 @@ fn graph_files_explicit_root_marks_existing_hidden_path_visible() {
assert!(!files.add_explicit_root(&page));
}

#[test]
fn graph_files_visible_probe_is_os_str_byte_identity() {
let root = crate::codebase::ts_resolver::normalize_path(
&PathBuf::from(env!("CARGO_MANIFEST_DIR"))
.join("../../fixtures/ts-source/normalized-path-membership"),
);
let nested = root.join("a/b.ts");
let dashed = root.join("a-b.ts");
let dotted = root.join("a/./b.ts");
let collapsed = crate::codebase::ts_resolver::normalize_path(&dotted);

let files = GraphFiles::from_files(vec![nested.clone(), dashed.clone()]);
// OsStr bytes put `a-b.ts` before `a/b.ts` (`-` < `/`). Path::cmp would
// reverse that because it compares the `a` component first.
assert_eq!(files.all(), [dashed.clone(), nested.clone()]);
assert_eq!(nested.as_os_str(), collapsed.as_os_str());
assert_ne!(dotted.as_os_str(), nested.as_os_str());
assert!(files.contains_visible(&nested));
assert!(files.contains_visible(&dashed));
assert!(!files.contains_visible(&dotted));
assert!(files.contains_visible(&collapsed));
}

#[cfg(unix)]
#[test]
fn graph_files_trait_contains_visible_is_exact_membership() {
Expand Down
11 changes: 6 additions & 5 deletions crates/no-mistakes/src/codebase/ts_source/file_inventory.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use super::cmp_os_str_paths;
use std::path::{Path, PathBuf};
use std::sync::Arc;

Expand Down Expand Up @@ -26,9 +27,9 @@ pub(crate) struct ClassifiedPath {

/// Deterministic, immutable file identities for one request-scoped path set.
///
/// Paths are normalized lexically, sorted, and deduplicated. Filesystem
/// canonicalization is intentionally avoided so a symlink and its target keep
/// distinct logical identities.
/// Paths are normalized lexically, sorted by OsStr bytes, and deduplicated.
/// Filesystem canonicalization is intentionally avoided so a symlink and its
/// target keep distinct logical identities.
#[doc(hidden)]
pub struct FileInventory {
paths: Arc<Vec<PathBuf>>,
Expand All @@ -51,7 +52,7 @@ impl FileInventory {
mut entries: Vec<ClassifiedPath>,
metadata_stats: usize,
) -> Self {
entries.sort_by(|left, right| left.path.cmp(&right.path));
entries.sort_by(|left, right| cmp_os_str_paths(&left.path, &right.path));
entries.dedup_by(|left, right| left.path == right.path);

assert!(
Expand Down Expand Up @@ -121,7 +122,7 @@ impl FileInventory {
#[doc(hidden)]
pub fn id_for_normalized_path(&self, path: &Path) -> Option<FileId> {
self.paths
.binary_search_by(|candidate| candidate.as_path().cmp(path))
.binary_search_by(|candidate| cmp_os_str_paths(candidate, path))
.ok()
.map(|index| FileId(index as u32))
}
Expand Down
31 changes: 31 additions & 0 deletions crates/no-mistakes/src/codebase/ts_source/file_inventory/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,12 @@ fn fixture(path: &str) -> PathBuf {
)
}

fn os_str_membership_fixture(path: &str) -> PathBuf {
PathBuf::from(env!("CARGO_MANIFEST_DIR"))
.join("../../fixtures/ts-source/normalized-path-membership")
.join(path)
}

#[test]
fn identities_are_sorted_deduplicated_and_lexically_normalized() {
let alpha = fixture("alpha.ts");
Expand All @@ -33,6 +39,31 @@ fn identities_are_sorted_deduplicated_and_lexically_normalized() {
assert_eq!(inventory.path(FileId(u32::MAX)), None);
}

#[test]
fn normalized_inventory_probe_is_os_str_byte_identity() {
let nested = crate::codebase::ts_resolver::normalize_path(&os_str_membership_fixture("a/b.ts"));
let dashed = crate::codebase::ts_resolver::normalize_path(&os_str_membership_fixture("a-b.ts"));
let dotted = os_str_membership_fixture("a/./b.ts");
let collapsed = crate::codebase::ts_resolver::normalize_path(&dotted);

let inventory = FileInventory::from_paths(&[nested.clone(), dashed.clone()]);

// OsStr bytes put `a-b.ts` before `a/b.ts` (`-` < `/`). Path::cmp would
// reverse that because it compares the `a` component first.
assert_eq!(
inventory.paths().as_slice(),
[dashed.clone(), nested.clone()]
);
assert_eq!(nested.as_os_str(), collapsed.as_os_str());
assert_ne!(dotted.as_os_str(), nested.as_os_str());

let id = inventory.id_for_normalized_path(&nested).unwrap();
assert_eq!(inventory.path(id).unwrap().as_os_str(), nested.as_os_str());
assert_eq!(inventory.id_for_normalized_path(&dotted), None);
assert_eq!(inventory.id_for_path(&dotted), Some(id));
assert_eq!(inventory.id_for_normalized_path(&collapsed), Some(id));
}

#[test]
fn from_classified_paths_keeps_moved_paths() {
let later = PathBuf::from("src/b.ts");
Expand Down
2 changes: 2 additions & 0 deletions crates/no-mistakes/src/codebase/ts_source/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,15 @@ pub mod jsx;
mod file_id_map;
mod file_inventory;
mod parser_diagnostic;
mod path_order;
mod path_remapper;
mod source_store;
pub(crate) use file_id_map::{FileIdMap, FileIdMapIter, FileIdMapIterMut};
pub(crate) use file_inventory::ClassifiedPath;
#[doc(hidden)]
pub use file_inventory::{FileClassification, FileId, FileInventory};
pub(crate) use parser_diagnostic::format_parse_diagnostic;
pub(crate) use path_order::{cmp_os_str_paths, sort_os_str_paths};
pub(crate) use path_remapper::FrozenPathRemapper;
#[doc(hidden)]
pub use source_store::{JsonLoadError, SourceReadOutcome, SourceStore};
Expand Down
14 changes: 14 additions & 0 deletions crates/no-mistakes/src/codebase/ts_source/path_order.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
use std::cmp::Ordering;
use std::path::{Path, PathBuf};

/// Byte-identity path order used by inventory and visible-membership probes.
///
/// Callers must normalize before probing; this does not collapse `.` / `..`
/// and must not canonicalize.
pub(crate) fn cmp_os_str_paths(left: &Path, right: &Path) -> Ordering {
left.as_os_str().cmp(right.as_os_str())
}

pub(crate) fn sort_os_str_paths(paths: &mut [PathBuf]) {
paths.sort_by(|left, right| cmp_os_str_paths(left, right));
}
Original file line number Diff line number Diff line change
Expand Up @@ -350,3 +350,66 @@ fn visible_snapshot_treats_non_git_fallback_and_supplied_paths_as_authoritative(
vec![ignored, visible]
);
}

#[test]
fn from_paths_tracked_membership_is_os_str_byte_identity() {
let root = crate::codebase::ts_resolver::normalize_path(
&PathBuf::from(env!("CARGO_MANIFEST_DIR"))
.join("../../fixtures/ts-source/normalized-path-membership"),
);
let nested = root.join("a/b.ts");
let dashed = root.join("a-b.ts");
let dotted = root.join("a/./b.ts");
let collapsed = crate::codebase::ts_source::normalize_discovery_path(&dotted);
let snapshot = crate::codebase::ts_source::VisiblePathSnapshot::from_paths(
&root,
&[nested.clone(), dashed.clone()],
);

// OsStr bytes put `a-b.ts` before `a/b.ts` (`-` < `/`). Path::cmp would
// reverse that because it compares the `a` component first.
assert_eq!(
snapshot.tracked_paths_for(&root).as_slice(),
[dashed.clone(), nested.clone()]
);
assert_eq!(nested.as_os_str(), collapsed.as_os_str());
assert_ne!(dotted.as_os_str(), nested.as_os_str());
assert_eq!(
snapshot.tracked_paths_from(&[dashed.clone(), nested.clone()]),
[dashed.clone(), nested.clone()]
);
// `tracked_paths_from` normalizes first; the unnormalized spelling hits
// only after `normalize_discovery_path`, not via Path component collapse.
assert_eq!(
snapshot.tracked_paths_from(std::slice::from_ref(&dotted)),
[nested]
);
}

#[test]
fn visible_snapshot_contains_path_probes_os_str_bytes() {
let source = include_str!("../visible_snapshot.rs");
let start = source
.find("fn contains_path(")
.expect("contains_path must stay in visible_snapshot.rs");
let body = &source[start..];
let end = body.find("\nfn ").unwrap_or(body.len());
let contains_path = &body[..end];
assert!(
contains_path.contains("cmp_os_str_paths")
&& !contains_path.contains("as_path().cmp")
&& !contains_path.contains("canonicalize"),
"contains_path must probe OsStr bytes without Path::cmp or canonicalize"
);
let snapshot_view = source
.find("fn snapshot_path_view(")
.expect("snapshot_path_view must stay in visible_snapshot.rs");
let view_body = &source[snapshot_view..];
let view_end = view_body.find("\nfn ").unwrap_or(view_body.len());
let snapshot_path_view = &view_body[..view_end];
assert!(
snapshot_path_view.contains("sort_os_str_paths")
&& !snapshot_path_view.contains("tracked_paths.sort();"),
"discovery tracked_paths must sort with the same OsStr comparator"
);
}
5 changes: 3 additions & 2 deletions crates/no-mistakes/src/codebase/ts_source/visible_snapshot.rs
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ fn snapshot_path_view(
.into_iter()
.map(|path| normalize_discovery_path(&path))
.collect::<Vec<_>>();
tracked_paths.sort();
sort_os_str_paths(&mut tracked_paths);
tracked_paths.dedup();
Arc::new(SnapshotPathView {
sources: Arc::new(SourceStore::new_observed(
Expand All @@ -193,8 +193,9 @@ fn snapshot_path_view_from_paths(
}

fn contains_path(paths: &[PathBuf], path: &Path) -> bool {
// Exact OsStr membership. Canonical remapping does not belong here.
paths
.binary_search_by(|candidate| candidate.as_path().cmp(path))
.binary_search_by(|candidate| cmp_os_str_paths(candidate, path))
.is_ok()
}

Expand Down
1 change: 1 addition & 0 deletions fixtures/ts-source/normalized-path-membership/a-b.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export const dashed = "a-b";
1 change: 1 addition & 0 deletions fixtures/ts-source/normalized-path-membership/a/b.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export const nested = "a/b";
Loading