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

allow for implicit impls for impl impl items #5915

Merged
merged 1 commit into from
Jul 1, 2024

Conversation

TomerStarkware
Copy link
Collaborator

@TomerStarkware TomerStarkware commented Jun 30, 2024

This change is Reviewable

@TomerStarkware TomerStarkware requested a review from orizi June 30, 2024 07:49
@TomerStarkware TomerStarkware force-pushed the tomer/infer_impl_impl branch from e1bbe1b to 434a54a Compare June 30, 2024 08:06
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 4 of 6 files at r1.
Reviewable status: 2 of 7 files reviewed, 6 unresolved discussions (waiting on @TomerStarkware)


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

    fn impl_item_by_name(&self, impl_def_id: ImplDefId, name: SmolStr)
    -> Maybe<Option<ImplItemId>>;
    /// Returns something

?


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

        trait_impl_id: TraitImplId,
    ) -> Maybe<ImplImplDefId>;
    /// Returns something

?


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

    #[salsa::invoke(items::imp::implicit_impl_impl_impl)]
    #[salsa::cycle(items::imp::implicit_impl_impl_impl_cycle)]
    fn implicit_impl_impl_impl(

doc


crates/cairo-lang-semantic/src/items/imp.rs line 906 at r1 (raw file):

    impl_def_id: ImplDefId,
    trait_impl_id: TraitImplId,
) -> Maybe<Option<TraitImplId>> {

is the input and the output the same value?

Code quote:

    trait_impl_id: TraitImplId,
) -> Maybe<Option<TraitImplId>> {

crates/cairo-lang-semantic/src/items/tests/trait line 461 at r1 (raw file):

}
impl MyImpl of MyTrait;
///TODO(TomerStarkware): improve diagnostics for missing impls.

Suggestion:

// TODO(TomerStarkware): improve diagnostics for missing impls.

crates/cairo-lang-semantic/src/items/tests/trait line 516 at r1 (raw file):

     ^****^

error: Trait has no implementation in context: test::OtherTrait.

this should point to the problematic member as well IMO.

@TomerStarkware TomerStarkware force-pushed the tomer/infer_impl_impl branch from 434a54a to 90eddc3 Compare June 30, 2024 09:15
Copy link
Collaborator Author

@TomerStarkware TomerStarkware 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: 2 of 7 files reviewed, 6 unresolved discussions (waiting on @orizi)


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

Previously, orizi wrote…

?

Done.


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

Previously, orizi wrote…

?

Done.


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

Previously, orizi wrote…

doc

Done.


crates/cairo-lang-semantic/src/items/imp.rs line 906 at r1 (raw file):

Previously, orizi wrote…

is the input and the output the same value?

if the outer impl does not explicitly contain the trait impl
can be refactored to is implicit impl impl


crates/cairo-lang-semantic/src/items/tests/trait line 461 at r1 (raw file):

}
impl MyImpl of MyTrait;
///TODO(TomerStarkware): improve diagnostics for missing impls.

Done.


crates/cairo-lang-semantic/src/items/tests/trait line 516 at r1 (raw file):

Previously, orizi wrote…

this should point to the problematic member as well IMO.

I agree, but infrence.finalize does not return the errors
I can check explicitly if the var created is in refuted, or ambiguous

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 2 of 3 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: 6 of 7 files reviewed, 4 unresolved discussions (waiting on @TomerStarkware)


crates/cairo-lang-semantic/src/db.rs line 760 at r3 (raw file):

        trait_impl_id: TraitImplId,
    ) -> Maybe<ImplImplDefId>;
    /// Returns whether `trait_impl_id` is an implicit impl in `impl_def_id`.

function does not return a bool.


crates/cairo-lang-semantic/src/items/imp.rs line 906 at r1 (raw file):

Previously, TomerStarkware wrote…

if the outer impl does not explicitly contain the trait impl
can be refactored to is implicit impl impl

please do.


crates/cairo-lang-semantic/src/items/tests/trait line 516 at r1 (raw file):

Previously, TomerStarkware wrote…

I agree, but infrence.finalize does not return the errors
I can check explicitly if the var created is in refuted, or ambiguous

but you do know the point you try to solve - have a diagnostic wrapping the inference error for impl item.

@TomerStarkware TomerStarkware force-pushed the tomer/infer_impl_impl branch from 90eddc3 to a322cf6 Compare June 30, 2024 10:30
Copy link
Collaborator Author

@TomerStarkware TomerStarkware 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: 4 of 7 files reviewed, 4 unresolved discussions (waiting on @orizi)


crates/cairo-lang-semantic/src/db.rs line 760 at r3 (raw file):

Previously, orizi wrote…

function does not return a bool.

changed


crates/cairo-lang-semantic/src/items/imp.rs line 906 at r1 (raw file):

Previously, orizi wrote…

please do.

Done.


crates/cairo-lang-semantic/src/items/tests/trait line 516 at r1 (raw file):

Previously, orizi wrote…

but you do know the point you try to solve - have a diagnostic wrapping the inference error for impl item.

if I use inference.solve is there a way to tell which impl was unsolved?

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 2 of 2 files at r4, all commit messages.
Reviewable status: 6 of 7 files reviewed, 1 unresolved discussion (waiting on @TomerStarkware)


crates/cairo-lang-semantic/src/items/tests/trait line 516 at r1 (raw file):

Previously, TomerStarkware wrote…

if I use inference.solve is there a way to tell which impl was unsolved?

Don't you solve per item now?

Copy link
Collaborator Author

@TomerStarkware TomerStarkware 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: 6 of 7 files reviewed, 1 unresolved discussion (waiting on @orizi)


crates/cairo-lang-semantic/src/items/tests/trait line 516 at r1 (raw file):

Previously, orizi wrote…

Don't you solve per item now?

wdym? I use inference.finalize in priv_implicit_impl_impl_semantic_data

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 1 of 6 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @TomerStarkware)


crates/cairo-lang-semantic/src/items/tests/trait line 516 at r1 (raw file):

Previously, TomerStarkware wrote…

wdym? I use inference.finalize in priv_implicit_impl_impl_semantic_data

But you can do it per item.

Copy link
Collaborator Author

@TomerStarkware TomerStarkware 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: all files reviewed, 1 unresolved discussion (waiting on @orizi)


crates/cairo-lang-semantic/src/items/tests/trait line 516 at r1 (raw file):

Previously, orizi wrote…

But you can do it per item.

I meant if when I use inference.solve there more error which could be unrelated to the current implicit impl I am trying to solve

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: all files reviewed, 1 unresolved discussion (waiting on @TomerStarkware)


crates/cairo-lang-semantic/src/items/tests/trait line 516 at r1 (raw file):

Previously, TomerStarkware wrote…

I meant if when I use inference.solve there more error which could be unrelated to the current implicit impl I am trying to solve

But you can still just solve it on the spot, item by item, so you'd know which what failed for which.

@TomerStarkware TomerStarkware force-pushed the tomer/infer_impl_impl branch from a322cf6 to ff41e1f Compare July 1, 2024 08:17
Copy link
Collaborator Author

@TomerStarkware TomerStarkware 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: 4 of 8 files reviewed, 1 unresolved discussion (waiting on @orizi)


crates/cairo-lang-semantic/src/items/tests/trait line 516 at r1 (raw file):

Previously, orizi wrote…

But you can still just solve it on the spot, item by item, so you'd know which what failed for which.

Done.

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.

:lgtm:

Reviewed 4 of 4 files at r5, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @TomerStarkware)

@TomerStarkware TomerStarkware enabled auto-merge July 1, 2024 08:25
@TomerStarkware TomerStarkware added this pull request to the merge queue Jul 1, 2024
Merged via the queue into main with commit 50d465c Jul 1, 2024
44 checks passed
@orizi orizi deleted the tomer/infer_impl_impl branch July 1, 2024 10:41
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.

2 participants