[ty] Split ScopedPlaceId into ScopedSymbolId and ScopedMemberId#19497
[ty] Split ScopedPlaceId into ScopedSymbolId and ScopedMemberId#19497MichaReiser merged 17 commits intomainfrom
ScopedPlaceId into ScopedSymbolId and ScopedMemberId#19497Conversation
4d1afec to
3616804
Compare
a610975 to
2ed78c7
Compare
|
This comment was marked as resolved.
This comment was marked as resolved.
|
There was a problem hiding this comment.
I'll write some more docs if we decide to go forward with this PR. The current docs are carried over from PlaceExpr
|
|
||
| /// Reference to a node that introduces a new scope. | ||
| #[derive(Copy, Clone, Debug)] | ||
| pub(crate) enum NodeWithScopeRef<'a> { |
There was a problem hiding this comment.
I moved everything Scope to scope.rs. That code doesn't need to belong into place.
| symbol_name: Name, | ||
| segments: SmallVec<[MemberSegment; 1]>, |
There was a problem hiding this comment.
I think we can optimize this further by:
- Storing a single
Namewhich is the entire path of the member - In the
SmallVec: Store the kind of each segment and where it starts as au32.
This doesn't change the size of Member, but it reduces the size of the stored segments because it doesn't require a length and capacity for each segment.
This should be sufficient because all we need is a unique key to hash, knowing if it has the form a.b, and a Display implementation
carljm
left a comment
There was a problem hiding this comment.
I think this is a strong improvement in clarity; if it's neutral in performance, I'm +1 to go ahead with it. Thank you!
| if self.is_method_of_class().is_some() { | ||
| if let PlaceExpr::Member(member) = &mut place_expr { | ||
| if member.is_instance_attribute_candidate() { |
There was a problem hiding this comment.
nit: it might be useful to reduce the nesting here by chaining the calls
There was a problem hiding this comment.
It's not clear to me how I would do this without let chains
cb7bc75 to
a56bb11
Compare
sharkdp
left a comment
There was a problem hiding this comment.
Thank you!
A minor naming quibble: "member" refers to something like self.x (which seems consistent with how we use that term elsewhere, as an attribute on something), but also to a subscript expression like xs[0]. But no need to change anything.
That's fair. Not sure what else to use. I considered |
4abc97b to
f14697f
Compare
e391e8b to
452d7a2
Compare
|
A possible alternative is |
|
Yes, the naming issue occurred to me in review also, but I didn't mention it because I didn't have any concrete suggestion that I liked. I don't know of a term that clearly means "either attribute or subscript", and "member" (borrowed from JS) seems not unreasonable (though I would also initially assume it to mean just an attribute). The clearest options would be the verbose ones: |
subscribute 😁 |
Summary
This PR changes the place structs in the semantic index module to enums over a symbol or a member:
ScopedPlaceId: Is now an enum overScopedSymbolIdandScopedMemberIdPlaceExpr: Is now an enum overSymbolandMemberPlaceExprRef: Is now an enum over&Symboland&MemberPlaceTable: now separately tracks symbols and membersThis idea was initially mentioned in #19470. The hope was that splitting
SymbolandMemberwould help to reduce memory usage becauseSymbolis only 32 bytes (compared toPlaceExpr, which is 40 bytes).However, the memory usage is roughly unchanged after running ty on a large repository (the saving must be less than 100MB if there's even any). Splitting symbol and members also introduces some extra memory overhead because all
IndexVecoverScopedPlaceIdnow needs to be two separate vectors.Performance also shows to be mostly neutral.
The main benefit I now see in this refactor is that it clarifies some usages of places. Before, it was often unclear whether some code iterates over symbols or members or both. In fact, I spent the majority of my time during this refactor on trying to understand which of the two it is. The split also helped me better understand which flags are only supported on flags/members and which flags are supported by both.
Now, whether this distinction makes sense long term heavily depends on whether we expect that many places that currently are only over symbols will need to handle members the same way in the future.
Overall: I don't have a strong opinion on this. I do think it helps clarify some code but it does come at a cost.
Code increase
This PR adds a fair amount of new code. However, most of it is just the boilerplate from having separate
Symbol/Member,SymbolTable/MemberTable/PlaceTableandSymbolTableBuilder/MemberTableBuilder/PlaceTableBuilder. Most of that code is fairly simple (many getters)