[ty] Do not crash if a FileRoot is a symlink #21052
Conversation
Gankra
requested review from
MichaReiser,
carljm,
dcreager and
sharkdp
as code owners
Gankra
changed the title
Gankra
added
ty
crates/ruff_db/src/files.rs
Outdated
| let absolute = SystemPath::absolute(path, db.system().current_directory()); | ||
| roots.at(&absolute) | ||
| // We need to resolve away symlinks here to avoid getting confused about subdirectories. | ||
| let canonicalized = db.system().canonicalize_path(&absolute).unwrap_or(absolute); |
There was a problem hiding this comment.
On paper the first absolute is redundant but I'm not sure if db.system().current_directory() is able to be not-the-process-level-current-directory that canonicalize_path uses.
|
CodSpeed Performance ReportMerging #21052 will not alter performanceComparing Summary
|
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
|
| // 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()) | ||
| } |
There was a problem hiding this comment.
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!
|
I'd very much prefer not to call |
Do you mean you want this logic moved into the routine it's calling (FileRoot::try_add, FileRoot::at)? |
|
Ideally, we'd canonicalize the path where we create it before adding it as a search path (or site package path or whatever it is). This should then also ensure that we only create paths that use the canonicalized representation. But I don't understand the specific problem enough to say whether that's possible |
|
Bafflingly I can't reproduce this anymore so I'm just gonna close this |
|
Ok micha got a reproducer. If you go in vscode and via the pallete select "Python Interpretter: /opt/homebrew/bin/python3" the crash occurs: On my system the path we select is a fractal symlink:
Path 4 is a real file. If you pass 1 or 2 to The actual crash involves two other dirs in a symlink relationship:
Path 6 is an actual directory. We also see that path 6 is, weirdly, what you would discover if you searched for site-packages from path 2, which is wild because path 2 was a bloody symlink! So when handed path 3 or 4 as Then at some later point we end up with a copy of the same path but with the symlink resolved (path 6), and we freak out because the two paths aren't lexically nested. |
|
Ok so the "root cause" is this canonicalize (and the absence of a paired canonicalize for adding a root): ruff/crates/ty_python_semantic/src/module_resolver/resolver.rs Lines 451 to 464 in b93d8f2 |
|
An unfortunate game of whackamole if you don't want to unconditionally canonicalize all paths. |
Alternative implementation to #21052
|
Obsolete |
This is a fix for the issue described here, where ty found my homebrew python site-packages, and then panicked because it was handling both symlink and non-symlink versions of the same directory (which, syntactically, are not subdirectories of eachother).
I am always suspicious of using canonicalize but it's not clear to me there's a way to avoid it here.