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
red-knot[salsa part 2]: Setup semantic DB and Jar #11837
red-knot[salsa part 2]: Setup semantic DB and Jar #11837
Changes from 4 commits
7083f2f
64d906c
7937e95
b299766
c516c4a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
It's surprising to me that we have to manually update the revision here; should
write_file
do that itself?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.
The
FileSystem
that I have in mind should be independent of thevfs
and salsa db. It's just an abstraction overstd::fs
. What I have in mind to avoid theset_revision
call here is that we have anapply_changes
method where we pass all file changes (adds, remove, modified) that updates the vfs state. It's probably theFileSystem'
s or the host's responsibility to collect all made changes.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.
That makes sense, and would address my concern: I don't want to expose/use APIs that make it easy to change a file's contents but forget to bump its revision.
I think this also gets back to what I find confusing about the
Vfs
andFileSystem
structure, which is that there is no single entity which really encapsulates our virtual file system. Where does thisapply_changes
method live? I guess it is just a free function that takes aDb
and some changes to apply?Probably this is just something I have to get used to with Salsa, that the
Db
owns all the state, so most API that we use for managing state is just a function that takes aDb
.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.
It would probably be a free standing function that takes
&mut Db
. And yes, the fact that it is a free standing function takes some time to get used to. An alternative is to make it a method inVfs
, but I think that won't work because it would require holding a mutable reference toDb
and a read only reference toFileSystem
.yes, that takes some time to get used to. It's also something I'm still figuring out