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

Time#utc and #localtime can mutate a frozen object #4655

Closed
iconara opened this Issue Jun 8, 2017 · 4 comments

Comments

Projects
None yet
2 participants
@iconara
Contributor

iconara commented Jun 8, 2017

Environment

This is one of the relevant environments, but the issue can be replicated in all environments I've tested:

$ ruby -v
jruby 9.1.8.0 (2.3.1) 2017-03-06 90fc7ab Java HotSpot(TM) 64-Bit Server VM 25.112-b16 on 1.8.0_112-b16 +jit [darwin-x86_64]
$ uname -a
Darwin quattro.local 16.6.0 Darwin Kernel Version 16.6.0: Fri Apr 14 16:21:16 PDT 2017; root:xnu-3789.60.24~6/RELEASE_X86_64 x86_64

Expected Behavior

When I freeze a Time instance I expect to not be able to run #utc since that method changes the instance:

irb> Time.new.freeze.utc
RuntimeError: can't modify frozen Time
	from (irb):1:in `utc'
	from (irb):1

The same goes for #localtime:

irb>  Time.new.utc.freeze.localtime
RuntimeError: can't modify frozen Time
	from (irb):1:in `localtime'
	from (irb):1

Actual Behavior

In all versions of JRuby I have tested (1.7.19, 1.7.23, 1.7.26, 9.0.5.0, 9.1.8.0) I can modify frozen Time instances:

irb> t = Time.now
=> 2017-06-08 10:12:04 +0200
irb> t.freeze
=> 2017-06-08 10:12:04 +0200
irb> t.frozen?
=> true
irb> t.utc
=> 2017-06-08 08:12:04 UTC
irb> t
=> 2017-06-08 08:12:04 UTC
irb> t.localtime
=> 2017-06-08 10:12:04 +0200
irb> t
=> 2017-06-08 10:12:04 +0200

It looks like problem lies in these two lines:

I've looked through the rest of RubyTime.java and it looks like these are the only place where dt or nsec are changed except for at initialization time.

Even though this is inconsistent behaviour with MRI it could potentially break a lot of code so I assume it's not something that could be rolled out in a patch release.

It looks like an easy fix, and I'd love to submit a PR, but I need is a pointer to where to add a test for it. Would adding a test/test_time_freeze.rb be the right thing to do?

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Jun 8, 2017

Member

The best place to add a test would be our copy of https://github.com/ruby/spec, in spec/ruby under our repo. The fix sounds like it just needs a frozen check, because presumably MRI modifies the object in the same way?

Member

headius commented Jun 8, 2017

The best place to add a test would be our copy of https://github.com/ruby/spec, in spec/ruby under our repo. The fix sounds like it just needs a frozen check, because presumably MRI modifies the object in the same way?

@iconara

This comment has been minimized.

Show comment
Hide comment
@iconara

iconara Jun 8, 2017

Contributor

@headius thank you for the pointer, I have opened a PR with a fix: #4657.

Contributor

iconara commented Jun 8, 2017

@headius thank you for the pointer, I have opened a PR with a fix: #4657.

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

@headius headius closed this Jun 9, 2017

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Jun 9, 2017

Member

Thanks for the report and PR!

Member

headius commented Jun 9, 2017

Thanks for the report and PR!

@iconara

This comment has been minimized.

Show comment
Hide comment
@iconara

iconara Jun 9, 2017

Contributor

My pleasure, thanks for the help.

Contributor

iconara commented Jun 9, 2017

My pleasure, thanks for the help.

eregon added a commit to ruby/spec that referenced this issue Jun 23, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment