-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Merged
Merged
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
7083f2f
Add Semantic DB, Jar, and TestDb
MichaReiser 64d906c
Add red_knot feature
MichaReiser 7937e95
Fix documentation
MichaReiser b299766
change `events` to `take_salsa_events` and `clear_salsa_events`
MichaReiser c516c4a
Update crates/ruff_db/src/lib.rs
MichaReiser File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,128 @@ | ||
use std::ops::Deref; | ||
use std::sync::Arc; | ||
|
||
use ruff_source_file::LineIndex; | ||
|
||
use crate::vfs::VfsFile; | ||
use crate::Db; | ||
|
||
/// Reads the content of file. | ||
#[salsa::tracked] | ||
pub fn source_text(db: &dyn Db, file: VfsFile) -> SourceText { | ||
let content = file.read(db); | ||
|
||
SourceText { | ||
inner: Arc::from(content), | ||
} | ||
} | ||
|
||
/// Computes the [`LineIndex`] for `file`. | ||
#[salsa::tracked] | ||
pub fn line_index(db: &dyn Db, file: VfsFile) -> LineIndex { | ||
let source = source_text(db, file); | ||
|
||
LineIndex::from_source_text(&source) | ||
} | ||
|
||
/// The source text of a [`VfsFile`]. | ||
/// | ||
/// Cheap cloneable in `O(1)`. | ||
#[derive(Clone, Eq, PartialEq)] | ||
pub struct SourceText { | ||
inner: Arc<str>, | ||
} | ||
|
||
impl SourceText { | ||
pub fn as_str(&self) -> &str { | ||
&self.inner | ||
} | ||
} | ||
|
||
impl Deref for SourceText { | ||
type Target = str; | ||
|
||
fn deref(&self) -> &str { | ||
self.as_str() | ||
} | ||
} | ||
|
||
impl std::fmt::Debug for SourceText { | ||
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | ||
f.debug_tuple("SourceText").field(&self.inner).finish() | ||
} | ||
} | ||
|
||
#[cfg(test)] | ||
mod tests { | ||
use filetime::FileTime; | ||
use salsa::EventKind; | ||
|
||
use ruff_source_file::OneIndexed; | ||
use ruff_text_size::TextSize; | ||
|
||
use crate::file_system::FileSystemPath; | ||
use crate::source::{line_index, source_text}; | ||
use crate::tests::TestDb; | ||
use crate::Db; | ||
|
||
#[test] | ||
fn re_runs_query_when_file_revision_changes() { | ||
let mut db = TestDb::new(); | ||
let path = FileSystemPath::new("test.py"); | ||
|
||
db.file_system_mut().write_file(path, "x = 10".to_string()); | ||
|
||
let file = db.file(path); | ||
|
||
assert_eq!(&*source_text(&db, file), "x = 10"); | ||
|
||
db.file_system_mut().write_file(path, "x = 20".to_string()); | ||
file.set_revision(&mut db).to(FileTime::now().into()); | ||
|
||
assert_eq!(&*source_text(&db, file), "x = 20"); | ||
} | ||
|
||
#[test] | ||
fn text_is_cached_if_revision_is_unchanged() { | ||
let mut db = TestDb::new(); | ||
let path = FileSystemPath::new("test.py"); | ||
|
||
db.file_system_mut().write_file(path, "x = 10".to_string()); | ||
|
||
let file = db.file(path); | ||
|
||
assert_eq!(&*source_text(&db, file), "x = 10"); | ||
|
||
// Change the file permission only | ||
file.set_permissions(&mut db).to(Some(0o777)); | ||
|
||
db.events().lock().unwrap().clear(); | ||
assert_eq!(&*source_text(&db, file), "x = 10"); | ||
|
||
let events = db.events(); | ||
let events = events.lock().unwrap(); | ||
|
||
assert!(!events | ||
.iter() | ||
.any(|event| matches!(event.kind, EventKind::WillExecute { .. }))); | ||
} | ||
|
||
#[test] | ||
fn line_index_for_source() { | ||
let mut db = TestDb::new(); | ||
let path = FileSystemPath::new("test.py"); | ||
|
||
db.file_system_mut() | ||
.write_file(path, "x = 10\ny = 20".to_string()); | ||
|
||
let file = db.file(path); | ||
let index = line_index(&db, file); | ||
let text = source_text(&db, file); | ||
|
||
assert_eq!(index.line_count(), 2); | ||
assert_eq!( | ||
index.line_start(OneIndexed::from_zero_indexed(0), &text), | ||
TextSize::new(0) | ||
); | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,91 @@ | ||
use ruff_db::{Db as SourceDb, Upcast}; | ||
use salsa::DbWithJar; | ||
|
||
// Salsa doesn't support a struct without fields, so allow the clippy lint for now. | ||
#[allow(clippy::empty_structs_with_brackets)] | ||
#[salsa::jar(db=Db)] | ||
pub struct Jar(); | ||
|
||
/// Database giving access to semantic information about a Python program. | ||
pub trait Db: SourceDb + DbWithJar<Jar> + Upcast<dyn SourceDb> {} | ||
|
||
#[cfg(test)] | ||
mod tests { | ||
use super::{Db, Jar}; | ||
use ruff_db::file_system::{FileSystem, MemoryFileSystem}; | ||
use ruff_db::vfs::Vfs; | ||
use ruff_db::{Db as SourceDb, Jar as SourceJar, Upcast}; | ||
use salsa::DebugWithDb; | ||
|
||
#[salsa::db(Jar, SourceJar)] | ||
pub(crate) struct TestDb { | ||
storage: salsa::Storage<Self>, | ||
vfs: Vfs, | ||
file_system: MemoryFileSystem, | ||
events: std::sync::Arc<std::sync::Mutex<Vec<salsa::Event>>>, | ||
} | ||
|
||
impl TestDb { | ||
#[allow(unused)] | ||
pub(crate) fn new() -> Self { | ||
Self { | ||
storage: salsa::Storage::default(), | ||
file_system: MemoryFileSystem::default(), | ||
events: std::sync::Arc::default(), | ||
vfs: Vfs::with_stubbed_vendored(), | ||
} | ||
} | ||
|
||
#[allow(unused)] | ||
pub(crate) fn memory_file_system(&self) -> &MemoryFileSystem { | ||
&self.file_system | ||
} | ||
|
||
#[allow(unused)] | ||
pub(crate) fn memory_file_system_mut(&mut self) -> &mut MemoryFileSystem { | ||
&mut self.file_system | ||
} | ||
|
||
#[allow(unused)] | ||
pub(crate) fn vfs_mut(&mut self) -> &mut Vfs { | ||
&mut self.vfs | ||
} | ||
} | ||
|
||
impl SourceDb for TestDb { | ||
fn file_system(&self) -> &dyn FileSystem { | ||
&self.file_system | ||
} | ||
|
||
fn vfs(&self) -> &Vfs { | ||
&self.vfs | ||
} | ||
} | ||
|
||
impl Upcast<dyn SourceDb> for TestDb { | ||
fn upcast(&self) -> &(dyn SourceDb + 'static) { | ||
self | ||
} | ||
} | ||
|
||
impl Db for TestDb {} | ||
|
||
impl salsa::Database for TestDb { | ||
fn salsa_event(&self, event: salsa::Event) { | ||
tracing::trace!("event: {:?}", event.debug(self)); | ||
let mut events = self.events.lock().unwrap(); | ||
events.push(event); | ||
} | ||
} | ||
|
||
impl salsa::ParallelDatabase for TestDb { | ||
fn snapshot(&self) -> salsa::Snapshot<Self> { | ||
salsa::Snapshot::new(Self { | ||
storage: self.storage.snapshot(), | ||
vfs: self.vfs.snapshot(), | ||
file_system: self.file_system.snapshot(), | ||
events: self.events.clone(), | ||
}) | ||
} | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
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.
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