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

Inference queues #3347

Merged
merged 10 commits into from
Jun 11, 2023
Merged

Inference queues #3347

merged 10 commits into from
Jun 11, 2023

Conversation

spapinistarkware
Copy link
Contributor

@spapinistarkware spapinistarkware commented Jun 10, 2023

This change is Reviewable

@spapinistarkware
Copy link
Contributor Author

spapinistarkware commented Jun 10, 2023

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

@spapinistarkware spapinistarkware requested a review from orizi June 10, 2023 06:24
@spapinistarkware spapinistarkware force-pushed the spapini/06-08-Inference_queues branch from 0356a16 to d94b2eb Compare June 10, 2023 06:30
@spapinistarkware spapinistarkware force-pushed the spapini/06-08-Inference_queues branch from 1c24518 to fe7e294 Compare June 10, 2023 16:38
This was referenced Jun 10, 2023
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 22 of 33 files at r1, 5 of 5 files at r2, 6 of 6 files at r3, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @spapinistarkware)


crates/cairo-lang-semantic/src/expr/inference.rs line 115 at r3 (raw file):

    ConstInferenceNotSupported,

    // TODO: These are only used for external interface.

assign

Code quote:

    // TODO: These are only used for external interface.

crates/cairo-lang-semantic/src/expr/inference.rs line 167 at r3 (raw file):

            InferenceError::Failed(diagnostic_added) => *diagnostic_added,
            // TODO(spapini): Better save the DiagnosticAdded on the variable.
            // InferenceError::AlreadyReported => skip_diagnostic(),

?

Code quote:

// InferenceError::AlreadyReported => skip_diagnostic(),

crates/cairo-lang-semantic/src/expr/inference.rs line 488 at r3 (raw file):

        mut lookup_context: ImplLookupContext,
    ) -> InferenceResult<SolutionSet<(CanonicalImpl, CanonicalMapping)>> {
        // TODO: This is done twice.

assign

Code quote:

/ TODO: This is done twice.

crates/cairo-lang-semantic/src/expr/inference/canonic.rs line 207 at r3 (raw file):

}

// Mapper rewriter. Maps variables according to a given [VarMapping].

Suggestion:

/// Mapper rewriter. Maps variables according to a given [VarMapping].

crates/cairo-lang-semantic/src/expr/inference/conform.rs line 14 at r3 (raw file):

};

pub trait InferenceConform {

doc


crates/cairo-lang-semantic/src/expr/inference/infers.rs line 18 at r3 (raw file):

};

pub trait InferenceEmbeddings {

doc


crates/cairo-lang-semantic/src/expr/test_data/method line 109 at r3 (raw file):

      ^***********************^

error: Candidate impl test::AnotherOptionTraitImpl::<core::felt252> has a free variable

i don't think this is understandable - can you make this clearer?

Code quote:

a free variable

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

    // Check fully resolved.
    if let Some((stable_ptr, inference_err)) = resolver.inference().finalize() {
        // TODO: Better location.

assign


crates/cairo-lang-semantic/src/items/trt.rs line 227 at r3 (raw file):

    // Check fully resolved.
    if let Some((stable_ptr, inference_err)) = resolver.inference().finalize() {
        // TODO: Better location.

doc

Copy link
Contributor Author

@spapinistarkware spapinistarkware 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, 9 unresolved discussions (waiting on @orizi)


crates/cairo-lang-semantic/src/expr/inference.rs line 115 at r3 (raw file):

Previously, orizi wrote…

assign

Done.


crates/cairo-lang-semantic/src/expr/inference.rs line 167 at r3 (raw file):

Previously, orizi wrote…

?

Done.


crates/cairo-lang-semantic/src/expr/inference.rs line 488 at r3 (raw file):

Previously, orizi wrote…

assign

Done.


crates/cairo-lang-semantic/src/expr/inference/canonic.rs line 207 at r3 (raw file):

}

// Mapper rewriter. Maps variables according to a given [VarMapping].

Done.


crates/cairo-lang-semantic/src/expr/inference/conform.rs line 14 at r3 (raw file):

Previously, orizi wrote…

doc

Done.


crates/cairo-lang-semantic/src/expr/inference/infers.rs line 18 at r3 (raw file):

Previously, orizi wrote…

doc

Done.


crates/cairo-lang-semantic/src/expr/test_data/method line 109 at r3 (raw file):

Previously, orizi wrote…

i don't think this is understandable - can you make this clearer?

Done.


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

Previously, orizi wrote…

assign

Done.


crates/cairo-lang-semantic/src/items/trt.rs line 227 at r3 (raw file):

Previously, orizi wrote…

doc

Done.

@spapinistarkware spapinistarkware force-pushed the spapini/06-08-Inference_queues branch from 212a2c1 to 1f6df34 Compare June 11, 2023 05:40
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 7 of 8 files at r4.
Reviewable status: 30 of 34 files reviewed, 1 unresolved discussion (waiting on @spapinistarkware)


crates/cairo-lang-semantic/src/expr/inference/solver.rs line 51 at r4 (raw file):

            Ambiguity::FreeVariable { impl_id, var: _ } => {
                format!(
                    "Candidate impl {:?} has a free variable with no constraints.",

Suggestion:

Candidate impl {:?} has a generic param with no constraints.

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 8 files at r4, 1 of 4 files at r5.
Reviewable status: 31 of 34 files reviewed, 1 unresolved discussion (waiting on @spapinistarkware)

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 4 files at r5.
Reviewable status: 33 of 34 files reviewed, all discussions resolved (waiting on @spapinistarkware)

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 1 of 4 files at r5, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @spapinistarkware)

@spapinistarkware spapinistarkware added this pull request to the merge queue Jun 11, 2023
Merged via the queue into main with commit b0954e6 Jun 11, 2023
@orizi orizi deleted the spapini/06-08-Inference_queues branch June 28, 2023 05:35
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