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

Changing the prioritization in output Diagnostics. #6884

Merged
merged 1 commit into from
Dec 19, 2024

Conversation

Tomer-StarkWare
Copy link
Collaborator

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

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 11 files at r1, all commit messages.
Reviewable status: 7 of 11 files reviewed, 1 unresolved discussion (waiting on @gilbens-starkware, @Tomer-StarkWare, and @TomerStarkware)


crates/cairo-lang-semantic/src/expr/test_data/closure line 430 at r1 (raw file):


//! > expected_diagnostics
error: Trait has no implementation in context: core::ops::function::Fn::<{[email protected]:5:15: 5:27}, (core::felt252,)>.

@TomerStarkware - suggestions for a better error?

Code quote:

error: Trait has no implementation in context: core::ops::function::Fn::<{[email protected]:5:15: 5:27}, (core::felt252,)>.

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

            },
        )?;

revert.

@Tomer-StarkWare Tomer-StarkWare force-pushed the tomerc/diagnostics-prioritizing branch from 5e8e0c5 to 44a4e0b Compare December 18, 2024 12:52
Copy link
Collaborator Author

@Tomer-StarkWare Tomer-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: 7 of 11 files reviewed, 1 unresolved discussion (waiting on @gilbens-starkware, @orizi, and @TomerStarkware)


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

Previously, orizi wrote…

revert.

Done.

Copy link
Collaborator

@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.

Reviewed 3 of 11 files at r1, all commit messages.
Reviewable status: 10 of 11 files reviewed, 2 unresolved discussions (waiting on @gilbens-starkware, @orizi, and @Tomer-StarkWare)


crates/cairo-lang-semantic/src/expr/test_data/closure line 430 at r1 (raw file):

Previously, orizi wrote…

@TomerStarkware - suggestions for a better error?

this is correct bar can only accept tuple of 3 not tuple of 1, if we want different error we should probably report FnTrait errors differently


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

            ));
        }
        if let Some((var, ambiguity)) = self.ambiguous.first() {

maybe ambigious last? or skip WillNotInfer at first and only report it after the types

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: 10 of 11 files reviewed, 2 unresolved discussions (waiting on @gilbens-starkware, @Tomer-StarkWare, and @TomerStarkware)


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

Previously, TomerStarkware wrote…

maybe ambigious last? or skip WillNotInfer at first and only report it after the types

i agree.


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

                return Some((InferenceVar::Type(var.id), InferenceError::TypeNotInferred(ty)));
            }
        }

save and find if it is "will not infer" - and skip if nothing else.

Code quote:

        if let Some((var, ambiguity)) = self.ambiguous.first() {
            let var = *var;
            // Note: do not rewrite `ambiguity`, since it is expressed in canonical variables.
            return Some((InferenceVar::Impl(var), InferenceError::Ambiguity(ambiguity.clone())));
        }
        for (id, var) in self.type_vars.iter().enumerate() {
            if self.type_assignment(LocalTypeVarId(id)).is_none() {
                let ty = TypeLongId::Var(*var).intern(self.db);
                return Some((InferenceVar::Type(var.id), InferenceError::TypeNotInferred(ty)));
            }
        }

@Tomer-StarkWare Tomer-StarkWare force-pushed the tomerc/diagnostics-prioritizing branch from 44a4e0b to 2e673c2 Compare December 18, 2024 14:09
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 r2, all commit messages.
Reviewable status: 4 of 11 files reviewed, 2 unresolved discussions (waiting on @gilbens-starkware, @Tomer-StarkWare, and @TomerStarkware)


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

                fallback_ret = 
                    Some((InferenceVar::Impl(*var), InferenceError::Ambiguity(ambiguity.clone())));
            }

Suggestion:

            // Note: do not rewrite `ambiguity`, since it is expressed in canonical variables.
            let ret = Some((InferenceVar::Impl(*var), InferenceError::Ambiguity(ambiguity.clone())));
            if !matches!(ambiguity, Ambiguity::WillNotInfer(_)) {
                return ret;
            } else {
                fallback_ret = ret;
            }

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 11 files at r1, 7 of 8 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @gilbens-starkware, @Tomer-StarkWare, and @TomerStarkware)

@Tomer-StarkWare Tomer-StarkWare force-pushed the tomerc/diagnostics-prioritizing branch from 2e673c2 to 6ffbe3a Compare December 18, 2024 23:16
Copy link
Collaborator Author

@Tomer-StarkWare Tomer-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: all files reviewed, 2 unresolved discussions (waiting on @gilbens-starkware, @orizi, and @TomerStarkware)


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

                fallback_ret = 
                    Some((InferenceVar::Impl(*var), InferenceError::Ambiguity(ambiguity.clone())));
            }

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

@Tomer-StarkWare Tomer-StarkWare force-pushed the tomerc/updating-contract-ABI-ptr branch 3 times, most recently from e69ea40 to 888428d Compare December 19, 2024 13:39
@Tomer-StarkWare Tomer-StarkWare changed the base branch from tomerc/updating-contract-ABI-ptr to main December 19, 2024 13:55
@Tomer-StarkWare Tomer-StarkWare force-pushed the tomerc/diagnostics-prioritizing branch from 6ffbe3a to 92467cb Compare December 19, 2024 13:55
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 4 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @gilbens-starkware, @Tomer-StarkWare, and @TomerStarkware)


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

            },
        )?;

revert.

@Tomer-StarkWare Tomer-StarkWare force-pushed the tomerc/diagnostics-prioritizing branch from 92467cb to 98d7b93 Compare December 19, 2024 15:16
Copy link
Collaborator Author

@Tomer-StarkWare Tomer-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: 10 of 11 files reviewed, 2 unresolved discussions (waiting on @gilbens-starkware, @orizi, and @TomerStarkware)


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

Previously, orizi wrote…

revert.

Done.

@Tomer-StarkWare Tomer-StarkWare force-pushed the tomerc/diagnostics-prioritizing branch from 98d7b93 to b30df80 Compare December 19, 2024 15:18
@Tomer-StarkWare Tomer-StarkWare force-pushed the tomerc/diagnostics-prioritizing branch from b30df80 to b881dc3 Compare December 19, 2024 15:21
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 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @gilbens-starkware and @TomerStarkware)

Copy link
Collaborator

@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.

Reviewed all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @gilbens-starkware)

@Tomer-StarkWare Tomer-StarkWare added this pull request to the merge queue Dec 19, 2024
Merged via the queue into main with commit 1719004 Dec 19, 2024
48 checks passed
@orizi orizi deleted the tomerc/diagnostics-prioritizing branch December 22, 2024 09:56
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.

4 participants