-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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] Support files outside of any workspace #13041
Conversation
65b6671
to
f5eac02
Compare
let db = match session.workspace_db_for_path(path.as_std_path()) { | ||
Some(db) => db.snapshot(), | ||
None => session.default_workspace_db().snapshot(), | ||
}; |
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.
I want this entire logic in the Session
but the borrow checker always complains when I do something like:
self.workspaces
.range_mut(..=path.as_ref().to_path_buf())
.next_back()
.map(|(_, db)| db)
.unwrap_or_else(|| self.default_workspace_db_mut())
There are two mutable references:
error[E0500]: closure requires unique access to `*self` but it is already borrowed
--> crates/red_knot_server/src/session.rs:104:29
|
97 | &mut self,
| - let's call the lifetime of this reference `'1`
...
100 | self.workspaces
| ---------------
| |
| _________borrow occurs here
| |
101 | | .range_mut(..=path.as_ref().to_path_buf())
102 | | .next_back()
103 | | .map(|(_, db)| db)
104 | | .unwrap_or_else(|| self.default_workspace_db_mut())
| |_____________________________^^_----___________________________- returning this value requires that `self.workspaces` is borrowed for `'1`
| | |
| | second borrow occurs due to use of `*self` in closure
| closure construction occurs here
For more information about this error, try `rustc --explain E0500`.
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.
This seems to work
pub(crate) fn workspace_db_for_path_or_default(&self, path: impl AsRef<Path>) -> &RootDatabase {
self.workspace_db_for_path(path)
.unwrap_or_else(|| self.default_workspace_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.
This also compiles fine for me...
pub(crate) fn workspace_db_for_path(&self, path: impl AsRef<Path>) -> &RootDatabase {
self.workspaces
.range(..=path.as_ref().to_path_buf())
.next_back()
.map(|(_, db)| db)
.unwrap_or_else(|| self.default_workspace_db())
}
Not sure if I'm doing something wrong. But I think you face the problem with workspace_db_for_path_mut
?
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.
But yeah, not sure why the mut
case doesn't work. Sounds like a borrow checker limitation? Maybe @BurntSushi has an idea
I tried to hint rust as best as I can but it just won't believe that we only return one mutable borrow
let workspace = self
.workspaces
.range_mut(..=path.as_ref().to_path_buf())
.next_back();
if let Some((_, db)) = workspace {
return db;
}
drop(workspace);
return self.workspaces.values_mut().next().unwrap();
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.
Not sure if I'm doing something wrong. But I think you face the problem with
workspace_db_for_path_mut
?
Yes, it's for the mutable version.
I tried to with as many hints as possible but it just won't believe that we only return one mutable borrow
Yeah, I tried a couple of them myself, all raised the same error
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.
Nothing is immediately popping out at me here. This might be something that is fixed by Polonius. Specifically, this might be the problem you're hitting? https://blog.rust-lang.org/inside-rust/2023/10/06/polonius-update.html#background-on-polonius
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.
Thanks. Yes, that might be it.
An alternative solution could be to accept a closure that applies the mutation to the db
. Not as nice but hopefully that works?
Otherwise transmute your way to happiness 😂
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.
An alternative solution could be to accept a closure that applies the mutation to the
db
. Not as nice but hopefully that works?
Huh, this works but it gets a little bit complicated because we need to move the path
to the ChangedEvent
(and also that the path could be system or virtual). I'll keep the existing logic for now with a TODO to link to this discussion.
let line = line.parse::<u32>().unwrap_or_default(); | ||
let line = line.parse::<u32>().unwrap_or_default().saturating_sub(1); | ||
let column = column.parse::<u32>().unwrap_or_default(); | ||
( | ||
Range::new( | ||
Position::new(line.saturating_sub(1), column.saturating_sub(1)), | ||
Position::new(line, column.saturating_sub(1)), |
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.
This is just to make sure the diagnostics are highlighted for at most one character. Without this, it would spill over to the next line.
|
f5eac02
to
bfa9e8e
Compare
bfa9e8e
to
028cb68
Compare
Some(db) => db, | ||
None => session.default_workspace_db_mut(), | ||
}; | ||
db.apply_changes(vec![ChangeEvent::file_created(path)], None); |
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.
I'm not sure if file_created
is the correct event here. The analyzer might already have seen the file when traversing the file system. I think it is only virtual files which would be "created".
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, I think that's correct. I think we might not need to emit any event here.
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.
Or maybe it's required when publishing diagnostics for clients that doesn't support pull diagnostics. I'm thinking of adding a new event kind like Opened(SystemPathBuf)
.
let db = session | ||
.workspace_db_for_path(path.as_std_path()) | ||
.map(RootDatabase::snapshot); | ||
let db = match session.workspace_db_for_path(path.as_std_path()) { |
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: We may want to consider using system path in the LSP as well to remove the back and forth between different types
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, I think that should be done after adding virtual file support because then there will be two paths.
let db = match session.workspace_db_for_path(path.as_std_path()) { | ||
Some(db) => db.snapshot(), | ||
None => session.default_workspace_db().snapshot(), | ||
}; |
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.
This seems to work
pub(crate) fn workspace_db_for_path_or_default(&self, path: impl AsRef<Path>) -> &RootDatabase {
self.workspace_db_for_path(path)
.unwrap_or_else(|| self.default_workspace_db())
}
let db = match session.workspace_db_for_path(path.as_std_path()) { | ||
Some(db) => db.snapshot(), | ||
None => session.default_workspace_db().snapshot(), | ||
}; |
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.
This also compiles fine for me...
pub(crate) fn workspace_db_for_path(&self, path: impl AsRef<Path>) -> &RootDatabase {
self.workspaces
.range(..=path.as_ref().to_path_buf())
.next_back()
.map(|(_, db)| db)
.unwrap_or_else(|| self.default_workspace_db())
}
Not sure if I'm doing something wrong. But I think you face the problem with workspace_db_for_path_mut
?
let db = match session.workspace_db_for_path(path.as_std_path()) { | ||
Some(db) => db.snapshot(), | ||
None => session.default_workspace_db().snapshot(), | ||
}; |
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.
But yeah, not sure why the mut
case doesn't work. Sounds like a borrow checker limitation? Maybe @BurntSushi has an idea
I tried to hint rust as best as I can but it just won't believe that we only return one mutable borrow
let workspace = self
.workspaces
.range_mut(..=path.as_ref().to_path_buf())
.next_back();
if let Some((_, db)) = workspace {
return db;
}
drop(workspace);
return self.workspaces.values_mut().next().unwrap();
028cb68
to
0a37fad
Compare
Summary
This PR adds basic support for files outside of any workspace in the red knot server.
This also limits the red knot server to only work in a single workspace. The server will not start if there are multiple workspaces.
Test Plan
Screen.Recording.2024-08-22.at.16.26.18.mov