You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@groovy.apache.org by GitBox <gi...@apache.org> on 2020/07/03 15:01:07 UTC
[GitHub] [groovy] eric-milles opened a new pull request #1296: GROOVY-9618: index "static getX()" as "X" if class field X also exists
eric-milles opened a new pull request #1296:
URL: https://github.com/apache/groovy/pull/1296
https://issues.apache.org/jira/browse/GROOVY-9618
```groovy
// 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
}
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [groovy] paulk-asert commented on pull request #1296: GROOVY-9618: index "static getX()" as "X" if class field X also exists
Posted by GitBox <gi...@apache.org>.
paulk-asert commented on pull request #1296:
URL: https://github.com/apache/groovy/pull/1296#issuecomment-653841780
I'll have a go at the dual index idea first and if I can't get that to work I will rollback.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [groovy] paulk-asert commented on pull request #1296: GROOVY-9618: index "static getX()" as "X" if class field X also exists
Posted by GitBox <gi...@apache.org>.
paulk-asert commented on pull request #1296:
URL: https://github.com/apache/groovy/pull/1296#issuecomment-660546351
PR#1316 now covers this case.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [groovy] paulk-asert edited a comment on pull request #1296: GROOVY-9618: index "static getX()" as "X" if class field X also exists
Posted by GitBox <gi...@apache.org>.
paulk-asert edited a comment on pull request #1296:
URL: https://github.com/apache/groovy/pull/1296#issuecomment-653841780
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [groovy] danielsun1106 commented on pull request #1296: GROOVY-9618: index "static getX()" as "X" if class field X also exists
Posted by GitBox <gi...@apache.org>.
danielsun1106 commented on pull request #1296:
URL: https://github.com/apache/groovy/pull/1296#issuecomment-653844144
@paulk-asert I rolled back the change for now.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [groovy] paulk-asert edited a comment on pull request #1296: GROOVY-9618: index "static getX()" as "X" if class field X also exists
Posted by GitBox <gi...@apache.org>.
paulk-asert edited a comment on pull request #1296:
URL: https://github.com/apache/groovy/pull/1296#issuecomment-653833254
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [groovy] paulk-asert commented on pull request #1296: GROOVY-9618: index "static getX()" as "X" if class field X also exists
Posted by GitBox <gi...@apache.org>.
paulk-asert commented on pull request #1296:
URL: https://github.com/apache/groovy/pull/1296#issuecomment-653833254
I think I might roll this back. Sorry for not getting time to review earlier.
For this class:
```
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [groovy] paulk-asert edited a comment on pull request #1296: GROOVY-9618: index "static getX()" as "X" if class field X also exists
Posted by GitBox <gi...@apache.org>.
paulk-asert edited a comment on pull request #1296:
URL: https://github.com/apache/groovy/pull/1296#issuecomment-653840138
We have different behavior (similar to after the proposed change) with static Groovy. Adding the necessary @CompileStatic 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [groovy] danielsun1106 merged pull request #1296: GROOVY-9618: index "static getX()" as "X" if class field X also exists
Posted by GitBox <gi...@apache.org>.
danielsun1106 merged pull request #1296:
URL: https://github.com/apache/groovy/pull/1296
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [groovy] danielsun1106 commented on pull request #1296: GROOVY-9618: index "static getX()" as "X" if class field X also exists
Posted by GitBox <gi...@apache.org>.
danielsun1106 commented on pull request #1296:
URL: https://github.com/apache/groovy/pull/1296#issuecomment-653792223
Merged. Thanks.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [groovy] paulk-asert edited a comment on pull request #1296: GROOVY-9618: index "static getX()" as "X" if class field X also exists
Posted by GitBox <gi...@apache.org>.
paulk-asert edited a comment on pull request #1296:
URL: https://github.com/apache/groovy/pull/1296#issuecomment-653833254
I think I might roll this back for the time being. Sorry for not getting time to review earlier.
For this class:
```
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [groovy] paulk-asert commented on pull request #1296: GROOVY-9618: index "static getX()" as "X" if class field X also exists
Posted by GitBox <gi...@apache.org>.
paulk-asert commented on pull request #1296:
URL: https://github.com/apache/groovy/pull/1296#issuecomment-653840138
We have different behavior (similar to after the proposed change) with static Groovy. Adding the necessary @CompileStatic 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
```
So, we have work to do to improve consistency regardless.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [groovy] eric-milles commented on pull request #1296: GROOVY-9618: index "static getX()" as "X" if class field X also exists
Posted by GitBox <gi...@apache.org>.
eric-milles commented on pull request #1296:
URL: https://github.com/apache/groovy/pull/1296#issuecomment-653588289
An alternative would be to dual index "static getX()" as "x" and "X".
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [groovy] paulk-asert edited a comment on pull request #1296: GROOVY-9618: index "static getX()" as "X" if class field X also exists
Posted by GitBox <gi...@apache.org>.
paulk-asert edited a comment on pull request #1296:
URL: https://github.com/apache/groovy/pull/1296#issuecomment-653840138
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [groovy] danielsun1106 commented on pull request #1296: GROOVY-9618: index "static getX()" as "X" if class field X also exists
Posted by GitBox <gi...@apache.org>.
danielsun1106 commented on pull request #1296:
URL: https://github.com/apache/groovy/pull/1296#issuecomment-653836551
@paulk-asert Good catch! We should avoid breaking existing code as possible as we could.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org