Fix self restriction with including generic module#3972
Fix self restriction with including generic module#3972bcardiff merged 2 commits intocrystal-lang:masterfrom
Conversation
3105ce2 to
5e0d96e
Compare
|
It's great to see contributions like this to compiler from the community. Great job @makenowjust 👍 |
5e0d96e to
025b58b
Compare
bcardiff
left a comment
There was a problem hiding this comment.
All changes seems legit.
I'm curious about which scenarios that are particularly better to be expressed with this feature. Do you know any already?
025b58b to
883a427
Compare
|
Rebased to resolve conflect. @bcardiff We can look the most general use case of including with |
|
ping. Are you missing this pull request? |
Ref: crystal-lang#3847 Now, we can get a compile error with such a code: module Foo(T) def foo(x : T) x end end abstract struct Bar include Foo(self) end struct Baz1 < Bar end struct Baz2 < Bar end Baz1.new.foo Baz2.new # => no overload matches 'Baz1#foo' with type Baz2 This commit adds `lazy_self` parameter to `lookup_type`. When `lazy_self` is `true`, `lookup_type` keeps `self` in generics type. It is used to look up type for `include` and `extend`.
Because old compiler wants this definition and CI uses old compiler...
883a427 to
8ace6d4
Compare
|
Rebased on master. Merging soon :-) |
|
Thanks @makenowjust and sorry for the delay |
| end | ||
|
|
||
| Baz.new.foo | ||
| ") { types["Baz"].metaclass } |
There was a problem hiding this comment.
From analyzing the reason of #6547 I bisect the commit to the merge of #6504 which leads to this PR.
I'm having second thoughts on the semantic here. I think references to self as generic arguments should be eager and the return value of Baz.new.foo should be Bar(Int32).class.
From the spec in this line, if the generic module to be included Foo(T) defines an operation do(t : T), I would expect that Baz#do would be callable with any Bar(Int32). Otherwise an instance of Bar(Int32) is not substitutable with an instance of Baz, breaking the inheritance relationship between Baz and Bar(Int32).
Maybe the way to achieve the effect that enum types are comparable with only them selfs and generating compile time errors is by using macro inherited hook. I think that with macro inherited hook is clear that the base type is not including the module.
Is there any other scenario where we would want the self in Comparable(self) to be lazy evaluated other than Enum?
There was a problem hiding this comment.
I think self as a type reference should be removed from the language. It's confusing because of that same reason: does self means the type where it's used, or as a substitute for subclasses? We can choose one or the other, but it'll be still be confusing.
For example, if we have:
class Foo
def method(other : self)
end
end
class Bar < Foo
endWhich of the following should work:
foo = Foo.new
bar = Bar.new
foo.method(foo)
foo.method(bar)
bar.method(foo)
bar.method(bar)Does self means "Foo or any of its subclasses" or "the actual type that's being called"? If it's the later case, it's also complex to implement if we have a Foo+ type.
If we disallow self, all of these ambiguities disappear, and we are forced to use Foo, which is very clear.
For Enum, maybe an inherited macro could be defined to include Comparable({{@type}}). Or this: https://play.crystal-lang.org/#/r/4ree
There was a problem hiding this comment.
I mean, this: https://play.crystal-lang.org/#/r/4ref
There was a problem hiding this comment.
I would not use {% raise .. to detect invalid comparison. The normal method missing between two uncomparable types should be enough.
I would expect that types respect the substitutability unless the method is defined via macros. I see the semantic of the macro expansion as a helper to avoid the user go and type things in multiple places. But I don't see include module as metaprograming, rather as a design tool so it falls more in the formal side :-)
If self would be gone as a type restriction, how would you refer to the type were the module is included and use it as a type restriction?
There was a problem hiding this comment.
To late to ask silly question.
If self would be gone as a type restriction, how would you refer to the type were the module is included and use it as a type restriction?
you either write the module name or there is a generic argument to use.
So, yes, it seems we can remove self as type restriction.
There was a problem hiding this comment.
Let's leave self as type restriction for DRY and in macros expanded up the hierarchy then.
I found a bit weird that the is_a?(self) expression. self.is_a?(self) is always true but those two self are evaluated to different things. I would expect either self.is_a?(typeof(self)) or self.is_a?(TheType) depending on what is wanted. But we can review that later.
There was a problem hiding this comment.
I use self a lot to type methods in modules to be included, and want it to refer to the final type. Without lazy self type, we can't type these methods or restrict args, right?
There was a problem hiding this comment.
There was a problem hiding this comment.
@ysbaddaden You can use generic type parameter and macro:
module Foo(X)
def foo: Array(X)
[] of X
end
end
macro include_foo
include Foo({{@type}})
end
class Bar
include_foo
end@bcardiff Is this what you said above #3972 (comment) ?
There was a problem hiding this comment.
Oh, right, then I'd have class Model; include Query({{@type}}); end. Not nearly as pretty but that could do the trick, I guess.
| end | ||
|
|
||
| Baz.new.foo Bar(Int32).new | ||
| ", "no overload matches" |
There was a problem hiding this comment.
Why should that error? I'd guess that T is Bar(Int32) here, if not what is T?
Ref: #3847 (comment)
Now, we can get a compile error with such a code:
This pull request adds
lazy_selfparameter tolookup_type. Whenlazy_selfistrue,lookup_typekeepsselfin generics type. It is used to look up type forincludeandextend.