[ty] Teach the resolver that vendored copies of typeshed in the ty cache are in fact typeshed#21012
[ty] Teach the resolver that vendored copies of typeshed in the ty cache are in fact typeshed#21012
Conversation
Gankra
requested review from
MichaReiser,
dcreager and
sharkdp
as code owners
| pub fn vendored_path_for_cache(path: &VendoredPath) -> SystemPathBuf { | ||
| SystemPathBuf::from(format!( | ||
| "vendored/typeshed/{}/{}", | ||
| // The vendored files are uniquely identified by the source commit. | ||
| ty_vendored::SOURCE_COMMIT, | ||
| path.as_str() | ||
| )) | ||
| } | ||
|
|
||
| /// Get the full `SystemPath` in the cache that a `VendoredPath` refers to | ||
| pub(crate) fn cached_vendored_path(db: &dyn Db, path: &VendoredPath) -> Option<SystemPathBuf> { | ||
| let writable = db.system().as_writable()?; | ||
| let system_path = vendored_path_for_cache(path); | ||
| Some(writable.cache_dir()?.join(system_path)) | ||
| } |
There was a problem hiding this comment.
We invoke these a lot now, so they might need to be cached...
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
|
MichaReiser
left a comment
MichaReiser
left a comment
There was a problem hiding this comment.
Could we achive the same if the LSP registered an additional search path for the cached vendored files (that comes after the standard library?) That would require less custom casing for which we have not tests for.
Another alternative would be to remap cached vendored file path to the corresponding vendored path within the LSP request handlers.
Could any of those solution lead to problems if the content of the cached vendored file and the vendored file diverge (e.g. because the user starts to make edits?)
| } | ||
|
|
||
| /// Get the cache-relative `SystemPath` that a `VendoredPath` refers to | ||
| pub fn vendored_path_for_cache(path: &VendoredPath) -> SystemPathBuf { |
There was a problem hiding this comment.
Hmm. I'd prefer if we can keep this logic in ty_server, if possible because not all clients have a cache location or they use a different cache location (e.g. playground)
AlexWaygood
changed the title
The additional search-path still requires something like
Hmm that was concerning to me as an approach earlier but maybe it's not actually that bad? We already do that mapping in the other direction. I'll give it a try. |
This is an alternative to #21012 that more narrowly handles this logic in the stub-mapping machinery rather than pervasively allowing us to identify cached files as typeshed stubs. Much of the logic is the same (pulling the logic out of ty_server so it can be reused). I don't have a good sense for if one approach is "better" or "worse" in terms of like, semantics and Weird Bugs that this can cause. This one is just "less spooky in its broad consequences" and "less muddying of separation of concerns" and puts the extra logic on a much colder path. I won't be surprised if one day the previous implementation needs to be revisited for its more sweeping effects but for now this is good. Fixes astral-sh/ty#1054
2 participants
Fixes astral-sh/ty#1054
I've confirmed it works on my machine in vscode, but it's not clear to me if there's a good way to test this in our test suite.