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

do not match generic invocations to their base body types #24690

Draft
wants to merge 6 commits into
base: devel
Choose a base branch
from

Conversation

metagn
Copy link
Collaborator

@metagn metagn commented Feb 15, 2025

fixes #24688, refs #5632

This would make sense if generic body types as the type of expressions are purely symbolic and never valid for an actual generic expression, which should produce some invocation type instead (liftParamType produces a generic invocation with args as tyAnything or the param constraints). Not sure if there is a valid counter case to this.

#5632 is a counter case which no longer works, but it doesn't really seem like a valid case, unless we still want to support it.

PR to arraymancer: mratsim/Arraymancer#674, doesn't mean I'm sure this is correct though

@Graveflo
Copy link
Contributor

Graveflo commented Feb 18, 2025

try this as the if statement's body:

    elif x.kind == tyGenericBody and f[0] == x:
      result = isGeneric
      for i in 1 ..< len(f):
        if not f[i].acceptsAllTypes:
          # not specific enough
          result = isNone
          break

with this defined in types.nim:

proc acceptsAllTypes*(t: PType): bool =
  result = false
  if t.kind == tyAnything:
    result = true
  elif t.kind == tyGenericParam:
    if tfImplicitTypeParam in t.flags:
      result = true
    if not(t.hasElementType) or t.elementType.kind == tyNone:
      result = true

actually this is similar to your first commit but there is no CI output. what happened?

@metagn
Copy link
Collaborator Author

metagn commented Feb 18, 2025

I opened the PR after the second commit, also CI is fine outside of that arraymancer PR (arraymancer CI is broken) there was just a lot of httpclient failures. The first commit didn't work for any "working" cases either.

A problem with matching Foo[T] etc is that T cannot be inferred, and the case where it already is inferred should fail since it's a specific type. The tyCompositeTypeClass case should probably directly match itself but I don't know if it's needed. Most of the use cases here are to match the type Foo symbolically and are contained in typedesc which does not transform to tyCompositeTypeClass.

@Graveflo
Copy link
Contributor

Graveflo commented Feb 18, 2025

Ok that makes sense. So in the case of #5632 that should actually fail because it's attempting to match Option[T] with Option[int]. I didn't notice that. I guess if it were to work it would have to be a kind of inference, but I don't know how that could work exactly. Not that I want it to work anyway. Thanks for explaining.

@Araq
Copy link
Member

Araq commented Mar 28, 2025

So what can we do to bring this over the finish line?

@metagn metagn marked this pull request as draft March 29, 2025 00:22
@metagn
Copy link
Collaborator Author

metagn commented Mar 29, 2025

Was waiting on mratsim/Arraymancer#674 to be merged, testing to see if it actually works since arraymancer's own CI is broken

@metagn
Copy link
Collaborator Author

metagn commented Mar 29, 2025

It does work but we should probably still wait for it to get merged as it would be bad to break arraymancer for this.

@arnetheduck
Copy link
Contributor

unless we still want to support it.

hm, unless there's a strong reason not to, it does look .. convenient to support - ie the resolution can be inferred based on the information present therefore it should be (or rather, why not?)

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

Successfully merging this pull request may close these issues.

cannot instantiate T, even when matching overload that doesn't require T exists
4 participants