groovy
  1. groovy
  2. GROOVY-2849

A property name in a nested closure interferes with a class property when refering with "this.prop"

    Details

    • Type: Bug Bug
    • Status: Closed Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.6-beta-1
    • Fix Version/s: 1.6-rc-1, 1.5.8, 1.7-beta-1
    • Component/s: parser
    • Labels:
      None
    • Testcase included:
      yes
    • Number of attachments :
      4

      Description

      I guess this may be a parser bug (I'm not sure).

      This class is working fine:

      private class TestClosure {
      def test = 1
      def c1 = {
      def test = 2
      def c2 =

      { println this.test this.test+=10 println this.test }

      c2()
      }
      }

      new TestClosure().c1()

      But when we change:

      def c2 =

      { println this.test this.test+=10 println this.test test = 3 }

      then thing is screwed up.
      'this.test' is resolved to be 'test' of the c1 closure, which is not correct.

      1. Groovy2849Bug_v2.groovy
        0.8 kB
        Roshan Dawrani
      2. Groovy2849Bug.groovy
        0.3 kB
        Chanwit Kaewkasi
      3. Groovy2849BugAST.groovy.xml
        43 kB
        Roshan Dawrani

        Issue Links

          Activity

          Hide
          Chanwit Kaewkasi added a comment -

          A proper test case.

          Show
          Chanwit Kaewkasi added a comment - A proper test case.
          Hide
          blackdrag blackdrag added a comment -

          The "implicit this" is set to true for "this.test", that must not happen. This error seems to be an old one. I confirmed it in 1.0, 1.5.4 and 1.5.x as well as 1.6 trunk

          Show
          blackdrag blackdrag added a comment - The "implicit this" is set to true for "this.test", that must not happen. This error seems to be an old one. I confirmed it in 1.0, 1.5.4 and 1.5.x as well as 1.6 trunk
          Hide
          Roshan Dawrani added a comment -

          The issue is found in all 1.5.8, 1.6-RC-1, and 1.7-beta-1 versions. I am attaching patches that fix the issue in all these 3 versions.

          It is an error found in the class-generator (AsmClassGenerator) and not in the parser.

          When the class bytecode is generated, for properties accessed as this.<prop>, if a class level declared field exists, it gets used. It is not correct for closure inner classes because inside a closure, if a property is explicitly accessed as this.<prop> [implicitThis = true], we don't want closure class level declared field to be used. In this scenario, we want the property access to be done on the outermost class.

          Modified org/codehaus/groovy/classgen/AsmClassGenerator.java with following 2 changes:
          1) visitAttributeOrProperty - Before using class level field, added a check if it was happening within a closure with implicitThis = true for the property.

          2) isExplicitThisInClosure() - Helper method for the new check.

          Show
          Roshan Dawrani added a comment - The issue is found in all 1.5.8, 1.6-RC-1, and 1.7-beta-1 versions. I am attaching patches that fix the issue in all these 3 versions. It is an error found in the class-generator (AsmClassGenerator) and not in the parser. When the class bytecode is generated, for properties accessed as this.<prop>, if a class level declared field exists, it gets used. It is not correct for closure inner classes because inside a closure, if a property is explicitly accessed as this.<prop> [implicitThis = true] , we don't want closure class level declared field to be used. In this scenario, we want the property access to be done on the outermost class. Modified org/codehaus/groovy/classgen/AsmClassGenerator.java with following 2 changes: 1) visitAttributeOrProperty - Before using class level field, added a check if it was happening within a closure with implicitThis = true for the property. 2) isExplicitThisInClosure() - Helper method for the new check.
          Hide
          blackdrag blackdrag added a comment -

          I didn't say it is a bug in the parser. One of the phases in between is setting the "implicitThis" to true. But "this.foo" is with an explicit this, therefor "implicitThis" must be false. Your patch may fix the problem in ACG, but it is only a workaround. The implicitThis must be set correctly, as other phases may use that. You do:

          isThisExpression(expression) && isInClosure() && !implicitThis;
          

          You said if we have in a closure this.prop, then implicitThis will be true. First, as you can clearly see, that is no situation with an implicit this. If the user had written: "prop", then it might have been changed to "this.prop", but with implicitThis set to true. If implicitThis is always set to true for "this.prop" and "prop", then your fix should do nothing, especially not fixing the problem.

          Then you removed the always executed field = classNode.getDeclaredField(name);... won't that affect direct field access in normal classes?

          And there is one additional problem. Is it even legal to directly access the field from the closure? That's a non trivial question to answer, because till now this.prop would go through getProperty of the enclosing class. After your change it might get through getAttribute in MetaClass. If we have this.foo in a normal class and foo is a field, then we generate a direct field access, that does not go through the MetaClass at all. So going through getAttribute would still mean a difference to normal classes. It would bypass most parts of the MOP. But it would not be equal to a direct field access. If we want to have that in the same way as for normal classes, then it would have to be a direct field access (maybe by using a helper method which is called directly).The discussion if it is legal to bypass the MOP for the closure is still open and postponed to 2.0... In fact that means this issue is targeting the wrong release, it should be
          a part of GROOVY-2503. in fact this is maybe even to be seen as duplicate of GROOVY-1569.

          The bug to fix as I see it, is to correct the handling of the implicitThis for now. That should also avoid the interference described. Setting the correct value for implicitThis is I think work of AntlrParserPlugin. Resolving a variable to the correct referent is task of the VariablesScopeVisitor. First APP needs to be fixed to set that value right, then VariableScopevisitor probably needs to be fixed to not to resolve to the local variable... but I think the later is not needed, because the implicitThis should be the reason VariableScopeVisitor has problems with evaluating the correct variable.

          Show
          blackdrag blackdrag added a comment - I didn't say it is a bug in the parser. One of the phases in between is setting the "implicitThis" to true. But "this.foo" is with an explicit this, therefor "implicitThis" must be false. Your patch may fix the problem in ACG, but it is only a workaround. The implicitThis must be set correctly, as other phases may use that. You do: isThisExpression(expression) && isInClosure() && !implicitThis; You said if we have in a closure this.prop, then implicitThis will be true. First, as you can clearly see, that is no situation with an implicit this. If the user had written: "prop", then it might have been changed to "this.prop", but with implicitThis set to true. If implicitThis is always set to true for "this.prop" and "prop", then your fix should do nothing, especially not fixing the problem. Then you removed the always executed field = classNode.getDeclaredField(name);... won't that affect direct field access in normal classes? And there is one additional problem. Is it even legal to directly access the field from the closure? That's a non trivial question to answer, because till now this.prop would go through getProperty of the enclosing class. After your change it might get through getAttribute in MetaClass. If we have this.foo in a normal class and foo is a field, then we generate a direct field access, that does not go through the MetaClass at all. So going through getAttribute would still mean a difference to normal classes. It would bypass most parts of the MOP. But it would not be equal to a direct field access. If we want to have that in the same way as for normal classes, then it would have to be a direct field access (maybe by using a helper method which is called directly).The discussion if it is legal to bypass the MOP for the closure is still open and postponed to 2.0... In fact that means this issue is targeting the wrong release, it should be a part of GROOVY-2503 . in fact this is maybe even to be seen as duplicate of GROOVY-1569 . The bug to fix as I see it, is to correct the handling of the implicitThis for now. That should also avoid the interference described. Setting the correct value for implicitThis is I think work of AntlrParserPlugin. Resolving a variable to the correct referent is task of the VariablesScopeVisitor. First APP needs to be fixed to set that value right, then VariableScopevisitor probably needs to be fixed to not to resolve to the local variable... but I think the later is not needed, because the implicitThis should be the reason VariableScopeVisitor has problems with evaluating the correct variable.
          Hide
          Roshan Dawrani added a comment -

          First of all, sorry about 2 typing errors in my first comment.

          In the both the cases, I meant to write (implicitThis = false) and not (implicitThis=true). I wanted to correct the mistake much sooner but there was no electricity for last one hour at this bloody place

          When the inner closure c2's code is generated, implicitThis is set correctly for both "test" (implicitThis = true) and "this.test" (implicitThis = false) property expressions.

          What I am doing is:

          if(!isExplicitThisInClosure(expression.getObjectExpression(), expression.isImplicitThis())){
              field = classNode.getDeclaredField(name);
          }
          
          protected boolean isExplicitThisInClosure(Expression expression, boolean implicitThis) {
              return isThisExpression(expression) && isInClosure() && !implicitThis;
          }
          

          isExplicitThisInClosure() returns true only when property is explicitly qualified by "this", it is happening within the closure and implicitThis is false (which happens for this.test and not for 'test').

          The method isExplicitThisInClosure() returns false if
          a) this.<prop> is not happening within the closure, or
          b) it is happening within the closure but prop is not qualified by "this"
          So, in all other cases except (this.test within a closure), "field = classNode.getDeclaredField(name);" always gets executed.

          I have tested the fix with all the tests that are these plus the test attached to this JIRA and I think the fix is correct (If it is not legal to directly access the field from the closure, it is a different thing altogether).

          rgds,
          Roshan

          Show
          Roshan Dawrani added a comment - First of all, sorry about 2 typing errors in my first comment. In the both the cases, I meant to write (implicitThis = false) and not (implicitThis=true). I wanted to correct the mistake much sooner but there was no electricity for last one hour at this bloody place When the inner closure c2's code is generated, implicitThis is set correctly for both "test" (implicitThis = true) and "this.test" (implicitThis = false) property expressions. What I am doing is: if (!isExplicitThisInClosure(expression.getObjectExpression(), expression.isImplicitThis())){ field = classNode.getDeclaredField(name); } protected boolean isExplicitThisInClosure(Expression expression, boolean implicitThis) { return isThisExpression(expression) && isInClosure() && !implicitThis; } isExplicitThisInClosure() returns true only when property is explicitly qualified by "this", it is happening within the closure and implicitThis is false (which happens for this.test and not for 'test'). The method isExplicitThisInClosure() returns false if a) this.<prop> is not happening within the closure, or b) it is happening within the closure but prop is not qualified by "this" So, in all other cases except (this.test within a closure), "field = classNode.getDeclaredField(name);" always gets executed. I have tested the fix with all the tests that are these plus the test attached to this JIRA and I think the fix is correct (If it is not legal to directly access the field from the closure, it is a different thing altogether). rgds, Roshan
          Hide
          Roshan Dawrani added a comment - - edited

          Before the fix, in 1.5.8, if c2 only "this.test += 10" and not an interfering expression like "test = 3", then "this.test += 10" gets written as:

          Object tmp120_117 = ScriptBytecodeAdapter.invokeMethodN(
            class$Groovy2849Property,
            ScriptBytecodeAdapter.getGroovyObjectProperty(
              class$Groovy2849Property, super.getThisObject(), "test"
            ), 
            "plus", new Object[] { new Integer(10) }
          ); 
          ScriptBytecodeAdapter.setGroovyObjectProperty(
            tmp120_117, 
            Groovy2849Property.class, super.getThisObject(), "test"
          ); 
          tmp120_117;
          

          After the fix, in c2 that has both "this.test += 10" and "test = 3", "this.test += 10" still gets written exactly same as above - absolutely no difference at all in what gets written in the class bytecode for "this.test += 10". You can verify it in the decompiled class before and after the fix.

          rgds,
          Roshan

          Show
          Roshan Dawrani added a comment - - edited Before the fix, in 1.5.8, if c2 only "this.test += 10" and not an interfering expression like "test = 3", then "this.test += 10" gets written as: Object tmp120_117 = ScriptBytecodeAdapter.invokeMethodN( class$Groovy2849Property, ScriptBytecodeAdapter.getGroovyObjectProperty( class$Groovy2849Property, super .getThisObject(), "test" ), "plus" , new Object [] { new Integer (10) } ); ScriptBytecodeAdapter.setGroovyObjectProperty( tmp120_117, Groovy2849Property.class, super .getThisObject(), "test" ); tmp120_117; After the fix, in c2 that has both "this.test += 10" and "test = 3", "this.test += 10" still gets written exactly same as above - absolutely no difference at all in what gets written in the class bytecode for "this.test += 10". You can verify it in the decompiled class before and after the fix. rgds, Roshan
          Hide
          Roshan Dawrani added a comment -

          The issue has been found in method calls too. Exactly same scenario as described in this bug for property access.

          I am attaching an updated testcase that covers the reported issue for both property access and method call.

          rgds,
          Roshan

          Show
          Roshan Dawrani added a comment - The issue has been found in method calls too. Exactly same scenario as described in this bug for property access. I am attaching an updated testcase that covers the reported issue for both property access and method call. rgds, Roshan
          Hide
          Roshan Dawrani added a comment -

          I'll add one last point for now and then you can decide if the fix is any use.

          When I say that implicitThis is correctly set inside c2 for "this.test = 10" as well as "test = 3", it is by the time expressions come to AsmClassGenerator for class writing.

          I didn't know which was the earliest phase implicitThis is correctly expected to be set so that it useful for all the relevant phases. Without that knowledge, I looked into the issue and made sure in the fix that classes get written fine and runtime behavior is as fixed for the issue and nothing else is broken.

          If implicitThis is supposed to be set correctly soon after AntlrParserPlugin has generated the AST, then obviously the fix is at the wrong place. But I couldn't be sure of that earlier with the AST modification responsibilities distributed all across visitors used in compilation.

          Well, at least an improved test case (property access as well as method call) will come out of it all

          rgds,
          Roshan

          Show
          Roshan Dawrani added a comment - I'll add one last point for now and then you can decide if the fix is any use. When I say that implicitThis is correctly set inside c2 for "this.test = 10" as well as "test = 3", it is by the time expressions come to AsmClassGenerator for class writing. I didn't know which was the earliest phase implicitThis is correctly expected to be set so that it useful for all the relevant phases. Without that knowledge, I looked into the issue and made sure in the fix that classes get written fine and runtime behavior is as fixed for the issue and nothing else is broken. If implicitThis is supposed to be set correctly soon after AntlrParserPlugin has generated the AST, then obviously the fix is at the wrong place. But I couldn't be sure of that earlier with the AST modification responsibilities distributed all across visitors used in compilation. Well, at least an improved test case (property access as well as method call) will come out of it all rgds, Roshan
          Hide
          blackdrag blackdrag added a comment -

          I edited the generated code example, because the page width was totally broken

          Show
          blackdrag blackdrag added a comment - I edited the generated code example, because the page width was totally broken
          Hide
          blackdrag blackdrag added a comment -

          I did probably now explain well enough... in the original test in "this.test+=10" the "this.test" was a PropertyExpression, which is correct, but the implicitThis was set to false... that is the first thing that needs to be fixed here. I confirmed that that bug is still present by using the debugger.

          What I did oversee before is that you don't do isExplicitThisInClosure, but !isExplicitThisInClosure. When you do isThisExpression(expression) && isInClosure() && !implicitThis and for this.test in a closure we have a this-expression, with implicitThis set to false, then it matches exactly that case and could be seen as a fix in some way. But what you are testing here is a condition that is already wrong.

          At last part of the fix has to be to set the implicitThis right, or else it might cause problems elsewhere. I am also positive that further changes to ACG are not required then anymore.

          Show
          blackdrag blackdrag added a comment - I did probably now explain well enough... in the original test in "this.test+=10" the "this.test" was a PropertyExpression, which is correct, but the implicitThis was set to false... that is the first thing that needs to be fixed here. I confirmed that that bug is still present by using the debugger. What I did oversee before is that you don't do isExplicitThisInClosure, but !isExplicitThisInClosure. When you do isThisExpression(expression) && isInClosure() && !implicitThis and for this.test in a closure we have a this-expression, with implicitThis set to false, then it matches exactly that case and could be seen as a fix in some way. But what you are testing here is a condition that is already wrong. At last part of the fix has to be to set the implicitThis right, or else it might cause problems elsewhere. I am also positive that further changes to ACG are not required then anymore.
          Hide
          Roshan Dawrani added a comment -

          I looked at it a bit more and there are a few things. I apologize for the longish mail but couldn't cut down much.:

          1) I don't see the bug that you see in setting of "implicitThis" for "this.test+=10".
          I have attached the generated AST xml for the original test case and here is how the c2 statements come up:
          a) this.test+=10 => BinaryExp [PropertyExp

          {implicitThis = false}, ConstantExp] - Correct because for explicit this, implicitThis should be false.
          b) test = 3 => BinaryExp [VariableExp, ConstantExp] - Correct. VariableExp does not carry implicitThis information. Anyway, it can be a local variable too and not necessarily this.var or super.var.
          c) return this.test=> PropertyExp{implicitThis = false}

          - Correct because of explicit this.

          You said "in the original test in "this.test+=10" the "this.test" was a PropertyExpression, which is correct, but the implicitThis was set to false... that is the first thing that needs to be fixed here." - Why is it implicitThis=false wrong for "this.test += 10", and why does not it need to be fixed?

          2) The phase where "PropertyExp.isImplcitThis = true" comes from ->

          Currently it happens in classgen phase and in ACG(visitVariableExpression()). When ACG visits VariableExp, if the variable is not defined on stack, it maps VariableExp to a PropertyExp[implicitThis = true].

          • You say that phases earlier than "classgen" may need the "implicitThis = true" information. It may be correct to move "mapping of VariableExp to PropertyExp[implicitThis = true]" to a phase earier than classgen so that other phases can utilize this information but that is a design change that is not related to/needed by current bug (because current bug has nothing to do with wrong/late (in terms of phases) setting of implicitThis). If it should be moved to an earlier phase, it should probably be taken as a different exercise.

          3) So, as far as I see and understand, there is no issue with value of implicitThis for any c2 statement. If that is correct, ACG will need a change because it can not generate bytecode for "this.<prop>" to always access a class(or parent) level declared field. It can not do it if "this.<prop>" is happening within a closure with implicitThis = false (in this case, it should access outermost class that contains the closures), which is exactly what I have fixed. So, I think ACG will need a change to differentiate between "closure[implicitThis = false]" and "all other cases". But let us wait and see.

          Let's first get it out of the way if implicitThis value setting has any problem or not. Everything related to current bug and fix dependes on this assumption.

          Show
          Roshan Dawrani added a comment - I looked at it a bit more and there are a few things. I apologize for the longish mail but couldn't cut down much.: 1) I don't see the bug that you see in setting of "implicitThis" for "this.test+=10". I have attached the generated AST xml for the original test case and here is how the c2 statements come up: a) this.test+=10 => BinaryExp [PropertyExp {implicitThis = false}, ConstantExp] - Correct because for explicit this, implicitThis should be false. b) test = 3 => BinaryExp [VariableExp, ConstantExp] - Correct. VariableExp does not carry implicitThis information. Anyway, it can be a local variable too and not necessarily this.var or super.var. c) return this.test=> PropertyExp{implicitThis = false} - Correct because of explicit this. You said "in the original test in "this.test+=10" the "this.test" was a PropertyExpression, which is correct, but the implicitThis was set to false... that is the first thing that needs to be fixed here." - Why is it implicitThis=false wrong for "this.test += 10", and why does not it need to be fixed? 2) The phase where "PropertyExp.isImplcitThis = true" comes from -> Currently it happens in classgen phase and in ACG(visitVariableExpression()). When ACG visits VariableExp, if the variable is not defined on stack, it maps VariableExp to a PropertyExp [implicitThis = true] . You say that phases earlier than "classgen" may need the "implicitThis = true" information. It may be correct to move "mapping of VariableExp to PropertyExp [implicitThis = true] " to a phase earier than classgen so that other phases can utilize this information but that is a design change that is not related to/needed by current bug (because current bug has nothing to do with wrong/late (in terms of phases) setting of implicitThis). If it should be moved to an earlier phase, it should probably be taken as a different exercise. 3) So, as far as I see and understand, there is no issue with value of implicitThis for any c2 statement. If that is correct, ACG will need a change because it can not generate bytecode for "this.<prop>" to always access a class(or parent) level declared field. It can not do it if "this.<prop>" is happening within a closure with implicitThis = false (in this case, it should access outermost class that contains the closures), which is exactly what I have fixed. So, I think ACG will need a change to differentiate between "closure [implicitThis = false] " and "all other cases". But let us wait and see. Let's first get it out of the way if implicitThis value setting has any problem or not. Everything related to current bug and fix dependes on this assumption.
          Hide
          blackdrag blackdrag added a comment -

          true, the xml shows that the implicitThis is set correctly.. but I assure you, that once ACG is reached, the implicitThis is changed! I confirmed that multiple times using the debugger. Your patch would not work if it where different. The AST is transformed multiple times and I think what you get in the XML is only the initial AST. For example the VariableExpression variables do not refer to a defined variable originating from a Parameter or DeclarationExpression.

          If in your code you write "this.test", then the implicitThis field has to be set to true. That is because we wrote "this", so the "this" is explicit and not implicit. Transformations may produce the same thing, but with an implicitThis set to true, which has a different semantic. With an explicit "this" we want to access the surrounding class, not the closure, with the implicit "this" we have to go through the lookup mechanisms at runtime, unless we reference a local variable.

          Now you say ACG is doing that transformation.. that's 2898... oh my... now I have to look why it was added in this way... hmm... rev 4287.. frankly I think that is just wrong. We are trying to process a class variable, so using the implicit this is quite questionable... I think that line should probably set the implicit this to false..

          I think now, that the whole usage of implicitThis in PropertyExpression should be... well, ignored in 1.5, deprecated in 1.6 and removed in 1.7.. that is if ignoring the implicit this in ACG does solve the issue.

          Show
          blackdrag blackdrag added a comment - true, the xml shows that the implicitThis is set correctly.. but I assure you, that once ACG is reached, the implicitThis is changed! I confirmed that multiple times using the debugger. Your patch would not work if it where different. The AST is transformed multiple times and I think what you get in the XML is only the initial AST. For example the VariableExpression variables do not refer to a defined variable originating from a Parameter or DeclarationExpression. If in your code you write "this.test", then the implicitThis field has to be set to true. That is because we wrote "this", so the "this" is explicit and not implicit. Transformations may produce the same thing, but with an implicitThis set to true, which has a different semantic. With an explicit "this" we want to access the surrounding class, not the closure, with the implicit "this" we have to go through the lookup mechanisms at runtime, unless we reference a local variable. Now you say ACG is doing that transformation.. that's 2898... oh my... now I have to look why it was added in this way... hmm... rev 4287.. frankly I think that is just wrong. We are trying to process a class variable, so using the implicit this is quite questionable... I think that line should probably set the implicit this to false.. I think now, that the whole usage of implicitThis in PropertyExpression should be... well, ignored in 1.5, deprecated in 1.6 and removed in 1.7.. that is if ignoring the implicit this in ACG does solve the issue.
          Hide
          Roshan Dawrani added a comment -

          I am sorry but I think you are understanding the implicitThis in just the opposite and that is causing all the confusion. Once that is in sync, all other things will fall in place and it'll be clear that issue has 0% relation to implicitThis value being set incorrectly. And then it should look like a simple bug that can be fixed in all versions easily

          You are saying "If in your code you write "this.test", then the implicitThis field has to be set to true. That is because we wrote "this", so the "this" is explicit and not implicit."
          -> Because we wrote "this", this is explicit, and not implicit, that is why implicitThis should be false.

          For "this.test = 10" -> implicitThis should never be true - whether in initial AST or after all the transformations. It is never becoming true - neither in initial AST, nor in any transformation of AST.

          For "test = 10", initial AST does not have any implicitThis information (as it is a VariableExpression). ACG (in classgen phase) sets implicitThis to true by mapping the VariableExpression to PropertyExpression (unless it is a reference to a local variable). ACG must write implicitThis to true here and not false (because it is processing "test = 10"). Making it implicitThis=true makes ACG access class level variable.

          The only mistake in the whole thing is in ACG when it is writing the bytecode for "this.test = 10". It is not checking whether it is doing it within a closure or not. It's an issue as simple as this.

          Show
          Roshan Dawrani added a comment - I am sorry but I think you are understanding the implicitThis in just the opposite and that is causing all the confusion. Once that is in sync, all other things will fall in place and it'll be clear that issue has 0% relation to implicitThis value being set incorrectly. And then it should look like a simple bug that can be fixed in all versions easily You are saying "If in your code you write "this.test", then the implicitThis field has to be set to true. That is because we wrote "this", so the "this" is explicit and not implicit." -> Because we wrote "this", this is explicit, and not implicit, that is why implicitThis should be false. For "this.test = 10" -> implicitThis should never be true - whether in initial AST or after all the transformations. It is never becoming true - neither in initial AST, nor in any transformation of AST. For "test = 10", initial AST does not have any implicitThis information (as it is a VariableExpression). ACG (in classgen phase) sets implicitThis to true by mapping the VariableExpression to PropertyExpression (unless it is a reference to a local variable). ACG must write implicitThis to true here and not false (because it is processing "test = 10"). Making it implicitThis=true makes ACG access class level variable. The only mistake in the whole thing is in ACG when it is writing the bytecode for "this.test = 10". It is not checking whether it is doing it within a closure or not. It's an issue as simple as this.
          Hide
          blackdrag blackdrag added a comment -

          I feel as if I told you kind of rubbish most of the time. Sorry for that.

          Still I don't feel good about the implicitThis. Normally if we do

          class {
            void foo() {
              this.x
              y
            }
          }
          

          then this.x should be a property expression with the implicitThis set to false... which it is. Now the problem is, that y is not a PropertyExpression, so it can not have the implicitThis. Since y is no local variable in the compileStack, it will produce a PropertyExpression with "this.y" where implicitThis is true. And well, that is totally correct! so there is really no bug with the implciitThis. I guess I got confused by the fact, that we are not in a normal class, but in a closure, where local variables referenced from the outside are actually fields of the current class. I mean I mostly wrote that by myself, still I forgot.

          So from the view of c2 which was

          c2 = {
          
                      this.test+=10
          
                      test = 3
          
                      return this.test
          
          }

          we actually have something like

          class c2 {
            Reference test
            call() {
              Groovy2849Bug_instance.test+=10 //setProperty+getProperty logic
              test = 3
              return Groovy2849Bug_instance.test //getProperty logic
            }  
          }
          

          So using implciitThis==true for "test=3" is ok. The implicitThis is in fact needed to make a difference between simply "test" and "this.test", because the names are equal.... ah well, so be it.

          Still I would change the patch... first remove the negation in the if, better build the negation into the method we call... so isNotExplipicitThisInClosure... next is... we already have checked with isThisOrSuper that we are on a this or super. In case of super the implicit this is of course false, so checking for "this" is needed, no change here... but maybe we could change the order a bit. Let us first check the implicitThis, then the Closure and then this, because in that order the checks are less expensive for many cases.... isNotExplipicitThisInClosure would then be:

             protected boolean isNotExplipicitThisInClosure(Expression expression, boolean implicitThis) {
             	return implicitThis || !isInClosure || !isThisExpression(expression);
             }
          

          And maybe I should add some comments to give me a hint what happens here, so I won't fall into the same trap again next time!

          Show
          blackdrag blackdrag added a comment - I feel as if I told you kind of rubbish most of the time. Sorry for that. Still I don't feel good about the implicitThis. Normally if we do class { void foo() { this .x y } } then this.x should be a property expression with the implicitThis set to false... which it is. Now the problem is, that y is not a PropertyExpression, so it can not have the implicitThis. Since y is no local variable in the compileStack, it will produce a PropertyExpression with "this.y" where implicitThis is true. And well, that is totally correct! so there is really no bug with the implciitThis. I guess I got confused by the fact, that we are not in a normal class, but in a closure, where local variables referenced from the outside are actually fields of the current class. I mean I mostly wrote that by myself, still I forgot. So from the view of c2 which was c2 = { this .test+=10 test = 3 return this .test } we actually have something like class c2 { Reference test call() { Groovy2849Bug_instance.test+=10 //setProperty+getProperty logic test = 3 return Groovy2849Bug_instance.test //getProperty logic } } So using implciitThis==true for "test=3" is ok. The implicitThis is in fact needed to make a difference between simply "test" and "this.test", because the names are equal.... ah well, so be it. Still I would change the patch... first remove the negation in the if, better build the negation into the method we call... so isNotExplipicitThisInClosure... next is... we already have checked with isThisOrSuper that we are on a this or super. In case of super the implicit this is of course false, so checking for "this" is needed, no change here... but maybe we could change the order a bit. Let us first check the implicitThis, then the Closure and then this, because in that order the checks are less expensive for many cases.... isNotExplipicitThisInClosure would then be: protected boolean isNotExplipicitThisInClosure(Expression expression, boolean implicitThis) { return implicitThis || !isInClosure || !isThisExpression(expression); } And maybe I should add some comments to give me a hint what happens here, so I won't fall into the same trap again next time!
          Hide
          Roshan Dawrani added a comment -

          Thank you.

          With so much on your plate, I am sure it is only natural to forget some details as their origin moves farther into past.

          I am glad that I'll probably now have a tiny fingerprint on ACG.

          I will make the negation related changes, test it with all 3 versions (1.5.8, 1.6, and 1.7) and re-attach the patches.

          Shall I also now include the fix for this same issue for method calls (ACG -> visitMethodCallExpression())?

          rgds,
          Roshan

          Show
          Roshan Dawrani added a comment - Thank you. With so much on your plate, I am sure it is only natural to forget some details as their origin moves farther into past. I am glad that I'll probably now have a tiny fingerprint on ACG. I will make the negation related changes, test it with all 3 versions (1.5.8, 1.6, and 1.7) and re-attach the patches. Shall I also now include the fix for this same issue for method calls (ACG -> visitMethodCallExpression())? rgds, Roshan
          Hide
          blackdrag blackdrag added a comment -

          Since the fix for this does not fix the other issue automatically we should probably split both issues and make a new one for the method call problem. We may also need further discussion on the fix... depends on the fix, so having a new fix should make things a bit more easy and allows us to close this issue here without thinking about the other issue.

          Show
          blackdrag blackdrag added a comment - Since the fix for this does not fix the other issue automatically we should probably split both issues and make a new one for the method call problem. We may also need further discussion on the fix... depends on the fix, so having a new fix should make things a bit more easy and allows us to close this issue here without thinking about the other issue.
          Hide
          Roshan Dawrani added a comment -

          I have re-attached the fix patches for this issue as per the latest discussions till this point - moving the condition negation inside the helper method and reversing the order of conditions.

          I have tested the fix with all 1.5.8, 1.6-RC1 and 1.7-beta-1 versions.

          Also, from isNotExplicitThisInClosure(), I have removed the use of isThisExpression() as this condition is already met in visitAttributeOrProperty().

          Also, as suggested, for the similar issue found with method calls, I have opened a separate bug (GROOVY-3156).

          rgds,
          Roshan

          Show
          Roshan Dawrani added a comment - I have re-attached the fix patches for this issue as per the latest discussions till this point - moving the condition negation inside the helper method and reversing the order of conditions. I have tested the fix with all 1.5.8, 1.6-RC1 and 1.7-beta-1 versions. Also, from isNotExplicitThisInClosure(), I have removed the use of isThisExpression() as this condition is already met in visitAttributeOrProperty(). Also, as suggested, for the similar issue found with method calls, I have opened a separate bug ( GROOVY-3156 ). rgds, Roshan
          Hide
          Roshan Dawrani added a comment -

          Re-attaching the test case too - removed the testing of method call interference from it as it has its JIRA item now. Moved that test in its JIRA.

          Show
          Roshan Dawrani added a comment - Re-attaching the test case too - removed the testing of method call interference from it as it has its JIRA item now. Moved that test in its JIRA.
          Hide
          Roshan Dawrani added a comment -

          Fixed by correcting the issue related to property resolution inside a nested closure.

          Show
          Roshan Dawrani added a comment - Fixed by correcting the issue related to property resolution inside a nested closure.

            People

            • Assignee:
              Roshan Dawrani
              Reporter:
              Chanwit Kaewkasi
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: