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

new-style concepts adjusments #24697

Open
wants to merge 10 commits into
base: devel
Choose a base branch
from
Open

Conversation

Graveflo
Copy link
Contributor

@Graveflo Graveflo commented Feb 16, 2025

Yet another one of these. Multiple changes piled up in this one. I've only minimally cleaned it for now (debug code is still here etc). Just want to start putting this up so I might get feedback. I know this is a lot and you all are busy with bigger things. As per my last PR, this might just contain changes that are not ready.

concept instantiation uniqueness

It has already been said that concepts like ArrayLike[int] is not unique for each matching type of that concept. Likewise the compiler needs to instantiate a new proc for each unique bound type not each unique invocation of ArrayLike

generic parameter bindings

Couple of things here. The code in sigmatch has to give it's bindings to the code in concepts, else the information is lost in that step. The code that prepares the generic variables bound in concepts was also changed slightly. Net effect is that it works better.
I did choose to use the LayedIdTable instead of the seqs in concepts.nim. This was mostly to avoid confusing myself. It also avoids some unnecessary movings around. I wouldn't doubt this is slightly less performant, but not much in the grand scheme of things and I would prefer to keep things as easy to understand as possible for as long as possible because this stuff can get confusing.

various fixes in the matching logic

Certain forms of modifiers like var and generic types like tyGenericInst and tyGenericInvocation have logic adjustments based on my testing and usage

signature matching method adjustment

This is the weird one, like my last PR. I thought a lot about the feedback from my last attempt and this is what I came up with. Perhaps unfortunately I am preoccupied with a slight grey area. consider the follwing:

type
  C1 = concept
    proc p[T](s: Self; x: T)
  C2[T] = concept
    proc p(s: Self; x: T)

It would be temping to say that these are the same, but I don't think they are. C2 makes each invocation distinct, and this has important implications in the type system. eg C2[int] is not the same type as C2[string] and this means that signatures are meant to accept a type that only matches p for a single type per unique binding. For C1 all are the same and the binding p accepts multiple types. There are multiple variations of this type classes, tyAnything and the like.

The make things more complicated, an implementation might match:

type
  A = object
  C3 = concept
    proc p(s: Self; x:  A)

if the implementation defines:

proc p(x: Impl; y: object)

while a concept that fits C2 may be satisfied by something like:

proc p(x: Impl; y: int)
proc spring[T](x: C2[T])

it just depends. None of this is really a problem, it just seems to provoke some more logic in concepts.nim that makes all of this (appear to?) work. The logic checks for both kinds of matches with a couple of caveats. The fist is that some unbind-able arrangements may be matched during overload resolution. I don't think this is avoidable and I actually think this is a good way to get a failed compilation. So, first note imo is that failing during binding is preferred to forcing the programming to write annoying stub procs and putting insane gymnastics in the compiler. Second thing is: I think this logic is way to accepting for some parts of overload resolutions. Particularly in checkGeneric when disambiguation is happening. Things get hard to understand for me here. I made it so the implicit bindings to not count during disambiguation. I still need to test this more, but the thought is that it would help curb excessive ambiguity errors.

Again, I'm sorry for this being so many changes. It's probably inconvenient.

@Graveflo Graveflo marked this pull request as draft February 16, 2025 19:58
@Graveflo
Copy link
Contributor Author

I think I over-complicated it. Instead of checkGeneric doing a different kind of matching it will just return true to devalue the concept match. This is what makes the most sense to me since if the overloads are meant to cooperate the concept is almost certainly meant to be a baseline. It's unclear what it means to do checkGeneric logic in the concept matching. That is why the catch all types were automatically returning false before. So the logic for check generic is:

  • if the arg is a concept do concept subset matching
  • if the arg is tyAnything or equivalent return false
  • else return true

@Graveflo Graveflo marked this pull request as ready for review February 18, 2025 03:13
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.

1 participant