-
Notifications
You must be signed in to change notification settings - Fork 451
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::finalize: return multiple inference diagnostics #5281
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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 25 files reviewed, 2 unresolved discussions (waiting on @yuvalsw)
crates/cairo-lang-semantic/src/expr/inference.rs
line 396 at r1 (raw file):
default_stable_ptr: SyntaxStablePtrId, ) { let mut reported = HashSet::new();
.
Suggestion:
let mut reported = UnorderedHashSet::new();
crates/cairo-lang-semantic/src/expr/inference.rs
line 413 at r1 (raw file):
InferenceError::Ambiguity(_) => 11, InferenceError::TypeNotInferred { .. } => 12, };
Suggestion:
let err_kind = std::mem::discriminant(err);
9dda017
to
4c407fa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 25 files at r1, all commit messages.
Reviewable status: 3 of 25 files reviewed, 6 unresolved discussions (waiting on @yuvalsw)
crates/cairo-lang-semantic/src/types.rs
line 546 at r2 (raw file):
let mut inference = inference_data.inference(db); let impl_id = inference.new_impl_var(concrete_trait_id, stable_ptr, lookup_context)?; if let Some(errs) = inference.finalize() {
Here not returning all is fine?
crates/cairo-lang-semantic/src/expr/inference.rs
line 368 at r2 (raw file):
#[derive(Debug)] pub struct InferenceErrors { errors: Vec<(Option<SyntaxStablePtrId>, InferenceError)>,
Doc all around
crates/cairo-lang-semantic/src/expr/inference.rs
line 612 at r2 (raw file):
inference_errors.push(self.stable_ptrs.get(&var).copied(), err); } // TODO(yg): change later to vector directly, without option.
.
crates/cairo-lang-semantic/src/expr/inference.rs
line 622 at r2 (raw file):
if self.type_assignment(LocalTypeVarId(id)).is_none() { let ty = self.db.intern_type(TypeLongId::Var(*var)); // TODO(yg): do the pops better (all remove(0)s).
Using a deque would work?
4c407fa
to
73f5b8b
Compare
73f5b8b
to
d82ca45
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 14 of 25 files at r1, 10 of 10 files at r4, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @yuvalsw)
crates/cairo-lang-starknet/src/contract.rs
line 221 at r4 (raw file):
.to_option()??; assert!( resolver.inference().finalize().is_none(),
consider adding a Debug::fmt
instead of this change.
This change is