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

Parameter type is inferred wrong when local variable with same name is present in unrelated method #880

Open
lptr opened this Issue Aug 22, 2018 · 3 comments

Comments

Projects
None yet
3 participants
@lptr

lptr commented Aug 22, 2018

Issue description

Using a where: clause, the type of a parameter is inferred incorrectly from the type of a local variable declared in an unrelated method when using Groovy 2.5. The issue does not occur with Groovy 2.4

How to reproduce

Consider the following code:

import spock.lang.Specification
import spock.lang.Unroll

@Unroll
class MyTest extends Specification {
    def "#a <=> #b: #result"() {
        expect:
        Math.signum(a <=> b) == result

        where:
        a            | b            | result
        "abcdef12"   | "abcdef12"   | 0
    }

    private double[] someOtherMethod() {
        double[] result = new double[0]
        return result
    }
}

This works fine when using Groovy 2.4, but fails when using Groovy 2.5 – both with Spock 1.0-groovy-2.4 and 1.2-RC1-groovy-2.5:

abcdef12 <=> abcdef12: [0.0]
Condition not satisfied:

Math.signum(a <=> b) == result
     |      | |   |  |  |
     0.0    | 0   |  |  [0.0]
            |     |  false
            |     abcdef12
            abcdef12

	at MyTest.#a <=> #b: #result(MyTest.groovy:8)

Notice that result's value is printed as [0.0], which matches the type of the local variable result defined in someOtherMethod(). If that method is removed, or the result parameter in the test is renamed, the test works as expected.

Link to a gist or similar (optional)

Here's a full build to reproduce the problem: https://github.com/lptr/spock-groovy-bug

Just run ./gradlew test

Additional Environment information

Gradle 4.9, Groovy 2.5.2, Spock 1.2-RC1

lptr added a commit to gradle/gradle that referenced this issue Aug 22, 2018

Fix test
For some reason "result" was interpreted as an array, but "expected" isn't.

See spockframework/spock#880
@leonard84

This comment has been minimized.

Show comment
Hide comment
@leonard84

leonard84 Aug 22, 2018

Member

@paulk-asert any idea what changed with groovy 2.5 that causes this?

@lptr my suggestion instead of changing the name of the parameter would be to explicitly declare the method parameters with their correct type.

Member

leonard84 commented Aug 22, 2018

@paulk-asert any idea what changed with groovy 2.5 that causes this?

@lptr my suggestion instead of changing the name of the parameter would be to explicitly declare the method parameters with their correct type.

lptr added a commit to gradle/gradle that referenced this issue Aug 23, 2018

Fix test
For some reason "result" was interpreted as an array, but "expected" isn't.

See spockframework/spock#880

lptr added a commit to gradle/gradle that referenced this issue Aug 23, 2018

Fix test
For some reason "result" was interpreted as an array, but "expected" isn't.

See spockframework/spock#880

lptr added a commit to gradle/gradle that referenced this issue Aug 23, 2018

Fix test
For some reason "result" was interpreted as an array, but "expected" isn't.

See spockframework/spock#880

lptr added a commit to gradle/gradle that referenced this issue Aug 28, 2018

Fix test
For some reason "result" was interpreted as an array, but "expected" isn't.

See spockframework/spock#880
@leonard84

This comment has been minimized.

Show comment
Hide comment
@leonard84

leonard84 Aug 29, 2018

Member

@paulk-asert In Groovy 2.5 during AST

Phase Instruction Selection

    public java.lang.Object $spock_feature_0_0proc(java.lang.Object $spock_p0, java.lang.Object $spock_p1, java.lang.Object $spock_p2) {
        java.lang.Object a = $spock_p0 
        java.lang.Object b = $spock_p1 
        java.lang.Object result = $spock_p2 
        return new java.lang.Object[]
    }

Phase Class Generation

    public java.lang.Object $spock_feature_0_0proc(java.lang.Object $spock_p0, java.lang.Object $spock_p1, java.lang.Object $spock_p2) {
        java.lang.Object a = $spock_p0 
        java.lang.Object b = $spock_p1 
        [D result = $spock_p2 
        return new java.lang.Object[]
    }

While in 2.4 it stays as java.lang.Object

Member

leonard84 commented Aug 29, 2018

@paulk-asert In Groovy 2.5 during AST

Phase Instruction Selection

    public java.lang.Object $spock_feature_0_0proc(java.lang.Object $spock_p0, java.lang.Object $spock_p1, java.lang.Object $spock_p2) {
        java.lang.Object a = $spock_p0 
        java.lang.Object b = $spock_p1 
        java.lang.Object result = $spock_p2 
        return new java.lang.Object[]
    }

Phase Class Generation

    public java.lang.Object $spock_feature_0_0proc(java.lang.Object $spock_p0, java.lang.Object $spock_p1, java.lang.Object $spock_p2) {
        java.lang.Object a = $spock_p0 
        java.lang.Object b = $spock_p1 
        [D result = $spock_p2 
        return new java.lang.Object[]
    }

While in 2.4 it stays as java.lang.Object

@paulk-asert

This comment has been minimized.

Show comment
Hide comment
@paulk-asert

paulk-asert Aug 29, 2018

Contributor

@leonard84 Nothing springs to mind but looks like a bug somewhere. I'll take a look next week.

Contributor

paulk-asert commented Aug 29, 2018

@leonard84 Nothing springs to mind but looks like a bug somewhere. I'll take a look next week.

lptr added a commit to gradle/gradle that referenced this issue Sep 7, 2018

Fix test
For some reason "result" was interpreted as an array, but "expected" isn't.

See spockframework/spock#880

lptr added a commit to gradle/gradle that referenced this issue Sep 7, 2018

Fix test
For some reason "result" was interpreted as an array, but "expected" isn't.

See spockframework/spock#880

lptr added a commit to gradle/gradle that referenced this issue Sep 17, 2018

Merge pull request #6682 from gradle/lptr/core/upgrade-to-groovy-2.5
Changes:

- Upgraded to Groovy 2.5.2.

    This required building our own `groovy-all.jar` (see https://github.com/gradle/gradle-groovy-all), because Groovy 2.5 stopped shipping the uber-jar and replaced it with a BOM. I tried to use the separate JARs, but it poses a number of issues, most importantly that the increased length of the classpath breaks our Windows tests. So we decided to go with repackaging Groovy. In the future it would be nice to remove the need for the repackaging, and maybe even remove some of the Groovy libs. I mean, c'mon, we ship classes from `groovy-swing`!

- Downgraded `opentest4j` to work around ota4j-team/opentest4j#49, introduced by an upgrade in TestNG.
- Upgraded compiler memory in IntelliJ from 2 GB to 3 GB because Groovy compilation with 2.5 seemed to slow down a lot, and produce OOMEs occasionally. With the increased amount of memory Groovy compilation is actually quite snappy.
- Replaced calls to `DateGroovyMethods` extension methods. This led to the realization that some of our timeouts mistakenly use days instead of milliseconds. We should fix these separately, added `TODO`s.
- Bumped Spock to the Groovy 2.5 variant and fixed a number of issues with our tests. Some of these issues are valid ones, others seem to indicate bugs in either the Groovy compiler or Spock. Reported the issue to Spock: spockframework/spock#880

    The `Specification` class added by @ldaley to fix type inference in IntelliJ doesn't work anymore unfortunately. I left the code commented out so it's easy to fix it if someone wants to.

- Groovy 2.5 adds local variable type information that needs to be remapped in our script remapper, so I added that.
- Groovy 2.5 uses the `ExtensionModule` mechanism slightly differently which necessitated the exposure of some more packages and resources from `gradleApi()` (see changes in `DefaultGradleApiSpecProvider.java`)
- Replaced references to types in `com.sun.jdi` in `JDWPUtil` with reflection to work around an IntelliJ bug we hit: https://youtrack.jetbrains.com/oauth?state=%2Fissue%2FIDEA-198715
- There is some weirdness wrt Tooling API progress tests where the order of events seems to have changed somewhat. See changes in `ToolingApiBuildExecutionCrossVersionSpec.groovy` and `ToolingApiModelCrossVersionSpec.groovy`.
- Stopped using Groovy JSON in `:buildSrc:binaryCompatibilityCheck` because of a classloading issue with Groovy's `FastStringUtils`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment