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

Traits unification #1625

Merged
merged 206 commits into from
Sep 9, 2024
Merged

Traits unification #1625

merged 206 commits into from
Sep 9, 2024

Conversation

gzanitti
Copy link
Collaborator

No description provided.

@gzanitti gzanitti requested a review from chriseth September 4, 2024 19:29
@chriseth
Copy link
Member

chriseth commented Sep 5, 2024

Just saw that side_effect_check also needs to take the impls into account. Maybe there is some code to be shared between side_effect_check and the trait checks?

// We filter out enum type declarations (the constructor functions have been added
// by the statement processor already).
// For Arrays, we also collect the inner expressions and expect them to be field elements.

for (name, trait_impls) in self.implementations.iter_mut() {
let (_, def) = self
.definitions
.get(name)
.get_mut(name)
Copy link
Member

Choose a reason for hiding this comment

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

Why do you need a mutable reference here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The generic type that I am going to “specialize” using the implementation, is stored in the trait declaration, that's why the mutability.

.function_by_name_mut(&named_expr.name)
.expect("Function not found in trait declaration");

trait_fn.ty.substitute_type_vars(&type_var_mapping);
Copy link
Member

Choose a reason for hiding this comment

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

Ah, now I see! Does it even have a type? Shouldn't we just take the type from the trait and then let the type inferenec algorithm do its job?

Copy link
Member

Choose a reason for hiding this comment

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

I.e. just return trait_type.clone().substitute_type_vars(&type_var_mapping)?

Copy link
Member

Choose a reason for hiding this comment

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

And then I think the type inference algorithm will store the type anyway at the end.

panic!("Expected trait declaration");
};

pub fn specialize_trait_type(&self, trait_decl: &TraitDeclaration, fn_name: &str) -> Type {
Copy link
Member

Choose a reason for hiding this comment

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

Nice! And now I think we could rename this to type_of_function.

@@ -220,7 +220,9 @@ impl PILAnalyzer {
for impl_ in impls {
Copy link
Member

Choose a reason for hiding this comment

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

Is this inside the loop over the definitions?

@chriseth chriseth enabled auto-merge September 9, 2024 13:29
@chriseth chriseth added this pull request to the merge queue Sep 9, 2024
Merged via the queue into powdr-labs:main with commit 73f49a4 Sep 9, 2024
14 checks passed
@gzanitti gzanitti deleted the traits_unification branch September 18, 2024 11:01
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