Skip to content

Commit

Permalink
GROOVY-11508: error: abstract and static or final method modifiers
Browse files Browse the repository at this point in the history
  • Loading branch information
@eric-milles
eric-milles committed Nov 1, 2024
1 parent 74f6866 commit d557f27
Show file tree
Hide file tree
Showing 3 changed files with 114 additions and 98 deletions.
14 changes: 6 additions & 8 deletions src/main/java/org/apache/groovy/parser/antlr4/ModifierManager.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,21 +65,19 @@ public int getModifierCount() {

private void validate(List<ModifierNode> modifierNodeList) {
Map<ModifierNode, Integer> modifierNodeCounter = new LinkedHashMap<>(modifierNodeList.size());
int visibilityModifierCnt = 0;
int visibilityModifierCount = 0;

for (ModifierNode modifierNode : modifierNodeList) {
Integer cnt = modifierNodeCounter.get(modifierNode);

if (null == cnt) {
var count = modifierNodeCounter.get(modifierNode);
if (count == null) {
modifierNodeCounter.put(modifierNode, 1);
} else if (1 == cnt && !modifierNode.isRepeatable()) {
} else if (count == 1 && !modifierNode.isRepeatable()) {
throw astBuilder.createParsingFailedException("Cannot repeat modifier[" + modifierNode.getText() + "]", modifierNode);
}

if (modifierNode.isVisibilityModifier()) {
visibilityModifierCnt++;

if (visibilityModifierCnt > 1) {
visibilityModifierCount += 1;
if (visibilityModifierCount > 1) {
throw astBuilder.createParsingFailedException("Cannot specify modifier[" + modifierNode.getText() + "] when access scope has already been defined", modifierNode);
}
}
Expand Down
84 changes: 34 additions & 50 deletions src/main/java/org/codehaus/groovy/classgen/ClassCompletionVerifier.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@
import org.codehaus.groovy.syntax.Types;
import org.codehaus.groovy.transform.trait.Traits;

import java.util.ArrayList;
import java.util.Arrays;
import java.util.HashMap;
import java.util.LinkedHashSet;
Expand All @@ -65,7 +66,6 @@
import static java.lang.reflect.Modifier.isVolatile;
import static org.objectweb.asm.Opcodes.ACC_ABSTRACT;
import static org.objectweb.asm.Opcodes.ACC_FINAL;
import static org.objectweb.asm.Opcodes.ACC_INTERFACE;
import static org.objectweb.asm.Opcodes.ACC_NATIVE;
import static org.objectweb.asm.Opcodes.ACC_PRIVATE;
import static org.objectweb.asm.Opcodes.ACC_PROTECTED;
Expand Down Expand Up @@ -118,7 +118,6 @@ public void visitClass(final ClassNode node) {
checkInterfaceMethodVisibility(node);
checkAbstractMethodVisibility(node);
checkClassForExtendingFinalOrSealed(node);
checkMethodsForIncorrectModifiers(node);
checkMethodsForIncorrectName(node);
checkMethodsForWeakerAccess(node);
checkMethodsForOverridingFinal(node);
Expand Down Expand Up @@ -251,38 +250,24 @@ private void checkClassExtendsOrImplementsSelfTypes(final ClassNode node) {
}

private void checkClassForIncorrectModifiers(final ClassNode node) {
checkClassForAbstractAndFinal(node);
checkClassForOtherModifiers(node);
}

private void checkClassForAbstractAndFinal(final ClassNode node) {
if (!node.isAbstract() || !isFinal(node.getModifiers())) return;
if (node.isInterface()) {
addError("The " + getDescription(node) + " must not be final. It is by definition abstract.", node);
} else {
addError("The " + getDescription(node) + " must not be both final and abstract.", node);
if (node.isAbstract() && isFinal(node.getModifiers())) {
addError("The " + getDescription(node) + " cannot be " + (node.isInterface() ? "final. It is by nature abstract" : "both abstract and final") + ".", node);
}
}

private void checkClassForOtherModifiers(final ClassNode node) {
checkClassForModifier(node, isTransient(node.getModifiers()), "transient");
checkClassForModifier(node, isVolatile(node.getModifiers()), "volatile");
checkClassForModifier(node, isNative(node.getModifiers()), "native");
List<String> modifiers = new ArrayList<>();

if (!(node instanceof InnerClassNode)) {
checkClassForModifier(node, isStatic(node.getModifiers()), "static");
checkClassForModifier(node, isPrivate(node.getModifiers()), "private");
if (isPrivate(node.getModifiers())) modifiers.add("private");
if (isStatic(node.getModifiers())) modifiers.add("static");
}
// don't check synchronized here as it overlaps with ACC_SUPER
}

private void checkMethodForModifier(final MethodNode node, final boolean condition, final String modifierName) {
if (!condition) return;
addError("The " + getDescription(node) + " has an incorrect modifier " + modifierName + ".", node);
}
// do not check for synchronized here; it overlaps with ACC_SUPER
if (isTransient(node.getModifiers())) modifiers.add("transient");
if (isVolatile(node.getModifiers())) modifiers.add("volatile");
if (isNative(node.getModifiers())) modifiers.add("native");

private void checkClassForModifier(final ClassNode node, final boolean condition, final String modifierName) {
if (!condition) return;
addError("The " + getDescription(node) + " has an incorrect modifier " + modifierName + ".", node);
for (String modifier : modifiers) {
addError("The " + getDescription(node) + " has invalid modifier " + modifier + ".", node);
}
}

private static String getDescription(final ClassNode node) {
Expand Down Expand Up @@ -409,16 +394,6 @@ private void checkMethodsForIncorrectName(final ClassNode cn) {
}
}

private void checkMethodsForIncorrectModifiers(final ClassNode cn) {
if (!cn.isInterface()) return;
for (MethodNode method : cn.getMethods()) {
if (method.isFinal()) {
addError("The " + getDescription(method) + " from " + getDescription(cn) +
" must not be final. It is by definition abstract.", method);
}
}
}

private void checkMethodsForWeakerAccess(final ClassNode cn) {
for (MethodNode method : cn.getMethods()) {
checkMethodForWeakerAccessPrivileges(method, cn);
Expand Down Expand Up @@ -483,7 +458,7 @@ public void visitMethod(final MethodNode node) {
checkAbstractDeclaration(node);
checkRepetitiveMethod(node);
checkOverloadingPrivateAndPublic(node);
checkMethodModifiers(node);
checkMethodForIncorrectModifiers(node);
checkGenericsUsage(node, node.getParameters());
checkGenericsUsage(node, node.getReturnType());
for (Parameter param : node.getParameters()) {
Expand All @@ -494,18 +469,27 @@ public void visitMethod(final MethodNode node) {
super.visitMethod(node);
}

private void checkMethodModifiers(final MethodNode node) {
// don't check volatile here as it overlaps with ACC_BRIDGE
// additional modifiers not allowed for interfaces
if ((this.currentClass.getModifiers() & ACC_INTERFACE) != 0) {
checkMethodForModifier(node, isStrict(node.getModifiers()), "strictfp");
checkMethodForModifier(node, isSynchronized(node.getModifiers()), "synchronized");
checkMethodForModifier(node, isNative(node.getModifiers()), "native");
private void checkMethodForIncorrectModifiers(final MethodNode node) {
if (node.isAbstract() && (node.isStatic() || (node.isFinal() && !currentClass.isInterface()))) { // GROOVY-11508
addError("The " + getDescription(node) + " can only be one of abstract, static, " + (currentClass.isInterface() ? "default" : "final") + ".", node);
}
// transient overlaps with varargs but we don't add varargs until AsmClassGenerator
// but we might have varargs set from e.g. @Delegate of a varargs method so skip generated

List<String> modifiers = new ArrayList<>();

if (currentClass.isInterface()) {
if (isFinal(node.getModifiers())) modifiers.add("final");
if (isNative(node.getModifiers())) modifiers.add("native");
if (isStrict(node.getModifiers())) modifiers.add("strictfp");
if (isSynchronized(node.getModifiers())) modifiers.add("synchronized");
}
// transient overlaps with varargs but we do not add varargs until AsmClassGenerator
// but we might have varargs set from @Delegate of varargs method, so skip generated
if (!AnnotatedNodeUtils.isGenerated(node)) {
checkMethodForModifier(node, isTransient(node.getModifiers()), "transient");
if (isTransient(node.getModifiers())) modifiers.add("transient");
}

for (String modifier : modifiers) {
addError("The " + getDescription(node) + " has invalid modifier " + modifier + ".", node);
}
}

Expand Down
Loading

0 comments on commit d557f27

Please sign in to comment.