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

Ensure all setter methods in the standard library return the value being set #10083

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

HertzDevil
Copy link
Contributor

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=).

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.

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
Copy link
Member

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.

Copy link
Contributor

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
Copy link
Member

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
Copy link
Member

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

@asterite
Copy link
Member

I wouldn't go this route if we'll eventually solve this at the language level. I can give it another try.

@straight-shoota
Copy link
Member

straight-shoota commented Dec 15, 2020

The effort is appreciated, but I honestly don't think this is a good idea. This change is unncessary if we enforce this behaviour in the compiler. It mostly bloats the code.
Sure, it certainly improves the API in many places, but I haven't seen an actual problem with this. So it's IMO not worthy to merge this at the current time.

@straight-shoota
Copy link
Member

straight-shoota commented Dec 29, 2021

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.
But we can change stdlib methods to explicitly return the assigned value. These are technically also breaking changes, but that's acceptable because they're fixing bugs. The current return values of these methods are completely unintentional and leak internal implementation.

@asterite
Copy link
Member

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.

@straight-shoota
Copy link
Member

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.
There could be code that relies on that as well. But it's based on an undocumented implementation detail which turns out to be a bug. In order to fix it, we must break code that (most likely unintentionally) relies on it.

@straight-shoota
Copy link
Member

I think we should go forward with this. Unfortunately, this PR has gone out of sync with master. So it needs some catching up.

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

5 participants