[GitHub] groovy pull request #717: Groovy 8562 wrong closure delegation for property ...

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/groovy/pull/717


---

[GitHub] groovy pull request #717: Groovy 8562 wrong closure delegation for property ...

[danielsun1106]

Github user danielsun1106 commented on a diff in the pull request:

https://github.com/apache/groovy/pull/717#discussion_r190359768
  
--- Diff: 
src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java 
---
@@ -596,6 +597,26 @@ public void visitVariableExpression(VariableExpression 
vexp) {
 } else if (accessedVariable instanceof FieldNode) {
 FieldNode fieldNode = (FieldNode) accessedVariable;
 
+TypeCheckingContext.EnclosingClosure enclosingClosure = 
typeCheckingContext.getEnclosingClosure();
+if (enclosingClosure != null) {
+// GROOVY-8562
+// when vexp has the same name as a property of the owner,
+// the IMPLICIT_RECEIVER must be set in case it's the 
delegate
+if (tryVariableExpressionAsProperty(vexp, vexp.getName())) 
{
+// IMPLICIT_RECEIVER is handled elsewhere
+// however other access needs to be fixed for private 
access
+if 
(vexp.getNodeMetaData(StaticTypesMarker.IMPLICIT_RECEIVER) == null) {
+boolean lhsOfEnclosingAssignment = 
isLHSOfEnclosingAssignment(vexp);
+ClassNode owner = (ClassNode) 
vexp.getNodeMetaData(StaticCompilationMetadataKeys.PROPERTY_OWNER);
+if (owner != null) {
+fieldNode = owner.getField(vexp.getName());
+vexp.setAccessedVariable(fieldNode);
+checkOrMarkPrivateAccess(vexp, fieldNode, 
lhsOfEnclosingAssignment);
--- End diff --

`lhsOfEnclosingAssignment` is only used here(in the `if` block)


---

[GitHub] groovy pull request #717: Groovy 8562 wrong closure delegation for property ...

[danielsun1106]

Github user danielsun1106 commented on a diff in the pull request:

https://github.com/apache/groovy/pull/717#discussion_r190359237
  
--- Diff: 
src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java 
---
@@ -596,6 +597,26 @@ public void visitVariableExpression(VariableExpression 
vexp) {
 } else if (accessedVariable instanceof FieldNode) {
 FieldNode fieldNode = (FieldNode) accessedVariable;
 
+TypeCheckingContext.EnclosingClosure enclosingClosure = 
typeCheckingContext.getEnclosingClosure();
+if (enclosingClosure != null) {
+// GROOVY-8562
+// when vexp has the same name as a property of the owner,
+// the IMPLICIT_RECEIVER must be set in case it's the 
delegate
+if (tryVariableExpressionAsProperty(vexp, vexp.getName())) 
{
+// IMPLICIT_RECEIVER is handled elsewhere
+// however other access needs to be fixed for private 
access
+if 
(vexp.getNodeMetaData(StaticTypesMarker.IMPLICIT_RECEIVER) == null) {
+boolean lhsOfEnclosingAssignment = 
isLHSOfEnclosingAssignment(vexp);
--- End diff --

`lhsOfEnclosingAssignment` seems to be able to inline


---

[GitHub] groovy pull request #717: Groovy 8562 wrong closure delegation for property ...

[MeneDev]

GitHub user MeneDev opened a pull request:

https://github.com/apache/groovy/pull/717

Groovy 8562 wrong closure delegation for property access and compile static

Roundup:

The StaticTypeCheckingVisitor would not look consider FieldNode to 
potentially delegate to a different Field. I think this could only occur for 
name collisions, i.e. the owner has a field with the same name as the delegate.
Calling tryVariableExpressionAsProperty solved this, however this broke 
other tests. To solve this I cheked IMPLICIT_RECEIVER, if unset and 
PROPERTY_OWNER is present, checkOrMarkPrivateAccess must be called to ensure 
the accessors are generated.

The VariableExpressionTransformer ignored delegation metadata when a 
private field accessor was present. I inversed that logic to give delegation 
metadata precedence over private field accessors.

Build passes https://travis-ci.org/MeneDev/groovy/builds/382561549

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/MeneDev/groovy 
GROOVY-8562_WrongClosureDelegationForPropertyAccessAndCompileStatic

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/groovy/pull/717.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #717


commit 0a47728777a15bfa986b89da7495c858396e737f
Author: Mene 
Date:   2018-05-16T18:28:13Z

Add first test case for GROOVY-8562 / first partial but destructive fix

commit 72a08191b525fdf48daca3a72b89b410fb9e
Author: Mene 
Date:   2018-05-16T18:36:44Z

Add problem from jhunovis / slack


https://groovy-community.slack.com/files/U9CM8G6AJ/FAR1PJT1U/behavior_of__with__and___compilestatic.groovy

commit 2930639fc2e25250d72f070091a9f996befcf9e4
Author: Mene 
Date:   2018-05-17T17:03:31Z

Fix first test case for GROOVY-8562 without breaking other tests

commit 5b73488561641cc5b94b4642f126c216675e2040
Author: Mene 
Date:   2018-05-17T17:44:40Z

Add missing tests from GROOVY-8562 / remove unrelated tests

commit 77ee4d4cda14e09288aceec422d9683bab103c56
Author: Mene 
Date:   2018-05-22T17:55:24Z

Fix writing for GROOVY-8562

Call tryTransformDelegateToProperty first then 
tryTransformDelegateToProperty

commit 8f833d3e322339e66cd8e18f90d1a7b4a10fa6ca
Author: Mene 
Date:   2018-05-23T08:43:06Z

Add Test from groovy slack for GROOVY-8562




---