-
-
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
Abstract def implementations should inherit return type restrictions by default #10904
Comments
The compiler should probably also see that it doesn't have a return type and inherit that from the abstract def when generating docs for that method. |
As a general reminder, a def might implement more than one abstract def at the same time: abstract class A
abstract def foo(x, y : Int32) : Enumerable(Int32)
abstract def foo(x : Int32, y) : Iterable(Int32)
end
class B < A
def foo(x, y) : Array(Int32) # okay
[1]
end
end So if we omit the return type restriction in |
@HertzDevil Implementing different abstract defs with a single method is probably very rare. Do you know any example for that? |
Another reason for enforcing return type restrictions in implementations and not inheriting them from the abstract def(s) is here: #7956 (comment) |
The problem with autogenerated return types in docs is that they only cover the existing subtypes (possibly generic instantiations) coming from the library itself, so if an intersection expands into the union of its subtypes, it could as well be a single type or even If the solution is to add a return type restriction then there is little point in allowing them to be inherited in the first place. So I disagree allowing docs to automatically generate return type restrictions based on overridden abstract defs, unless #2404 is implemented. |
I really don't think docs is very relevant here. The important thing is to make sure the implementation uses the correct return type as per the abstract def contract. |
We can't just start enforcing return type restrictions on implementation methods. That's a breaking change and needs to wait for 2.0 (We could add deprecation warnings before that, though). And I think that should be the ultimate goal we're aiming for. I would consider automatically inheriting return type restrictions from the abstract def as a useful short-term action. If an implementation has an incorrect implicit return type, that's a bug and the compiler can discover that. That's not a breaking change because it only fails to compile code that was already broken anyways, it just makes it obvious and helps identify bugs. This can happen way before we can make return type restrictions mandatory in 2.0. |
So are you asking to reapply #7999 and #9810 within every major release? Because that's what a "short-term action" sounds like to me. If later we discover during 2.x that some abstract defs in the standard library have incorrect restrictions, we would repeat this procedure until 3.0 drops. I don't think that's a healthy process. Instead, here are two ways to add return type restrictions to abstract defs in the middle of a major release. The first one is non-binding: module Iterator(T)
{% begin %}
# New in 1.2: This method must return a `T | Iterator::Stop`.
abstract def next {% if compare_versions(::Crystal::VERSION, "2.0.0") >= 0 %} : T | Iterator::Stop {% end %}
{% end %}
end The second is to create a new annotation for this: module Iterator(T)
@[NonStrict(:abstract_def_return_type, "1.2")]
abatract def next : T | Iterator::Stop
end such that if |
Yeah, adding a return type conditionally or disabling a strict error might actually be a better idea than implicit inheritance 👍 |
Wrapping the return type in a conditional macro looks nice for its simplicity. But I don't think it's very useful, because it only becomes effective with the specified release. Before that, it's neither visible in the documentation nor can you check if there would be errors once it becomes active. |
The compiler enforces abstract def implementations to specify a return type restriction if the abstract def has one as well:
I think implementations could by default inherit the restriction of the abstract def. This makes it easier to write abstract def implementations, and in particular allows introducing return type restrictions to existing abstract defs without breaking all implementations (see #10903).
The text was updated successfully, but these errors were encountered: