groovy

groovyc hangs if a type is declared to extend itself

Details

  • Type: Bug Bug
  • Status: Closed Closed
  • Priority: Minor Minor
  • Resolution: Fixed
  • Affects Version/s: 1.6.5, 1.7-beta-2
  • Fix Version/s: 1.6.6, 1.7-rc-1
  • Component/s: Compiler
  • Labels:
    None
  • Number of attachments :
    2

Description

This code will hang groovyc 1.7beta2:

class XXX extends XXX {
}

With stack:

at org.codehaus.groovy.ast.ClassNode.getUnresolvedSuperClass(ClassNode.java:919)
 at org.codehaus.groovy.ast.ClassNode.getSuperClass(ClassNode.java:913)
 at org.codehaus.groovy.classgen.InnerClassVisitor.getObjectDistance(InnerClassVisitor.java:517)
 at org.codehaus.groovy.classgen.InnerClassVisitor.addDispatcherMethods(InnerClassVisitor.java:378)
 at org.codehaus.groovy.classgen.InnerClassVisitor.visitClass(InnerClassVisitor.java:67)
 at org.codehaus.groovy.control.CompilationUnit$3.call(CompilationUnit.java:173)
 at org.codehaus.groovy.control.CompilationUnit.applyToPrimaryClassNodes(CompilationUnit.java:936)

basically this method loops indefinetly InnerClassVisitor.getObjectDistance():

private int getObjectDistance(ClassNode node) {
        int count = 1;
        while (node!=null && node!=ClassHelper.OBJECT_TYPE) {
            count++;
            node = node.getSuperClass();
        }
        return count;
    }

whereas 1.6.5 prints:

org.codehaus.groovy.control.MultipleCompilationErrorsException: startup failed, cyclic inheritance involving XXX in class XXX

This variant will hang 1.6.5 and 1.7b2:

class XXX extends XXX {
  public static void main(String []argv) {
    print 'hey'
  }
}

it instead loops indefinetly 'around' this location:

at java.util.AbstractList.iterator(AbstractList.java:273)
at java.util.AbstractCollection.toArray(AbstractCollection.java:120)
at java.util.ArrayList.addAll(ArrayList.java:472)
at org.codehaus.groovy.ast.ClassNode.getMethods(ClassNode.java:807)
at org.codehaus.groovy.ast.ClassNode.hasPossibleStaticMethod(ClassNode.java:1169)
at org.codehaus.groovy.control.StaticImportVisitor.transformMethodCallExpression(StaticImportVisitor.java:189)
at org.codehaus.groovy.control.StaticImportVisitor.transform(StaticImportVisitor.java:77)
at org.codehaus.groovy.ast.ClassCodeExpressionTransformer.visitExpressionStatement(ClassCodeExpressionTransformer.java:139)
at org.codehaus.groovy.ast.stmt.ExpressionStatement.visit(ExpressionStatement.java:40)
at org.codehaus.groovy.ast.CodeVisitorSupport.visitBlockStatement(CodeVisitorSupport.java:35)

Attachments

  1. Text File
    3904_v17x_v1.txt
    25/Nov/09 9:04 PM
    2 kB
    Roshan Dawrani
  2. Text File
    3904_v17x_v2.txt
    25/Nov/09 9:04 PM
    4 kB
    Roshan Dawrani

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Roshan Dawrani added a comment -

Attaching 2 versions of the fix for review.

Prior to inner class changes, (upto 1.6.6), although it fails with "cyclic inheritence" error, it goes all the way through to the class generation. I think it has enough information to fail as early as ResolveVisitor#visitClass()
to reject the class if both class and superclass are the same ClassNode.

So,

v1 of patch) Simply implements the above and fails in ResolveVisitor#visitClass() when there are cyclic inheritence

v2 of patch) Removes the earlier cyclic check from CompilationUnit that used to happen after classgen phase. After it starts catching the error in "resolve" phase itself, this code doesn't seem needed to me.

I think it's safe to go ahead with v2 but wanted to double check.

All existing tests are, of-course, going through with both the patches.

Show
Roshan Dawrani added a comment - Attaching 2 versions of the fix for review. Prior to inner class changes, (upto 1.6.6), although it fails with "cyclic inheritence" error, it goes all the way through to the class generation. I think it has enough information to fail as early as ResolveVisitor#visitClass() to reject the class if both class and superclass are the same ClassNode. So, v1 of patch) Simply implements the above and fails in ResolveVisitor#visitClass() when there are cyclic inheritence v2 of patch) Removes the earlier cyclic check from CompilationUnit that used to happen after classgen phase. After it starts catching the error in "resolve" phase itself, this code doesn't seem needed to me. I think it's safe to go ahead with v2 but wanted to double check. All existing tests are, of-course, going through with both the patches.
Hide
Permalink
blackdrag blackdrag added a comment -

what about class A extends B{}, class B extends A{}. I think that was the original case to check for the code in Compilation Unit. And I think the check should maybe moved to Verifier if we keep it.

Show
blackdrag blackdrag added a comment - what about class A extends B{}, class B extends A{}. I think that was the original case to check for the code in Compilation Unit. And I think the check should maybe moved to Verifier if we keep it.
Hide
Permalink
Roshan Dawrani added a comment -

Is there no test for class A extends B{}, class B extends A{}, if that was the case to check for CompilationUnit code? Because nothing broke in tests when I removed it.

Even if we decide to keep it in Verifier, the check in ResolveVisitor that I am introducing in the patch will still be required because the compiler going into infinite loops in the cases reported here - in one case, it is in InnerClassVisitor and in other case in StaticImportVisitor, so only having that check moved to Verifier will not help as it will be later in the compiler phases.

Show
Roshan Dawrani added a comment - Is there no test for class A extends B{}, class B extends A{}, if that was the case to check for CompilationUnit code? Because nothing broke in tests when I removed it. Even if we decide to keep it in Verifier, the check in ResolveVisitor that I am introducing in the patch will still be required because the compiler going into infinite loops in the cases reported here - in one case, it is in InnerClassVisitor and in other case in StaticImportVisitor, so only having that check moved to Verifier will not help as it will be later in the compiler phases.
Hide
Permalink
blackdrag blackdrag added a comment -

well, ok, then in ResolveVisitor... still it should check more than the direct parent

Show
blackdrag blackdrag added a comment - well, ok, then in ResolveVisitor... still it should check more than the direct parent
Hide
Permalink
Roshan Dawrani added a comment -

Can you help provide a test that should pass for what you are suggesting? The test that CompilationUnit code was supposed to serve?

Show
Roshan Dawrani added a comment - Can you help provide a test that should pass for what you are suggesting? The test that CompilationUnit code was supposed to serve?
Hide
Permalink
Roshan Dawrani added a comment -

Or if you know for sure that CompilationUnit code is serving some scenarios by being there, then we can let it be there and just add the change in ResolveVisitor that is getting rid of both the issues reported here. That will be the smallest change that will rid us of both the issues of this JIRA.

May be later you can add a test to the codebase for that CompilationUnit code?

Show
Roshan Dawrani added a comment - Or if you know for sure that CompilationUnit code is serving some scenarios by being there, then we can let it be there and just add the change in ResolveVisitor that is getting rid of both the issues reported here. That will be the smallest change that will rid us of both the issues of this JIRA. May be later you can add a test to the codebase for that CompilationUnit code?
Hide
Permalink
blackdrag blackdrag added a comment -

looks like my commit for rev 3691 is missing the test, even though the commit tells different.

The test would be that

class A extends B{}
class B extends A{}

should not compile. And I think the check in CompilationUnit would catch that, if it had a chance.

Show
blackdrag blackdrag added a comment - looks like my commit for rev 3691 is missing the test, even though the commit tells different. The test would be that class A extends B{} class B extends A{} should not compile. And I think the check in CompilationUnit would catch that, if it had a chance.
Hide
Permalink
Roshan Dawrani added a comment -

No, CompilationUnit is not getting a chance to catch that now as much before classgen, it is going into infinite loop in InnerClassVisitor (semantic analysis) itself (same as 1st case above)

Which means that this case has stopped working since InnerClassVisitor was introduced.

It also means that that code in CompilationUnit has lost its usefulness now - and it must be moved to some place before it reaches InnerClassVisitor/StaticImportVisitor logic where it is going into infinite loops making compiler hang.

Show
Roshan Dawrani added a comment - No, CompilationUnit is not getting a chance to catch that now as much before classgen, it is going into infinite loop in InnerClassVisitor (semantic analysis) itself (same as 1st case above) Which means that this case has stopped working since InnerClassVisitor was introduced. It also means that that code in CompilationUnit has lost its usefulness now - and it must be moved to some place before it reaches InnerClassVisitor/StaticImportVisitor logic where it is going into infinite loops making compiler hang.
Hide
Permalink
blackdrag blackdrag added a comment -

"if it had a chance", bad thing it is not having one. Now I know I introduced that because of the sorting that happens there and because that was causing an infinite loop without the test too. So I guess moving the check to ResolveVisitor might be a good idea.. just your initial check seems not to be enough to me.

Show
blackdrag blackdrag added a comment - "if it had a chance", bad thing it is not having one. Now I know I introduced that because of the sorting that happens there and because that was causing an infinite loop without the test too. So I guess moving the check to ResolveVisitor might be a good idea.. just your initial check seems not to be enough to me.
Hide
Permalink
Roshan Dawrani added a comment -

The logic I currently have in the patch is just in line with the issues reported in this JIRA. If this other A->B, B->A test had existed, then the current changes to ResolveVisitor would not have sufficed and I would have looked for other ways (probably bringing the CompilationUnit logic into ResolveVisitor)

Anyway, I will give that a try and consult you if I have any questions on that. Thanks.

Show
Roshan Dawrani added a comment - The logic I currently have in the patch is just in line with the issues reported in this JIRA. If this other A->B, B->A test had existed, then the current changes to ResolveVisitor would not have sufficed and I would have looked for other ways (probably bringing the CompilationUnit logic into ResolveVisitor) Anyway, I will give that a try and consult you if I have any questions on that. Thanks.
Hide
Permalink
Roshan Dawrani added a comment -

Fixed.

Show
Roshan Dawrani added a comment - Fixed.

People

Vote (0)
Watch (0)

Dates

  • Created:
    Updated:
    Resolved:
Atlassian JIRA (v4.4.3#663-r165197) | Report a problem
Powered by a free Atlassian JIRA open source license for Codehaus. Try JIRA - bug tracking software for your team.