Skip to content
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.

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

CompileStatic: Spurious errors on equals method shown in eclipse editor and not in Problems #1209

Closed
ssadedin opened this issue Dec 6, 2020 · 15 comments
Assignees
Labels
Milestone

Comments

@ssadedin
Copy link

ssadedin commented Dec 6, 2020

I am having a lot of trouble nailing down when this happens and doesn't happen, and unfortunately
so far don't have a simple example I can share.

The problem is always in an equals comparison and occurs on the base language types like Object, String and null - here's an example where it claims it cannot compare two strings:

image

Here's another example where it can't compare something to null:

image

Sorry I don't yet have a reproducible example - it seems to be sporadic enough that I can't yet make a simple example of it. However I wanted to log this in case there's a way to fix it anyway, or if other people are encountering it.

I was encountering this using the stable release of 3.9.0 and now upgraded to the snapshot release of 4.0:

image

Both of them seem to have the same behaviour.

@eric-milles
Copy link
Member

There is a block in BinaryExpressionTransformer that creates a call to ScriptBytecodeAdapter#compareEquals:

            MethodNode adapter = StaticCompilationTransformer.BYTECODE_BINARY_ADAPTERS.get(operationType);
            if (adapter != null) {
                Expression sba = classX(StaticCompilationTransformer.BYTECODE_ADAPTER_CLASS);
                call = callX(sba, "compareEquals", args(expr, right));
                call.setMethodTarget(adapter);
            } else {
                ...
            }

I think it is supposed to be "compareEqual" -- excerpt from ScriptBytecodeAdapter.java:

    public static boolean compareEqual(Object left, Object right) {

@eric-milles
Copy link
Member

Looking at the "compareTo" transformation (also in BinaryExpressionTransformer), there is a slight chance that the transformed expression "left" and the inferred type "getType(leftExpression)" are not aligned. I think it should be getType(left) and getType(right) when checking for Comparables.

            MethodCallExpression call;
            Expression left = staticCompilationTransformer.transform(leftExpression);
            Expression right = staticCompilationTransformer.transform(rightExpression);

            if (operationType == Types.COMPARE_TO
                    && findType(leftExpression).implementsInterface(ClassHelper.COMPARABLE_TYPE)
                    && findType(rightExpression).implementsInterface(ClassHelper.COMPARABLE_TYPE)) {
                call = callX(left, "compareTo", args(right));
                call.setImplicitThis(false);
                call.setMethodTarget(COMPARE_TO_METHOD);
                call.setSourcePosition(bin);

asfgit pushed a commit to apache/groovy that referenced this issue Dec 6, 2020
@ssadedin
Copy link
Author

ssadedin commented Dec 7, 2020

Wow, thanks for the fast followup 👍

I will wait for a snapshot to include this and then give it a try (unless that's not appropriate at this point?).

@eric-milles
Copy link
Member

ready to test

@ssadedin
Copy link
Author

ssadedin commented Dec 7, 2020

That fixed most of them! there's still a problem with null comparisons:

image

In this case, if I explicitly cast null to the type of the LHS then it removes the error:

image

But that I think should not be necessary.

Thanks for the work on this!

@eric-milles
Copy link
Member

Are you using Groovy 3 or 2.5? I did not port the whole fix to 2.5 because it requires a bit more reogranizing.

@ssadedin
Copy link
Author

ssadedin commented Dec 8, 2020

Are you using Groovy 3 or 2.5? I did not port the whole fix to 2.5 because it requires a bit more reogranizing.

Ah - I'm stuck on 2.5 sorry, should have mentioned. Thanks for working on this so fast!

@eric-milles
Copy link
Member

Is there any more context you can provide? Maybe a small sample project?

@eric-milles
Copy link
Member

I think I have it. "x == null" or "x != null" can be converted to CompareToNullExpression. This expression creates token type COMPARE_TO and it should be COMPARE_EQUAL or COMPARE_NOT_EQUAL. If this is processed by StaticTypeCheckingVisitor#getResultType it is possible to find no compareTo method or no compatible one.

@eric-milles
Copy link
Member

I am wondering if you have a case of extra processing somewhere. How are you enabling static type checking and/or static compilation?

@ssadedin
Copy link
Author

ssadedin commented Dec 9, 2020

Sorry for the lack of context - it's actually an open source project so you can see the full source context for one of the above problems here:

https://github.com/ssadedin/groovy-ngs-utils/blob/master/src/main/groovy/gngs/CoverageGaps.groovy#L231

As shown there, the static compilation is enabled by the @CompileStatic annotation at the method level. There shouldn't be any other weirdness (annotation processing etc) for this project I believe - though since you are looking at this so much if you suspect that could be involved I am more than happy to set up a clean isolated eclipse instance and see if this reproduces in that context.

@ssadedin
Copy link
Author

ssadedin commented Dec 9, 2020

NB: as followup - just did the experiment of applying @CompileStatic at the class level and all the errors went away. So you are onto something in terms of how that is part of the picture here. I guess it changes how all the types are resolved since at the class level the fields are now statically typed too?

@eric-milles
Copy link
Member

there should be a new snapshot available for testing

@ssadedin
Copy link
Author

ssadedin commented Dec 9, 2020

That fixed the rest of the errors - thanks so much for the work on this!

👍

@eric-milles
Copy link
Member

Is it possible for you to setup a separate eclipse instance with 3.9.0.RELEASE? If so, could you check if removing the default value from the parameter makes the error go away?

    @CompileStatic
    void outputBlock(int endPos=-1) { // try remove "=-1"
        blockCount++
        if(endPos>=0)
            block.end = endPos
        
        if(block.end < block.start)
            assert false
            
        blocks.add(block)
        
        if(this.gapProcessor != null)
            this.gapProcessor.sendTo(block)
            
        block = null
    }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants