Skip to content

Commit

Permalink
Fixed #2161. Hash inspection not working as in MRI for sprintf %p.
Browse files Browse the repository at this point in the history 
Fixed specs:
-String#% when format string contains %<> formats should raise ArgumentError if no hash given
-String#% pads with spaces for %E with Inf, -Inf, and NaN

This also fixes a whole class of errors with mixed argument usage but it is
foggy enough where it did not lead to any obvious specs.
  • Loading branch information
@enebo
enebo committed Feb 10, 2017
1 parent ae9cff9 commit 86a1058
Show file tree
Hide file tree
Showing 2 changed files with 140 additions and 74 deletions.
212 changes: 140 additions & 72 deletions core/src/main/java/org/jruby/util/Sprintf.java
View file
Expand Up @@ -102,8 +102,9 @@ private static final class Args {
private final RubyArray rubyArray;
private final RubyHash rubyHash;
private final int length;
private int unnumbered; // last index (+1) accessed by next()
private int numbered; // last index (+1) accessed by get()
private int positionIndex; // last index (+1) accessed by next()
private int nextIndex;
private IRubyObject nextObject;

Args(Locale locale, IRubyObject rubyObject) {
if (rubyObject == null) throw new IllegalArgumentException("null IRubyObject passed to sprintf");
Expand All @@ -117,12 +118,16 @@ private static final class Args {
// allow a hash for args if in 1.9 mode
this.rubyHash = (RubyHash)rubyObject;
this.rubyArray = null;
this.length = -1;
this.length = 1;
} else {
this.length = 1;
this.rubyArray = null;
this.rubyHash = null;
}

positionIndex = 0;
nextIndex = 1;

this.runtime = rubyObject.getRuntime();
}

Expand Down Expand Up @@ -152,10 +157,94 @@ void warning(ID id, String message) {
if (runtime.isVerbose()) runtime.getWarnings().warning(id, message);
}

private IRubyObject getHashValue(ByteList name) {
// FIXME: get_hash does hash conversion of argv and arity check...this is a bit complicated with
// our version. Implement it.
if (rubyHash == null) {
raiseArgumentError("one hash required");
}

checkNameArg(name);
RubySymbol nameSym = runtime.newSymbol(name);
IRubyObject object = rubyHash.fastARef(nameSym);

// if not found, try dispatching to pick up default hash value
// MRI: spliced together bits from rb_hash_default_value
if (object == null) {
object = rubyHash.getIfNone();
if (object == RubyBasicObject.UNDEF) {
raiseKeyError("key<" + name + "> not found");
} else if (rubyHash.hasDefaultProc()) {
object = object.callMethod(runtime.getCurrentContext(), "call", nameSym);
}

if (object.isNil()) throw runtime.newKeyError("key " + nameSym + " not found");
}

return object;
}

private IRubyObject getNthArg(int index) {
if (index > length) raiseArgumentError("too few arguments");

return rubyArray == null ? rubyObject : rubyArray.eltInternal(index - 1);
}


// MRI: GETARG
private IRubyObject getArg() {
if (nextObject != null) {
// This is different in MRI. The do a retry and avoid part of the for loop
// which resets nextvalue. We do not do that so we null out here since we
// cannot get same value twice.
IRubyObject result = nextObject;
nextObject = null;
return result;
}

return getNextArg();
}

// MRI: GETNEXTARG
private IRubyObject getNextArg() {
checkNextArg();
positionIndex = nextIndex++;
return getNthArg(positionIndex);
}

// MRI: GETPOSARG
private IRubyObject getPositionArg(int index) {
checkPositionArg(index);
positionIndex = -1;
return getNthArg(index);
}

// MRI: check_next_arg
private void checkNextArg() {
if (positionIndex == -1) raiseArgumentError("unnumbered(" + nextIndex + ") mixed with numbered");
if (positionIndex == -2) raiseArgumentError("unnumbered(" + nextIndex + ") mixed with named");
}

// MRI: check_pos_arg
private void checkPositionArg(int nthArg) {
if (positionIndex > 0) raiseArgumentError("numbered(" + nthArg + ") after unnumbered(" + positionIndex + ")");
if (positionIndex == -2) raiseArgumentError("numbered(" + nthArg + ") after named");
if (nthArg < 1) raiseArgumentError("invalid index - " + nthArg + "$");
}

// MRI: check_name_arg, CHECKNAMEARG
private void checkNameArg(ByteList name) {
if (positionIndex > 0) raiseArgumentError("named " + name + " after unnumbered(" + positionIndex + ")");
if (positionIndex == -1) raiseArgumentError("named " + name + " after numbered");

positionIndex = -2;
}

@Deprecated
IRubyObject next(ByteList name) {
// for 1.9 hash args
if (name != null) {
if (rubyHash == null) raiseArgumentError("positional args mixed with named args");
if (rubyHash == null && positionIndex == -1) raiseArgumentError("positional args mixed with named args");

RubySymbol nameSym = runtime.newSymbol(name);
IRubyObject object = rubyHash.fastARef(nameSym);
Expand All @@ -181,32 +270,29 @@ IRubyObject next(ByteList name) {
}

// this is the order in which MRI does these two tests
if (numbered > 0) raiseArgumentError("unnumbered" + (unnumbered + 1) + "mixed with numbered");
if (unnumbered >= length) raiseArgumentError("too few arguments");
IRubyObject object = rubyArray == null ? rubyObject : rubyArray.eltInternal(unnumbered);
unnumbered++;
if (positionIndex == -1) raiseArgumentError("unnumbered" + (positionIndex + 1) + "mixed with numbered");
if (positionIndex >= length) raiseArgumentError("too few arguments");
IRubyObject object = rubyArray == null ? rubyObject : rubyArray.eltInternal(positionIndex);
positionIndex++;
return object;
}

@Deprecated
IRubyObject get(int index) {
// for 1.9 hash args
if (rubyHash != null) raiseArgumentError("positional args mixed with named args");
// this is the order in which MRI does these tests
if (unnumbered > 0) raiseArgumentError("numbered("+numbered+") after unnumbered("+unnumbered+")");
if (index < 0) raiseArgumentError("invalid index - " + (index + 1) + '$');
if (index >= length) raiseArgumentError("too few arguments");
numbered = index + 1;
return rubyArray == null ? rubyObject : rubyArray.eltInternal(index);
return getPositionArg(index);
}

@Deprecated
IRubyObject getNth(int formatIndex) {
return get(formatIndex - 1);
return getPositionArg(formatIndex);
}

@Deprecated
int nextInt() {
return intValue(next(null));
}

@Deprecated
int getNthInt(int formatIndex) {
return intValue(get(formatIndex - 1));
}
Expand Down Expand Up @@ -347,6 +433,8 @@ private static boolean rubySprintfToBuffer(ByteList buf, CharSequence charFormat
name = new ByteList(format, nameStart, nameEnd - nameStart, encoding, false);

if (oldName != null) raiseArgumentError(args, "name<" + name + "> after <" + oldName + ">");
// we retrieve value from hash so we can generate argument error as side-effect.
args.nextObject = args.getHashValue(name);

break;
}
Expand All @@ -367,7 +455,7 @@ private static boolean rubySprintfToBuffer(ByteList buf, CharSequence charFormat
if (nameEnd == nameStart) raiseArgumentError(args, ERR_MALFORMED_NAME);

ByteList localName = new ByteList(format, nameStart, nameEnd - nameStart, encoding, false);
buf.append(args.next(localName).asString().getByteList());
buf.append(args.getHashValue(localName).asString().getByteList());
incomplete = false;

break;
Expand Down Expand Up @@ -405,10 +493,10 @@ private static boolean rubySprintfToBuffer(ByteList buf, CharSequence charFormat
checkOffset(args, offset, length, ERR_MALFORMED_NUM);
}
if (fchar == '$') {
if (arg != null) {
if (args.nextObject != null) {
raiseArgumentError(args,"value given twice - " + number + "$");
}
arg = args.getNth(number);
args.nextObject = args.getPositionArg(number);
offset++;
} else {
width = number;
Expand All @@ -424,28 +512,28 @@ private static boolean rubySprintfToBuffer(ByteList buf, CharSequence charFormat
// TODO: factor this chunk as in MRI/YARV GETASTER
checkOffset(args,++offset,length,ERR_MALFORMED_STAR_NUM);
mark = offset;
// GETASTER
number = 0;
for ( ; offset < length && isDigit(fchar = format[offset]); offset++) {
number = extendWidth(args,number,fchar);
}
checkOffset(args,offset,length,ERR_MALFORMED_STAR_NUM);

IRubyObject tmp;
if (fchar == '$') {
width = args.getNthInt(number);
if (width < 0) {
flags |= FLAG_MINUS;
width = -width;
}
tmp = args.getPositionArg(number);
offset++;
} else {
width = args.nextInt();
if (width < 0) {
flags |= FLAG_MINUS;
width = -width;
}
// let the width (if any), get processed in the next loop,
// so any leading 0 gets treated correctly
tmp = args.getNextArg();
offset = mark;
}

width = args.intValue(tmp);
if (width < 0) {
flags |= FLAG_MINUS;
width = -width;
}

break;

case '.':
Expand All @@ -456,31 +544,27 @@ private static boolean rubySprintfToBuffer(ByteList buf, CharSequence charFormat
checkOffset(args,++offset,length,ERR_MALFORMED_DOT_NUM);
fchar = format[offset];
if (fchar == '*') {
// TODO: factor this chunk as in MRI/YARV GETASTER
checkOffset(args,++offset,length,ERR_MALFORMED_STAR_NUM);
mark = offset;
// GETASTER
number = 0;
{ // MRI: GETNUM macro
for (; offset < length && isDigit(fchar = format[offset]); offset++) {
number = extendWidth(args, number, fchar);
}
checkOffset(args, offset, length, ERR_MALFORMED_STAR_NUM);
for (; offset < length && isDigit(fchar = format[offset]); offset++) {
number = extendWidth(args, number, fchar);
}
checkOffset(args, offset, length, ERR_MALFORMED_STAR_NUM);

if (fchar == '$') {
precision = args.getNthInt(number);
if (precision < 0) {
flags &= ~FLAG_PRECISION;
}
tmp = args.getPositionArg(number);
offset++;
} else {
precision = args.nextInt();
if (precision < 0) {
flags &= ~FLAG_PRECISION;
}
// let the width (if any), get processed in the next loop,
// so any leading 0 gets treated correctly
tmp = args.getNextArg();
offset = mark;
}

precision = args.intValue(tmp);
if (precision < 0) {
flags &= ~FLAG_PRECISION;
}
} else {
number = 0;
for ( ; offset < length && isDigit(fchar = format[offset]); offset++) {
Expand All @@ -503,14 +587,11 @@ private static boolean rubySprintfToBuffer(ByteList buf, CharSequence charFormat
break;

case 'c': {
if (arg == null || name != null) {
arg = args.next(name);
name = null;
}
arg = args.getArg();

int c = 0;
int n = 0;
IRubyObject tmp = arg.checkStringType19();
tmp = arg.checkStringType19();
if (!tmp.isNil()) {
if (((RubyString)tmp).strLength() != 1) {
throw runtime.newArgumentError("%c requires a character");
Expand Down Expand Up @@ -554,10 +635,7 @@ else if ((flags & FLAG_MINUS) != 0) {
}
case 'p':
case 's': {
if (arg == null || name != null) {
arg = args.next(name);
name = null;
}
arg = args.getArg();

if (fchar == 'p') {
arg = arg.callMethod(arg.getRuntime().getCurrentContext(),"inspect");
Expand Down Expand Up @@ -611,10 +689,7 @@ else if ((flags & FLAG_MINUS) != 0) {
case 'b':
case 'B':
case 'u': {
if (arg == null || name != null) {
arg = args.next(name);
name = null;
}
arg = args.getArg();

ClassIndex type = arg.getMetaClass().getClassIndex();
if (type != ClassIndex.FIXNUM && type != ClassIndex.BIGNUM) {
Expand Down Expand Up @@ -798,10 +873,7 @@ else if ((flags & FLAG_MINUS) != 0) {
case 'f':
case 'G':
case 'g': {
if (arg == null || name != null) {
arg = args.next(name);
name = null;
}
arg = args.getArg();

if (!(arg instanceof RubyFloat)) {
// FIXME: what is correct 'recv' argument?
Expand Down Expand Up @@ -846,16 +918,12 @@ else if ((flags & FLAG_MINUS) != 0) {
}
width -= len;

if (width > 0 && (flags & (FLAG_ZERO|FLAG_MINUS)) == 0) {
buf.fill(' ',width);
if (width > 0 && (flags & FLAG_MINUS) == 0) {
buf.fill(' ', width);
width = 0;
}
if (signChar != 0) buf.append(signChar);

if (width > 0 && (flags & FLAG_MINUS) == 0) {
buf.fill('0',width);
width = 0;
}
buf.append(digits);
if (width > 0) buf.fill(' ', width);

Expand Down Expand Up @@ -1348,7 +1416,7 @@ else if ((flags & FLAG_MINUS) != 0) {
} // main while loop (offset < length)

// MRI behavior: validate only the unnumbered arguments
if ((args.numbered == 0) && args.unnumbered < args.length) {
if (args.positionIndex >= 0 && args.nextIndex < args.length) {
if (args.runtime.getDebug().isTrue()) {
args.raiseArgumentError("too many arguments for format string");
} else if (args.runtime.isVerbose()) {
Expand Down
2 changes: 0 additions & 2 deletions spec/tags/ruby/core/string/modulo_tags.txt
View file

This file was deleted.

0 comments on commit 86a1058

Please sign in to comment.