diff options
author | Reinier Zwitserloot <r.zwitserloot@projectlombok.org> | 2020-02-14 01:20:14 +0100 |
---|---|---|
committer | Reinier Zwitserloot <r.zwitserloot@projectlombok.org> | 2020-02-14 01:21:24 +0100 |
commit | 89f98da78d3ffd9e9f6f7151fcaf5e4329d2e8dd (patch) | |
tree | 7e77630855e4a66851f4c3972a54263f024e58a7 | |
parent | 15b09ee27466baa9107ce6556e9302191f1cd7b5 (diff) | |
download | lombok-89f98da78d3ffd9e9f6f7151fcaf5e4329d2e8dd.tar.gz lombok-89f98da78d3ffd9e9f6f7151fcaf5e4329d2e8dd.tar.bz2 lombok-89f98da78d3ffd9e9f6f7151fcaf5e4329d2e8dd.zip |
[fixes #678] `@Synchronize` an instance method on static variable no longer emits a warning.
16 files changed, 156 insertions, 44 deletions
diff --git a/doc/changelog.markdown b/doc/changelog.markdown index 9fca4ac6..2b513a2b 100644 --- a/doc/changelog.markdown +++ b/doc/changelog.markdown @@ -4,6 +4,7 @@ Lombok Changelog ### v.18.13 "Edgy Guinea Pig" * BREAKING CHANGE: mapstruct users should not add a dependency to lombok-mapstruct-binding. This solves compiling modules with lombok (and mapstruct). * FEATURE: Similar to `@Builder`, you can now configure a `@SuperBuilder`'s 'setter' prefixes via `@SuperBuilder(setterPrefix = "set")` for example. We still discourage doing this. [Pull Request #2357](https://github.com/rzwitserloot/lombok/pull/2357). +* FEATURE: If using `@Synchronized("lockVar")`, if `lockVar` is referring to a static field, the code lombok generates no longer causes a warning about accessing a static entity incorrectly. [Issue #678](https://github.com/rzwitserloot/lombok/issues/678) * BUGFIX: Using `@SuperBuilder` on a class that has some fairly convoluted generics usage would fail with 'Wrong number of type arguments'. [Issue #2359](https://github.com/rzwitserloot/lombok/issues/2359) [Pull Request #2362](https://github.com/rzwitserloot/lombok/pull/2362) ### v1.18.12 (February 1st, 2020) diff --git a/src/core/lombok/eclipse/handlers/EclipseHandlerUtil.java b/src/core/lombok/eclipse/handlers/EclipseHandlerUtil.java index 25999e85..252fd998 100644 --- a/src/core/lombok/eclipse/handlers/EclipseHandlerUtil.java +++ b/src/core/lombok/eclipse/handlers/EclipseHandlerUtil.java @@ -992,7 +992,7 @@ public class EclipseHandlerUtil { * Given for example {@code class Outer { class Inner {} }} this would generate {@code char[][] { "Outer", "Inner" }}. * For method local and top level types, this generates a size-1 char[][] where the only char[] element is {@code name} itself. */ - private static char[][] getQualifiedInnerName(EclipseNode parent, char[] name) { + public static char[][] getQualifiedInnerName(EclipseNode parent, char[] name) { int count = 0; EclipseNode n = parent; @@ -1692,12 +1692,14 @@ public class EclipseHandlerUtil { */ public static MemberExistsResult fieldExists(String fieldName, EclipseNode node) { node = upToTypeNode(node); + char[] fieldNameChars = null; if (node != null && node.get() instanceof TypeDeclaration) { - TypeDeclaration typeDecl = (TypeDeclaration)node.get(); + TypeDeclaration typeDecl = (TypeDeclaration) node.get(); if (typeDecl.fields != null) for (FieldDeclaration def : typeDecl.fields) { char[] fName = def.name; if (fName == null) continue; - if (fieldName.equals(new String(fName))) { + if (fieldNameChars == null) fieldNameChars = fieldName.toCharArray(); + if (Arrays.equals(fName, fieldNameChars)) { return getGeneratedBy(def) == null ? MemberExistsResult.EXISTS_BY_USER : MemberExistsResult.EXISTS_BY_LOMBOK; } } diff --git a/src/core/lombok/eclipse/handlers/HandleSynchronized.java b/src/core/lombok/eclipse/handlers/HandleSynchronized.java index 22f7f967..08d00d91 100644 --- a/src/core/lombok/eclipse/handlers/HandleSynchronized.java +++ b/src/core/lombok/eclipse/handlers/HandleSynchronized.java @@ -1,5 +1,5 @@ /* - * Copyright (C) 2009-2014 The Project Lombok Authors. + * Copyright (C) 2009-2020 The Project Lombok Authors. * * Permission is hereby granted, free of charge, to any person obtaining a copy * of this software and associated documentation files (the "Software"), to deal @@ -25,6 +25,7 @@ import static lombok.core.handlers.HandlerUtil.*; import static lombok.eclipse.handlers.EclipseHandlerUtil.*; import java.lang.reflect.Modifier; +import java.util.Arrays; import lombok.ConfigurationKeys; import lombok.Synchronized; @@ -34,6 +35,7 @@ import lombok.core.AST.Kind; import lombok.eclipse.DeferUntilPostDiet; import lombok.eclipse.EclipseAnnotationHandler; import lombok.eclipse.EclipseNode; +import lombok.eclipse.handlers.EclipseHandlerUtil.MemberExistsResult; import org.eclipse.jdt.internal.compiler.ast.Annotation; import org.eclipse.jdt.internal.compiler.ast.ArrayAllocationExpression; @@ -47,6 +49,7 @@ import org.eclipse.jdt.internal.compiler.ast.QualifiedTypeReference; import org.eclipse.jdt.internal.compiler.ast.Statement; import org.eclipse.jdt.internal.compiler.ast.SynchronizedStatement; import org.eclipse.jdt.internal.compiler.ast.ThisReference; +import org.eclipse.jdt.internal.compiler.ast.TypeDeclaration; import org.eclipse.jdt.internal.compiler.lookup.TypeConstants; import org.mangosdk.spi.ProviderFor; @@ -63,31 +66,52 @@ public class HandleSynchronized extends EclipseAnnotationHandler<Synchronized> { @Override public void preHandle(AnnotationValues<Synchronized> annotation, Annotation source, EclipseNode annotationNode) { EclipseNode methodNode = annotationNode.up(); if (methodNode == null || methodNode.getKind() != Kind.METHOD || !(methodNode.get() instanceof MethodDeclaration)) return; - MethodDeclaration method = (MethodDeclaration)methodNode.get(); + MethodDeclaration method = (MethodDeclaration) methodNode.get(); if (method.isAbstract()) return; - createLockField(annotation, annotationNode, method.isStatic(), false); + createLockField(annotation, annotationNode, new boolean[] {method.isStatic()}, false); } - public char[] createLockField(AnnotationValues<Synchronized> annotation, EclipseNode annotationNode, boolean isStatic, boolean reportErrors) { + public char[] createLockField(AnnotationValues<Synchronized> annotation, EclipseNode annotationNode, boolean[] isStatic, boolean reportErrors) { char[] lockName = annotation.getInstance().value().toCharArray(); Annotation source = (Annotation) annotationNode.get(); boolean autoMake = false; if (lockName.length == 0) { autoMake = true; - lockName = isStatic ? STATIC_LOCK_NAME : INSTANCE_LOCK_NAME; + lockName = isStatic[0] ? STATIC_LOCK_NAME : INSTANCE_LOCK_NAME; } - if (fieldExists(new String(lockName), annotationNode) == MemberExistsResult.NOT_EXISTS) { + EclipseNode typeNode = upToTypeNode(annotationNode); + MemberExistsResult exists = MemberExistsResult.NOT_EXISTS; + + if (typeNode != null && typeNode.get() instanceof TypeDeclaration) { + TypeDeclaration typeDecl = (TypeDeclaration) typeNode.get(); + if (typeDecl.fields != null) for (FieldDeclaration def : typeDecl.fields) { + char[] fName = def.name; + if (fName == null) continue; + if (Arrays.equals(fName, lockName)) { + exists = getGeneratedBy(def) == null ? MemberExistsResult.EXISTS_BY_USER : MemberExistsResult.EXISTS_BY_LOMBOK; + boolean st = def.isStatic(); + if (!st && isStatic[0]) { + if (reportErrors) annotationNode.addError(String.format("The field %s is non-static and thus cannot be used on this static method", new String(lockName))); + return null; + } + isStatic[0] = st; + break; + } + } + } + + if (exists == MemberExistsResult.NOT_EXISTS) { if (!autoMake) { - if (reportErrors) annotationNode.addError(String.format("The field %s does not exist.", new String(lockName))); + if (reportErrors) annotationNode.addError(String.format("The field %s does not exist", new String(lockName))); return null; } FieldDeclaration fieldDecl = new FieldDeclaration(lockName, 0, -1); setGeneratedBy(fieldDecl, source); fieldDecl.declarationSourceEnd = -1; - fieldDecl.modifiers = (isStatic ? Modifier.STATIC : 0) | Modifier.FINAL | Modifier.PRIVATE; + fieldDecl.modifiers = (isStatic[0] ? Modifier.STATIC : 0) | Modifier.FINAL | Modifier.PRIVATE; //We use 'new Object[0];' because unlike 'new Object();', empty arrays *ARE* serializable! ArrayAllocationExpression arrayAlloc = new ArrayAllocationExpression(); @@ -111,20 +135,21 @@ public class HandleSynchronized extends EclipseAnnotationHandler<Synchronized> { int p1 = source.sourceStart -1; int p2 = source.sourceStart -2; - long pos = (((long)p1) << 32) | p2; + long pos = (((long) p1) << 32) | p2; EclipseNode methodNode = annotationNode.up(); if (methodNode == null || methodNode.getKind() != Kind.METHOD || !(methodNode.get() instanceof MethodDeclaration)) { annotationNode.addError("@Synchronized is legal only on methods."); return; } - MethodDeclaration method = (MethodDeclaration)methodNode.get(); + MethodDeclaration method = (MethodDeclaration) methodNode.get(); if (method.isAbstract()) { annotationNode.addError("@Synchronized is legal only on concrete methods."); return; } - char[] lockName = createLockField(annotation, annotationNode, method.isStatic(), true); + boolean[] isStatic = { method.isStatic() }; + char[] lockName = createLockField(annotation, annotationNode, isStatic, true); if (lockName == null) return; if (method.statements == null) return; @@ -137,18 +162,22 @@ public class HandleSynchronized extends EclipseAnnotationHandler<Synchronized> { block.sourceStart = method.bodyStart; Expression lockVariable; - if (method.isStatic()) lockVariable = new QualifiedNameReference(new char[][] { - methodNode.up().getName().toCharArray(), lockName }, new long[] { pos, pos }, p1, p2); - else { + if (isStatic[0]) { + EclipseNode typeNode = upToTypeNode(annotationNode); + char[][] n = getQualifiedInnerName(typeNode, lockName); + long[] ps = new long[n.length]; + Arrays.fill(ps, pos); + lockVariable = new QualifiedNameReference(n, ps, p1, p2); + } else { lockVariable = new FieldReference(lockName, pos); ThisReference thisReference = new ThisReference(p1, p2); setGeneratedBy(thisReference, source); - ((FieldReference)lockVariable).receiver = thisReference; + ((FieldReference) lockVariable).receiver = thisReference; } setGeneratedBy(lockVariable, source); method.statements = new Statement[] { - new SynchronizedStatement(lockVariable, block, 0, 0) + new SynchronizedStatement(lockVariable, block, 0, 0) }; // Positions for in-method generated nodes are special diff --git a/src/core/lombok/javac/handlers/HandleSynchronized.java b/src/core/lombok/javac/handlers/HandleSynchronized.java index 20e85d7e..b6f1e47f 100644 --- a/src/core/lombok/javac/handlers/HandleSynchronized.java +++ b/src/core/lombok/javac/handlers/HandleSynchronized.java @@ -1,5 +1,5 @@ /* - * Copyright (C) 2009-2014 The Project Lombok Authors. + * Copyright (C) 2009-2020 The Project Lombok Authors. * * Permission is hereby granted, free of charge, to any person obtaining a copy * of this software and associated documentation files (the "Software"), to deal @@ -37,11 +37,14 @@ import lombok.javac.handlers.JavacHandlerUtil.MemberExistsResult; import org.mangosdk.spi.ProviderFor; import com.sun.tools.javac.code.Flags; +import com.sun.tools.javac.tree.JCTree; import com.sun.tools.javac.tree.JCTree.JCAnnotation; +import com.sun.tools.javac.tree.JCTree.JCClassDecl; import com.sun.tools.javac.tree.JCTree.JCExpression; import com.sun.tools.javac.tree.JCTree.JCMethodDecl; import com.sun.tools.javac.tree.JCTree.JCNewArray; import com.sun.tools.javac.tree.JCTree.JCStatement; +import com.sun.tools.javac.tree.JCTree.JCTypeParameter; import com.sun.tools.javac.tree.JCTree.JCVariableDecl; import com.sun.tools.javac.util.Context; import com.sun.tools.javac.util.List; @@ -65,7 +68,6 @@ public class HandleSynchronized extends JavacAnnotationHandler<Synchronized> { if (methodNode == null || methodNode.getKind() != Kind.METHOD || !(methodNode.get() instanceof JCMethodDecl)) { annotationNode.addError("@Synchronized is legal only on methods."); - return; } @@ -73,21 +75,41 @@ public class HandleSynchronized extends JavacAnnotationHandler<Synchronized> { if ((method.mods.flags & Flags.ABSTRACT) != 0) { annotationNode.addError("@Synchronized is legal only on concrete methods."); - return; } - boolean isStatic = (method.mods.flags & Flags.STATIC) != 0; + + boolean[] isStatic = new boolean[] {(method.mods.flags & Flags.STATIC) != 0}; String lockName = annotation.getInstance().value(); boolean autoMake = false; if (lockName.length() == 0) { autoMake = true; - lockName = isStatic ? STATIC_LOCK_NAME : INSTANCE_LOCK_NAME; + lockName = isStatic[0] ? STATIC_LOCK_NAME : INSTANCE_LOCK_NAME; } JavacTreeMaker maker = methodNode.getTreeMaker().at(ast.pos); Context context = methodNode.getContext(); - if (fieldExists(lockName, methodNode) == MemberExistsResult.NOT_EXISTS) { + JavacNode typeNode = upToTypeNode(annotationNode); + + MemberExistsResult exists = MemberExistsResult.NOT_EXISTS; + + if (typeNode != null && typeNode.get() instanceof JCClassDecl) { + for (JCTree def : ((JCClassDecl) typeNode.get()).defs) { + if (def instanceof JCVariableDecl) { + if (((JCVariableDecl) def).name.contentEquals(lockName)) { + exists = getGeneratedBy(def) == null ? MemberExistsResult.EXISTS_BY_USER : MemberExistsResult.EXISTS_BY_LOMBOK; + boolean st = ((((JCVariableDecl) def).mods.flags) & Flags.STATIC) != 0; + if (isStatic[0] && !st) { + annotationNode.addError("The field " + lockName + " is non-static and this cannot be used on this static method"); + return; + } + isStatic[0] = st; + } + } + } + } + + if (exists == MemberExistsResult.NOT_EXISTS) { if (!autoMake) { annotationNode.addError("The field " + lockName + " does not exist."); return; @@ -95,18 +117,18 @@ public class HandleSynchronized extends JavacAnnotationHandler<Synchronized> { JCExpression objectType = genJavaLangTypeRef(methodNode, ast.pos, "Object"); //We use 'new Object[0];' because unlike 'new Object();', empty arrays *ARE* serializable! JCNewArray newObjectArray = maker.NewArray(genJavaLangTypeRef(methodNode, ast.pos, "Object"), - List.<JCExpression>of(maker.Literal(CTC_INT, 0)), null); + List.<JCExpression>of(maker.Literal(CTC_INT, 0)), null); JCVariableDecl fieldDecl = recursiveSetGeneratedBy(maker.VarDef( - maker.Modifiers(Flags.PRIVATE | Flags.FINAL | (isStatic ? Flags.STATIC : 0)), - methodNode.toName(lockName), objectType, newObjectArray), ast, context); + maker.Modifiers(Flags.PRIVATE | Flags.FINAL | (isStatic[0] ? Flags.STATIC : 0)), + methodNode.toName(lockName), objectType, newObjectArray), ast, context); injectFieldAndMarkGenerated(methodNode.up(), fieldDecl); } if (method.body == null) return; JCExpression lockNode; - if (isStatic) { - lockNode = chainDots(methodNode, ast.pos, methodNode.up().getName(), lockName); + if (isStatic[0]) { + lockNode = namePlusTypeParamsToTypeReference(maker, typeNode, methodNode.toName(lockName), false, List.<JCTypeParameter>nil()); } else { lockNode = maker.Select(maker.Ident(methodNode.toName("this")), methodNode.toName(lockName)); } diff --git a/test/transform/resource/after-delombok/SynchronizedName.java b/test/transform/resource/after-delombok/SynchronizedName.java index 91fd7ea7..9c160166 100644 --- a/test/transform/resource/after-delombok/SynchronizedName.java +++ b/test/transform/resource/after-delombok/SynchronizedName.java @@ -8,7 +8,7 @@ class SynchronizedName { } } void test4() { - synchronized (this.READ) { + synchronized (SynchronizedName.READ) { System.out.println("four"); } } diff --git a/test/transform/resource/after-delombok/SynchronizedNameStaticToInstanceRef.java b/test/transform/resource/after-delombok/SynchronizedNameStaticToInstanceRef.java index effa036f..2bc056e9 100644 --- a/test/transform/resource/after-delombok/SynchronizedNameStaticToInstanceRef.java +++ b/test/transform/resource/after-delombok/SynchronizedNameStaticToInstanceRef.java @@ -2,8 +2,6 @@ class SynchronizedNameStaticToInstanceRef { private Object read = new Object(); private static Object READ = new Object(); static void test3() { - synchronized (SynchronizedNameStaticToInstanceRef.read) { - System.out.println("three"); - } + System.out.println("three"); } } diff --git a/test/transform/resource/after-delombok/SynchronizedOnStatic.java b/test/transform/resource/after-delombok/SynchronizedOnStatic.java new file mode 100644 index 00000000..30ec16f7 --- /dev/null +++ b/test/transform/resource/after-delombok/SynchronizedOnStatic.java @@ -0,0 +1,18 @@ +class SynchronizedOnStatic<Z> { + static class Inner { + private static Object LCK = new Object[0]; + public void foo() { + synchronized (SynchronizedOnStatic.Inner.LCK) { + System.out.println(); + } + } + } + class Inner2 { + private Object LCK = new Object[0]; + public void foo() { + synchronized (this.LCK) { + System.out.println(); + } + } + } +}
\ No newline at end of file diff --git a/test/transform/resource/after-ecj/SynchronizedName.java b/test/transform/resource/after-ecj/SynchronizedName.java index b7474373..f2c3fea3 100644 --- a/test/transform/resource/after-ecj/SynchronizedName.java +++ b/test/transform/resource/after-ecj/SynchronizedName.java @@ -13,7 +13,7 @@ class SynchronizedName { } } @lombok.Synchronized("READ") void test4() { - synchronized (this.READ) + synchronized (SynchronizedName.READ) { System.out.println("four"); } diff --git a/test/transform/resource/after-ecj/SynchronizedNameStaticToInstanceRef.java b/test/transform/resource/after-ecj/SynchronizedNameStaticToInstanceRef.java index 103c714c..72557c4b 100644 --- a/test/transform/resource/after-ecj/SynchronizedNameStaticToInstanceRef.java +++ b/test/transform/resource/after-ecj/SynchronizedNameStaticToInstanceRef.java @@ -7,9 +7,6 @@ class SynchronizedNameStaticToInstanceRef { super(); } static @lombok.Synchronized("read") void test3() { - synchronized (SynchronizedNameStaticToInstanceRef.read) - { - System.out.println("three"); - } + System.out.println("three"); } } diff --git a/test/transform/resource/after-ecj/SynchronizedOnStatic.java b/test/transform/resource/after-ecj/SynchronizedOnStatic.java new file mode 100644 index 00000000..20dd3514 --- /dev/null +++ b/test/transform/resource/after-ecj/SynchronizedOnStatic.java @@ -0,0 +1,31 @@ +class SynchronizedOnStatic<Z> { + static class Inner { + private static Object LCK = new Object[0]; + <clinit>() { + } + Inner() { + super(); + } + public @lombok.Synchronized("LCK") void foo() { + synchronized (SynchronizedOnStatic.Inner.LCK) + { + System.out.println(); + } + } + } + class Inner2 { + private Object LCK = new Object[0]; + Inner2() { + super(); + } + public @lombok.Synchronized("LCK") void foo() { + synchronized (this.LCK) + { + System.out.println(); + } + } + } + SynchronizedOnStatic() { + super(); + } +}
\ No newline at end of file diff --git a/test/transform/resource/before/SynchronizedOnStatic.java b/test/transform/resource/before/SynchronizedOnStatic.java new file mode 100644 index 00000000..23c4818d --- /dev/null +++ b/test/transform/resource/before/SynchronizedOnStatic.java @@ -0,0 +1,16 @@ +class SynchronizedOnStatic<Z> { + static class Inner { + private static Object LCK = new Object[0]; + @lombok.Synchronized("LCK") + public void foo() { + System.out.println(); + } + } + class Inner2 { + private Object LCK = new Object[0]; + @lombok.Synchronized("LCK") + public void foo() { + System.out.println(); + } + } +} diff --git a/test/transform/resource/messages-delombok/SynchronizedNameStaticToInstanceRef.java.messages b/test/transform/resource/messages-delombok/SynchronizedNameStaticToInstanceRef.java.messages index fe0653c8..6fedd538 100644 --- a/test/transform/resource/messages-delombok/SynchronizedNameStaticToInstanceRef.java.messages +++ b/test/transform/resource/messages-delombok/SynchronizedNameStaticToInstanceRef.java.messages @@ -1 +1 @@ -5 non-static variable read cannot be referenced from a static context
\ No newline at end of file +5 The field read is non-static and this cannot be used on this static method
\ No newline at end of file diff --git a/test/transform/resource/messages-ecj/SynchronizedName.java.messages b/test/transform/resource/messages-ecj/SynchronizedName.java.messages deleted file mode 100644 index 5322f9d2..00000000 --- a/test/transform/resource/messages-ecj/SynchronizedName.java.messages +++ /dev/null @@ -1 +0,0 @@ -8 The static field SynchronizedName.READ should be accessed in a static way diff --git a/test/transform/resource/messages-ecj/SynchronizedNameNoSuchField.java.messages b/test/transform/resource/messages-ecj/SynchronizedNameNoSuchField.java.messages index ae351773..e72c4166 100644 --- a/test/transform/resource/messages-ecj/SynchronizedNameNoSuchField.java.messages +++ b/test/transform/resource/messages-ecj/SynchronizedNameNoSuchField.java.messages @@ -1 +1 @@ -5 The field write does not exist. +5 The field write does not exist diff --git a/test/transform/resource/messages-ecj/SynchronizedNameStaticToInstanceRef.java.messages b/test/transform/resource/messages-ecj/SynchronizedNameStaticToInstanceRef.java.messages index 98b9c948..68cb9d6c 100644 --- a/test/transform/resource/messages-ecj/SynchronizedNameStaticToInstanceRef.java.messages +++ b/test/transform/resource/messages-ecj/SynchronizedNameStaticToInstanceRef.java.messages @@ -1 +1 @@ -5 Cannot make a static reference to the non-static field SynchronizedNameStaticToInstanceRef.read +5 The field read is non-static and thus cannot be used on this static method diff --git a/test/transform/resource/messages-idempotent/SynchronizedNameStaticToInstanceRef.java.messages b/test/transform/resource/messages-idempotent/SynchronizedNameStaticToInstanceRef.java.messages deleted file mode 100644 index 53b39d98..00000000 --- a/test/transform/resource/messages-idempotent/SynchronizedNameStaticToInstanceRef.java.messages +++ /dev/null @@ -1 +0,0 @@ -5 non-static variable read cannot be referenced from a static context |