groovy

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

Details

  • Type: Bug Bug
  • Status: Open Open
  • Priority: Critical Critical
  • Resolution: Unresolved
  • Affects Version/s: 1.8-beta-1
  • Fix Version/s: None
  • 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

Ascending order - Click to sort in descending order
Hide
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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:
    blackdrag blackdrag
    Reporter:
    Roshan Dawrani
Vote (0)
Watch (0)

Dates

  • Created:
    Updated: