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

matching un-imported traits #7035

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dean-starkware
Copy link
Collaborator

  • This PR adds proper diagnostics to suggest importing a relevant trait containing the method being used when it is not found on the given type.

  • However, this behavior does not function correctly in our current test framework due to a bug with in the test infrastructure (where the test's crate is not properly integrated into the db).
    This will require a designated PR to deal with this issue.

  • Note that the added test does not return the proper diagnostic as a result of this bug- and we need to make sure it will be updated after the fix.

  • Also note that the changes were implemented in the compute.rs and not in the diagnostics.rs file due to the direct involvement of virtual files in the computation process.

  • It is also necessary to evaluate and refactor the functions used here to better align with the existing code within the langauge-server repo. The find_methods_for_type function particularly relevant for review and potential integration (and to decide whether and how to move them back into our repo).

@reviewable-StarkWare
Copy link

This change is Reviewable

@dean-starkware dean-starkware marked this pull request as ready for review January 9, 2025 10:00
Copy link
Collaborator

@orizi orizi left a 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: 0 of 3 files reviewed, 5 unresolved discussions (waiting on @dean-starkware and @gilbens-starkware)


crates/cairo-lang-semantic/src/expr/compute.rs line 3849 at r1 (raw file):

    resolver: &mut Resolver<'_>,
    ty: crate::TypeId,
    stable_ptr: cairo_lang_syntax::node::ids::SyntaxStablePtrId,

Suggestion:

    stable_ptr: SyntaxStablePtrId,

crates/cairo-lang-semantic/src/expr/compute.rs line 3859 at r1 (raw file):

    // Find methods on type.
    // TODO(spapini): Look only in current crate dependencies.
    for crate_id in db.crates() {

Suggestion:

    for (name, setting) in &resolver.settings.dependencies {
        let crate_id = CrateLongId::Real { name: name.clone(), discriminator: setting.discriminator.clone() }
                    .intern(db)

crates/cairo-lang-semantic/src/diagnostic_test_data/not_found line 112 at r1 (raw file):

 --> lib.cairo:19:7
    x.foo()
      ^^^

Suggestion:

//! > Test suggesting import for missing method.

//! > test_runner_name
test_expr_diagnostics(expect_diagnostics: true)

//! > expr_code
{
// TODO: Ensure the diagnostic matches the expected output- add the tests' crate to the db.
    let x: a_struct = A {a: 0};
    x.foo()
}

//! > module_code
struct A {
    a: felt252,
}
mod some_module {
    use super::A;
    pub trait Trt1 {
        fn foo(self: A) -> felt252;
    }
    impl Imp1 of Trt1 {
        fn foo(self: A) -> felt252 {
            0
        }
    }
}

//! > function_body

//! > expected_diagnostics
error: Unknown member.
 --> lib.cairo:18:38
    let x: a_struct = a_struct{a: 0, b: 0};
                                     ^
// I don't see the new message.
error[E0002]: Method `foo` not found on type `test::a_struct`. Did you import the correct trait and impl?
 --> lib.cairo:19:7
    x.foo()
      ^^^

crates/cairo-lang-semantic/src/diagnostic.rs line 816 at r1 (raw file):

                        let suggestions = relevant_traits
                            .iter()
                            .map(|trait_path| format!("`{}`", trait_path))

Suggestion:

                            .map(|trait_path| format!("`{trait_path}`"))

crates/cairo-lang-semantic/src/diagnostic.rs line 1236 at r1 (raw file):

        method_name: SmolStr,
        inference_errors: TraitInferenceErrors,
        relevant_traits: Option<Vec<String>>,

Suggestion:

        relevant_traits: Vec<String>,

@dean-starkware dean-starkware force-pushed the dean/diagnostics_missing_traits_for_methods branch 2 times, most recently from cdd9fb3 to 7cb6fd4 Compare January 9, 2025 11:27
Copy link
Collaborator Author

@dean-starkware dean-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 3 files reviewed, 5 unresolved discussions (waiting on @gilbens-starkware and @orizi)


crates/cairo-lang-semantic/src/expr/compute.rs line 3849 at r1 (raw file):

    resolver: &mut Resolver<'_>,
    ty: crate::TypeId,
    stable_ptr: cairo_lang_syntax::node::ids::SyntaxStablePtrId,

Done.


crates/cairo-lang-semantic/src/expr/compute.rs line 3859 at r1 (raw file):

    // Find methods on type.
    // TODO(spapini): Look only in current crate dependencies.
    for crate_id in db.crates() {

Please explain- I'm not sure I understand what you did here.


crates/cairo-lang-semantic/src/diagnostic_test_data/not_found line 112 at r1 (raw file):

 --> lib.cairo:19:7
    x.foo()
      ^^^

Please look at the PR desription on Github.


crates/cairo-lang-semantic/src/diagnostic.rs line 816 at r1 (raw file):

                        let suggestions = relevant_traits
                            .iter()
                            .map(|trait_path| format!("`{}`", trait_path))

Done.


crates/cairo-lang-semantic/src/diagnostic.rs line 1236 at r1 (raw file):

        method_name: SmolStr,
        inference_errors: TraitInferenceErrors,
        relevant_traits: Option<Vec<String>>,

Why? I'm not sure it's right.

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 3 files reviewed, 5 unresolved discussions (waiting on @dean-starkware and @gilbens-starkware)


crates/cairo-lang-semantic/src/diagnostic.rs line 1236 at r1 (raw file):

Previously, dean-starkware wrote…

Why? I'm not sure it's right.

Since you don't handle differently that case, just 'unwrap_or_default()' on construction.


crates/cairo-lang-semantic/src/diagnostic.rs line 1236 at r2 (raw file):

        method_name: SmolStr,
        inference_errors: TraitInferenceErrors,
        relevant_traits: Option<Vec<String>>,

And probably the name 'trait_suggestions' is more exact.

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 3 files reviewed, 5 unresolved discussions (waiting on @dean-starkware and @gilbens-starkware)


crates/cairo-lang-semantic/src/diagnostic_test_data/not_found line 112 at r1 (raw file):

Previously, dean-starkware wrote…

Please look at the PR desription on Github.

Most of the comment is still relevant.
In any case, is still don't see the code that causes the actual missing issue.


crates/cairo-lang-semantic/src/expr/compute.rs line 3859 at r1 (raw file):

Previously, dean-starkware wrote…

Please explain- I'm not sure I understand what you did here.

this just provides you with your dependency traits.
You probably just need to add to it your own crate and the core crate.

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 3 files reviewed, 5 unresolved discussions (waiting on @dean-starkware and @gilbens-starkware)


crates/cairo-lang-semantic/src/expr/compute.rs line 3859 at r1 (raw file):

Previously, orizi wrote…

this just provides you with your dependency traits.
You probably just need to add to it your own crate and the core crate.

more specifically:

for crate_id in chain!([resolver.owning_crate_id, corelib::core_crate()], resolver.settings.dependencies.iter().map(|(name, setting)| {
            CrateLongId::Real { name: name.clone(), discriminator: dep.discriminator.clone() }
                    .intern(db)
        })) {

@dean-starkware dean-starkware force-pushed the dean/diagnostics_missing_traits_for_methods branch from 7cb6fd4 to d785eff Compare January 9, 2025 13:29
Copy link
Collaborator Author

@dean-starkware dean-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 3 files reviewed, 5 unresolved discussions (waiting on @gilbens-starkware and @orizi)


crates/cairo-lang-semantic/src/diagnostic.rs line 1236 at r1 (raw file):

Previously, orizi wrote…

Since you don't handle differently that case, just 'unwrap_or_default()' on construction.

Done.


crates/cairo-lang-semantic/src/diagnostic.rs line 1236 at r2 (raw file):

Previously, orizi wrote…

And probably the name 'trait_suggestions' is more exact.

Done.


crates/cairo-lang-semantic/src/diagnostic_test_data/not_found line 112 at r1 (raw file):

Previously, orizi wrote…

Most of the comment is still relevant.
In any case, is still don't see the code that causes the actual missing issue.

It is fixed now (we get the correct diagnostics. However the bug still exists).


crates/cairo-lang-semantic/src/expr/compute.rs line 3859 at r1 (raw file):

Previously, orizi wrote…

more specifically:

for crate_id in chain!([resolver.owning_crate_id, corelib::core_crate()], resolver.settings.dependencies.iter().map(|(name, setting)| {
            CrateLongId::Real { name: name.clone(), discriminator: dep.discriminator.clone() }
                    .intern(db)
        })) {

It doesn't fix the bug though.

Copy link
Collaborator

@orizi orizi left a 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: 0 of 3 files reviewed, 4 unresolved discussions (waiting on @dean-starkware and @gilbens-starkware)


crates/cairo-lang-semantic/src/expr/compute.rs line 3859 at r1 (raw file):

Previously, dean-starkware wrote…

It doesn't fix the bug though.

this actually does fix it.

resolver.owning_crate_id should exactly bring your crate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants