Issue Details (XML | Word | Printable)

Key: GROOVY-3237
Type: Bug Bug
Status: Closed Closed
Resolution: Fixed
Priority: Major Major
Assignee: Jim White
Reporter: Alexander Veit
Votes: 0
Watchers: 2
Operations

If you were logged in you would be able to see more operations.
groovy

println is inconsistent

Created: 25/Dec/08 03:31 PM   Updated: 22/Jan/09 09:34 PM
Component/s: groovy-jdk
Affects Version/s: 1.6-rc-1
Fix Version/s: 1.6-rc-2

Time Tracking:
Not Specified

File Attachments: 1. File Groovy3237Bug.groovy (1 kB)

Issue Links:
Related
 
dependent
 


 Description  « Hide
The following test case fails with Groovy version 1.5.6.
NEWLINE = System.getProperty("line.separator")

void doTest(def param) {
	try {
		StringWriter sw1 = new StringWriter()
		StringWriter sw2 = new StringWriter()
		StringWriter sw3 = new StringWriter()

		sw1.write(String.valueOf(param))
		sw2.print(String.valueOf(param))
		new PrintWriter(sw3).print(param)

		assert sw1.toString() == sw2.toString()
		assert sw1.toString() == sw3.toString()

		sw1 = new StringWriter()
		sw2 = new StringWriter()
		sw3 = new StringWriter()

		sw1.write(String.valueOf(param))
		sw1.write(NEWLINE)
		sw2.println(param)
		new PrintWriter(sw3).println(param)

		assert sw1.toString() == sw2.toString()
		assert sw1.toString() == sw3.toString()
	}
	catch (AssertionError e) {
		System.err.println(param)

		throw e
	}
}

doTest(null)
doTest("foo")
doTest(true)
doTest(false)
doTest((byte)123)
doTest((short)1234)
doTest(new Integer(1234))
doTest(new Long(9999999999))
doTest(new Float(1234.5678))
doTest(new Double(1234.5678))
doTest(new BigInteger("123456789012345678901234567890"))
doTest(new BigDecimal("12345678901234567890.1234567890123456789"))
doTest(new Date())
doTest(new StringBuffer("bar"))
doTest([null, "foo", true, false, new Integer(1234)])
doTest(["foo" : "bar", "true": true, "int": new Integer(1234)])


 All   Comments   Work Log   Change History      Sort Order: Ascending order - Click to sort in descending order
Jim White added a comment - 25/Dec/08 03:50 PM
Converted to a GroovyTestCase.

Jim White added a comment - 25/Dec/08 03:53 PM - edited
OK, now I got a test that fails.

I don't have time to work on this more right now, but the first difference looks like something to do with InvokerHelper.toString(Object) vs String.valueOf(Object).


Jim White added a comment - 25/Dec/08 04:00 PM
Here's a version with the other half the test.

Alexander Veit added a comment - 25/Dec/08 04:33 PM
groovy -v
Groovy Version: 1.6-RC-1 JVM: 1.6.0_11

groovy Groovy3237Bug.groovy
.E
Time: 0,125
There was 1 error:
1) testGroovy3227(groovy.bugs.Groovy3237Bug)java.lang.AssertionError: Expression: (t1 == t2).
Values: t1 = 1.2345678901234568E29, t2 = 123456789012345678901234567890

at org.codehaus.groovy.runtime.InvokerHelper.assertFailed(InvokerHelper.java:373)
at org.codehaus.groovy.runtime.ScriptBytecodeAdapter.assertFailed(ScriptBytecodeAdapter.java:658)
at groovy.bugs.Groovy3237Bug.doTest(Groovy3237Bug.groovy:36)
at groovy.bugs.Groovy3237Bug$doTest.callCurrent(Unknown Source)
at org.codehaus.groovy.runtime.callsite.CallSiteArray.defaultCallCurrent(CallSiteArray.java:47)
at org.codehaus.groovy.runtime.callsite.AbstractCallSite.callCurrent(AbstractCallSite.java:142)
at org.codehaus.groovy.runtime.callsite.AbstractCallSite.callCurrent(AbstractCallSite.java:150)
at groovy.bugs.Groovy3237Bug.testGroovy3227(Groovy3237Bug.groovy:51)

and

.E
Time: 0,188
There was 1 error:
1) testGroovy3227(groovy.bugs.Groovy3237Bug)java.lang.AssertionError: Expression: (t1 == t2).
Values: t1 = {foo=bar, true=true, int=1234}, t2 = [foo:bar, true:true, int:1234]

at org.codehaus.groovy.runtime.InvokerHelper.assertFailed(InvokerHelper.java:373)
at org.codehaus.groovy.runtime.ScriptBytecodeAdapter.assertFailed(ScriptBytecodeAdapter.java:658)
at groovy.bugs.Groovy3237Bug.doTest(Groovy3237Bug.groovy:36)
at groovy.bugs.Groovy3237Bug$doTest.callCurrent(Unknown Source)
at org.codehaus.groovy.runtime.callsite.CallSiteArray.defaultCallCurrent(CallSiteArray.java:47)
at org.codehaus.groovy.runtime.callsite.AbstractCallSite.callCurrent(AbstractCallSite.java:142)
at org.codehaus.groovy.runtime.callsite.AbstractCallSite.callCurrent(AbstractCallSite.java:150)
at groovy.bugs.Groovy3237Bug.testGroovy3227(Groovy3237Bug.groovy:56)


groovy -v
Groovy Version: 1.5.6 JVM: 11.0-b16

groovy Groovy3237Bug.groovy
.E
Time: 0,078
There was 1 error:
1) testGroovy3227(groovy.bugs.Groovy3237Bug)java.lang.AssertionError: Expression: (t1 == t2).
Values: t1 = 1.2345678901234568E29, t2 = 123456789012345678901234567890

at org.codehaus.groovy.runtime.InvokerHelper.assertFailed(InvokerHelper.java:393)
at org.codehaus.groovy.runtime.ScriptBytecodeAdapter.assertFailed(ScriptBytecodeAdapter.java:683)
at groovy.bugs.Groovy3237Bug.doTest(Groovy3237Bug.groovy:36)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
at org.codehaus.groovy.reflection.CachedMethod.invoke(CachedMethod.java:86)
at groovy.lang.MetaMethod.doMethodInvoke(MetaMethod.java:230)
at groovy.lang.MetaClassImpl.invokeMethod(MetaClassImpl.java:912)
at org.codehaus.groovy.runtime.ScriptBytecodeAdapter.invokeMethodOnCurrentN(ScriptBytecodeAdapter.java:78)
at groovy.bugs.Groovy3237Bug.testGroovy3227(Groovy3237Bug.groovy:51)

and

groovy Groovy3237Bug.groovy
.E
Time: 0,078
There was 1 error:
1) testGroovy3227(groovy.bugs.Groovy3237Bug)java.lang.AssertionError: Expression: (t1 == t2).
Values: t1 = [null, foo, true, false, 1234], t2 = [null, "foo", true, false, 1234]

at org.codehaus.groovy.runtime.InvokerHelper.assertFailed(InvokerHelper.java:393)
at org.codehaus.groovy.runtime.ScriptBytecodeAdapter.assertFailed(ScriptBytecodeAdapter.java:683)
at groovy.bugs.Groovy3237Bug.doTest(Groovy3237Bug.groovy:36)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
at org.codehaus.groovy.reflection.CachedMethod.invoke(CachedMethod.java:86)
at groovy.lang.MetaMethod.doMethodInvoke(MetaMethod.java:230)
at groovy.lang.MetaClassImpl.invokeMethod(MetaClassImpl.java:912)
at org.codehaus.groovy.runtime.ScriptBytecodeAdapter.invokeMethodOnCurrentN(ScriptBytecodeAdapter.java:78)
at groovy.bugs.Groovy3237Bug.testGroovy3227(Groovy3237Bug.groovy:55)

and

groovy Groovy3237Bug.groovy
.E
Time: 0,078
There was 1 error:
1) testGroovy3227(groovy.bugs.Groovy3237Bug)java.lang.AssertionError: Expression: (t1 == t2).
Values: t1 = {foo=bar, true=true, int=1234}, t2 = ["foo":"bar", "true":true, "int":1234]

at org.codehaus.groovy.runtime.InvokerHelper.assertFailed(InvokerHelper.java:393)
at org.codehaus.groovy.runtime.ScriptBytecodeAdapter.assertFailed(ScriptBytecodeAdapter.java:683)
at groovy.bugs.Groovy3237Bug.doTest(Groovy3237Bug.groovy:36)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
at org.codehaus.groovy.reflection.CachedMethod.invoke(CachedMethod.java:86)
at groovy.lang.MetaMethod.doMethodInvoke(MetaMethod.java:230)
at groovy.lang.MetaClassImpl.invokeMethod(MetaClassImpl.java:912)
at org.codehaus.groovy.runtime.ScriptBytecodeAdapter.invokeMethodOnCurrentN(ScriptBytecodeAdapter.java:78)
at groovy.bugs.Groovy3237Bug.testGroovy3227(Groovy3237Bug.groovy:56)


Alexander Veit added a comment - 25/Dec/08 05:06 PM

the first difference looks like something to do with InvokerHelper.toString(Object) vs String.valueOf(Object).

PrintWriter#print(ln) does not go through the DGM whereas other Writers do. So PrintWriter output does not respect the InvokerHelper#toString()


Jim White added a comment - 26/Dec/08 12:54 AM
The trouble is that Groovy is not actually calling String.valueOf(Object) when the parameter is a BigInteger. Don't know why. I'll open a separate issue for that specifically because I want to keep this issue on bringing order to println.

Alexander Veit added a comment - 26/Dec/08 06:53 AM
String.valueOf() in the above test case is used to mimic the behaviour of PrintWriter to ensure that PrintWriter and other Writers produce the same output.

It is questionable if String.valueOf() should actually be used to implement print(ln) methods in the GDK since it bypasses the MOP and hence does not respect the possibly dynamic nature of Groovy objects.

However, to be consistent Writer#print(ln) must produce the same output as PrintWriter#print(ln). I'm not sure if the special handling of PrintWriter could be turned off easily.

See also
http://jira.codehaus.org/browse/GROOVY-2599
http://jira.codehaus.org/browse/GROOVY-2998


Jim White added a comment - 26/Dec/08 10:10 AM - edited
Yes, it acts like there is a MOP override of String.valueOf(BigInteger) although I don't know where that code is. And if there is such an override, that is a bug because Groovy has no business making incompatible changes in that fundamental Java API. One of the reasons such a change can't/shouldn't be done is that there is no way to get Java code (like PrintWriter) to use it.

But I don't think it is actually an intentional override, like i just commented in GROOVY-3238, the behavior is consistent with the wrong static method being chosen and a conversion to double being performed. The test fails on BigInteger because the result of String.valueOf(BigInteger) in the Groovy code produces the wrong result (it gets converted to a double and toStringed that way) unlike what happens when making that call in Java.

Oh, and the current implementation of DGM print(ln) uses InvokerHelper.toString which does a bunch of Groovy-specific formatting stuff that is different than the toString for those classes. That is part of the helter-skelter state of Groovy formatting and means that we're unlikely to get PrintWriter and String.valueOf doing exactly what they're supposed to do any time soon.


Roshan Dawrani added a comment - 03/Jan/09 12:52 AM
After the fix of GROOVY-3238, String.valueOf(BigInteger) related issue has gone away and that portion of the test provided in this JIRA goes through now.

The last remaining thing that is failing in the given test is following call:

doTest(["foo" : "bar", "true": true, "int": new Integer(1234)])

and the reason this is failing is because of the map param's formatting done by groovy, as in the code below

def map = ["foo" : "bar", "true": true, "int": new Integer(1234)]
// output of the following produced by java.lang.String.valueOf(Object) is {foo=bar, true=true, int=1234}
println String.valueOf(map)
// output of the following produced by groovy's InvokerHelper.formatMap is [foo:bar, true:true, int:1234]
println map

Why does Groovy have its own logic for formatting maps? Couldn't it simply delegate to java's implementation to be consistent in such formatting?


Jim White added a comment - 03/Jan/09 01:54 AM
Groovy likes to display them it's own way. This (along with arrays) has been an area being cleaned up. I've found additional issues related to this but haven't had time to detail them here yet.

I don't think Groovy is going to give up on it's special output formatting, but I think we can get to a more consistent approach. The short version of what I think we need to do is define a GroovyPrintWriter which will work the same way as our non-PrintWriter print methods. A plain PrintWriter is not going to be able to do the same thing as Groovy does when it converts things to strings.


Jim White added a comment - 03/Jan/09 07:49 AM - edited
I have committed a first attempt at bringing order to this printing situation (cs14792). There are several decisions that are somewhat arbitrary, and several difficult ones that I have deferred until you folks weigh in on these changes.

There are three significant new features:

  1. groovy.io.GroovyPrintWriter - This a PrintWriter that implements print(Object) so that it prints the same thing that Groovy does for print(Object). In addition to it's use in DGM for the new/withPrintWriter methods, this is the only public API I'm aware of that can be called from Java to format objects the way Groovy does.
  2. Object as String has been implemented to format the string the way Groovy does when internally 'toStringing' things (like in regex, join, etc). Turns out there hasn't been a way to do that in Groovy (apart from printing). If you use Object.toString() or String.valueOf(Object) you get Java formatting, not Groovy (even in Groovy code).
  3. char[] as String will now return a String consisting of those characters. That is different than the way other arrays are treated, but it is consistent with the way java.io.PrintWriter.print(char[]) works.

So the idea here is that whatever you print with GroovyPrintWriter or DGM.print will be the same as the String you get from Object as String. That is very much like the relationship PrintWriter and String.valueOf(Object) have. It is also consistent with as doing "groovy" formatting.

So we have a some consistent rules now and they can be seen in PrintTest.groovy.

Probably the most significant remaining issue is that arrays of primitives are not displayed the same way as arrays of objects. The arrays of primitives are coerced to lists and are displayed that way with square brackets. Arrays of objects are displayed with curly braces. If we want the curly braces to signify "array", then it should apply to array of primitive too.

  • GROOVY-3253 has changed Object[] to print with square brackets - table updated 1/5.

In that same vein, while char[] is now displayed like a string, Character[] is not. I think that it should display the same as char[] and the PrintWriter.print(char[]) style (like a string) is the way to go. OTOH, there are cases where you may want to display individual characters as an array, so perhaps leaving Character[] the way it is now is better.

A summary of the current (trunk) state of affairs:

Groovy print/as String
(null) null
("foo") foo
(true) true
(false) false
((byte)123) 123
((short)1234) 1234
(new Integer(1234)) 1234
(new Long(9999999999)) 9999999999
(new Float(1234.5678)) 1234.5677
(new Double(1234.5678)) 1234.5678
(new BigInteger("123456789012345678901234567890")) 123456789012345678901234567890
(new BigDecimal("12345678901234567890.1234567890123456789")) 12345678901234567890.1234567890123456789
(new Date(107, 12, 31)) Thu Jan 31 00:00:00 PST 2008
(new StringBuffer("bar")) bar
([null, "foo", true, false, new Integer(1234)]) [null, foo, true, false, 1234]
(["foo" : "bar", "true": true, "int": new Integer(1234)]) [foo:bar, true:true, int:1234]
([null, "foo", true, false, new Integer(1234)] as Object[]) [null, foo, true, false, 1234]
(["foo",new Integer(1234)] as String[]) [foo, 1234]
([true, false] as Boolean[]) [true, false]
([true, false] as boolean[]) [true, false]
([1, 2, 3] as int[]) [1, 2, 3]
([1, 2, 3] as Integer[]) [1, 2, 3]
(['a', 'b', 'c'] as char[]) abc
(['a', 'b', 'c'] as Character[]) [a, b, c]

Once this settles down, an optimization in the DGM.print methods will be to convert directly to strings and use the Writer rather than constructing GroovyPrintWriters.

And finally I haven't addressed PrintStream, but it probably needs GroovyPrintStream in order to match what the DGM.print methods do when they don't get a Writer (which is to format as string and print to System.out).


Jim White added a comment - 05/Jan/09 11:54 PM
Committed to trunk (cs14863) what seems to be most of the changes and a test case that demonstrates what I think the API is supposed to be.

A key difference btw Groovy print and Java print is that Groovy is defined in terms of Object.toString() rather than String.valueOf(Object). That is a necessary difference because Groovy can't override a static method. And this works out OK because in Groovy null.toString() results in "null" (rather than a NPE) which is one of the key virtues of valueOf. Furthermore Groovy code that wants the Java string for an object rather than the Groovy style can still call String.valueOf. That would also be necessary for large and/or recursive structures which Groovy will currently do poorly with (for another JIRA).

There are now DGM for PrintStream.print and PrintStream.println so that Groovy code that does a System.out.println(Object) will work just like println(Object).


Jim White added a comment - 06/Jan/09 02:30 AM
Committed to 1.6 branch (cs14868).