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

simplify instantiation of generic types (semtypinst) #846

Closed
5 tasks done
zerbina opened this issue Aug 18, 2023 · 1 comment · Fixed by #850, #854, #858, #861 or #864
Closed
5 tasks done

simplify instantiation of generic types (semtypinst) #846

zerbina opened this issue Aug 18, 2023 · 1 comment · Fixed by #850, #854, #858, #861 or #864
Assignees
Labels
compiler/sem Related to semantic-analysis system of the compiler simplification Removal of the old, unused, unnecessary or un/under-specified language features.

Comments

@zerbina
Copy link
Collaborator

zerbina commented Aug 18, 2023

Goal

Have semtypinst only produce fully resolved non-meta types (i.e., types that don't contain any unresolved generic parameters, type-classes, any, etc.).

Why?

At present, the type parameter application implemented in semtypinst supports two modes: with meta-types and without (signaled by the allowMetaTypes option). The largest differences between both modes is that in "meta-types allowed" mode:

  • simple copies are created (via exactReplica) instead of proper ones
  • expressions in types (ranges, dependent types, etc.) are skipped
  • no types are committed to the instantiation cache
  • interaction with ModuleGraph state (e.g., hooks) is disabled

A different way to look at the "allow meta-type" mode is that it allows for replacing type variables while some of them may be still unknown.

Two problems that arise from this:

  1. there a two paths through semtypinst, which makes the code very non-linear, and thus hard to follow
  2. the types coming out of semtypinst in "meta-types allowed" are improper ones, whose presence complicates other things (for example, the sameObjectType logic, which now can't rely on ID comparisons)

The reason for meta-tyGenericInst types existing seems to mainly be for the convenience of typeRel, which lacks proper support for unapplied tyGenericInvocations (a tyGenericInst is effectively an evaluated tyGenericInvocation).

Concrete benefits

Not having the "allow meta-type" mode would significantly simplify the logic in semtypinst and make the code easier to reason about. In addition:

  • tyGenericInst would change to always represent fully resolved non-meta types
  • while some exactReplica usages are likely going to be still required, a lot of them can removed

Both, in turn, would either help with or make possible further improvements to semtypinst and other type-related fixes.

How?

Removing the "meta-types allowed mode" will first require moving all of its users. These are (in no specific order):

Many of the aforementioned boil down to duplicating and adjusting the relevant replaceTypeVarsT and handleGenericInvocation parts into standalone procedures.

@zerbina zerbina added 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 18, 2023
@zerbina zerbina self-assigned this Aug 18, 2023
@zerbina zerbina linked a pull request Aug 31, 2023 that will close this issue
@zerbina
Copy link
Collaborator Author

zerbina commented Sep 1, 2023

All the users of the "meta-types allowed" mode are now removed. I'll make a PR that removes the mode itself and after that focus on cleaning up / reworking semtypinst.

github-merge-queue bot pushed a commit that referenced this issue Sep 1, 2023
## Summary

With all users of the mode gone, the mode itself is removed, preventing
it from becoming accidentally relied on again.

## Details

* remove all usages of the `TReplTypeVars.allowMetaTypes` field
* perform some small clean-up of unused code
* remove the `allowedMetaTypes` field itself
* remove the `allowedMetaTypes` parameter from `instGenericContainer`

Closes #846
@zerbina zerbina closed this as completed Sep 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment