-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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 self restriction with including generic module #3972
Fix self restriction with including generic module #3972
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
end
Which 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, this: https://play.crystal-lang.org/#/r/4ref
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_self
parameter tolookup_type
. Whenlazy_self
istrue
,lookup_type
keepsself
in generics type. It is used to look up type forinclude
andextend
.