-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Open
Labels
C-ArchitectureBig architectural things which we need to figure up-front (or suggestions for rewrites :0) )Big architectural things which we need to figure up-front (or suggestions for rewrites :0) )C-featureCategory: feature requestCategory: feature requestE-hardS-actionableSomeone could pick this issue up and work on it right nowSomeone could pick this issue up and work on it right now
Description
Currently const params are not lowered at all:
https://github.com/rust-analyzer/rust-analyzer/blob/aa91a0268bae6deda964a9fdfcbdbd2d8ca5802f/crates/hir_def/src/path/lower.rs#L177-L178
These kind of define bodies, so they come with all the goodies(and problems like local items) that other items like functions, statics and consts come with, and just like those they should ideally be lowered lazily.
See corresponding zulip discussion https://rust-lang.zulipchat.com/#narrow/stream/185405-t-compiler.2Fwg-rls-2.2E0/topic/ConstArg.20lowering
FeldrinH, Nufflee, theoparis and Logarithmus
Metadata
Metadata
Assignees
Labels
C-ArchitectureBig architectural things which we need to figure up-front (or suggestions for rewrites :0) )Big architectural things which we need to figure up-front (or suggestions for rewrites :0) )C-featureCategory: feature requestCategory: feature requestE-hardS-actionableSomeone could pick this issue up and work on it right nowSomeone could pick this issue up and work on it right now
Type
Projects
Milestone
Relationships
Development
Select code repository
Activity
dickytamara commentedon Jan 27, 2021
Merge #11140
Merge #11688
HKalbasi commentedon Mar 17, 2022
If each constant needs a body for its own, locating them/allocating id for them will be hard. My suggestion is generalizing body to an expression store, and include expressions of constants in generics in the related definition's body. So for example:
all of
2 + 2
would getExprId
from function's body. Similarly, expressions inside const and static type annotations will be included inside of their bodies. And in this way, type alias will become aDefWithBody
so it can lower and evaluate expressions. Body of a type alias has no meaningful root, so we may want to splitBody
into two abstractions, one withbody_expr
andparams
and one with the rest of fields (the expression allocator part).Comments?
flodiebold commentedon Mar 17, 2022
I think having a common expression store for all constants probably makes sense, but maybe not making that the body? How about we introduce something like an
ExpressionStore
and haveBody
contain one of those (basically what you're suggesting in the last sentence). SoTypeAlias
wouldn't be aDefWithBody
.16 remaining items
Auto merge of #14892 - HKalbasi:mir, r=HKalbasi
Auto merge of #14932 - HKalbasi:dev, r=HKalbasi
Veykril commentedon Dec 14, 2023
To summarize the current state of things, we have not taken the approach of using an expression store, but instead handling const args as separate bodies (called
InTypeConst
s), added in #14932. The implementation currently has some problems forcing us to basically having it disabled as it stands, namely storing the expected type of such a const arg and giving it an ID that is stable enough across random edits.We currently store the type (as a trait object due to the type being defined in hir-ty, while the container is defined in hir-def, a dependency of hir-ty) inside the interned location of the const arg. Interned things are supposed to be light weight though, and what's worse about this is if the expected type changes in some minor way, the hash changes meaning that the interned version will get a new slot allocated (without cleaning up the rpevious one, this is a salsa downside atm). It also just does not fit there from a architecture PoV.
Ideally we would have a query that calculates the expected type of a general const arg that could be used here then.
This is the main reason why the current implementation is not usable, any edit across the file can invalidate all const args in the file currently as we merely use the const args AstId which becomes more unstable the deeper in the tree we go. Ideally this should be an ID relative to the owner of the const arg.
The expression store approach on the other hand trivially solves the stable ID issue (it would be relative to the store, like expression IDs are). It would also make const arg inference a bit less fine grained, instead of doing every const arg separately, we'd now be able to do it per store which I think is a general perf win, as body inference is already the finest inference unit we have anyways.
Unfortunately though, for current bodies we would have to split the expression store into two, one for the signature and one for the actual body, as otherwise we would lose lazyness when only being interested in the signature of an item, so this approach could have a noticable memory impact.
Regarding the expected type calculation, the query approach would be ideal here as well still, and I think more generally doable as t he expression store should already hold the required information for finding the relevant parts that are needed to lower the expected type of the AstId.
Generally I think I'd still be in favor of the expression store solution, as it seems like the robuster (albeit possibly more effort-requiring) way to implement this.
So with that said, the first step would be to extract the
ExpressionStore
part out ofBody
andBodySourceMap
, and redefineBody
andBodySourceMap
on top of this new construct. Then we should start buildingExpressionStore
s for relevant item signature and finally move theInTypeConst
architecture over to this newExpressionStore
construct.unresolvedReference
as their semantic type #18236Veykril commentedon Jan 25, 2025
I've started looking into this again, and noticed some things.
As it turns out, this requires a bigger refactor. Basically we want to store
ExprId
in ourTypeRef
IR which is currently included in theItemTree
which is pre-expansion where as expression collection is post-expansion. Now, theItemTree
containingTypeRef
s doesn't actually make sense in the first place. TheItemTree
is supposed to be an incrementality later for theDefMap
(our early name resolution), but types do not do anything on that level, so right now editing a parameter type will invalidate our early name resolution when it actually shouldn't! So the idea here is now to kick out everything fromItemTree
that is not needed by theDefMap
and create a new thing (ExpressionStore
) that will serve as the incrementality layer for type holding items. This is a very rought explanation, the overall change is rather big as this touches upon a lot of core IRs.ExpressionStore
fromBody
#19036Veykril commentedon Apr 9, 2025
#19462 has landed, doing the required refactors that should unblock this now