-
Notifications
You must be signed in to change notification settings - Fork 488
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
LS: Skip warnings in deps #6358
Conversation
1524391
to
675ec0f
Compare
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.
Reviewed 2 of 3 files at r1.
Reviewable status: 1 of 3 files reviewed, 5 unresolved discussions (waiting on @Arcticae, @Draggu, @mkaput, and @orizi)
crates/cairo-lang-language-server/src/lib.rs
line 376 at r1 (raw file):
#[tracing::instrument(level = "debug", skip_all)] async fn refresh_diagnostics(&self) -> LSPResult<()> { fn is_virtual_file(db: &AnalysisDatabase, file: FileId) -> bool {
Suggestion:
is_compiler_generated_file
crates/cairo-lang-language-server/src/lib.rs
line 377 at r1 (raw file):
async fn refresh_diagnostics(&self) -> LSPResult<()> { fn is_virtual_file(db: &AnalysisDatabase, file: FileId) -> bool { !matches!(
Better to do exhaustive match here I think (without _
pattern) so compiler complains when a new variant is added
crates/cairo-lang-language-server/src/lib.rs
line 412 at r1 (raw file):
&file, &file_modules_ids, is_virtual_file(&db, file), // Show warnings for open files even if this is dependency.
You can also add sth like this if u want:
We don't check if the open file belongs to a dependency - we want to show warnings for open files from dependencies.
Suggestion:
// Ignore warnings for compiler generated files.
crates/cairo-lang-language-server/src/lib.rs
line 424 at r1 (raw file):
let (rest_of_files, skip_warnings_files) = async { let mut rest_of_files: HashSet<FileId> = HashSet::default(); let mut skip_warnings_files: HashSet<FileId> = HashSet::default();
This way this logic is immediately obvious
let skip_warnings = files_from_dependencies.contains(&file);
Suggestion:
files_from_dependencies
crates/cairo-lang-language-server/src/project/scarb.rs
line 34 at r1 (raw file):
let mut crates_grouped_by_group_id = HashMap::new(); *deps = metadata
This will only work when we support multiroot workspaces and separate LS instance will be spawned for each scarb workspace - otherwise you will override the dependencies when a file from a different package is opened. Cc @Arcticae @mkaput to double check if I'm right
675ec0f
to
568de77
Compare
fix #6310 commit-id:4536ca03
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.
Reviewable status: 1 of 4 files reviewed, 7 unresolved discussions (waiting on @Arcticae, @Draggu, @orizi, and @piotmag769)
crates/cairo-lang-language-server/src/lib.rs
line 376 at r1 (raw file):
#[tracing::instrument(level = "debug", skip_all)] async fn refresh_diagnostics(&self) -> LSPResult<()> { fn is_virtual_file(db: &AnalysisDatabase, file: FileId) -> bool {
disagree given the broader context, see my comment: https://reviewable.io/reviews/starkware-libs/cairo/6358#-O61qK6C1upZzvByW8x4
crates/cairo-lang-language-server/src/lib.rs
line 377 at r1 (raw file):
Previously, piotmag769 (Piotr Magiera) wrote…
Better to do exhaustive match here I think (without
_
pattern) so compiler complains when a new variant is added
I think we should match for FileLongId::External(PluginGeneratedFileId(...) as salsa::InternId)
here. FileLongId::Virtual
are completely fine for our purposes and I don't think we should hide diags for them
cc @orizi how could this be expressed efficiently? I'd definitely like this check to be present in compiler APIs instead of being done on LS side
crates/cairo-lang-language-server/src/lib.rs
line 375 at r2 (raw file):
#[tracing::instrument(level = "debug", skip_all)] async fn refresh_diagnostics(&self) -> LSPResult<()> { fn is_virtual_file(db: &AnalysisDatabase, file: FileId) -> bool {
Suggestion:
show_warnings_for_file
crates/cairo-lang-language-server/src/lib.rs
line 411 at r2 (raw file):
&file, &file_modules_ids, is_virtual_file(&db, file), /* Show warnings for open files even if this is
what is this comment man, plz use //
like everywhere 😆
568de77
to
44c0b23
Compare
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.
Reviewable status: 1 of 4 files reviewed, 6 unresolved discussions (waiting on @Arcticae, @mkaput, @orizi, and @piotmag769)
crates/cairo-lang-language-server/src/lib.rs
line 375 at r2 (raw file):
#[tracing::instrument(level = "debug", skip_all)] async fn refresh_diagnostics(&self) -> LSPResult<()> { fn is_virtual_file(db: &AnalysisDatabase, file: FileId) -> bool {
This is partially true, later is check if this file is from dependency crate, and this is checking only for virtual file. I don't think that naming it show_warnings_for_file
is good here.
crates/cairo-lang-language-server/src/lib.rs
line 411 at r2 (raw file):
Previously, mkaput (Marek Kaput) wrote…
what is this comment man, plz use
//
like everywhere 😆
This was done by formatter XD, missed it.
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.
Reviewed 1 of 2 files at r2, 1 of 2 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @Arcticae, @Draggu, @mkaput, and @orizi)
crates/cairo-lang-language-server/src/lib.rs
line 376 at r1 (raw file):
Previously, mkaput (Marek Kaput) wrote…
disagree given the broader context, see my comment: https://reviewable.io/reviews/starkware-libs/cairo/6358#-O61qK6C1upZzvByW8x4
Agree, ur name is better
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.
Reviewed all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @Arcticae, @Draggu, @mkaput, and @orizi)
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.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @Arcticae and @orizi)
crates/cairo-lang-language-server/src/project/scarb.rs
line 34 at r1 (raw file):
Previously, piotmag769 (Piotr Magiera) wrote…
This will only work when we support multiroot workspaces and separate LS instance will be spawned for each scarb workspace - otherwise you will override the dependencies when a file from a different package is opened. Cc @Arcticae @mkaput to double check if I'm right
Yeah, I think we should invert this logic. Let's discuss this offline.
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.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @Arcticae, @Draggu, and @orizi)
crates/cairo-lang-language-server/src/lib.rs
line 375 at r2 (raw file):
Previously, Draggu (Piotr Figiela) wrote…
This is partially true, later is check if this file is from dependency crate, and this is checking only for virtual file. I don't think that naming it
show_warnings_for_file
is good here.
we're checking if file is generated precisely because we want to identify if we want to show warnings for it. this way we are explicitly saying our intention, which is esp. useful because you're passing this as unnamed argument
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.
Reviewed 1 of 3 files at r1.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @Arcticae, @Draggu, and @orizi)
crates/cairo-lang-language-server/src/state.rs
line 23 at r4 (raw file):
pub config: Owned<Config>, pub client_capabilities: Owned<ClientCapabilities>, pub dependencies: Owned<Dependencies>,
why not keep this in AnalysisDatabase in custom query group?
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.
Reviewed all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @Arcticae and @Draggu)
crates/cairo-lang-language-server/src/lib.rs
line 387 at r4 (raw file):
// Refresh open files modules first for better UX async { let db = self.db_snapshot().await;
do note this lock locks across all files handling.
is it what you actually want?
Code quote:
let db = self.db_snapshot().await;
Need to be redone after #6469 |
fix #6310
This change is