LS: Skip warnings in deps

[Draggu]

fix #6310

---

This change is 

[piotmag769]

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

[mkaput]

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 😆

[Draggu]

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.

[piotmag769]

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

[mkaput]

Reviewed all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @Arcticae, @Draggu, @mkaput, and @orizi)

[mkaput]

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.

[mkaput]

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

[mkaput]

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?

[orizi]

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;