Skip to content

fix: constants unify with implicitly added named generics#8279

Closed
asterite wants to merge 6 commits intomasterfrom
ab/implicit-generics-unification
Closed

fix: constants unify with implicitly added named generics#8279
asterite wants to merge 6 commits intomasterfrom
ab/implicit-generics-unification

Conversation

@asterite
Copy link
Collaborator

Description

Problem

Resolves #8252

Summary

Additional Context

I think unbound named generics or unbound type variables should always unify with constants, but I just added the minimum necessary thing to solve the issue... mainly because I'm not entirely sure that's true.

Documentation

Check one:

  • No documentation needed.
  • Documentation included in this PR.
  • [For Experimental Features] Documentation to be submitted in a separate PR.

PR Checklist

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt on default settings.

@asterite asterite requested a review from a team April 29, 2025 17:46
@TomAFrench TomAFrench added this pull request to the merge queue Apr 29, 2025
@jfecher jfecher removed this pull request from the merge queue due to a manual request Apr 29, 2025
| (
Type::NamedGeneric(types::NamedGeneric { type_var, implicit: true, .. }),
Constant(_, kind),
) if type_var.borrow().is_unbound() => kind.unify(&type_var.kind()),
Copy link
Contributor

Choose a reason for hiding this comment

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

So if the type variable is unbound we don't unify it - we only unify it's Kind?
So the original issue was that the constant kind and the type variable's Kind didn't match - maybe because the constant's kind was inferred?

I'm not sure this is the right solution. This'd let any implicitly added named generic unify with any constant value. E.g. you could unify it with both 5 and 8 as long as they all had the same kind. Usually we also don't bind NamedGenerics at all until monomorphization. I suppose binding the kind isn't bad but there's nothing preventing this from unifying against 5 and 8.

I haven't looked into it too much so I don't know what issue the original example stemmed from - if it was a constant with a polymorphic kind or maybe the kind in the NamedGeneric was polymorphic for some reason - but I don't think we should be unifying NamedGenerics with constants unless we know that NamedGeneric must be that constant. It'd be better to go all the way and actually unify type_var with the constant that way when you unify with 5 it'll later fail when you unify again with 8 - although I'm not 100% on this either and would need to investigate some more.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks! Do you mind taking a look at this, then? There's a small test for this in the frontend tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, I just got to a stopping point with the SSA interpreter. I'll take a look at this tomorrow.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thank you! There's another PR that's related to this: #8184 . There I changed the equality of these implicitly added named generics should it's maybe safer or more correct, though now I realize I didn't check their kind (but I won't fix that until understanding a bit more about all of this)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This'd let any implicitly added named generic unify with any constant value. E.g. you could unify it with both 5 and 8 as long as they all had the same kind.

What's a code example for this?

Copy link
Contributor

@jfecher jfecher May 7, 2025

Choose a reason for hiding this comment

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

trait TraitWithAssociatedConstant{
    let N: u32;

    fn foo(_: Self) -> bool {
        true
    }

    fn make_array(self) -> [Field; Self::N];
}

struct Foo{}

impl TraitWithAssociatedConstant for Foo {
    let N: u32 = 5;

    fn make_array(self) -> [Field; Self::N] {
        std::mem::zeroed()
    }
}


struct Wrapper<T>{
    inner: T
}

impl<U> std::cmp::Eq for Wrapper<U>
where
    U: TraitWithAssociatedConstant,
{

    fn eq(self, _other: Self) -> bool {
        let _array1: [Field; 5] = self.inner.make_array();
        let _array2: [Field; 6] = self.inner.make_array(); // get the same method to return an array of a different size
        self.inner.foo()
    }
}


fn main() {
    let wrapper = Wrapper{ inner: Foo {}};
    assert_eq(wrapper, wrapper);
}

Type checks with this PR for example (although it fails during monomorphization with "Could not determine array length ..."

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks! I changed the code to still unify named generics with constants, but now they are tried to be unified with bindings, etc. (I copied the code from a bit above, I don't fully understand how type bindings work though I have an intuition). Now the old test still passes, and the program you put above fails to compile.

Would that be the correct solution?

Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately not, I tried that when I first looked at this PR but realized that'd permanently bind the generic when we don't want to bind named generics at all.

E.g. add another trait call

let wrapper = Wrapper{ inner: Bar {}};
assert_eq(wrapper, wrapper);

To another empty struct Bar with an identical impl but with let N: u32 = 6; and it now fails on Bar.

@asterite asterite closed this May 13, 2025
@asterite asterite deleted the ab/implicit-generics-unification branch May 13, 2025 14: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.

Using traits with associated constants in trait bounds in trait impls results in No matching impl found

3 participants