Skip to content
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

Deal with panics on core crate mismatch #6174

Closed
mkaput opened this issue Aug 8, 2024 · 2 comments · Fixed by #6204 or #6213
Closed

Deal with panics on core crate mismatch #6174

mkaput opened this issue Aug 8, 2024 · 2 comments · Fixed by #6204 or #6213
Assignees
Labels
bug Something isn't working

Comments

@mkaput
Copy link
Member

mkaput commented Aug 8, 2024

Bug Report

Cairo version: 2.7.0

Current behavior:

Users happen to get into situation when LS is fed with core crate coming from different compiler release. This results in all analysis actions to panic because the compiler expects existence and particular implementation of various language items in core, that is fulfilled by the exactly same core version, but usually is not happening in past and future releases.

Expected behavior:

Although panicking (at the place that does it) makes sense implementation-wise, we have to think how and implement a feature that will tell users what is the problem and how they could fix it. The exact UX is not decided on right now.

One idea that is the most pleasing right now is to implement a way of checking core and LS' compiler compatibility. Perhaps by comparing versions of both?

Steps to reproduce:

We know of several scenarios that trigger the invalid state, but there are probably more. Users are almost always unable to tell what they did.

  1. The easiest unrealistic reproduction:
    1. Open some regular Cairo codebase in VSCode.

    2. Put the following in .vscode/settings.json (adjust versions to whatever you have installed, but they must be different):

      {
          "cairo1.languageServerPath": "${userHome}/.asdf/installs/scarb/2.7.0/bin/scarb-cairo-language-server",
          "cairo1.scarbPath": "${userHome}/.asdf/installs/scarb/2.6.4/bin/scarb",
          "cairo1.preferScarbLanguageServer": false
      }
    3. You can also do this with manually built the LS.

  2. A possible scenario that happens when users are reporting when new release is happening, and then they say that the issue solved magically:
    1. Use asdf local scarb 2.6.4 to set Scarb locally in some Cairo codebase.
    2. Launch VSCode, open some *.cairo file and thus let CairoLS load and analyse the project.
    3. Open terminal panel.
    4. Run asdf local scarb 2.7.0.
    5. Open another *.cairo file and thus let CairoLS call detect_crate_for again and query scarb metadata from a new Scarb installation. This should replace the core crate that is already loaded in analyzer database (2.6.4) with new one that new Scarb provides (2.7.0).
    6. Note: This scenario interferes with New Project Model MVP #6124 that drops detect_crate_for. Perhaps we should resolve this case specifically in this feature implementation?
    7. Note: Reloading VSCode will fix the problem because it will restart with newer CairoLS.

Related code:

Mostly this function panics, but in general, this applies to this entire file:

/// Given a core library context and trait name, returns [TraitId].
pub fn get_core_trait(db: &dyn SemanticGroup, context: CoreTraitContext, name: SmolStr) -> TraitId {
let base_module = match context {
CoreTraitContext::TopLevel => db.core_module(),
CoreTraitContext::Ops => core_submodule(db, "ops"),
CoreTraitContext::Iterator => core_submodule(db, "iter"),
CoreTraitContext::MetaProgramming => core_submodule(db, "metaprogramming"),
};
// This should not fail if the corelib is present.
let item_id = db
.module_item_by_name(base_module, name.clone())
.unwrap_or_else(|_| {
panic!(
"Core module `{module}` failed to compile.",
module = base_module.full_path(db.upcast())
)
})
.unwrap_or_else(|| {
panic!(
"Core module `{module}` is missing an use item for trait `{name}`.",
module = base_module.full_path(db.upcast()),
)
});
match item_id {
ModuleItemId::Trait(id) => id,
ModuleItemId::Use(use_id) => {
extract_matches!(
db.use_resolved_item(use_id).unwrap_or_else(|_| panic!(
"Could not resolve core trait `{module}::{name}`.",
module = base_module.full_path(db.upcast()),
)),
ResolvedGenericItem::Trait
)
}
_ => panic!("Expecting only traits, or uses pointing to traits."),
}
}

@mkaput mkaput added bug Something isn't working ide labels Aug 8, 2024
@github-project-automation github-project-automation bot moved this to Triage in CairoLS Aug 8, 2024
@orizi orizi linked a pull request Aug 13, 2024 that will close this issue
@orizi
Copy link
Collaborator

orizi commented Aug 13, 2024

Released in 2.7.1, and merged to main.

@orizi orizi closed this as completed Aug 13, 2024
@github-project-automation github-project-automation bot moved this from Backlog to Done in CairoLS Aug 13, 2024
@piotmag769
Copy link
Collaborator

@orizi this requires more tweaks from our side (in LS crate) as we don't setup the db using RootDatabaseBuilder, reopening and assigning to myself

@piotmag769 piotmag769 reopened this Aug 13, 2024
@github-project-automation github-project-automation bot moved this from Done to Todo in CairoLS Aug 13, 2024
@piotmag769 piotmag769 assigned piotmag769 and unassigned orizi Aug 13, 2024
@github-project-automation github-project-automation bot moved this from In Progress to Done in CairoLS Aug 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
No open projects
Archived in project
3 participants