feat: Complete diagnostics in ty lowering groundwork and serve a first diagnostic 🎉 #18541
Conversation
rustbot
added
the
S-waiting-on-review
ChayimFriedman2
changed the title
20mb seems fine for this, also the new salsa will have a feature called accumulators which will likely improve this (as its basically a concept exactly doing what the diagnostic query duplication is for). Although we could make the non-diagnostic version of the queries here transparent, then they will not allocate a cache for themselves and instead just call out their function directly. I think we do this for the other queries that have diagnostics like the |
Veykril
left a comment
Veykril
left a comment
There was a problem hiding this comment.
Very nice to see ❤️ This unlocks some very useful diagnostics!
| ForLifetime(Box<[Name]>, PathId), | ||
| Lifetime(LifetimeRef), | ||
| Use(Box<[UseArgRef]>), |
There was a problem hiding this comment.
Could shrink it by another 8 bytes now if we made these wide boxes thin boxes, something to keep in mind
| &TypeBound::Path(path, _) => Some( | ||
| *data.types_map[path] | ||
| .segments() | ||
| .iter() | ||
| .last() | ||
| .unwrap() | ||
| .args_and_bindings | ||
| .unwrap() | ||
| .bindings[0] | ||
| .type_ref | ||
| .as_ref() | ||
| .unwrap(), |
There was a problem hiding this comment.
Whew that's an ugly unwrap chain (not that this PR changed it, it was ugly before)
| generic_args: self.generic_args.map(|it| it.get(..len).unwrap_or(it)), | ||
| } | ||
| } | ||
| pub fn strip_last(&self) -> PathSegments<'a> { |
There was a problem hiding this comment.
Was wondering what this kind of function is usually called, given everything but the head is called usually tail. Haskell calls everything but the last element init, not sure I like that though
|
|
||
| use crate::{AssocItem, Field, Local, Trait, Type}; | ||
|
|
||
| pub use hir_def::VariantId; |
There was a problem hiding this comment.
Let's re-export this from the lib.rs file instead if we have to its not really diagnostics related, though ideally we aren't re-exporting this out of hir at all
There was a problem hiding this comment.
It was here previously, I just shuffled the imports to organize them.
|
☔ The latest upstream changes (presumably #18609) made this pull request unmergeable. Please resolve the merge conflicts. |
Most paths are types and therefore already are in the source map, but the trait in impl trait and in bounds are not. We do this by storing them basically as `TypeRef`s. For convenience, I created a wrapper around `TypeRefId` called `PathId` that always stores a path, and implemented indexing from the types map to it. Fortunately, this change impacts memory usage negligibly (adds 2mb to `analysis-stats .`, but that could be just fluff). Probably because there aren't that many trait bounds and impl traits, and this also shrinks `TypeBound` by 8 bytes. I also added an accessor to `TypesSourceMap` to get the source code, which will be needed for diagnostics.
…rst diagnostic The diagnostic implemented is a simple one (E0109). It serves as a test for the new foundation. This commit only implements diagnostics for type in bodies and body-carrying signatures; the next commit will include diagnostics in the rest of the things. Also fix one weird bug that was detected when implementing this that caused `Fn::(A, B) -> C` (which is a valid, if bizarre, alternative syntax to `Fn(A, B) -> C` to lower incorrectly. And also fix a maybe-bug where parentheses were sneaked into a code string needlessly; this was not detected until now because the parentheses were removed (by the make-AST family API), but with a change in this commit they are now inserted. So fix that too.
Implement diagnostics in all places left: generics (predicates, defaults, const params' types), fields, and type aliases. Unfortunately this results in a 20mb addition in `analysis-stats .` due to many type methods returning an addition diagnostics result now (even if it's `None` in most cases). I'm not sure if this can be improved. An alternative strategy that can prevent the memory usage growth is to never produce diagnostics in hir-ty methods. Instead, lower all types in the hir crate when computing diagnostics from scratch (with diagnostics this time). But this has two serious disadvantages: 1. This can cause code duplication (although it can probably be not that bad, it will still mean a lot more code). 2. I believe we eventually want to compute diagnostics for the *entire* workspace (either on-type or on-save or something alike), so users can know when they have diagnostics even in inactive files. Choosing this approach will mean we lose all precomputed salsa queries. For one file this is fine, for the whole workspace this will be very slow.
ChayimFriedman2
force-pushed
the
different-generic-args
branch
from
4cca548 to
21ad3b5
Compare
4 participants
The first diagnostic is a simple one (E0109), but this enable more complicated diagnostics. So: Yay!
Best reviewed commit-by-commit. The commit messages are also important.