-
-
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
add an "overrides" annotation #1647
Comments
also will enforce using same arg types and same return types or return types substitutable by the Liskov principle |
I'm not sure, I find it utterly verbose for little benefit, such typos are not really common IME. If anything I'd like to see a keyword like |
it is very common for major typed languages for a reason. But maybe you're right and this should be handled by some tool like rubocop |
feel free to close if you think it best |
A good thing of highlighting overrides is to detect code that requires attention if the base class changed. I like the idea of a redef as syntax. Maybe to do a check of raising an error if there is not match for the overload handled by that definition previously in the context... (just thinking out loud here) |
I actually think some sort of |
I would like to see here |
+1 for override |
Haha, I was thinking about adding this issue just yesterday! Timing :) [ed: just saw that it was entered before that ;) ] I like @jhass proposal of
Since Crystal can re-open types, if one uses a lot of 3rd part modules, chances are they will define a method you also define, and you haven't learned about. If you've defined it with |
+1 for raising when trying to override without redef |
I support this. |
+1 redef or more usual override keyword. |
+1 for
It's not the regular definition of |
+1 for redef |
+1 for redef, but only for methods with the same argument arity. Return type changes should require |
Additionally, such a keyword (e.g.
Then, if we want to enforce that something never change via reopening/extending/including/inheriting, a
EDIT: Just to clarify, the use case of this for modules, etc. would be to prevent the following (analogous to the use case for methods):
I am not advocating a EDIT 2: I have changed |
keep simple :) |
@sevk "Simple" could be defined as not having to second-guess whether one is unintentionally monkey-patching or getting monkey-patched...or vice versa when it's desired :-) |
I like the idea of overrides and I like The problem I see with overrides is that it is a breaking change that breaks every piece of code written so far or converted from ruby later, so
|
take 90% for syntax simple vs other language . |
@konovod |
I strongly dislike the I also don't think that this should be a breaking change: the annotation should be completely optional and only add a check that you're overriding a method. This makes it useful when writing code to ensure you haven't typoed the method name, and when upgrading code to ensure that the method you're overriding hasn't changed name. I don't think that accidentally overriding methods is a problem that this proposal should attempt to solve. |
I like the idea of having this be opt-in. |
If it's not going to be a breaking change and just an opt in check, it should IMO be an attribute and not a specifier like |
@konovod To answer your question about the use cases of |
@asoffa i know about As for optional attribute instead of required keyword, personally i don't like to see |
@konovod Yes, my understanding is also that Crystal aims to be more dynamic, and there are always tradeoffs that must be made. Adding yet another keyword (e.g. As for |
I think there is no point to mark only few methods @zatherz detection of overrides will require full compilation (or at least analysis of all included modules and stdlib), that is slower than just parsing, so i'm not sure that this function should be added to frequently used |
I guess this is possible since #6063. Something like this should do: class Object
annotation Override
end
macro inherited
macro method_added(method)
\{% if method.annotation(Override) && !@type.ancestors.any? &.methods.includes?(method) %}
\{% raise "Override error: '#{method.name}' does not exist in '{{@type.superclass}}'" %}
\{% end %}
end
end
end
# ===
abstract class GrandParent
def my_method
end
end
abstract class Parent < GrandParent
end
class Child < Parent
@[Override]
def non_existent # Override error: 'non_existent' does not exist in 'Parent'
end
@[Override]
def my_method # Good to go!
end
end |
I like the idea of it being opt in. |
In a spirit similar to #6983, I have changed my original proposal above to a (possibly optional) However, independent of the specifics, a key issue to consider is how this feature should play with |
+1 for annotations like in retired PR #6983 but with @[Reopen], i.e. @[Redefine], @[Override] and @[Reopen]. |
If the intent of an class Bar
end
class Foo1
def foo(x : Bar)
1
end
end
class Foo2 < Foo1
class Bar
end
# does not override `Foo1#foo`!!
def foo(x : Bar)
2
end
end
Foo2.new.foo(Bar.new) # => 1 Thus if
#1647 (comment) does not work because def equality compares also the def body, meaning the only defs that don't raise a compile-time error are verbatim copies (and as shown above, even these are inadequate for |
@HertzDevil I think this definition kind of makes, but I'm not really sure if it's worth going for. You don't mention return type restrictions, but I suppose they would need to be an exact match? Or maybe the return type of The semantics are to tell the compiler that the annotated def's signature exactly matches a parent's def and thus effectively overrides it completely. When there's only a single method overload in the parent, it's easy to reason about overriding it. But overloads add a factor of complexity. If there are multiple parent defs with similar signatures, Another practical use case is only partially overriding a parent def with more specific parameter type restrictions. So, this could certainly be useful, but it's also very limited to a use case that seems pretty narrow to me. Maybe it's important and widespread enough in the wild. Not sure. But as I currently see it, I'm not really happy about it. |
That definition above would indeed be more suitable for In relation to #10904 I am thinking that if an implementation is annotated with abstract class Base
abstract def foo : Int32
abstract def foo(x) : Int
abstract def bar : Nil
def baz; end
end
class Derived < Base
@[Overrides] # okay, overrides `Base#foo`
def foo # return type must be `Int32` or a subtype of it
1
end
@[Overrides] # okay, overrides `Base#foo(x)`
def foo(x) # return type must be `Int` or a subtype of it
2_i64
end
@[Overrides] # error, cannot override more than one def
def foo(*x)
3
end
@[Overrides] # okay, overrides `Base#bar`
def bar # return type must be `Nil` or a subtype of it
4 # not allowed, unless return type restriction of `: Nil` is explicitly given
end
@[Overrides] # error, cannot override non-abstract def
def baz
5
end
end This assumes that abstract defs themselves also override properly: (related to #9998) abstract class Foo
abstract def foo : Int
end
abstract class Bar < Foo
@[Overrides] # okay, overrides `Foo#foo`
abstract def foo : Int32 # return type restriction is `Int` if not given
end
class Baz < Bar
@[Overrides] # okay, overrides `Bar#foo` but not `Foo#foo`
def foo # return type is `Int32`
1
end
end
|
I think it would be useful to have an annotation to indicate that a def overrides the behaviour of a parent's implementation, simply for documentation purposes (see #14518 (comment) for example). This is certainly a different use case than the implementation of an abstract method with exactly matching signature. |
this will prevent you from doing something like
The text was updated successfully, but these errors were encountered: