-
-
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
Ensure all setter methods in the standard library return the value being set #10083
base: master
Are you sure you want to change the base?
Conversation
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 the returned type should match the corresponding getter.
So the expressions foo.prop ||= value
that is expanded as foo.prop || (foo.prop = value)
will not generate unions, and the result will be less confusing IMO.
@@ -1023,6 +1023,7 @@ class Array(T) | |||
# :nodoc: | |||
protected def size=(size : Int) | |||
@size = size.to_i | |||
size |
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.
This change is wrong. The size
argument and the Array#size
getter may have different types.
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's no need for them to be the same. Setters ought to return the given value, not the one actually returned by the corresponding getter.
@@ -14,6 +14,7 @@ module Crystal | |||
|
|||
def warning=(warning) | |||
@warning = !!warning | |||
warning |
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.
ditto Array#size=
@@ -14,16 +14,19 @@ module Crystal | |||
def color=(color) | |||
@color = !!color | |||
inner.try &.color=(color) | |||
color |
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.
ditto Array#size=
and others in this file
I wouldn't go this route if we'll eventually solve this at the language level. I can give it another try. |
|
I recall my previous comment. We're in a different state now. I don't think we can change the language mechanics before 2.0. That would be an inexcusable breaking change. |
Why would fixing the return values be fixing bugs, but changing it for every method be a breaking change? It might be worth trying the global change and see how, and if, ir affects the ecosystem. |
For stdlib methods we can be sure that the leaking return value is unintended. But third party code might rely on that. Changing this would break the language specification. Changing stdlib methods would only change each method's implicit contract which was never specified. |
I think we should go forward with this. Unfortunately, this PR has gone out of sync with master. So it needs some catching up. |
Until the more general #7707 is resolved, this ensures all setter methods always return the value arguments that are passed to them.
The setters were identified using
/def (self\.)?([a-z_0-9]+|\[\])=/
. Setters inside specs are ignored. One conflicting return type restriction was removed, but it was internal to the compiler (Crystal::Signal.child_handler=
).