Allowed to get the diagnostics mappings in the LS.

[orizi]

This change is 

[orizi]

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @Arcticae, @Draggu, @mkaput, and @piotmag769)

a discussion (no related file):
@mkaput tested by hand.
any ideas about proper testing? this is, by definition, dependent on possibly unstable ids.

---

[piotmag769]

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

---

crates/cairo-lang-language-server/src/env_config.rs line 32 at r1 (raw file):

}

/// Whether to generate LS tracing profile in the opened project directory.

I don't think this description is accurate, how is tracing profile == non-deduplicated diagnostics? You generate the tracing anyways

---

crates/cairo-lang-language-server/src/lang/diagnostics/lsp.rs line 88 at r1 (raw file):

            }
            related_info.push(DiagnosticRelatedInformation {
                location: Location { uri: db.url_for_file(mapped.file_id), range: mapped_range },

Isn't it the location we return? If so, why add a note about it, the mapped range will already point to this

[orizi]

Reviewable status: 0 of 2 files reviewed, 3 unresolved discussions (waiting on @Arcticae, @Draggu, @mkaput, and @piotmag769)

---

crates/cairo-lang-language-server/src/env_config.rs line 32 at r1 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

I don't think this description is accurate, how is tracing profile == non-deduplicated diagnostics? You generate the tracing anyways

you are correct - it is accidentally copied comment.
Done.

---

crates/cairo-lang-language-server/src/lang/diagnostics/lsp.rs line 88 at r1 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

Isn't it the location we return? If so, why add a note about it, the mapped range will already point to this

Good catch once again.
Done.

[mkaput]

Reviewed all commit messages.
Reviewable status: 0 of 2 files reviewed, 6 unresolved discussions (waiting on @Arcticae, @Draggu, @orizi, and @piotmag769)

a discussion (no related file):
I have no idea what this PR brings to the table. Please elaborate.

---

---

-- commits line 2 at r2:
What are mappings?

---

crates/cairo-lang-language-server/src/env_config.rs line 34 at r2 (raw file):

/// Whether to include the diagnostic mapping as a note for all diagnostic locations.
pub fn include_diag_mapping() -> bool {
    env::var_os(CAIRO_LS_INCLUDE_DIAG_MAPPING).map(env_to_bool).unwrap_or_default()

Why is this enabled by environment variable (which means that this is probably for our use as we do not expect regular users to ever touch these)?

[orizi]

Reviewable status: 0 of 2 files reviewed, 6 unresolved discussions (waiting on @Arcticae, @Draggu, @mkaput, and @piotmag769)

a discussion (no related file):

Previously, mkaput (Marek Kaput) wrote…

I have no idea what this PR brings to the table. Please elaborate.

power users write code that is heavily reliant on generated code - when there are errors - mapping makes the received code sometime too restrictive.

---

---

-- commits line 2 at r2:

Previously, mkaput (Marek Kaput) wrote…

What are mappings?

the thing that causes everything to point to actual user code instead of some internal virtual files.

---

crates/cairo-lang-language-server/src/env_config.rs line 34 at r2 (raw file):

Previously, mkaput (Marek Kaput) wrote…

Why is this enabled by environment variable (which means that this is probably for our use as we do not expect regular users to ever touch these)?

power users write code that is heavily reliant on generated code - when there are errors - mapping makes the received code sometime too restrictive.

[piotmag769]

to unblock, discuss it with @mkaput

Reviewable status: 0 of 2 files reviewed, 4 unresolved discussions (waiting on @Arcticae, @Draggu, and @mkaput)

[piotmag769]

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

[mkaput]

Reviewed 2 of 2 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Arcticae, @Draggu, and @orizi)

a discussion (no related file):

Previously, orizi wrote…

@mkaput tested by hand.
any ideas about proper testing? this is, by definition, dependent on possibly unstable ids.

we have deterministic diagnostics emission (per file), aren't we?

if so then I think it should be totally fine to just dump vector of diagnostics as-is to the fixture, with separate files going to separate test file sections

---

---

crates/cairo-lang-language-server/src/env_config.rs line 34 at r2 (raw file):

Previously, orizi wrote…

power users write code that is heavily reliant on generated code - when there are errors - mapping makes the received code sometime too restrictive.

gotcha

so it looks like env vars are not the place to store this configuration. this should be kept in editor's settings (workspace/configuration request). we are wrapping all of this in this object:

cairo/crates/cairo-lang-language-server/src/config.rs

Line 23 in a39bf32

pub struct Config {

+ there's pairing stuff in vscode's package.json to define the settings. please use this mechanism

don't bother with regenerating diagnostics on config change if this would happen overly complicated (give yourself 15 minutes budget for this); otherwise please make an issue (and attach it to our board by adding the ide label) so this won't be forgotten.

[orizi]

Reviewable status: 0 of 5 files reviewed, 2 unresolved discussions (waiting on @Arcticae, @Draggu, @mkaput, and @piotmag769)

a discussion (no related file):

Previously, mkaput (Marek Kaput) wrote…

we have deterministic diagnostics emission (per file), aren't we?

if so then I think it should be totally fine to just dump vector of diagnostics as-is to the fixture, with separate files going to separate test file sections

the diagnostics are - but the generated file-ids are not - and these are part of the vfs path.

---

---

crates/cairo-lang-language-server/src/env_config.rs line 34 at r2 (raw file):

Previously, mkaput (Marek Kaput) wrote…

gotcha

so it looks like env vars are not the place to store this configuration. this should be kept in editor's settings (workspace/configuration request). we are wrapping all of this in this object:

cairo/crates/cairo-lang-language-server/src/config.rs

Line 23 in a39bf32

pub struct Config {

+ there's pairing stuff in vscode's package.json to define the settings. please use this mechanism

don't bother with regenerating diagnostics on config change if this would happen overly complicated (give yourself 15 minutes budget for this); otherwise please make an issue (and attach it to our board by adding the ide label) so this won't be forgotten.

Done.

[mkaput]

Reviewed 3 of 5 files at r3, 3 of 3 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Arcticae, @Draggu, and @orizi)

a discussion (no related file):

Previously, orizi wrote…

the diagnostics are - but the generated file-ids are not - and these are part of the vfs path.

these files have plugin-defined file names, do I recall correctly? could we use them?

I remember there is something that has format path[nondeterministic id]; perhaps just assuming some prefix (like everything until [, or first 10 characters) could give enough stable precision for our purpose?

---

---

vscode-cairo/package.json line 124 at r4 (raw file):

            "description": "Enable getting unmapped diagnostics information.",
            "scope": "window"
          },

config item name & description are not descriptive IMO (I can only understand these because of the context I gained by reviewing this PR).

here is something that comes to my mind, but please don't hesitate to propose something yet different

Suggestion:

          "cairo1.traceMacroDiagnostics": {
            "type": "boolean",
            "description": "Attach additional information to diagnostics coming from macros, providing diagnostic source in macro generated code.",
            "scope": "window"
          },

[orizi]

Reviewable status: 1 of 6 files reviewed, 2 unresolved discussions (waiting on @Arcticae, @Draggu, and @mkaput)

a discussion (no related file):

Previously, mkaput (Marek Kaput) wrote…

these files have plugin-defined file names, do I recall correctly? could we use them?

I remember there is something that has format path[nondeterministic id]; perhaps just assuming some prefix (like everything until [, or first 10 characters) could give enough stable precision for our purpose?

nothing in the area of this testing framework area supports that..

---

---

vscode-cairo/package.json line 124 at r4 (raw file):

Previously, mkaput (Marek Kaput) wrote…

config item name & description are not descriptive IMO (I can only understand these because of the context I gained by reviewing this PR).

here is something that comes to my mind, but please don't hesitate to propose something yet different

Done.

[mkaput]

Reviewed 3 of 5 files at r5, 2 of 2 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Arcticae and @Draggu)

a discussion (no related file):

Previously, orizi wrote…

nothing in the area of this testing framework area supports that..

discussed offline. let's do that as separate task: #6291

---

[Draggu]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Arcticae)

[piotmag769]

Reviewed 1 of 5 files at r3, 3 of 5 files at r5, 2 of 2 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved