Skip to content

Commit ce37e28

Browse files
eric-milleseric-milles
committed
GROOVY-11675: split property may declare final modifier for accessors
3_0_X backport
1 parent 5fcfd4e commit ce37e28

4 files changed

Lines changed: 132 additions & 74 deletions

File tree

‎src/main/java/org/codehaus/groovy/ast/tools/PropertyNodeUtils.java‎

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -23,21 +23,31 @@
2323
import java.lang.reflect.Modifier;
2424

2525
public class PropertyNodeUtils {
26+
2627
/**
2728
* Fields within the AST that have no explicit visibility are deemed to be properties
2829
* and represented by a PropertyNode. The Groovy compiler creates accessor methods and
2930
* a backing field for such property nodes. During this process, all modifiers
3031
* from the property are carried over to the backing field (so a property marked as
3132
* {@code transient} will have a {@code transient} backing field) but when creating
3233
* the accessor methods we don't carry over modifier values which don't make sense for
33-
* methods (this includes VOLATILE and TRANSIENT) but other modifiers are carried over,
34+
* methods (such as {@code volatile} and {@code transient}) but other modifiers are carried over,
3435
* for example {@code static}.
3536
*
36-
* @param propNode the original property node
37+
* @since 2.4.8
38+
* @param node the original property node
3739
* @return the modifiers which make sense for an accessor method
3840
*/
39-
public static int adjustPropertyModifiersForMethod(PropertyNode propNode) {
40-
// GROOVY-3726: clear volatile, transient modifiers so that they don't get applied to methods
41-
return ~(Modifier.TRANSIENT | Modifier.VOLATILE) & propNode.getModifiers();
41+
public static int adjustPropertyModifiersForMethod(final PropertyNode node) {
42+
// GROOVY-3726, GROOVY-7969: clear modifiers that do not apply to methods
43+
int mods = node.getModifiers() & ~(Modifier.TRANSIENT|Modifier.VOLATILE);
44+
/* GROOVY-11675: split property case may declare final modifier
45+
if (node.getField() == null || node.getField().isSynthetic()) {
46+
mods &= ~Modifier.FINAL;
47+
}*/
48+
if (node.isStatic()) {
49+
mods &= ~Modifier.FINAL;
50+
}
51+
return mods;
4252
}
4353
}

‎src/main/java/org/codehaus/groovy/classgen/Verifier.java‎

Lines changed: 14 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -714,60 +714,52 @@ public void visitProperty(final PropertyNode node) {
714714
String name = node.getName();
715715
FieldNode field = node.getField();
716716

717+
String isserName = "is" + capitalize(name);
717718
String getterName = "get" + capitalize(name);
718719
String setterName = "set" + capitalize(name);
719720

720-
int accessorModifiers = adjustPropertyModifiersForMethod(node);
721-
722721
Statement getterBlock = node.getGetterBlock();
723722
if (getterBlock == null && !node.isPrivate()) {
724723
MethodNode getter = classNode.getGetterMethod(getterName, !node.isStatic());
725724
if (getter == null && node.getType().equals(ClassHelper.boolean_TYPE)) {
726-
getter = classNode.getGetterMethod("is" + capitalize(name));
725+
getter = classNode.getGetterMethod(isserName, !node.isStatic());
727726
}
728727
if (methodNeedsReplacement(getter)) {
729728
getterBlock = createGetterBlock(node, field);
730729
}
731730
}
732731
Statement setterBlock = node.getSetterBlock();
733-
if (setterBlock == null && !node.isPrivate() && !isFinal(accessorModifiers)) {
732+
if (setterBlock == null && !node.isPrivate() && !isFinal(node.getModifiers())) {
734733
boolean voidOnly = false; // accept setter with non-void return type
735734
MethodNode setter = classNode.getSetterMethod(setterName, voidOnly);
736735
if (methodNeedsReplacement(setter)) {
737736
setterBlock = createSetterBlock(node, field);
738737
}
739738
}
740739

741-
int getterModifiers = accessorModifiers;
742-
// don't make static accessors final
743-
if (node.isStatic()) {
744-
getterModifiers &= ~ACC_FINAL;
745-
}
740+
int accessorModifiers = adjustPropertyModifiersForMethod(node);
741+
746742
if (getterBlock != null) {
747-
visitGetter(node, getterBlock, getterModifiers, getterName);
743+
addAccessMethod(getterName, accessorModifiers, node.getType(), Parameter.EMPTY_ARRAY, getterBlock);
748744

749745
if (node.getType().equals(ClassHelper.boolean_TYPE) || node.getType().equals(ClassHelper.Boolean_TYPE)) {
750-
String isserName = "is" + capitalize(name);
751746
MethodNode isser = classNode.getGetterMethod(isserName, !node.isStatic());
752747
if (methodNeedsReplacement(isser)) {
753-
visitGetter(node, getterBlock, getterModifiers, isserName);
748+
addAccessMethod(isserName, accessorModifiers, node.getType(), Parameter.EMPTY_ARRAY, getterBlock);
754749
}
755750
}
756751
}
757752
if (setterBlock != null) {
758-
Parameter[] setterParameterTypes = {new Parameter(node.getType(), "value")};
759-
MethodNode setter = new MethodNode(setterName, accessorModifiers, ClassHelper.VOID_TYPE, setterParameterTypes, ClassNode.EMPTY_ARRAY, setterBlock);
760-
setter.setSynthetic(true);
761-
addPropertyMethod(setter);
762-
visitMethod(setter);
753+
Parameter[] setterParameters = {new Parameter(node.getType(), "value")};
754+
addAccessMethod(setterName, accessorModifiers, ClassHelper.VOID_TYPE, setterParameters, setterBlock);
763755
}
764756
}
765757

766-
private void visitGetter(final PropertyNode node, final Statement getterBlock, final int getterModifiers, final String getterName) {
767-
MethodNode getter = new MethodNode(getterName, getterModifiers, node.getType(), Parameter.EMPTY_ARRAY, ClassNode.EMPTY_ARRAY, getterBlock);
768-
getter.setSynthetic(true);
769-
addPropertyMethod(getter);
770-
visitMethod(getter);
758+
private void addAccessMethod(final String name, final int mods, final ClassNode returnType, final Parameter[] parameters, final Statement accessBlock) {
759+
MethodNode accessor = new MethodNode(name, mods, returnType, parameters, ClassNode.EMPTY_ARRAY, accessBlock);
760+
accessor.setSynthetic(true);
761+
addPropertyMethod(accessor);
762+
visitMethod(accessor);
771763
}
772764

773765
protected void addPropertyMethod(final MethodNode method) {

‎src/main/java/org/codehaus/groovy/transform/trait/TraitASTTransformation.java‎

Lines changed: 18 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,7 @@
6161
import java.util.List;
6262
import java.util.Set;
6363

64+
import static java.lang.reflect.Modifier.isFinal;
6465
import static org.apache.groovy.ast.tools.AnnotatedNodeUtils.markAsGenerated;
6566
import static org.apache.groovy.ast.tools.ClassNodeUtils.addGeneratedMethod;
6667
import static org.apache.groovy.ast.tools.MethodNodeUtils.getCodeAsBlock;
@@ -77,6 +78,7 @@
7778
import static org.codehaus.groovy.ast.tools.GeneralUtils.returnS;
7879
import static org.codehaus.groovy.ast.tools.GeneralUtils.stmt;
7980
import static org.codehaus.groovy.ast.tools.GeneralUtils.varX;
81+
import static org.codehaus.groovy.ast.tools.PropertyNodeUtils.adjustPropertyModifiersForMethod;
8082
import static org.codehaus.groovy.transform.trait.SuperCallTraitTransformer.UNRESOLVED_HELPER_CLASS;
8183

8284
/**
@@ -390,48 +392,48 @@ private static void generatePropertyMethods(final ClassNode cNode) {
390392
private static void processProperty(final ClassNode cNode, final PropertyNode node) {
391393
String name = node.getName();
392394
FieldNode field = node.getField();
393-
int propNodeModifiers = node.getModifiers() & 0x1F; // GROOVY-3726
394395

395-
String getterName = GeneralUtils.getGetterName(node);
396-
String setterName = GeneralUtils.getSetterName(name);
396+
String isserName = "is" + capitalize(name);
397+
String getterName = "get" + capitalize(name);
398+
String setterName = "set" + capitalize(name);
397399

398400
Statement getterBlock = node.getGetterBlock();
399-
if (getterBlock == null) {
401+
if (getterBlock == null && !node.isPrivate()) {
400402
MethodNode getter = cNode.getGetterMethod(getterName);
401403
if (getter == null && node.getType().equals(ClassHelper.boolean_TYPE)) {
402-
getter = cNode.getGetterMethod("is" + capitalize(name));
404+
getter = cNode.getGetterMethod(isserName);
403405
}
404-
if (!node.isPrivate() && methodNeedsReplacement(cNode, getter)) {
406+
if (methodNeedsReplacement(cNode, getter)) {
405407
getterBlock = stmt(fieldX(field));
406408
}
407409
}
408410
Statement setterBlock = node.getSetterBlock();
409-
if (setterBlock == null) {
410-
// 2nd arg false below: though not usual, allow setter with non-void return type
411-
MethodNode setter = cNode.getSetterMethod(setterName, false);
412-
if (!node.isPrivate() && (propNodeModifiers & ACC_FINAL) == 0
413-
&& methodNeedsReplacement(cNode, setter)) {
411+
if (setterBlock == null && !node.isPrivate() && !isFinal(node.getModifiers())) {
412+
MethodNode setter = cNode.getSetterMethod(setterName, /*void-only:*/false);
413+
if (methodNeedsReplacement(cNode, setter)) {
414414
setterBlock = assignS(fieldX(field), varX(name));
415415
}
416416
}
417417

418+
int methodModifiers = adjustPropertyModifiersForMethod(node); // GROOVY-3726
419+
418420
if (getterBlock != null) {
419-
MethodNode getter = new MethodNode(getterName, propNodeModifiers, node.getType(), Parameter.EMPTY_ARRAY, ClassNode.EMPTY_ARRAY, getterBlock);
421+
MethodNode getter = new MethodNode(getterName, methodModifiers, node.getType(), Parameter.EMPTY_ARRAY, ClassNode.EMPTY_ARRAY, getterBlock);
420422
getter.setSynthetic(true);
421423
addGeneratedMethod(cNode, getter);
422424

423425
if (node.getType().equals(ClassHelper.boolean_TYPE) || node.getType().equals(ClassHelper.Boolean_TYPE)) {
424-
MethodNode secondGetter = new MethodNode("is" + capitalize(name), propNodeModifiers, node.getType(), Parameter.EMPTY_ARRAY, ClassNode.EMPTY_ARRAY, getterBlock);
425-
secondGetter.setSynthetic(true);
426-
addGeneratedMethod(cNode, secondGetter);
426+
getter = new MethodNode(isserName, methodModifiers, node.getType(), Parameter.EMPTY_ARRAY, ClassNode.EMPTY_ARRAY, getterBlock);
427+
getter.setSynthetic(true);
428+
addGeneratedMethod(cNode, getter);
427429
}
428430
}
429431
if (setterBlock != null) {
430432
VariableExpression var = (VariableExpression) ((BinaryExpression) ((ExpressionStatement) setterBlock).getExpression()).getRightExpression();
431433
Parameter setterParameter = new Parameter(node.getType(), name);
432434
var.setAccessedVariable(setterParameter);
433435

434-
MethodNode setter = new MethodNode(setterName, propNodeModifiers, ClassHelper.VOID_TYPE, params(setterParameter), ClassNode.EMPTY_ARRAY, setterBlock);
436+
MethodNode setter = new MethodNode(setterName, methodModifiers, ClassHelper.VOID_TYPE, params(setterParameter), ClassNode.EMPTY_ARRAY, setterBlock);
435437
setter.setSynthetic(true);
436438
addGeneratedMethod(cNode, setter);
437439
}

‎src/test/groovy/PropertyTest.groovy‎

Lines changed: 85 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -117,38 +117,92 @@ class PropertyTest extends GroovyTestCase {
117117
assert foo.body == "James"
118118
}
119119

120+
void testSplitProperty() {
121+
assertScript '''
122+
import java.lang.reflect.*
123+
import groovy.transform.Generated
124+
125+
class C {
126+
@Deprecated private final Integer one
127+
final Integer one
128+
129+
protected synchronized Integer two
130+
synchronized Integer two
131+
132+
public Integer three
133+
@Deprecated Integer three
134+
}
135+
136+
Member m = C.getDeclaredField('one')
137+
assert m.isAnnotationPresent(Deprecated)
138+
assert m.modifiers == Modifier.PRIVATE + Modifier.FINAL
139+
140+
m = C.getDeclaredMethod('getOne')
141+
assert m.isAnnotationPresent(Generated)
142+
assert !m.isAnnotationPresent(Deprecated)
143+
assert m.modifiers == Modifier.PUBLIC + Modifier.FINAL
144+
145+
groovy.test.GroovyAssert.shouldFail(NoSuchMethodException) {
146+
m = C.getDeclaredMethod('setOne', Integer)
147+
}
148+
149+
m = C.getDeclaredField('two')
150+
assert m.modifiers == Modifier.PRIVATE
151+
// field cannot carry modifier SYNCHRONIZED
152+
153+
m = C.getDeclaredMethod('getTwo')
154+
assert m.modifiers == Modifier.PUBLIC + Modifier.SYNCHRONIZED
155+
assert m.isAnnotationPresent(Generated)
156+
157+
m = C.getDeclaredMethod('setTwo', Integer)
158+
assert m.modifiers == Modifier.PUBLIC + Modifier.SYNCHRONIZED
159+
assert m.isAnnotationPresent(Generated)
160+
161+
m = C.getDeclaredField('three')
162+
assert m.modifiers == Modifier.PRIVATE
163+
assert m.isAnnotationPresent(Deprecated)
164+
165+
m = C.getDeclaredMethod('getThree')
166+
assert m.modifiers == Modifier.PUBLIC
167+
assert m.isAnnotationPresent(Generated)
168+
assert !m.isAnnotationPresent(Deprecated)
169+
170+
m = C.getDeclaredMethod('setThree', Integer)
171+
assert m.modifiers == Modifier.PUBLIC
172+
assert m.isAnnotationPresent(Generated)
173+
assert !m.isAnnotationPresent(Deprecated)
174+
'''
175+
}
176+
120177
void testFinalProperty() {
121-
def shell = new GroovyShell();
122-
assertScript """
123-
class A {
124-
final foo = 1
125-
}
126-
A.class.declaredMethods.each {
127-
assert it.name!="setFoo"
128-
129-
}
130-
assert new A().foo==1
131-
"""
132-
shouldFail {
133-
shell.execute """
134-
class A {
135-
final foo = 1
136-
}
137-
new A().foo = 2
138-
"""
139-
}
178+
assertScript '''
179+
class C {
180+
final foo = 1
181+
}
182+
C.declaredMethods.each {
183+
assert it.name != "setFoo"
184+
}
185+
186+
assert new C().foo == 1
187+
'''
188+
189+
shouldFail '''
190+
class C {
191+
final foo = 1
192+
}
193+
194+
new C().foo = 2
195+
'''
140196
}
141197

142198
void testFinalField() {
143-
def shell = new GroovyShell();
144-
shouldFail {
145-
shell.execute """
146-
class A {
147-
public final foo = 1
148-
}
149-
new A().foo = 2
150-
"""
151-
}
199+
shouldFail '''
200+
class C {
201+
public final foo = 1
202+
}
203+
204+
new C().foo = 2
205+
'''
152206
}
153207

154208
void testFinalPropertyWithInheritance() {
@@ -255,7 +309,7 @@ class PropertyTest extends GroovyTestCase {
255309
}
256310
class A extends Base {
257311
private String name = 'AA'
258-
312+
259313
@Override
260314
String getName() {
261315
this.name
@@ -437,7 +491,7 @@ class PropertyTest extends GroovyTestCase {
437491
static String getAProp() { 'AProp' }
438492
static String getpNAME() { 'pNAME' }
439493
}
440-
494+
441495
import static A.*
442496
443497
assert prop == 'Prop'
@@ -467,7 +521,7 @@ class PropertyTest extends GroovyTestCase {
467521
static String getAProp() { 'AProp' }
468522
static String getpNAME() { 'pNAME' }
469523
}
470-
524+
471525
import static A.*
472526
473527
class B {

0 commit comments

Comments
 (0)