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

bind generic param/or to constraint for conversion match #24250

Open
wants to merge 1 commit into
base: devel
Choose a base branch
from

Conversation

metagn
Copy link
Collaborator

@metagn metagn commented Oct 7, 2024

fixes #4858, fixes #10027, fixes #12552, fixes #15721, fixes #21331, fixes but does not test #18121 (maybe it should fail the match in this case?), refs #1214, follows up #24216, split from #24198

When an or type or a generic param constraint match a type with a convertible match, bind it to the branch that was deemed convertible rather than the matched type, so that a conversion can be generated. There were some required changes to make this work.

or types now choose a "best matching" branch to use for the conversion. This branch is the earliest branch in the type list with the highest level type match. The "earliest" choice resolves the ambiguity in cases like matching an integer literal against int16 | uint16. The compiler used to compile on these cases before (by just matching them as int), so to keep code working this ambiguity has to be resolved.

When matching types to the constraint type of generic parameters, the compiler previously disabled all bindings to typeclasses (including or types). This is so the bindings for the typeclasses in the constraint don't leak into the entire candidate, i.e. the binding for T: SomeInteger should not be used for all SomeIntegers afterward. However this means we can't reuse the binding from an or typeclass when it's a convertible match, because we will lose both the information of it being a conversion match (since it becomes a generic match instead) and which branch of the or type matched (i.e. the type to convert to).

To deal with this, we push a new type binding layer (see the refactor in #24216) for generic parameter constraints which contains all bindings to itself (except generic parameter bindings which are propagated up to the root). Then, if the constraint is an or type, we bind the generic param to the type that the or type was bound to in the type binding layer. We may not need to restrict to or types here and just use the binding for any type that has a binding, but this is risky and I can't think of a reason any other typeclass would give something other than the matched type.

There was also code that skipped range types for all bound generic types as mentioned in #10027, but this code broke the test for #10027, so it is removed, i.e. range types can be bound to typeclasses now. Fixing the other issues may have removed the need for this skip, I don't know why it was there.

or types also do not consider any type match below isIntConv, for example isConvertible, to match, which is the cause of #4902, #15722 and #18697 (not sure about iterable though). If this was just a limitation caused by the issues fixed in this PR (seems to be) then maybe we can loosen it to fix these issues, i.e. make isConvertible match for or types. But in another PR.

@metagn metagn marked this pull request as ready for review October 7, 2024 08:55
@Araq
Copy link
Member

Araq commented Oct 7, 2024

This is so the bindings for the typeclasses in the constraint don't leak into the entire candidate, i.e. the binding for T: SomeInteger should not be used for all SomeIntegers afterward.

To me this implies that the SomeInteger typeclass needs to be copied rather than shared inside a proc type.

@metagn
Copy link
Collaborator Author

metagn commented Oct 7, 2024

To me this implies that the SomeInteger typeclass needs to be copied rather than shared inside a proc type.

I was only guessing the reason for that behavior, I just figured it was a consequence of Nim's bind-once behavior in general, as evidenced by the test in bf4ce87 where it was added.

The bindings are still bound to the IDs regardless, we would need to guarantee that every typeclass inside a generic constraint has a copied ID and maybe these copied IDs would even have to be equal for the same types in the same generic constraint if we want to keep some of the bind-once behavior. The guarantee of "nothing bound in this context except generic parameters will matter" seems easier to maintain than "every use of a typeclass will have its own ID".

Arguably the only things that should ever bind are generic parameters but no binding for the or type here would mean we wouldn't know what type to convert to. A very shallow idea of a fix for this would be to make typeRel return a matched type along with a match kind, which is a huge refactor for just the first problem that comes to my head.

@metagn
Copy link
Collaborator Author

metagn commented Oct 22, 2024

Another issue popped up that I think has a similar solution: #24337

It points out a small issue in this PR though: the putRecursive for generic parameters should only apply to generic parameters that belong to the current candidate. Since we don't open new layers for anything that has its own generic parameters yet (as we would in #24337 for tyCompositeTypeClass) this hasn't caused an issue here.

I'm not sure if a first version of the check for "generic parameter belongs to the current candidate" would be perfectly reliable, so maybe this can be tackled in another PR. Alternatively, we can implement this binding behavior for generic parameters first, then have separate PRs for actually creating the new binding layers (i.e. this one and one for #24337).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants