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 self restriction with including generic module #3972

Merged

Conversation

makenowjust
Copy link
Contributor

@makenowjust makenowjust commented Feb 2, 2017

Ref: #3847 (comment)

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 pull request 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.

@sdogruyol
Copy link
Member

It's great to see contributions like this to compiler from the community. Great job @makenowjust 👍

@makenowjust
Copy link
Contributor Author

Copy link
Member

@bcardiff bcardiff left a 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?

@makenowjust
Copy link
Contributor Author

Rebased to resolve conflect.

@bcardiff We can look the most general use case of including with self in src/enum.cr: include Comparable(self), which ensures ==, <=> and other compare methods are restricted by just only derived type. This behavior cannot be expressed by other syntax. And also, I believe this pull request is just bug fix.

@makenowjust
Copy link
Contributor Author

ping. Are you missing this pull request?

@makenowjust
Copy link
Contributor Author

@asterite @bcardiff Why is not this pull request merged? I want to know what I should do to merge it.

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...
@bcardiff bcardiff force-pushed the fix/crystal/self-restriction branch from 883a427 to 8ace6d4 Compare March 23, 2017 20:13
@bcardiff
Copy link
Member

Rebased on master. Merging soon :-)

@bcardiff bcardiff added this to the Next milestone Mar 23, 2017
@bcardiff bcardiff merged commit 917971d into crystal-lang:master Mar 23, 2017
@bcardiff
Copy link
Member

Thanks @makenowjust and sorry for the delay

end

Baz.new.foo
") { types["Baz"].metaclass }
Copy link
Member

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?

Copy link
Member

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

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

@bcardiff bcardiff Aug 17, 2018

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?

Copy link
Member

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.

Copy link
Member

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.

Copy link
Contributor

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

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) ?

Copy link
Contributor

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"
Copy link
Contributor

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants