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

Enrich members caching. #6343

Merged
merged 1 commit into from
Sep 5, 2024
Merged

Enrich members caching. #6343

merged 1 commit into from
Sep 5, 2024

Conversation

orizi
Copy link
Collaborator

@orizi orizi commented Sep 3, 2024

This change is Reviewable

@orizi orizi force-pushed the orizi/enrich-member-caching branch 3 times, most recently from 4386df7 to 3a93f05 Compare September 3, 2024 16:09
@ilyalesokhin-starkware
Copy link
Contributor

crates/cairo-lang-semantic/src/resolve/mod.rs line 125 at r1 (raw file):

}
impl EnrichedMembers {
    /// Returns the enriched member structure for a single member if exists.

The return type is EnrichedMembers

Code quote:

enriched member structure

Copy link
Collaborator Author

@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 2 files reviewed, 1 unresolved discussion (waiting on @gilbens-starkware and @ilyalesokhin-starkware)


crates/cairo-lang-semantic/src/resolve/mod.rs line 125 at r1 (raw file):

Previously, ilyalesokhin-starkware wrote…

The return type is EnrichedMembers

Done.

@orizi orizi force-pushed the orizi/enrich-member-caching branch from 3a93f05 to a178a8a Compare September 4, 2024 08:31
@ilyalesokhin-starkware
Copy link
Contributor

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

                return Ok(e.clone());
            }
            entry.swap_remove()

why is the entry removed?

Code quote:

 entry.swap_remove()

Copy link
Collaborator Author

@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 2 files reviewed, 2 unresolved discussions (waiting on @gilbens-starkware and @ilyalesokhin-starkware)


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

Previously, ilyalesokhin-starkware wrote…

why is the entry removed?

just preventing copy - as it is re-added post enrich_members call.

@ilyalesokhin-starkware
Copy link
Contributor

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

                return Ok(e.clone());
            }
            entry.swap_remove()

doc

@orizi orizi force-pushed the orizi/enrich-member-caching branch from a178a8a to a9b6c31 Compare September 4, 2024 13:05
Copy link
Collaborator Author

@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 2 files reviewed, 3 unresolved discussions (waiting on @gilbens-starkware and @ilyalesokhin-starkware)


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

Previously, ilyalesokhin-starkware wrote…

doc

Done.

@orizi orizi force-pushed the orizi/enrich-member-caching branch 2 times, most recently from 5eae2a8 to 18ce47d Compare September 4, 2024 16:17
@ilyalesokhin-starkware
Copy link
Contributor

crates/cairo-lang-semantic/src/resolve/mod.rs line 130 at r4 (raw file):

    /// `EnrichedTypeMemberAccessFailure::NotFound` otherwise returns
    /// `EnrichedTypeMemberAccessFailure::NotFinal`.
    pub fn accessed(

why is is called accessed?
maybe get_member?

Code quote:

accessed

@ilyalesokhin-starkware
Copy link
Contributor

crates/cairo-lang-semantic/src/resolve/mod.rs line 162 at r4 (raw file):

    /// The member was not found.
    NotFound,
    /// The member was not found, but the enriched type info is not final.

can you elaborate on what final means?

Code quote:

  /// The member was not found, but the enriched type info is not final.

Copy link
Collaborator Author

@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 2 files reviewed, 5 unresolved discussions (waiting on @gilbens-starkware and @ilyalesokhin-starkware)


crates/cairo-lang-semantic/src/resolve/mod.rs line 130 at r4 (raw file):

Previously, ilyalesokhin-starkware wrote…

why is is called accessed?
maybe get_member?

Done.


crates/cairo-lang-semantic/src/resolve/mod.rs line 162 at r4 (raw file):

Previously, ilyalesokhin-starkware wrote…

can you elaborate on what final means?

Done.

@orizi orizi force-pushed the orizi/enrich-member-caching branch from 18ce47d to ddf7764 Compare September 5, 2024 09:15
@ilyalesokhin-starkware
Copy link
Contributor

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

        };

    // If the variable is mutable, and implements DerefMut, we use DerefMut in the first iteration.

Why is the first iteration special?

Code quote:

in the first iteration

@ilyalesokhin-starkware
Copy link
Contributor

crates/cairo-lang-semantic/src/resolve/mod.rs line 119 at r5 (raw file):

    /// The sequence of deref functions needed to access the members.
    pub deref_functions: Vec<(FunctionId, Mutability)>,
    /// The tail remaining for required computation - the current expression `Deref`/`DerefMut`

Suggestion:

The tail of the current computation computation. The search for additional members will continue from this point.

@ilyalesokhin-starkware
Copy link
Contributor

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

            // If member is contained we can stop the calculation post the lookup.
            if members.contains_key(accessed_member_name) {
                *calc_tail = Some(expr.id);

why is the calc_tail assigment related to finding the member? what happens if the member is not found?

Code quote:

    *calc_tail = Some(expr.id);

@orizi orizi force-pushed the orizi/enrich-member-caching branch from ddf7764 to 95274b9 Compare September 5, 2024 11:28
Copy link
Collaborator Author

@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 2 files reviewed, 8 unresolved discussions (waiting on @gilbens-starkware and @ilyalesokhin-starkware)


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

Previously, ilyalesokhin-starkware wrote…

Why is the first iteration special?

since out DerefMut is simulated - after it returned a value next calls would be Deref (as there's no longer a mut var available)


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

Previously, ilyalesokhin-starkware wrote…

why is the calc_tail assigment related to finding the member? what happens if the member is not found?

if it wasn't found - it was fully enriched.


crates/cairo-lang-semantic/src/resolve/mod.rs line 119 at r5 (raw file):

    /// The sequence of deref functions needed to access the members.
    pub deref_functions: Vec<(FunctionId, Mutability)>,
    /// The tail remaining for required computation - the current expression `Deref`/`DerefMut`

Done.

@ilyalesokhin-starkware
Copy link
Contributor

crates/cairo-lang-semantic/src/resolve/mod.rs line 120 at r6 (raw file):

    pub deref_functions: Vec<(FunctionId, Mutability)>,
    /// The tail of the current computation computation. The search for additional members will
    /// continue from this point.

Suggestion:

    /// The tail of deref chain explored so far. The search for additional members will
    /// continue from this point.

@ilyalesokhin-starkware
Copy link
Contributor

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

                if !enriched.contains_key(member_name) {
                    enriched.insert(member_name.clone(), (member.clone(), n_deref));
                }

Suggestion:

                if enriched.entry(member_name).or_insert_with(...

@ilyalesokhin-starkware
Copy link
Contributor

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

            // member access.
            break;
        }

Why is it ok to leave the tail as None in this case?

Code quote:

        if !visited_types.insert(long_ty.intern(ctx.db)) {
            // Break if we have a cycle. A diagnostic will be reported from the impl and not from
            // member access.
            break;
        }

Copy link
Contributor

@ilyalesokhin-starkware ilyalesokhin-starkware 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 2 files at r1, 1 of 1 files at r2, 1 of 2 files at r4.
Reviewable status: 0 of 2 files reviewed, 6 unresolved discussions (waiting on @gilbens-starkware and @orizi)

@ilyalesokhin-starkware
Copy link
Contributor

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

    let expr_id = calc_tail.expect("`enrich_members` should be called with a `calc_tail` value.");
    *calc_tail = None;

make sure the comment above the exploration tail is consistent with the new name.

Suggestion:

 // This function either finds a member and sets the exploration_tail or finishes the exploration and
 // leaves that exploration tail as None
 *exploration_tail = None;

@orizi orizi force-pushed the orizi/enrich-member-caching branch from 95274b9 to 001b5b8 Compare September 5, 2024 11:58
Copy link
Collaborator Author

@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 2 files reviewed, 7 unresolved discussions (waiting on @gilbens-starkware and @ilyalesokhin-starkware)


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

Previously, ilyalesokhin-starkware wrote…

make sure the comment above the exploration tail is consistent with the new name.

Done.


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

                if !enriched.contains_key(member_name) {
                    enriched.insert(member_name.clone(), (member.clone(), n_deref));
                }

Done.


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

Previously, ilyalesokhin-starkware wrote…

Why is it ok to leave the tail as None in this case?

because members won't be properly found anyway.


crates/cairo-lang-semantic/src/resolve/mod.rs line 120 at r6 (raw file):

    pub deref_functions: Vec<(FunctionId, Mutability)>,
    /// The tail of the current computation computation. The search for additional members will
    /// continue from this point.

Done.

Copy link
Contributor

@ilyalesokhin-starkware ilyalesokhin-starkware left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @gilbens-starkware)

@orizi orizi added this pull request to the merge queue Sep 5, 2024
Merged via the queue into main with commit 3f92254 Sep 5, 2024
43 of 44 checks passed
orizi added a commit that referenced this pull request Sep 8, 2024
@orizi orizi deleted the orizi/enrich-member-caching branch September 9, 2024 06:22
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