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

internal: keep composite type-classes unresolved #864

Merged

Conversation

zerbina
Copy link
Collaborator

@zerbina zerbina commented Aug 31, 2023

Summary

Change composite type-classes (tyCompositeTypeClass) to store an
unresolved type application (tyGenericInvocation) instead of a fully
resolved and instantiated meta-type (tyGenericInst). This removes the
last usage of semtypinst's "meta-types allowed" mode, meaning that
the latter can be removed now.

As part of the change, a type relation bug is fixed: generic
instantiations were incorrectly treated as having a 'generic'
relationship with composite type-classes of different generics that had
the same structure. For example, A[int] is B returned 'true' when
A[T] = (T,) and B[T] = (T,).

Details

  • update the documentation of tyCompositeTypeClass
  • in type lifting (liftParamType), don't resolve the invocation lifted
    from the tyGenericBody, but instead store the invocation in the
    tyCompositeTypeClass directly
  • update typesrenderer to render a tyCompositeTypeClass as the
    original type expression (which is stored in the first slot)
  • partially resolve the invocations stored in tyCompositeTypeClass
    when skipping them during .borrow handling, approximating how
    the skipped type would have looked previously

Parametric aliases of parametric concepts

  • support non-lifted invocations of parametric user type-classes
    (concept) with matchUserTypeClass
  • in typeRel, dispatch to matchUserTypeClass when matching against
    such invocations

Invocations of parametric concepts without arguments are lifted into
tyUserTypeClassInst instead of tyCompositeTypeClass, but for
parametric aliases of parametric concepts (e.g.,
Type[T] = Concept[T] where a Concept is a parametric concept), this
is not case, meaning that tyGenericInvocations where the body is a
tyUserTypeClass reach typeRel.

Parametric aliases of parametric concepts in the composite type-class
case were previously handled by way of them being resolved to
tyUserTypeClassInst as part of instGenericContainer, but without the
latter, they now need to be handled in typeRel, which is what changes
detailed above implement.

However, there is a problem. A tyUserTypeClassInst can bind types
for later retrieval, while a tyGenericInvocation cannot. While a
solution is going to be needed at some point, no user-facing regression
is introduced with the changes here, as tyCompositeTypeClass types in
routine signatures act as type variables (they're replaced with their
bound type during instantiation) and for conversions-to-composite-type-
classes (e.g., Generic(x)), the type bound to the resolved
tyUserTypeClassInst was already ignored previously.

Type relation changes

The typeRel logic for tyCompositeTypeClass is changed back to how it
was prior to e97d640, which is both
now-necessary (for proper matching against a tyGenericInvocation's)
and fixes a type relation bug. By skipping the first instance, that
commit changed generic instantiation to be treated as having a 'generic'
relationship with different but structurally equal generic types, which
seems to have been an unintended side-effect. A test is added for the
fixed behaviour is added.

Coercions to distinct generic types are kept working by allowing generic
instances to be matched against the partially-resolved body of a
generic distinct type -- if coerceDistincts is 'false', the call to
typeRel with formal = tyDistinct is going to return isNone.

Instead of storing a fully instantiated meta-type (`tyGenericInst`
containing type variables) in the last position of `tyCompositeTypeClass`,
use a `tyGenericInvocation`. This removes the last usage of `semtypinst`'s
"meta-types allowed" mode.

Rendering of `tyCompositeTypeClass` (`typesrenderer`), their type-
relation logic (`sigmatch`), and `inferWithMetatype` are adjusted to
account for this change.

Not skipping the first generic instance of a `tyCompositeTypeClass` in
`typeRel` also fixes a type relation bug with generic types.
Invocations of parametric user-type classes not collapsed
into `tyUserTypeClassInst` weren't properly considered and simply used
the fall-through case.

They're now special-cased in `tyGenericInvocation`s handling and
support for passing `tyGenericInvocation`s (which is more or less
identical to `tyUserTypeClassInst` in the case of `tyUserTypeClass`)
is added to `matchUserTypeClass`.
In simple cases (e.g., `type Typ[T] = distinct T`), it should work the same
as it did before, but in more complex cases, it likely doesn't.
Since the matching `tyCompositeTypeClass` doesn't skip the invocation/
instance anymore.
@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 31, 2023
@zerbina zerbina requested a review from saem August 31, 2023 18:28
@zerbina zerbina linked an issue Aug 31, 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.

The new behaviour where different generic types don't match makes sense, it's not very clear when we should use nominal vs structural matching and the nominal behaviour is a good default.

I'm really glad to see the meta types allowed mode being dropped. 🚀

@saem
Copy link
Collaborator

saem commented Aug 31, 2023

/merge

@github-actions
Copy link

Merge requested by: @saem

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


Notes for Reviewers

  • wraps up removing users of semtypinst's "meta-types allowed" mode
  • I'm going to remove the "meta-types allowed" mode of semtypinst in a separate PR

@chore-runner chore-runner bot added this pull request to the merge queue Aug 31, 2023
Merged via the queue into nim-works:devel with commit f228a27 Aug 31, 2023
18 checks passed
@zerbina zerbina deleted the composite-type-class-keep-unresolved branch September 1, 2023 15:48
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