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 type restriction logic for generic module instances #10519

Merged

Conversation

HertzDevil
Copy link
Contributor

@HertzDevil HertzDevil commented Mar 18, 2021

Revival of #5132. Fixes #4287. Fixes #4684. Does not affect #9660.

typeof(begin
  x = uninitialized Enumerable(Int64)
  x.is_a?(Indexable(Int64)) ? x : raise ""
end) # => (StaticArray(Int64, 3) | Tuple(Int64, Int64))

The union above is essentially all including types of Indexable(Int64) in the standard library defined prior to this typeof. This is not ideal because if the other type implements the current type then we could simply return Indexable(Int64) here (see also #2404 (comment)), but this behaviour is consistent with non-generic module instances:

module A; end
module B; include A; end
class C; include B; end
class D; include B; end

typeof(begin
  x = uninitialized A
  x.is_a?(B) ? x : raise ""
end) # => (C | D)

The reason that previous PR fails seems to be that without the type restriction in Crystal::GenericModuleInstanceType#restrict it catches all AST node restrictions too, and leads to an infinite recursion inside GenericInstanceType#restrict(other : Generic, context). From my own experience it happened between Comparable and Pointer in the prelude:

  • Comparable(Array(T)) v.s. Pointer(T)
  • (Array(Array(Crystal::DWARF::LineNumbers::Row)) | Array(Array(PrettyPrint::Group)) | ...) v.s. Pointer(T)
  • Array(Array(Crystal::DWARF::LineNumbers::Row)) v.s. Pointer(T)
  • Comparable(Array(T)) v.s. Pointer(T)
  • ...

@asterite
Copy link
Member

I put a ❤️ in the issue, but here are some more:

❤️ ❤️ ❤️ ❤️ ❤️

@HertzDevil
Copy link
Contributor Author

HertzDevil commented Mar 25, 2021

The stack overflow is back if this is rebased on top of #10522. I'll need to rethink about this... (that PR could go ahead first)

My hypothesis is that code that does:

y = restrict(x, other)
return ... unless y
# y isn't used afterwards

can be replaced with restriction_of?. Then the infinite recursion will probably go away (it happens between parents and including_types). We can't use implements? because non-strict matching is still here.

Should be fixed now.

@HertzDevil HertzDevil marked this pull request as draft March 25, 2021 16:03
@HertzDevil HertzDevil marked this pull request as ready for review March 26, 2021 12:04
Copy link
Member

@asterite asterite left a comment

Choose a reason for hiding this comment

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

Great!

@asterite asterite added this to the 1.1.0 milestone May 29, 2021
@asterite asterite merged commit 9b562c2 into crystal-lang:master May 29, 2021
@HertzDevil HertzDevil deleted the bug/generic-module-instance-is-a branch May 29, 2021 20:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:compiler:semantic
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Type Range misidentified as Enumerable Generic modules: is_a? returns incorrect value
3 participants