Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
salsa 3.0 #490
salsa 3.0 #490
Changes from all commits
225a81a
4533cd9
e24ace2
a320781
20cb307
ea1d452
79d24e0
5ce5e3c
b6311d8
cb1a2bb
6e2647f
b050bd8
44a8a2f
e95c8b2
a84777d
fe4ff98
04e041b
4f74037
8ba6e60
54c9586
97fc6a0
3441666
d190beb
4822013
d6d5226
af94b25
d92f2aa
5095d79
0b8c27b
9d8a60b
9607638
d361e8a
8d0f8fc
cf2fa67
ab70786
7519c3e
b4b49fb
56030df
06b7097
2800076
d98485d
4f4d019
b005820
1560634
68502ab
ce88a8f
a7b2805
81942f3
8c51f37
07d0ead
88b964d
0ad0be8
b9ab8fc
ce750da
5326683
f91eeb9
c02f30a
af2c973
bcad24c
ab9aa3a
1544ee9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the addition of the
db
lifetime also allow queries to return data that reference the DB?One use case that we have is that we need a mapping from
AstNode
->Id
whereId
for example uniquely identifie's a scope, or a symbol in the program.The challenge we're facing is that our
Ast
doesn't useArc
s internally, thus cloning aNode
always clones the entire sub-tree. Our "work-around" for this is to keep hold to the AST's root (wrapped in an Arc) and store a raw pointer referencing the actual node. This works pretty well, but requires heavy use ofArc
s (a lot of cloning). I "think" your changes would allow us to directly store a&'db Expr
instead.If not, then the "work-around" would just be to make the
AstNode
->Id
map a salsa tracked so that we get access to the db lifetimeThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did intend to support storing
&T
references like that, but it's a subtle case, and I've gone back and forth on whether it works with the stacked borrows rules etc.Suppose you do
let f = ast.field(db)
in R1 and it yields a&'db Foo
(reference to some field ofast
) and then you store that in the database as the result of a query (or part of the result). Now say we start a new revision R2 and, in R2, the value offield
changes. This means thatf
(considered as a pointer) still points to the same memory, but the value behindf
has changed. There are two challenges: (a) under the stacked borrow rules, it is UB to usef
again; (b) should we consider functions that were dependent onf
as needing to be re-executed?I've tried to write an exploration of this question in this comment like 3 times but it keeps getting unwieldy. I think I will defer it for documentation or in-person discussion, it's a good one. I'm not entirely sure if and under what conditions this can be made to be safe at the moment. =)
That said, I also want to mention a feature I've been considering that I think is may help with your use case. The idea would be to make it easy to have a value that carries a memory arena and references into that arena. This is meant to model things like MIR, where we have some data structure that represents a function, and to allow it to go through phases where it is changed and updated, but without requiring everything to be in vectors nor requiring everything to be cloned constantly. I'm not sure the ergonomics exactly but the idea is roughly that you can declare a struct with two lifetimes...
...and the procedural macro will create a type
AstRoot
that "hides" the first one:Later you can do
root.open(|r| { .. })
to work with the data. One of the goals is that you can create new, derived values based on the same arena that have different pointers -- so e.g. it should be possible to extra subvalues from the tree. Each of them would carry a reference count to the same base arena.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that sounds very similar to our "work-around", except that it is more flexible and the unsafety is handled by salsa instead of sprinkled through our code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I'm not sure if that's mentioned above. But I think that's because the operations require a
&'db mut Db
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes