Avoiding stack overflow when decoding generic types

[will-lauer]

Fixes #148.

• Copies GenericsVisitor from Groovy
• In checkGenericsUsage, ensure wildcards don't cause generic resolution to fail
• In checkGenericsUsage, don't get caught in an infinite loop while printing generic type names

[pivotal-issuemaster]

@will-lauer Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

[pivotal-issuemaster]

@will-lauer Thank you for signing the Contributor License Agreement!

[will-lauer]

I've signed the CLA, but I just wanted to note the origin of this change. I've copied the file GenericsVisitor.java from the main goovy distribution from Apache and updated it to fix this particular issue, so the file does still retain the Apache copyright.

[eric-milles]

The file would retain the original copyright. Have you wrapped your change in "// GRECLIPSE add" and "// GRECLIPSE end"? Did you patch each of the Groovy versions that this affects? Is there a test case that can reproduce the issue?

[will-lauer]

I missed adding the GRECLIPSE markers. I've just pushed another commit to include those. I've only addressed Groovy 2.4, as that is the only version I use and is where I originally found the problem. I don't know if problem occurs with other versions of groovy as well.

I haven't figured out how to implement a unit test for this, but this is a simple example that triggers the problem in the associated git issue (#148). If anyone can point me in the right direction for implementing this unit test, I can take a crack at it.

[eric-milles]

Yes indeed, Issue #148 example still fails as described. Add this to GenericsTests in the core.tests.compiler project.

    void testIssue148() {
        String[] sources = [
            'A.java', '''
            public interface A<Q extends A<? super Q>> {
            }''',

            'B.groovy', '''
            class B {
              public void test(A<?> a) {
              }
            }'''
        ]

        runConformTest(sources)
    }

[eric-milles]

It appears from the test that the essential change is only this one:

            // GRECLIPSE edit
            //if (!nType.isDerivedFrom(cnType)) {
            if (!nTypes[i].isWildcard() && !nType.isDerivedFrom(cnType)) {
            // GRECLIPSE end

Or should there be an error generated for this case and the stack overflow is getting in the way of that?

[will-lauer]

Its been a while since I looked at this, so its only coming back to me slowly. I think the problem listed in #148 was only one of two related issues. I guess I'll need to dig up an example of the other. But what was happening was an infinite look trying to resolve the type name. I'll see if I can put together an example of that as well.

[eric-milles]

So if I compile both as Java, all is well. So no warning for the Groovy case. When I compile with groovyc, all is well. So it does seem a Greclipse patch is the right route.