Be more permissive when detecting frozen objects #1401
Conversation
JonRowe
changed the title
|
Hey there! Any reason this isn't being merged? I'm hitting this during a Rails update and could really use the fix. |
|
As the original source of this bug is Rails not implementing |
|
Do I need to address the failures in the "legacy" tests? I've having some trouble getting rvm to install ruby-1.8.7... |
If you want this merged to main for a potential bug fix release, yes, if you want it merged to 4-0-dev for when that gets released no, looking at it you have a behaviour difference with frozen objects for those rubies. |
keegangroth
force-pushed
the
main
branch
2 times, most recently
from
39bdbd5 to
e446e02
Compare
|
I think this is ready for another look. I manged to get 1.8.7 running and the tests passing there. Also rearranged and added more tests, the diff for those looks worse than it really is though. |
|
for real this time, set up the actions on my fork and the only failure was some ffi compilation issue on one windows run... |
keegangroth
force-pushed
the
main
branch
2 times, most recently
from
0076660 to
66b2c58
Compare
Locally? Thanks for this effort, I know it is not easy. |
yeah, this took me a little while, but doing it with docker ended up not being too bad. Likely not perfect, but here's a gist for it if anyone else needs it. Then you just have to find working docker images, here's what I used: |
lib/rspec/mocks/proxy.rb
Outdated
| @@ -35,8 +35,7 @@ def initialize(object, order_group, options={}) | |||
|
|
|||
| # @private | |||
| def ensure_can_be_proxied!(object) | |||
| return unless object.is_a?(Symbol) || object.frozen? | |||
| return if object.nil? | |||
| return unless object.is_a?(Symbol) || (object.is_a?(String) && object.frozen?) | |||
There was a problem hiding this comment.
This check is wrong, we still need to check other types of objects are frozen. If you want to improve this check to actually check the real object is frozen please do, (prehaps checking that the frozen method is kernels method) but as it is I don't want to only warn for frozen strings.
There was a problem hiding this comment.
It's actually simply redundant, the new exception handling takes care of all objects regardless of this check. I had mentioned this previously in #1401 (comment) and @pirj advised minimizing the changes, but I would be happy to remove this method entirely to avoid this confusion.
|
This one is biting me too with Rails - happy to help get it across the finish line if I can. |
|
@cknoxrun You're welcome to. |
|
I'm still watching this, was just waiting @JonRowe to say how he wanted it, but I guess I'll just take another shot |
Rather than rely on an object's #frozen? method, always attempt to proxy a method when requested. Catch the errors resulting from failed attempts to proxy and reraise (or log) them with helpful messages. One notable use case for this is rails' activerecord's Model objects becoming frozen after being deleted. In reality, only the underlying hash of attributes is frozen and it should still be possible to write proxies for these objects.
There was a problem hiding this comment.
@keegangroth Apologies, I thought I made it clear that you still needed to restore some existing behaviour for this to be mergable, as that hadn't happened I didn't re-review. I've highlighted the issues again.
Let the logic in method_double handle this like it does for other classes.
|
Any updates on this? We are still encountering this as a blocker |
|
Merged in 727ba92 |
Rather than rely on an object's #frozen? method, always attempt to
proxy a method when requested. Catch the errors resulting from failed
attempts to proxy and reraise (or log) them with helpful messages.
One notable use case for this is rails' activerecord's Model objects
becoming frozen after be deleted. In reality, only the underlying hash
of attributes is frozen and it should still be possible to write
proxies for these objects.