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

fix crash with tyBuiltInTypeClass matching itself #24462

Merged
merged 1 commit into from
Nov 23, 2024

Conversation

metagn
Copy link
Collaborator

@metagn metagn commented Nov 21, 2024

fixes #24449

The standalone seq type is a tyBuiltInTypeClass with a single child of kind tySequence, which itself has no children. This is also the case for most other tyBuiltInTypeClass kinds. However this can cause a crash in sigmatch when calling isEmptyContainer on this child type, which expects the sequence type to have children. This check was added in #5557 to prevent empty collections like @[] from matching their respective typeclass, but it's not useful when matching against another typeclass (which is done here to resolve an ambiguity). So to avoid the crash, this empty container check is disabled when matching against another typeclass.

@Graveflo
Copy link
Contributor

I'm tempted to think that the type of @[] should match with seq, not that it's a valid construction and assuming that it can even be assigned a type in the first place. If tySequence is allowed to have no kids it calls into question what it is mean to represent. As far as I know there are two "weird" seq formats, one with tyNone appended to it and one with no kids. The one with an extra tyNone should probably go away, but that is the one that is pulled up from searching for seq in scope.
It's probably beyond the scope of this PR, but in the future it might be better the just have seq[T] capable of handling all the invalid cases without having either of these modes.

@metagn
Copy link
Collaborator Author

metagn commented Nov 23, 2024

This is the only place where seq with no children is analyzed, as a child to tyBuiltInTypeClass. tyBuiltInTypeClass is generally not skipped and there isn't any analysis done on the seq type other than checking if it's empty. Same goes for the other types it supports, array, object, ref, proc (which also checks for flags for iterators/calling conventions), var etc. They all have "invalid" structures but they are entirely contained inside tyBuiltInTypeClass.

@Araq Araq merged commit 96043bd into nim-lang:devel Nov 23, 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 96043bd

Hint: mm: orc; opt: speed; options: -d:release
177834 lines; 8.536s; 653.633MiB 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.

problem with underspecified seq types in proc definitions of the same name
3 participants