-
-
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
A setter type is the argument's type #7711
Conversation
What about |
Oh, some obscure codegen bug. Maybe later... |
bind_to block.break if block | ||
# For a setter like `foo.x = y` we'd like to use `y`'s type | ||
if self.setter? | ||
bind_to args.first |
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.
Note for later: it should probably be bind_to args.last
instead to handle []=
I've updated this PR and fixed the original codegen failure (it seems to be caused by the One spec causes a codegen bug. I've deactivated it for now, to check other things (seems it didn't get far, unfortunately). That spec was introduced by #9972. There's no obvious connection between both changes. The compiler change from #9972 itself doesn't seem to have an influence on any other code as far as I can see, it's just that single spec that breaks. But there's also an ICE not covered by specs. |
@straight-shoota Thanks! I'll take a look at this in about two weeks |
@straight-shoota I'm not sure what I'm supposed to do here. Which spec is failing with an ICE? I see CI just gives an ICE but I don't know where. |
I don't know anything more, really. The deactivated spec I talked about is https://github.com/asterite/crystal-1/blob/d864a13b1c6df229b520a9e98b1982aea646b1b9/spec/compiler/codegen/proc_spec.cr#L885 As for the ICE, I didn't dig any further since you said you would take a look at it. |
Oh, I thought there was a single spec that ICEd. Hmm... I might not have time to look at this these days, not sure. |
I think we won't go with this because it's considered a breaking change, so closing. |
For the record: Breaking change doesn't mean this feature will never come. We can have a feature flag for that and make it the default in 2.0. |
Fixes #7707
The way it's done is probably not very efficient, but it probably doesn't matter.