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

include generic bodies in allowMetaTypes #23968

Merged
merged 1 commit into from
Aug 20, 2024

Conversation

metagn
Copy link
Collaborator

@metagn metagn commented Aug 16, 2024

fixes #19848

Not sure why this wasn't the case already. The if cl.allowMetaTypes: return line below for tyFromExpr was added 10 years ago. Hopefully it was just negligence?

@Araq
Copy link
Member

Araq commented Aug 16, 2024

Why do we need allowMetaTypes at all?

@metagn
Copy link
Collaborator Author

metagn commented Aug 16, 2024

I'm not too familiar with it, in general it seems to be to produce tyGenericInst for unresolved param types instead of tyGenericInvocation/tyGenericBody for easier type checking?

For this instance it's triggered here in the only call to prepareMetatypeForSigmatch. The code around it is strongly implying it's a hack and should be moved to liftParamType but liftParamType also uses it to convert tyGenericBody to a tyGenericInst in a tyCompositeTypeClass and tyGenericInvocation to a resolved concept type.

I think @zerbina has managed to remove it in case they have any insight, as far as I can tell it would mostly involve adding more logic to typeRel for tyGenericBody and tyGenericInvocation to mirror what tyGenericInst does but maybe this comes with some compromises/technical challenges.

@zerbina
Copy link
Contributor

zerbina commented Aug 16, 2024

I'm not too familiar with it, in general it seems to be to produce tyGenericInst for unresolved param types instead of tyGenericInvocation/tyGenericBody for easier type checking?

That, and, like you said, also for the tyCompositeTypeClass lifting.

I think @zerbina has managed to remove it in case they have any insight

It's been a while since I've worked on that area of the compiler, and I don't remember all the details. There's an issue I created back then that goes into more detail and also references the various prerequisites for removing allowMetaTypes. The relevant PRs should contain more information about the various challenges encountered on the way.

The existence of allowMetaTypes has far reaching consequences, and its removal made significant stability improvements possible.

@Araq Araq merged commit 1befb8d into nim-lang:devel Aug 20, 2024
18 checks passed
Copy link
Contributor

Thanks for your hard work on this PR!
The lines below are statistics of the Nim compiler built from 1befb8d

Hint: mm: orc; opt: speed; options: -d:release
173486 lines; 8.075s; 654.527MiB peakmem

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.

Can't implicitly instantiate proc with generic params and generic type constraint
3 participants