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

Details

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.

Peter Niederwieser
added a comment - 19/Aug/10 10:51 PM - 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".

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.

Roshan Dawrani
added a comment - 19/Aug/10 11:02 PM 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.

Peter Niederwieser
added a comment - 20/Aug/10 1:43 PM 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().

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.

Roshan Dawrani
added a comment - 20/Aug/10 9:38 PM 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.

Roshan Dawrani
added a comment - 20/Aug/10 9:47 PM 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.

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.

blackdrag blackdrag
added a comment - 21/Aug/10 5:35 AM 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.