Make the cache work with rust-analyzer #23
Conversation
| // In a rustc invocation, there will only ever be one entry in this map, since every crate is | ||
| // compiled with its own rustc process. However, the same is not (currently) the case for | ||
| // rust-analyzer. | ||
| type Cache = BTreeMap<String, CacheEntry>; |
There was a problem hiding this comment.
Style suggestion: I usually avoid type aliases like this, because, while they might make the code slightly more dense, this comes at the cost of an extra indirection that makes it harder to understand the underlying types.
Why not use a HashMap instead of a BTreeMap here?
There was a problem hiding this comment.
Hashing is useless in the rustc use case, and anything proc-macro related carries the risk of screwing with rust's caches if it uses non-deterministic things like a hash map. I know the non-deterinistic iteration doesn't actually surface here, but I consider it a good rule of thumb to just stay away from Hash(Map|Set) in proc-macro crates entirely.
There was a problem hiding this comment.
Re. having a type alias, here it provides a nice place to put the comment about rust-analyzer though, which is unrelated to the comment above the CACHE declaration and I thus prefer to be separate.
|
|
||
| let manifest = open_cargo_toml(&manifest_path)?; | ||
| let crate_names = extract_crate_names(&manifest)?; | ||
| let crate_names = match cache.entry(manifest_dir) { |
There was a problem hiding this comment.
Style nit: you could yield cache_entry from the match expression directly, deduplicating the .crate_names field access.
There was a problem hiding this comment.
I find the code more readable this way.
|
|
||
| Ok(CacheEntry { | ||
| manifest_ts, | ||
| crate_names, |
There was a problem hiding this comment.
Style nit: could directly assign the crate_names field instead of binding it separately.
There was a problem hiding this comment.
Same thing, I think the separate binding makes it easier to read. Would also be shorter if rustfmt would put this struct literal on one line, which it unfortunately doesn't do in the default configuration.
Fixes #22.