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

typerel: rework tyGenericInvocation handling #854

Merged

Conversation

zerbina
Copy link
Collaborator

@zerbina zerbina commented Aug 24, 2023

Summary

Make the type relationship computation (typeRel) for instantiated
generic types (tyGenericInst) and unresolved type applications
(tyGenericInvocation) more consistent with each other, which, apart
from removing another dependency on semtypinst's "meta-types allowed"
mode, is also a preparation for fixing the handling of phantom types.

Fixes:

  • invocations of types like Body[T] = T.param and
    Body[N] = range[0..(N+1)] with generic parameters in routine
    signatures now work properly during parameter matching
  • for the procedure with the signature proc f[T](x: Type[Type[T]])
    and Type[A] = (A,), inferring T as X now works when passing
    a (Type[X],)

A temporary regression is that constraints of generic parameters are now
ignored when a non-generic-instance is matched against the invocation of
a generic type.

Details

Key changes:

  • typeRel doesn't directly use prepareMetatypeForSigmatch anymore,
    which is another step towards removing semtypinst's "meta-types
    allowed" mode
  • the replaceTypeVarsInType procedure is added, which replaces type
    variables without also resolving tyGenericInvocations
  • tyGenericInvocation handling in typeRel is now much more similar
    to that of tyGenericInst

When computing the relationship with a tyGenericInvocation (e.g., a
Type[T] in a routine signature where T is a generic parameter) and:

  • the argument type is an array type or an instantiated generic type
    (i.e., tyGenericInst)
  • typeRel happens in the context of routine signature parameter type
    matching

the invocation was first resolved via prepareMetatypeForSigmatch and
the argument type then matched against the instantiated type. The reason
for this seems to have been being able to consistently handle generic
aliases (by skipping them) and, in the case of arrays, that static
parameter inference works.

What prepareMetatypeForSigmatch does is create a fully instantiated
tyGenericInst for a tyGenericInvocation, using the "meta-types
allowed" mode of semtypinst. For example, the instance produced for
Body[A] = (A, A) with an invocation like Body[T] would be a (T, T)
type.

Reworked handling

In order to not having to rely on the "meta-types allowed" mode, the
handling of tyGenericInvocation is reworked to be the following (f
is the formal type and a the argument type after generic alias are
skipped):

  1. if a is a tyGenericInvocation, the behaviour is the same as
    previously
  2. if a is a tyGenericInst and it's an instance of the generic type
    that the invocation invokes, their relation is that of their
    arguments (the comparison works the same as for two tyGenericInst)
  3. if the formal invocation is of an object type (including ref and
    ptr versions), f and a have a sub-type relationship if a is a
    sub-type of f, otherwise there's no relation
  4. if f is an invocation of a and|or type or of another
    invocation and a is a tyGenericInst, there is no relationship
    (this mirrors the tyGenericInst-against-tyGenericInst handling)
  5. otherwise (i.e., none of the above were true), the type parameters in
    f's body are replaced with the types passed to the invocation, and
    a is matched against the resulting type

The replacing in step 5. is implemented by the new
replaceTypeParamsInType procedure, which is similar to
replaceTypeVarsT, but with the important difference that:

  • it doesn't instantiate tyGenericInvocations
  • it properly handles tyFromExpr, by also replacing the type
    parameters in the attached AST
  • the attached AST for object, tuple, and proc types is not modified, as
    this is not needed for typeRel to work with the types

For the case where a is not a tyGenericInst or tyArray, the
important difference is that a is now not matched against the body of
f first, with the inferred parameters of f then being matched
against its arguments. This leads to inference working in more complex
cases, but it also means that the parameter constraints of the invoked
types are ignored. For example:

type Type[A: object] = (A,)
proc f[T](x: Type[T]) = discard

f((1,))

doesn't fail now, while it previously (correctly) did.

Related changes:

  • move the logic for comparing the arguments of two tyGenericInst
    types into a standalone procedure and adjust it to also work with
    tyGenericInvocations
  • change skipToObject to not skip over tyGenericBody, as doing
    so is almost never what is wanted
  • adjust isSubtypeOfGenericInstance to support f being a
    tyGenericInvocation
  • remove the now-obsolete isGenericSubtype
  • add a known-issue test for a pre-existing issue with generic range
    types

In preparation for the upcoming changes, the code is adjusted to also
support `tyGenericInvocation`s as both the formal and the actual type.
Instead of creating an improper instantiation to match `tyGenericInst`
and `tyArray`s against, `tyGenericInvocation` are handled directly by
`typrel`. The implementation attempts to capture the behaviour of the
path taken through the `tyGenericInst` handling for the lifted
instance.
The existing `replaceTypeVarsT` not only replaces type variables, it
also resolves invocations. For properly implementing step-by-step
matching against `tyGenericInvocation`, a procedure for only replacing
type parameters is needed, which is what `replaceTypeParamsInType`
does.
Go back to using opaque type-parameter forwarding, which fixes type
parameters in the AST of ranges and type expressions not being
replaced.

However, instead of relying on `replaceTypeVarsT` or
`prepareMetatypeForSigmatch`, the new `replaceTypeParamsInType` is
used.
While pre-existing, the cause does change slightly due to the changes in
`typeRel`.
@zerbina zerbina added bug Something isn't working compiler/sem Related to semantic-analysis system of the compiler simplification Removal of the old, unused, unnecessary or un/under-specified language features. labels Aug 24, 2023
@zerbina zerbina requested a review from saem August 24, 2023 15:58
@zerbina zerbina linked an issue Aug 24, 2023 that may be closed by this pull request
5 tasks
Copy link
Collaborator

@saem saem left a comment

Choose a reason for hiding this comment

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

I agree the temporary regression is acceptable.

Found a non-blocking very minor typo, I wouldn't bother fixing it -- could easily be done in a future PR, and since it's in 'known issue' text might be removed entirely.

Comment on lines +1620 to +1622
# XXX: this seems like a really bad idea, why should that work? The
# procedure wants a typedesc for an instantiated type, but it gets
# an (unlifted) composite-type-class
Copy link
Collaborator

Choose a reason for hiding this comment

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

It shouldn't, pretty sure it should be an error where Generic[int] is expected, but received a Generic.

Specifying the type parameter on the procedure should affect the formal parameter's type definition and interpretation of any argument (the instantiated procedure's declaration), but not the actual argument.


# XXX: since we're forwarding the arguments here, the constraints on the
# body's parameters are ignored...
let other = replaceTypeParamsInType(c.c, bindings, body)
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's a temporary regression because, it's not unreasonable to do the check as part of the replace in the future?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep, my current plan goes into that direction. In short, the idea is to, should the typeRel against the patched body return a match, perform a typeRel for all type arguments against their passed-to parameter's constraint. For that to work, typeRel is going to need some more improvements, which is why I haven't implemented it as part of this PR.


A complementary improvement could be leveraging the tyInferred mechanism (or an improved/extended version thereof) to infer the constraints for a generic parameter on definition:

type Generic[A: object] = (A, A)

proc f[T](x: Generic[T]) = ...

here T would be inferred to be constrained to an object type. This would make it possible to detect conflicting constraints on definition of a generic type/routine (instead of during parameter type matching).

knownIssue: '''
The type variable in the range expression reaches the final
``semtypinst.prepareNode`` call as something that is not detected as a
type variables
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
type variables
type variable

# test with range:
var a: range[7..10]
doAssert f(3, a) == 8
# negative test: make sure that range types not overlapping are rejected
Copy link
Collaborator

Choose a reason for hiding this comment

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

these are super valuable, nice!

@zerbina
Copy link
Collaborator Author

zerbina commented Aug 25, 2023

/merge

@github-actions
Copy link

Merge requested by: @zerbina

Contents after the first section break of the PR description has been removed and preserved below:


Notes for Reviewers

  • the regression is unfortunate, although I personally think it's acceptable for the time being

@chore-runner chore-runner bot added this pull request to the merge queue Aug 25, 2023
Merged via the queue into nim-works:devel with commit 323e1df Aug 25, 2023
18 checks passed
@zerbina zerbina deleted the typerel-improve-invocation-handling branch August 25, 2023 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working compiler/sem Related to semantic-analysis system of the compiler simplification Removal of the old, unused, unnecessary or un/under-specified language features.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

simplify instantiation of generic types (semtypinst)
2 participants