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

Make Time#utc and #localtime raise RuntimeError when the object is frozen #4657

Merged
merged 2 commits into from Jun 9, 2017

Conversation

Projects
None yet
3 participants
@iconara
Contributor

iconara commented Jun 8, 2017

This is a fix for #4655 that replicates the MRI behaviour of frozen Time objects.

It adds three tests to spec/ruby/core/time that cover the MRI behaviour of frozen Time objects – I've tried to match the style of the existing tests and copied tests for the same behaviour in Array. I hope it looks ok.

The actual fix is very simple, in #utc and #localtimeI've added a check that will throw aRuntimeError` when the instance is frozen.

There are two additional public methods that mutate Time objects, but they're not Ruby methods, and I'm unsure about whether or not they should throw errors:

They also seem to be duplicates of each other, an neither has been touched in 10 years.

Should they be changed to throw the same error, or how is frozen-ness handled when objects are changed from Java code?

iconara added some commits Jun 8, 2017

Ensure that mutatning frozen Time instances raises an error
Time#utc and #localtime mutate the receiving instance even if it is frozen. In MRI this raises a RuntimeError.
@iconara

This comment has been minimized.

Show comment
Hide comment
@iconara

iconara Jun 8, 2017

Contributor

Even though this makes JRuby consistent with MRI, and that this obviously is a bug, it's been in all versions of JRuby, as far as I can tell, so merging this could potentially break lots of libraries and applications.

Contributor

iconara commented Jun 8, 2017

Even though this makes JRuby consistent with MRI, and that this obviously is a bug, it's been in all versions of JRuby, as far as I can tell, so merging this could potentially break lots of libraries and applications.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Jun 8, 2017

Member

Generally, if we are doing behavior for which MRI errors, I'm not worried about fixing it. The truth is that all Ruby code is MRI code first and if they're not hitting the errors they're pretty much guaranteed not to have a problem with this change on JRuby. Of course there's plenty of JRuby-specific code out there but I doubt much of it is freezing and then calling mutative methods on Time objects.

I'll review and merge if @enebo has no concerns.

Member

headius commented Jun 8, 2017

Generally, if we are doing behavior for which MRI errors, I'm not worried about fixing it. The truth is that all Ruby code is MRI code first and if they're not hitting the errors they're pretty much guaranteed not to have a problem with this change on JRuby. Of course there's plenty of JRuby-specific code out there but I doubt much of it is freezing and then calling mutative methods on Time objects.

I'll review and merge if @enebo has no concerns.

@headius

headius approved these changes Jun 8, 2017

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Jun 8, 2017

Member

Yeah I agree fully with @headius. If something​ breaks then it is one off logic. Our strength is in good Ruby compatibility over the long run

Member

enebo commented Jun 8, 2017

Yeah I agree fully with @headius. If something​ breaks then it is one off logic. Our strength is in good Ruby compatibility over the long run

@headius headius added this to the JRuby 9.1.11.0 milestone Jun 9, 2017

@headius headius merged commit dfec566 into jruby:master Jun 9, 2017

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment