groovy
  1. groovy
  2. GROOVY-4346

Set isSynthetic()/setSynthetic() behaviour right for AST nodes

    Details

    • Type: Bug Bug
    • Status: Open Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: 1.8-beta-1
    • Fix Version/s: 4.0
    • Component/s: class generator
    • Labels:
      None
    • Number of attachments :
      0

      Description

      Currently there is no relation between the very interdependent properties - synthetic of AnnotatedNode and modifiers of various sub-classes like MethodNode, FieldNode, etc - which means that if the developer remembers to call one but forgets another, it leaves the AST in an inconsistent state. It can have synthetic == true but modifiers with SYNTHETIC bit off and vice versa.

      This leads to issues at various places currently because fields, methods, classes that were intended to be synthetic don't necessarily get marked so in the bytecode.

      For the nodes that have modifiers, isSynthetic()/setSynthetic()/setModifiers() should all be in sync - only based on modifiers value behind the scenes.

        Activity

        Hide
        Peter Niederwieser added a comment - - edited

        We really should have two independent properties: one that means "compiler generated" (preferably on ASTNode), and another one that means "to be marked synthetic in bytecode".

        Show
        Peter Niederwieser added a comment - - edited We really should have two independent properties: one that means "compiler generated" (preferably on ASTNode), and another one that means "to be marked synthetic in bytecode".
        Hide
        Roshan Dawrani added a comment -

        It seems wrong to use various combinations of synthetic/modifier properties to achieve the effect of "compiler generated" and/or "to be marked synthetic in bytecode"

        What we currently have in place - synthetic + modifier properties - should logically be one property - "to be marked synthetic in bytecode" - and should be manipulated consistently - whether through setModifier() or through setSynthetic()

        And probably a new property "compiler generated" should be introduced which should be set everywhere AST enhancements are made by the compiler.

        Show
        Roshan Dawrani added a comment - It seems wrong to use various combinations of synthetic/modifier properties to achieve the effect of "compiler generated" and/or "to be marked synthetic in bytecode" What we currently have in place - synthetic + modifier properties - should logically be one property - "to be marked synthetic in bytecode" - and should be manipulated consistently - whether through setModifier() or through setSynthetic() And probably a new property "compiler generated" should be introduced which should be set everywhere AST enhancements are made by the compiler.
        Hide
        Peter Niederwieser added a comment -

        Changing the documented semantics of AnnotatedNode.get/setSynthetic is risky. It may be worth it, but we risk breaking 3rd party transforms that rely on the current semantics.

        > And probably a new property "compiler generated" should be introduced which should be set everywhere AST enhancements are made by the compiler.

        Such a field (on all ASTNode's) would have helped me out several times already. It would also allow us to get rid of AnnotatedNode.hasNoRealSourcePosition().

        Show
        Peter Niederwieser added a comment - Changing the documented semantics of AnnotatedNode.get/setSynthetic is risky. It may be worth it, but we risk breaking 3rd party transforms that rely on the current semantics. > And probably a new property "compiler generated" should be introduced which should be set everywhere AST enhancements are made by the compiler. Such a field (on all ASTNode's) would have helped me out several times already. It would also allow us to get rid of AnnotatedNode.hasNoRealSourcePosition().
        Hide
        Roshan Dawrani added a comment -

        It needs to be cleaned up - one way of the other - so that at the end we have 2 separate properties - one for reliably marking nodes as synthetic in bytecode, and other for reliably conveying that the node was added by compiler (it may or may not be synthetic in the bytecode).

        If we end up using "isSynthetic" for the latter, it will be unfortunate because it will carry a meaning that conflicts with the more established "synthetic" meaning and will be a fertile ground for confusions/bugs.

        I would rather be in favor of changing isSynthetic() to isCompilerGenerated() or something - if it's too late for 1.8, then may be in 1.9 or 2.0, but not stick with it forever.

        Show
        Roshan Dawrani added a comment - It needs to be cleaned up - one way of the other - so that at the end we have 2 separate properties - one for reliably marking nodes as synthetic in bytecode, and other for reliably conveying that the node was added by compiler (it may or may not be synthetic in the bytecode). If we end up using "isSynthetic" for the latter, it will be unfortunate because it will carry a meaning that conflicts with the more established "synthetic" meaning and will be a fertile ground for confusions/bugs. I would rather be in favor of changing isSynthetic() to isCompilerGenerated() or something - if it's too late for 1.8, then may be in 1.9 or 2.0, but not stick with it forever.
        Hide
        Roshan Dawrani added a comment -

        It may also be a good idea to make ClassNode#getMethods()/getProperties()/getFields()/getDeclaredConstructors(), etc return immutable wrappers.

        Right now this data seems ill-encapsulated. I can call ClassNode#getFields() and add a field to this collection outside ClassNode's knowledge.

        Show
        Roshan Dawrani added a comment - It may also be a good idea to make ClassNode#getMethods()/getProperties()/getFields()/getDeclaredConstructors(), etc return immutable wrappers. Right now this data seems ill-encapsulated. I can call ClassNode#getFields() and add a field to this collection outside ClassNode's knowledge.
        Hide
        Guillaume Laforge added a comment -

        Agreed for having a clean isSynthetic()/isCompilerGenerated() distinction.

        Show
        Guillaume Laforge added a comment - Agreed for having a clean isSynthetic()/isCompilerGenerated() distinction.
        Hide
        blackdrag blackdrag added a comment -

        about the immutabel wrappers... afaik it was on pupose to not to use them. It allows a more lean API... the fact that many helper methods were added over time makes it of course less lean. But in a cleaned-up version for 2.0 I would maybe consider removing many of those helpers.

        Show
        blackdrag blackdrag added a comment - about the immutabel wrappers... afaik it was on pupose to not to use them. It allows a more lean API... the fact that many helper methods were added over time makes it of course less lean. But in a cleaned-up version for 2.0 I would maybe consider removing many of those helpers.
        Hide
        Roshan Dawrani added a comment -

        Shall I create a JIRA for helpers removal or making the returned collections immutable and set it for 2.0 in that case? Or, you'll remember it?

        Show
        Roshan Dawrani added a comment - Shall I create a JIRA for helpers removal or making the returned collections immutable and set it for 2.0 in that case? Or, you'll remember it?

          People

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

            Dates

            • Created:
              Updated: