diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTMethodDeclaration.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTMethodDeclaration.java index 6f7909bcea..b50255ca51 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTMethodDeclaration.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTMethodDeclaration.java @@ -82,11 +82,13 @@ public class ASTMethodDeclaration extends AbstractMethodOrConstructorDeclaration * Returns true if this method is abstract, so doesn't * declare a body. Interface members are * implicitly abstract, whether they declare the - * {@code abstract} modifier or not. + * {@code abstract} modifier or not. Default interface + * methods are not abstract though, consistently with the + * standard reflection API. */ @Override public boolean isAbstract() { - return isInterfaceMember() || super.isAbstract(); + return isInterfaceMember() && !isDefault() || super.isAbstract(); } diff --git a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/ast/ASTMethodDeclarationTest.java b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/ast/ASTMethodDeclarationTest.java deleted file mode 100644 index 516f7739b7..0000000000 --- a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/ast/ASTMethodDeclarationTest.java +++ /dev/null @@ -1,44 +0,0 @@ -/** - * BSD-style license; for more info see http://pmd.sourceforge.net/license.html - */ - -package net.sourceforge.pmd.lang.java.ast; - -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertTrue; - -import org.junit.Test; - -import net.sourceforge.pmd.lang.java.ParserTstUtil; - -public class ASTMethodDeclarationTest { - - @Test - public void testGetVariableName() { - int id = 0; - - ASTMethodDeclaration md = new ASTMethodDeclaration(id++); - ASTMethodDeclarator de = new ASTMethodDeclarator(id++); - de.setImage("foo"); - md.jjtAddChild(de, 0); - - assertEquals("foo", md.getMethodName()); - } - - @Test - public void testPrivateInterfaceMethods() { - ASTCompilationUnit node = ParserTstUtil.parseJava9("public interface Foo { private void bar() { } }"); - ASTMethodDeclaration methodDecl = node.getFirstDescendantOfType(ASTMethodDeclaration.class); - assertTrue(methodDecl.isPrivate()); - assertFalse(methodDecl.isPublic()); - } - - @Test - public void testPublicInterfaceMethods() { - ASTCompilationUnit node = ParserTstUtil.parseJava9("public interface Foo { void bar(); }"); - ASTMethodDeclaration methodDecl = node.getFirstDescendantOfType(ASTMethodDeclaration.class); - assertFalse(methodDecl.isPrivate()); - assertTrue(methodDecl.isPublic()); - } -} diff --git a/pmd-java/src/test/kotlin/net/sourceforge/pmd/lang/java/ast/ASTMethodDeclarationTest.kt b/pmd-java/src/test/kotlin/net/sourceforge/pmd/lang/java/ast/ASTMethodDeclarationTest.kt new file mode 100644 index 0000000000..0ef2d267f9 --- /dev/null +++ b/pmd-java/src/test/kotlin/net/sourceforge/pmd/lang/java/ast/ASTMethodDeclarationTest.kt @@ -0,0 +1,86 @@ +package net.sourceforge.pmd.lang.java.ast + +import io.kotlintest.should +import io.kotlintest.specs.FunSpec +import net.sourceforge.pmd.lang.ast.test.shouldBe +import net.sourceforge.pmd.lang.java.ast.JavaVersion.Companion.Earliest +import net.sourceforge.pmd.lang.java.ast.JavaVersion.Companion.Latest +import net.sourceforge.pmd.lang.java.ast.JavaVersion.J1_8 +import net.sourceforge.pmd.lang.java.ast.JavaVersion.J9 + +class ASTMethodDeclarationTest : FunSpec({ + + // notes about dsl: + // * testGroup generates one test per "should" assertion that + // uses a node matcher, which is nice to know which one failed + // (without explicitly giving them each a specific name) + // * the it::isPublic syntax allows including the property name in the error message in case of failure + + testGroup("Non-private interfaces members should be public", javaVersions = Earliest..Latest) { + + genClassHeader = "interface Bar" + + "int foo();" should matchDeclaration(ignoreChildren = true) { + it::isPublic shouldBe true + it::isPrivate shouldBe false + it::isSyntacticallyPublic shouldBe false + } + + "public int kk();" should matchDeclaration(ignoreChildren = true) { + it::isPublic shouldBe true + it::isPrivate shouldBe false + it::isSyntacticallyPublic shouldBe true + } + + "int FOO = 0;" should matchDeclaration(ignoreChildren = true) { + it::isPublic shouldBe true + it::isPrivate shouldBe false + it::isSyntacticallyPublic shouldBe false + } + + "public int FOO = 0;" should matchDeclaration(ignoreChildren = true) { + it::isPublic shouldBe true + it::isPrivate shouldBe false + it::isSyntacticallyPublic shouldBe true + } + } + + parserTest("Private methods in interface should be private", J9..Latest) { + + "private int de() { return 1; }" should matchDeclaration(ignoreChildren = true) { + it::isPublic shouldBe false + it::isPrivate shouldBe true + it::isSyntacticallyPublic shouldBe false + } + + } + + testGroup("Non-default methods in interfaces should be abstract", javaVersions = J1_8..Latest) { + + genClassHeader = "interface Bar" + + "int bar();" should matchDeclaration(ignoreChildren = true) { + it::isDefault shouldBe false + it::isAbstract shouldBe true + } + + "abstract int bar();" should matchDeclaration(ignoreChildren = true) { + it::isDefault shouldBe false + it::isAbstract shouldBe true + } + + } + + parserTest("Default methods in interfaces should not be abstract", javaVersions = J1_8..Latest) { + + genClassHeader = "interface Bar" + + "default int kar() { return 1; } " should matchDeclaration(ignoreChildren = true) { + it::isDefault shouldBe true + it::isAbstract shouldBe false + } + + // default abstract is an invalid combination of modifiers so we won't encounter it in real analysis + } + +}) diff --git a/pmd-java/src/test/kotlin/net/sourceforge/pmd/lang/java/ast/KotlinTestingDsl.kt b/pmd-java/src/test/kotlin/net/sourceforge/pmd/lang/java/ast/KotlinTestingDsl.kt index a955c584e8..4ba66f6d5f 100644 --- a/pmd-java/src/test/kotlin/net/sourceforge/pmd/lang/java/ast/KotlinTestingDsl.kt +++ b/pmd-java/src/test/kotlin/net/sourceforge/pmd/lang/java/ast/KotlinTestingDsl.kt @@ -10,7 +10,6 @@ import net.sourceforge.pmd.lang.ast.test.* import net.sourceforge.pmd.lang.java.ParserTstUtil import io.kotlintest.should as kotlintestShould - /** * Represents the different Java language versions. */ @@ -129,7 +128,7 @@ fun AbstractFunSpec.testGroup(name: String, class GroupTestCtx(private val funspec: AbstractFunSpec, private val groupName: String, javaVersion: JavaVersion) : ParserTestCtx(javaVersion) { infix fun String.should(matcher: Matcher) { - funspec.parserTest("$groupName: '$this'") { + funspec.parserTest("$groupName: '$this'", javaVersion = javaVersion) { this@should kotlintestShould matcher } } @@ -173,10 +172,12 @@ class GroupTestCtx(private val funspec: AbstractFunSpec, private val groupName: * @property javaVersion The java version that will be used for parsing. * @property importedTypes Types to import at the beginning of parsing contexts * @property otherImports Other imports, without the `import` and semicolon + * @property genClassHeader Header of the enclosing class used in parsing contexts like parseExpression, etc. E.g. "class Foo" */ open class ParserTestCtx(val javaVersion: JavaVersion = JavaVersion.Latest, val importedTypes: MutableList> = mutableListOf(), - val otherImports: MutableList = mutableListOf()) { + val otherImports: MutableList = mutableListOf(), + var genClassHeader: String = "class Foo") { /** Imports to add to the top of the parsing contexts. */ internal val imports: List @@ -218,6 +219,25 @@ open class ParserTestCtx(val javaVersion: JavaVersion = JavaVersion.Latest, noinline nodeSpec: NWrapper.() -> Unit) = makeMatcher(TypeParsingCtx(this), ignoreChildren, nodeSpec) + /** + * Returns a String matcher that parses the node using [parseToplevelDeclaration] with + * type param [N], then matches it against the [nodeSpec] using [matchNode]. + */ + inline fun matchToplevelType( + ignoreChildren: Boolean = false, + noinline nodeSpec: NWrapper.() -> Unit) = + makeMatcher(TopLevelTypeDeclarationParsingCtx(this), ignoreChildren, nodeSpec) + + /** + * Returns a String matcher that parses the node using [parseBodyDeclaration] with + * type param [N], then matches it against the [nodeSpec] using [matchNode]. + * + * Note that the enclosing type declaration can be customized by changing [genClassHeader]. + */ + inline fun matchDeclaration( + ignoreChildren: Boolean = false, + noinline nodeSpec: NWrapper.() -> Unit) = makeMatcher(EnclosedDeclarationParsingCtx(this), ignoreChildren, nodeSpec) + /** * Expect a parse exception to be thrown by [block]. @@ -239,6 +259,11 @@ open class ParserTestCtx(val javaVersion: JavaVersion = JavaVersion.Latest, fun parseAstType(type: String): ASTType = TypeParsingCtx(this).parseNode(type) + fun parseToplevelAnyTypeDeclaration(type: String): ASTAnyTypeDeclaration = TopLevelTypeDeclarationParsingCtx(this).parseNode(type) + + fun parseBodyDeclaration(type: String): ASTAnyTypeBodyDeclaration = EnclosedDeclarationParsingCtx(this).parseNode(type) + + // reified shorthands, fetching the node inline fun parseExpression(expr: String): N = ExpressionParsingCtx(this).parseAndFind(expr) @@ -247,6 +272,9 @@ open class ParserTestCtx(val javaVersion: JavaVersion = JavaVersion.Latest, inline fun parseType(type: String): N = TypeParsingCtx(this).parseAndFind(type) + inline fun parseToplevelDeclaration(decl: String): N = TopLevelTypeDeclarationParsingCtx(this).parseAndFind(decl) + + inline fun parseDeclaration(decl: String): N = EnclosedDeclarationParsingCtx(this).parseAndFind(decl) companion object { @@ -261,11 +289,7 @@ open class ParserTestCtx(val javaVersion: JavaVersion = JavaVersion.Latest, */ fun Node.findFirstNodeOnStraightLine(klass: Class): N? { return when { - klass.isInstance(this) -> { - @Suppress("UNCHECKED_CAST") - val n = this as N - n - } + klass.isInstance(this) -> klass.cast(this) this.numChildren == 1 -> getChild(0).findFirstNodeOnStraightLine(klass) else -> null } @@ -326,7 +350,7 @@ open class ParserTestCtx(val javaVersion: JavaVersion = JavaVersion.Latest, override fun getTemplate(construct: String): String = """ ${ctx.imports.joinToString(separator = "\n")} - class Foo { + ${ctx.genClassHeader} { { Object o = $construct; } @@ -342,7 +366,7 @@ open class ParserTestCtx(val javaVersion: JavaVersion = JavaVersion.Latest, override fun getTemplate(construct: String): String = """ ${ctx.imports.joinToString(separator = "\n")} - class Foo { + ${ctx.genClassHeader} { { $construct } @@ -353,11 +377,34 @@ open class ParserTestCtx(val javaVersion: JavaVersion = JavaVersion.Latest, override fun retrieveNode(acu: ASTCompilationUnit): ASTBlockStatement = acu.getFirstDescendantOfType(ASTBlockStatement::class.java) } + class EnclosedDeclarationParsingCtx(ctx: ParserTestCtx) : NodeParsingCtx("enclosed declaration", ctx) { + + override fun getTemplate(construct: String): String = """ + ${ctx.imports.joinToString(separator = "\n")} + ${ctx.genClassHeader} { + $construct + } + """.trimIndent() + + override fun retrieveNode(acu: ASTCompilationUnit): ASTAnyTypeBodyDeclaration = + acu.getFirstDescendantOfType(ASTAnyTypeBodyDeclaration::class.java)!! + } + + class TopLevelTypeDeclarationParsingCtx(ctx: ParserTestCtx) : NodeParsingCtx("top-level declaration", ctx) { + + override fun getTemplate(construct: String): String = """ + ${ctx.imports.joinToString(separator = "\n")} + $construct + """.trimIndent() + + override fun retrieveNode(acu: ASTCompilationUnit): ASTAnyTypeDeclaration = acu.getFirstDescendantOfType(ASTAnyTypeDeclaration::class.java)!! + } + class TypeParsingCtx(ctx: ParserTestCtx) : NodeParsingCtx("type", ctx) { override fun getTemplate(construct: String): String = """ ${ctx.imports.joinToString(separator = "\n")} - class Foo { + ${ctx.genClassHeader} { $construct foo; } """.trimIndent() diff --git a/pmd-lang-test/src/main/kotlin/net/sourceforge/pmd/lang/ast/test/TestUtils.kt b/pmd-lang-test/src/main/kotlin/net/sourceforge/pmd/lang/ast/test/TestUtils.kt new file mode 100644 index 0000000000..a919082ecc --- /dev/null +++ b/pmd-lang-test/src/main/kotlin/net/sourceforge/pmd/lang/ast/test/TestUtils.kt @@ -0,0 +1,50 @@ +package net.sourceforge.pmd.lang.ast.test + +import io.kotlintest.Matcher +import kotlin.reflect.KCallable +import io.kotlintest.shouldBe as ktShouldBe + +/** + * Extension to add the name of a property to error messages. + * + * @see [shouldBe]. + */ +infix fun KCallable.shouldEqual(expected: V?) = + assertWrapper(this, expected) { n, v -> n ktShouldBe v } + +private fun assertWrapper(callable: KCallable, right: V, asserter: (N, V) -> Unit) { + + fun formatName() = "::" + callable.name.removePrefix("get").decapitalize() + + val value: N = try { + callable.call() + } catch (e: Exception) { + throw RuntimeException("Couldn't fetch value for property ${formatName()}", e) + } + + try { + asserter(value, right) + } catch (e: AssertionError) { + + if (e.message?.contains("expected:") == true) { + // the exception has no path, let's add one + throw AssertionError(e.message!!.replace("expected:", "expected property ${formatName()} to be")) + } + + throw e + } +} + +/** + * Extension to add the name of the property to error messages. + * Use with double colon syntax, eg `it::isIntegerLiteral shouldBe true`. + * For properties synthesized from Java getters starting with "get", you + * have to use the name of the getter instead of that of the generated + * property (with the get prefix). + * + * If this conflicts with [io.kotlintest.shouldBe], use the equivalent [shouldEqual] + * + */ +infix fun KCallable.shouldBe(expected: V?) = this.shouldEqual(expected) + +infix fun KCallable.shouldBe(expected: Matcher) = assertWrapper(this, expected) { n, v -> n ktShouldBe v }