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

GROOVY-9618: index "static getX()" as "X" if class field X also exists #1296

Merged
merged 1 commit into from Jul 4, 2020

Conversation

eric-milles
Copy link
Member

https://issues.apache.org/jira/browse/GROOVY-9618

    // one class meta-property "X"
    class C {
      static private X
      static getX()
    }

    // one class meta-property "x"
    class C {
      static private x
      static getX()
    }

    // one class meta-property "x" and one class meta-field "X"
    class C {
      static private x, X
      static getX()
    }

    // one class meta-property "X" and one instance meta-field "x"
    class C {
      static private X
      static getX()
      private x
    }

    // one meta-property "X"
    class C {
      static private X
      static getX()
    }

    // one meta-property "x"
    class C {
      static private x
      static getX()
    }

    // one meta-property "x" and one meta-field "X"
    class C {
      static private x, X
      static getX()
    }

    // one class meta-property "X" and one instance meta-field "x"
    class C {
      static private X
      static getX()
      private x
    }
@eric-milles
Copy link
Member Author

An alternative would be to dual index "static getX()" as "x" and "X".

@daniellansun daniellansun merged commit a63ae22 into master Jul 4, 2020
@daniellansun
Copy link
Contributor

Merged. Thanks.

@paulk-asert
Copy link
Contributor

paulk-asert commented Jul 5, 2020

I think I might roll this back for the time being. Sorry for not getting time to review earlier.

For this script:

class A {
  private Y = 1
  def getY() { 2 }
  private static X = 1
  static getX() { 2 }
  static class B { }
  class Z { }
}
class AB extends A.B {
  def m() {
    println X
    println x
  }
}
class AZ extends A.Z {
  AZ() {
    super(new A())
  }
  def n() {
    println Y
    println y
  }
}
new AB().m()
new AZ().n()
new A().with {
    println Y
    println y
    println X
    println x
}

In 3.0.4 we get:

1
2
1
2
1
2
1
2

But after this breaking change we get:

2
MPE: No such property: x for class: A
1
2
1
2
2
MPE: No such property: x for class: A

To me this is an edge case from the JavaBean spec which isn't clear.
Some folks might expect "1" from "print X" and "2" from "print x". It can be argued from both points of view.

I am not necessarily against some kind of change here and the dual index idea might be close to what we need, but the above is inconsistent as well as likely to break some existing code.

@daniellansun
Copy link
Contributor

@paulk-asert Good catch! We should avoid breaking existing code as possible as we could.

@paulk-asert
Copy link
Contributor

paulk-asert commented Jul 5, 2020

We have different behavior (similar to after the proposed change) with static Groovy. Adding the necessary @CompileStatic annotations gives, with 3.0.4:

2
2
GroovyCastException: Cannot cast object 'AZ@0123xyz' with class 'AZ' to class 'A'
GroovyCastException: Cannot cast object 'AZ@0123xyz' with class 'AZ' to class 'A'
2
2
2
2

but with 3.0.5-SNAPSHOT with this change we have the subtly different:

2
2
ClassCastException: AZ cannot be cast to A
GroovyCastException: Cannot cast object 'AZ@0123xyz' with class 'AZ' to class 'A'
2
2
2
2

I suspect the GCE/CCE is an orthogonal issue to this change, but we have work to do to improve consistency regardless.

@paulk-asert
Copy link
Contributor

paulk-asert commented Jul 5, 2020

I'll have a go at the dual index idea, and aligning static/instance behavior first and if I can't get that to work I will rollback.

asfgit pushed a commit that referenced this pull request Jul 5, 2020
asfgit pushed a commit that referenced this pull request Jul 5, 2020
…so exists"

See the discussion: #1296

(cherry picked from commit 5416fda)
@daniellansun
Copy link
Contributor

@paulk-asert I rolled back the change for now.

@eric-milles eric-milles deleted the GROOVY-9618 branch July 5, 2020 11:52
@paulk-asert
Copy link
Contributor

PR#1316 now covers this case.

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