groovy
  1. groovy
  2. GROOVY-3172

Add setter for field MethodNode.name

    Details

    • Type: Improvement Improvement
    • Status: Closed Closed
    • Priority: Major Major
    • Resolution: Won't Fix
    • Affects Version/s: 1.6-rc-1
    • Fix Version/s: 1.6-rc-1, 1.5.8, 1.7-beta-1
    • Component/s: Compiler
    • Labels:
      None
    • Patch Submitted:
      Yes
    • Number of attachments :
      1

      Description

      I propose to add a setter for field MethodNode.name that allow AST transformations to change a method name, which can be useful at times. My current use case is a DSL for which I want to allow method names composed of arbitrary characters (whitespace, question mark, etc.). For this to work, method names will occasionally have to be encoded to avoid a VerifyError later on.

        Activity

        Hide
        blackdrag blackdrag added a comment -

        the alternative is to remove the method from the class, create a new MethodNode and add that new Method node to the class.

        Show
        blackdrag blackdrag added a comment - the alternative is to remove the method from the class, create a new MethodNode and add that new Method node to the class.
        Hide
        Peter Niederwieser added a comment -

        Isn't this inefficient and error-prone? MethodNode has no copy constructor. And what if other parts of the code already hold a reference to the old MethodNode?

        Show
        Peter Niederwieser added a comment - Isn't this inefficient and error-prone? MethodNode has no copy constructor. And what if other parts of the code already hold a reference to the old MethodNode?
        Hide
        Paul King added a comment -

        Just a comment on the previous comment: what if other parts of the code already contain the method node in a map using its name as the key?

        Show
        Paul King added a comment - Just a comment on the previous comment: what if other parts of the code already contain the method node in a map using its name as the key?
        Hide
        blackdrag blackdrag added a comment -

        and there is already such a map in ClassNode I think

        Show
        blackdrag blackdrag added a comment - and there is already such a map in ClassNode I think
        Hide
        Peter Niederwieser added a comment -

        You are right. ClassNode has a field "MapOfLists methods", which indexes methods by name. Now it's clear why name used to be final. So a better solution might be to leave name final and introduce another field, e.g. outputName, that is used for bytecode generation. (Although the patch doesn't cause immediate problems and works for my use case, others might run into problems once they start to use setName).

        Show
        Peter Niederwieser added a comment - You are right. ClassNode has a field "MapOfLists methods", which indexes methods by name. Now it's clear why name used to be final. So a better solution might be to leave name final and introduce another field, e.g. outputName, that is used for bytecode generation. (Although the patch doesn't cause immediate problems and works for my use case, others might run into problems once they start to use setName).
        Hide
        blackdrag blackdrag added a comment -

        but that would not resolve the issues you get when the map is used for queries... I think static imports do hat for example. I am not sure but the covariants support might be affected too. Maybe also the property method generation. It all depends on what you do with these methods.

        Show
        blackdrag blackdrag added a comment - but that would not resolve the issues you get when the map is used for queries... I think static imports do hat for example. I am not sure but the covariants support might be affected too. Maybe also the property method generation. It all depends on what you do with these methods.
        Hide
        Peter Niederwieser added a comment -

        My conclusion: Although this patch is useful for me and doesn't cause any immediate problems, it is nevertheless unsound if left as-is, in particular because ClassNode.methods won't reflect changes to method names. Therefore I suggest to revert the patch and apologize for the work this has caused. In retrospect I think the patch has fulfilled its purpose by starting a good discussion (thanks to Paul).

        I will propose an improvement for 1.7 to support arbitrary method names, or at least all method names allowed by the JVM spec. Especially the latter should be fairly easy to achieve - it already seems to work now unless the method name becomes part of a class name, for example due to the definition of a closure. Another improvement would be to disallow unsupported method names in the first place in order to avoid strange errors later on.

        Show
        Peter Niederwieser added a comment - My conclusion: Although this patch is useful for me and doesn't cause any immediate problems, it is nevertheless unsound if left as-is, in particular because ClassNode.methods won't reflect changes to method names. Therefore I suggest to revert the patch and apologize for the work this has caused. In retrospect I think the patch has fulfilled its purpose by starting a good discussion (thanks to Paul). I will propose an improvement for 1.7 to support arbitrary method names, or at least all method names allowed by the JVM spec. Especially the latter should be fairly easy to achieve - it already seems to work now unless the method name becomes part of a class name, for example due to the definition of a closure. Another improvement would be to disallow unsupported method names in the first place in order to avoid strange errors later on.
        Hide
        Guillaume Laforge added a comment -

        I've reverted that patch, and close the issue as "won't fix", as per the discussion on this issue.

        Show
        Guillaume Laforge added a comment - I've reverted that patch, and close the issue as "won't fix", as per the discussion on this issue.

          People

          • Assignee:
            Guillaume Laforge
            Reporter:
            Peter Niederwieser
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: