Skip to content

[ty] Do not crash if a FileRoot is a symlink #21052

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed

[ty] Do not crash if a FileRoot is a symlink #21052

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b96e1b9
Do not crash if a FileRoot is a symlink
Gankra Oct 23, 2025
5473636
fix latent bug in MdtestSystem::canonicalize_path
Gankra Oct 23, 2025
8e12163
more narrow
Gankra Oct 31, 2025
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
6 changes: 5 additions & 1 deletion crates/ruff_db/src/files.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,7 @@ impl Files {
let roots = self.inner.roots.read().unwrap();

let absolute = SystemPath::absolute(path, db.system().current_directory());

roots.at(&absolute)
}

Expand All @@ -211,7 +212,10 @@ impl Files {
let mut roots = self.inner.roots.write().unwrap();

let absolute = SystemPath::absolute(path, db.system().current_directory());
roots.try_add(db, absolute, kind)
// We need to resolve away symlinks here to avoid getting confused about subdirectories.
let canonicalized = db.system().canonicalize_path(&absolute).unwrap_or(absolute);

roots.try_add(db, canonicalized, kind)
}

/// Updates the revision of the root for `path`.
Expand Down
19 changes: 13 additions & 6 deletions crates/ty_test/src/db.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use camino::{Utf8Component, Utf8PathBuf};
use camino::{Utf8Component, Utf8PathBuf, Utf8Prefix};
use ruff_db::Db as SourceDb;
use ruff_db::diagnostic::Severity;
use ruff_db::files::{File, Files};
Expand Down Expand Up @@ -180,11 +180,18 @@ impl System for MdtestSystem {
.canonicalize_path(&self.normalize_path(path))?;

if let MdtestSystemInner::Os { os_system, .. } = &*self.0 {
// Make the path relative to the current directory
Ok(canonicalized
.strip_prefix(os_system.current_directory())
.unwrap()
.to_owned())
// Make the path relative to the current directory if the path doesn't require
// UNC gunk (`//?/`) to be valid (`strip_prefix` gets really messy otherwise).
if let Some(Utf8Component::Prefix(prefix)) = canonicalized.components().next()
&& let Utf8Prefix::VerbatimDisk(_) = prefix.kind()
{
Ok(canonicalized)
} else {
Ok(canonicalized
.strip_prefix(os_system.current_directory())
.unwrap()
.to_owned())
}
Comment on lines +183 to +194
Copy link
Contributor Author

@Gankra Gankra Oct 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a "Hillarious" interaction between strip_prefix and dunce.

dunce removes "unnecessary" UNC prefixes, but the import_basic mdtest actually passes in a really really really long path, which dunce then goes "ah UNC is needed".

This results in canonicalized.strip_prefix(os_system.current_directory()) failing.

If you then go "ah I will simply canonicalize os_system.current_directory()" you still lose because dunce is very helpful and removes the unnecessary UNC prefix from the much shorter cwd!

} else {
Ok(canonicalized)
}
Expand Down
Loading