Skip to content

Fix Time.new parsing (time/new_tags.txt) #8890

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.

Sign up for GitHub

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

Merged
enebo merged 1 commit into from
Jul 5, 2025
Merged

Fix Time.new parsing (time/new_tags.txt) #8890

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 8 additions & 12 deletions core/src/main/java/org/jruby/RubyTime.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1858,7 +1858,7 @@ public IRubyObject initialize(ThreadContext context, IRubyObject[] args) {
boolean keywords = hasKeywords(ThreadContext.resetCallInfo(context));
int argc = args.length;
IRubyObject zone = context.nil;
IRubyObject precision = context.nil;
IRubyObject precision = asFixnum(context, 9);

if (keywords) {
IRubyObject[] opts = ArgsUtil.extractKeywordArgs(context, args[args.length - 1], "in", "precision");
Expand All @@ -1874,10 +1874,6 @@ public IRubyObject initialize(ThreadContext context, IRubyObject[] args) {
}

if (opts[1] != null) {
if (!(opts[1] instanceof RubyNumeric)) {
// Weird error since all numerics work at this point so why mention Integer?
throw typeError(context, str(context.runtime, "no implicit conversion of ", typeAsString(opts[1]), " into Integer"));
}
precision = opts[1];
}
}
Expand All @@ -1890,20 +1886,20 @@ public IRubyObject initialize(ThreadContext context, IRubyObject[] args) {
return switch (argc) {
case 0 -> initializeNow(context, zone);
case 1 -> timeInitParse(context, args[0], zone, precision);
case 2 -> initialize(context, args[0], args[1], nil, nil, nil, nil, precision, zone);
case 3 -> initialize(context, args[0], args[1], args[2], nil, nil, nil, precision, zone);
case 4 -> initialize(context, args[0], args[1], args[2], args[3], nil, nil, precision, zone);
case 5 -> initialize(context, args[0], args[1], args[2], args[3], args[4], nil, precision, zone);
case 6 -> initialize(context, args[0], args[1], args[2], args[3], args[4], args[5], precision, zone);
case 7 -> initialize(context, args[0], args[1], args[2], args[3], args[4], args[5], precision, zone);
case 2 -> initialize(context, args[0], args[1], nil, nil, nil, nil, nil, zone);
case 3 -> initialize(context, args[0], args[1], args[2], nil, nil, nil, nil, zone);
case 4 -> initialize(context, args[0], args[1], args[2], args[3], nil, nil, nil, zone);
case 5 -> initialize(context, args[0], args[1], args[2], args[3], args[4], nil, nil, zone);
case 6 -> initialize(context, args[0], args[1], args[2], args[3], args[4], args[5], nil, zone);
case 7 -> initialize(context, args[0], args[1], args[2], args[3], args[4], args[5], nil, zone);
default -> throw argumentError(context, argc, 0, 7);
};
}

private IRubyObject timeInitParse(ThreadContext context, IRubyObject arg, IRubyObject zone, IRubyObject precision) {
IRubyObject strArg = arg.checkStringType();
if (strArg.isNil()) {
return initialize(context, arg, context.nil, context.nil, context.nil, context.nil, context.nil, precision, zone);
return initialize(context, arg, context.nil, context.nil, context.nil, context.nil, context.nil, context.nil, zone);
}
RubyString str = (RubyString) strArg;

Expand Down
39 changes: 17 additions & 22 deletions core/src/main/java/org/jruby/util/RubyTimeParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,6 @@
import static org.jruby.util.RubyStringBuilder.str;

public class RubyTimeParser {
public static final long SIZE_MAX = Long.MAX_VALUE;


private int beg;
private int ptr;
private int end;
Expand All @@ -39,19 +36,20 @@ public IRubyObject parse(ThreadContext context, RubyTime self, RubyString str, I
IRubyObject year;
IRubyObject subsec = context.nil;
int mon = -1, mday = -1, hour = -1, min = -1, sec = -1;
long prec = precision.isNil() ? SIZE_MAX : toLong(context, precision);
long prec = precision.isNil() ? Long.MAX_VALUE : toLong(context, precision);
if (prec < 0) prec = Long.MAX_VALUE;

if (!isEOS() && (isSpace() || Character.isSpaceChar(bytes[end - 1]))) {
throw argumentError(context, str(context.runtime, "can't parse: ", str));
if (!isEOS() && (isSpace() || Character.isWhitespace(bytes[end - 1]))) {
throw argumentError(context, str(context.runtime, "can't parse: ", str.inspect()));
}
year = parseInt(context, true);
if (year.isNil()) throw argumentError(context, str(context.runtime, "can't parse: ", str));
year = parseInt(context, true, Long.MAX_VALUE);
if (year.isNil()) throw argumentError(context, str(context.runtime, "can't parse: ", str.inspect()));
if (ndigits < 4) {
IRubyObject invalidYear = context.runtime.newString(new ByteList(bytes, ptr - ndigits - beg, ndigits, true));
throw argumentError(context, str(context.runtime, "year must be 4 or more digits: ", invalidYear));
}
if (ptr == end) { // only year provided
return self.initialize(context, year, context.nil, context.nil, context.nil, context.nil, context.nil, precision, zone);
return self.initialize(context, year, context.nil, context.nil, context.nil, context.nil, context.nil, context.nil, zone);
}
do {
if (peek() != '-') break;
Expand All @@ -75,17 +73,12 @@ public IRubyObject parse(ThreadContext context, RubyTime self, RubyString str, I
sec = expectTwoDigits(context, "sec", 60);
if (peek() == '.') {
advance();
int digits;
for (digits = 0; digits < prec; digits++) {
if (!isDigit(digits)) break;
}
if (digits == 0) {
// FIXME: some precise mbc for garbled time string
IRubyObject invalidSecs = context.runtime.newString(new ByteList(bytes, timePart, ptr - timePart + 1, true));
if (isEOS() || !isDigit(0)) {
if (!isEOS()) advance();
IRubyObject invalidSecs = context.runtime.newString(new ByteList(bytes, timePart, ptr - timePart, true));
throw argumentError(context, str(context.runtime, "subsecond expected after dot: ", invalidSecs));
}
subsec = parseInt(context, false);
if (subsec.isNil()) break;
subsec = parseInt(context, false, prec);
while (!isEOS() && isDigit(0)) ptr++;
}
} while (false);
Expand All @@ -112,7 +105,7 @@ public IRubyObject parse(ThreadContext context, RubyTime self, RubyString str, I
var value = ((RubyInteger) subsec).asLong(context) * mul;
subsec = RubyRational.newRational(context.runtime, value, 1000);
} else if (ndigits > TIME_SCALE_DIGITS) {
int mul = (int) Math.pow(10, ndigits - TIME_SCALE_DIGITS - 3/*1000*/);
int mul = (int) Math.pow(10, ndigits - TIME_SCALE_DIGITS + 3/*1000*/);
subsec = RubyRational.newRational(context.runtime, ((RubyInteger) subsec).asLong(context), mul);
} else if (subsec instanceof RubyInteger) {
subsec = RubyRational.newRational(context.runtime, ((RubyInteger) subsec).asLong(context), 1000);
Expand Down Expand Up @@ -149,7 +142,7 @@ private boolean isDigit(int offset) {
}

private boolean isSpace() {
return Character.isSpaceChar(peek());
return Character.isWhitespace(peek());
}

private void needColon(ThreadContext context, String label, int start) {
Expand Down Expand Up @@ -232,7 +225,7 @@ private int expectTwoDigits(ThreadContext context, String label, int max) {
return total;*/
}

private IRubyObject parseInt(ThreadContext context, boolean parseSign) {
private IRubyObject parseInt(ThreadContext context, boolean parseSign, long precision) {
int sign = 1;
ndigits = 0;
if (parseSign) {
Expand All @@ -247,12 +240,14 @@ private IRubyObject parseInt(ThreadContext context, boolean parseSign) {
}

long total = 0;
while (!isEOS() && isDigit(0)) {
while (!isEOS() && isDigit(0) && ndigits < precision) {
total = total * 10 + (peek() - '0');
advance();
ndigits++;
}

if (ndigits == 0) return context.nil;

return new RubyFixnum(context.runtime, sign * total);
}
}
2 changes: 0 additions & 2 deletions core/src/main/java/org/jruby/util/time/TimeArgs.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,7 @@
import org.jruby.runtime.ThreadContext;
import org.jruby.runtime.builtin.IRubyObject;

import static org.jruby.RubyRational.rationalCanonicalize;
import static org.jruby.RubyTime.TIME_SCALE;
import static org.jruby.RubyTime.TIME_SCALE_DIGITS;
import static org.jruby.api.Convert.asFixnum;
import static org.jruby.api.Convert.toDouble;
import static org.jruby.api.Convert.toInt;
Expand Down
7 changes: 0 additions & 7 deletions spec/tags/ruby/core/time/new_tags.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,20 +1,13 @@
fails:Time.new with a utc_offset argument returns a Time with a UTC offset of the specified number of Rational seconds
fails:Time.new with a utc_offset argument raises ArgumentError if the String argument is not in an ASCII-compatible encoding
fails:Time.new with a utc_offset argument with an argument that responds to #to_r coerces using #to_r
fails:Time.new with a utc_offset argument raises ArgumentError if the month is greater than 12
Copy link
Contributor Author

@PChambino PChambino Jul 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was already passing, so I just removed it.

fails(not implemented, jruby/jruby#6161):Time.new with a timezone argument the #abbr method is used by '%Z' in #strftime
fails(not implemented, jruby/jruby#6161):Time.new with a timezone argument Time-like argument of #utc_to_local and #local_to_utc methods has attribute values the same as a Time object in UTC
fails(not implemented, jruby/jruby#6161):Time.new with a timezone argument #name method uses the optional #name method for marshaling
fails(not implemented, jruby/jruby#6161):Time.new with a timezone argument #name method cannot marshal Time if #name method isn't implemented
fails(not implemented, jruby/jruby#6161):Time.new with a timezone argument subject's class implements .find_timezone method calls .find_timezone to build a time object at loading marshaled data
fails(only during full spec run):Time.new with a utc_offset argument raises ArgumentError if the String argument is not of the form (+|-)HH:MM
fails:Time.new with a timezone argument returned value by #utc_to_local and #local_to_utc methods cannot have arbitrary #utc_offset if it is an instance of Time
fails(https://github.com/jruby/jruby/issues/8736):Time.new with a timezone argument :in keyword argument allows omitting minor arguments
Copy link
Contributor Author

@PChambino PChambino Jul 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was already passing, I think I might have fixed it in my previous PR: #8858
But I wasn't aware of these tag files.

fails(https://github.com/jruby/jruby/issues/8736):Time.new with a timezone argument Time.new with a String argument accepts precision keyword argument and truncates specified digits of sub-second part
Copy link
Contributor Author

@PChambino PChambino Jul 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RubyTime only has precision up to 9 (nanoseconds) so it is not possible to remove this tag, but the microseconds conversion was wrong ( + 3 vs - 3) when there were more than 9 digits. So it now truncates correctly.

fails(https://github.com/jruby/jruby/issues/8736):Time.new with a timezone argument Time.new with a String argument converts precision keyword argument into Integer if is not nil
Copy link
Contributor Author

@PChambino PChambino Jul 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing the type error in RubyTime basically fixed this. We already call toLong in the RubyTimeParser.

I also noticed that the precision keyword argument is only supposed to be used when parsing a ruby time string and ignore otherwise, and it should default to 9.

https://arc.net/l/quote/hugtggyb

fails(https://github.com/jruby/jruby/issues/8736):Time.new with a timezone argument Time.new with a String argument raises ArgumentError if date/time parts values are not valid
fails(https://github.com/jruby/jruby/issues/8736):Time.new with a timezone argument Time.new with a String argument raises ArgumentError if utc offset parts are not valid
fails(https://github.com/jruby/jruby/issues/8736):Time.new with a timezone argument Time.new with a String argument raises ArgumentError if string doesn't start with year
Copy link
Contributor Author

@PChambino PChambino Jul 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed the RubyTimeParser.parseInt to return nil if no digits, so we can raise the correct error. Also, this error needs to have the str.inspect() in the error message instead of the plain string.

fails(https://github.com/jruby/jruby/issues/8736):Time.new with a timezone argument Time.new with a String argument raises ArgumentError when there are leading space characters
fails(https://github.com/jruby/jruby/issues/8736):Time.new with a timezone argument Time.new with a String argument raises ArgumentError when there are trailing whitespaces
Comment on lines -18 to -19
Copy link
Contributor Author

@PChambino PChambino Jul 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two were just about ignoring more "whitespace" chars, not just spaces.

fails:Time.new with a timezone argument Time.new with a String argument returns Time with correct subseconds when given seconds fraction is longer than 9 digits
Copy link
Contributor Author

@PChambino PChambino Jul 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was fixed by adding support for "precision" in RubyTimeParser.parseInt.

Loading