[java] Multiple improvements to JavaQualifiedName #895

oowekyala · 2018-01-31T23:43:51Z

Hi,

I thought that the commit history is clean enough to submit all that at once, but let me know if you want me to split the PR

Many refactorings were made in view of #864, and the PR adds support for anonymous class and lambda expression qnames

Commit b30d64d fixes #837

Remove getQualifiedName's copypasta.

The unnamed package is not represented as a lone full stop anymore. Rename QualifiedNameTest into JavaQualifiedNameTest

* Simplify the API of JavaQualifiedName * The name of an anonymous class is for now only available via JavaQualifiedName factories, not from the nodes

Use an immutable list to avoid array copies everywhere

Use possessive quantifiers to make it fail faster

Add MethodLike superclass to ASTMethodDecl, ASTConstructorDecl, ASTLambdaExpr

Qualified names now use structural comparison to determine equality. Using toString hid away some bugs. We still use toString to shortcut the comparison, since it's cached.

Doesn't support method overriden from generic supertype, anonymous enum constants (due to a bug in typeres. The fix is somewhere in my branches, after pmd#895)

Doesn't support method overriden from generic supertype. Known limitations that are supposed to be fixed shortly are pmd#910 and anonymous enum constants (the fix for that is somewhere in my branches, after pmd#895)

oowekyala · 2018-03-01T23:26:51Z

This is part of a bigger changeset, which aims to implement #864. The next PR in line is an implementation of the qualified name resolving visitor discussed in that issue, which significantly reduces the complexity of the qualified name resolution code, and simplifies some parts of ClassTypeResolver.

The changeset as a whole fixes a lot of type resolution bugs and clarifies its code. I think this PR should be scheduled for 6.2.0 to move forward.

jsotuyod · 2018-03-02T04:04:05Z

@oowekyala great! I'll schedule some time for this over my weekend. It's a large PR, but I think I can make it.

oowekyala · 2018-03-02T16:21:15Z

@jsotuyod Great, thanks :)

jsotuyod

This changeset introduces too many breaking API changes to the AST... as it stands, it can't be part of 6.2.0. I think we can work it out in 2 stages, where we incrementaly change things in 6.2.0 and then in 7.0.0 remove everything made deprecated / redundant, but it will take a little more effort.

Otherwise, we would have to wait for 7.0.0 to be able to see this productive...

jsotuyod · 2018-03-03T18:49:05Z

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTAnnotationTypeDeclaration.java

@@ -7,9 +7,8 @@

 import java.util.List;

-public class ASTAnnotationTypeDeclaration extends AbstractJavaAccessTypeNode implements ASTAnyTypeDeclaration {
+public class ASTAnnotationTypeDeclaration extends ASTAnyTypeDeclaration {


this is a breaking API change. Do we really need to do it in 6.2.0? Can't we just deprecate the methods?

Which methods?

jsotuyod · 2018-03-03T18:49:59Z

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTAnyTypeDeclaration.java

 *
 * @author Clément Fournier
 */
-public interface ASTAnyTypeDeclaration extends JavaQualifiableNode, AccessNode, JavaNode {
+public abstract class ASTAnyTypeDeclaration extends AbstractJavaAccessTypeNode implements JavaQualifiableNode, AccessNode, JavaNode {


the change from interface to abstract class is also a breaking API change

I had not thought about that. We could very much keep the abstract class and interface separate, which wouldn't be API breaking. We'd just insert an abstract class in the hierarchy.

jsotuyod · 2018-03-03T18:51:23Z

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTClassOrInterfaceDeclaration.java

@@ -10,13 +10,12 @@
 import net.sourceforge.pmd.lang.ast.Node;


-public class ASTClassOrInterfaceDeclaration extends AbstractJavaAccessTypeNode implements ASTAnyTypeDeclaration {
+public class ASTClassOrInterfaceDeclaration extends ASTAnyTypeDeclaration {


jsotuyod · 2018-03-03T18:51:31Z

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTConstructorDeclaration.java


-public class ASTConstructorDeclaration extends AbstractJavaAccessNode implements ASTMethodOrConstructorDeclaration {
+public class ASTConstructorDeclaration extends ASTMethodOrConstructorDeclaration {


jsotuyod · 2018-03-03T18:51:43Z

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTEnumDeclaration.java

@@ -7,9 +7,8 @@

 import java.util.List;

-public class ASTEnumDeclaration extends AbstractJavaAccessTypeNode implements ASTAnyTypeDeclaration {
+public class ASTEnumDeclaration extends ASTAnyTypeDeclaration {


jsotuyod · 2018-03-03T18:53:24Z

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ImmutableList.java

+ * @author Clément Fournier
+ * @since 6.1.0
+ */
+public interface ImmutableList<E> extends Iterable<E> {


why do we need our own inmutable list implementation? What's wrong with Collections.unmodifiableList?

Collections.unmodifiableList isn't persistent, and doesn't have a free prepend or append operation. I feel like an immutable list like what's found in functional languages is perfect for the problem, and especially for the visitor (next PR). It's good for clarity and conciseness of code, clarity of interface, and performance too. I implemented what I was missing.

If using this interface instead of List for the return type of a few methods doesn't bother you in itself, then the factories are private so the only dependable code we introduce is the interface, which is in itself very thin. Otherwise, well we could use util.List, but I think it would feel like a workaround...

I'm just very cautious about including (and maintaining!) ~600 LoCs for a basic data structure for which Java has several implementations and decorators.... Even if you want a smaller API just allowing prepends, does it earn us anything at all to manually implement the linked list over just using LinkedList?

This implementation has constant-time prepend, size, head and tail

LinkedList:

constant-time addFirst()

constant-time size()

constant-time getFirst()

constant-time getLast()

Most other operations are linear-time, but caching helps reduce the overhead of some common operations (reverse)

Equally applicable over a LinkedList

Allows structural sharing of the tail, which makes many operations constant- or zero-memory cost.

LinkedList:

subList() provides a "view" of the original list, effectively sharing whatever part of the list you want

Even if we keep the ImmutableList<E> interface as API, most of the current implementation is just a LinkedList when a thin wrapper over LinkedList suffices; so I'd rather just use it than reinvent the wheel

The interface of a LinkedList is very different from the interface of that list: void addFirst(E) vs ImmutableList<E> prepend(E). A LinkedList can add an element to a list in o(1), while an immutable list can create a new list with one more element in o(1) (time and space). We don't want to add elements fast, we want to create a new, bigger collection fast, and never add elements. This is something you only get with o(n) time and space in java, because you have to copy the old list each time

Besides subList cannot be used for proper structural sharing. You could use it for a tail operation but not for a prepend. From the subList doc:

The semantics of the list returned by this method become undefined if the backing list (i.e., this list) is structurally modified in any way other than via the returned list. (Structural modifications are those that change the size of this list, or otherwise perturb it in such a fashion that iterations in progress may yield incorrect results.)

I can't see a way to wrap a LinkedList to make it conform to that interface, without creating a full new LinkedList at each prepend operation, which is exactly what I was trying to avoid.

I understand the concern about not reimplementing the wheel because it's hard to maintain, but the Java standard lib lacks immutable and persistent collections, which is a shame. Alternatively, I found a light 3rd party lib that implements some basic ones, and is very compatible with the standard Collections framework. It's less than 20 classes, and I think it could be useful in other places -- some algorithms are better expressed with them. I'd much rather use that than keep the current full implementation -- i just didn't bother to search

I'd rather not add a dependency for a single class.

I'm still hesitant, specially given you seems to be using little of that behavior on this PR, but I'll give you benefit of the doubt to see what's in follow up PRs. I hope you can convince me, I'd hate to see us roll it back in 7.0.0

However, I still strongly believe this class should not be part of this package.

jsotuyod · 2018-03-03T18:53:42Z

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTMethodOrConstructorDeclaration.java

-                                                   JavaQualifiableNode,
-                                                   AccessNode,
-                                                   JavaNode {
+public abstract class ASTMethodOrConstructorDeclaration extends MethodLike implements SignedNode<ASTMethodOrConstructorDeclaration> {


jsotuyod · 2018-03-03T18:55:50Z

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/QualifiedNameFactory.java

+ * BSD-style license; for more info see http://pmd.sourceforge.net/license.html
+ */
+
+package net.sourceforge.pmd.lang.java.ast;


this class should probably not be part of this package

jsotuyod · 2018-03-03T18:56:19Z

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/MethodLike.java

+ * @author Clément Fournier
+ * @since 6.1.0
+ */
+public abstract class MethodLike extends AbstractJavaAccessNode implements JavaQualifiableNode, JavaNode {


why is this the only class on the AST that is not named a Node?

jsotuyod · 2018-03-03T18:56:32Z

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/JavaQualifiedName.java

@@ -4,379 +4,156 @@

 package net.sourceforge.pmd.lang.java.ast;


this class should probably not be part of this package

Moving it out would be API breaking

True, I've added it to #881. I'd avoid having the factory on this package non the less to avoid that breaking change in 7.0.0

The next PR moves ~80% of the factory out into the visitor. If you're ok with it, I'd rather move it out of the package then, otherwise I'd have some merge conflicts to resolve ^^

If the next PR doesn't make it into 6.2.0 we would be in the very difficult position of either having to roll this back before the release, or deprecate it in 6.3.0. Not the happiest path in either case... I'm trusting you on this one, but let's be careful.

I'll open the next PR asap

Removes a breaking API change

Remove one breaking API change

Spotted using japicmp

Checked with japicmp

oowekyala · 2018-03-10T18:29:09Z

@jsotuyod I used Japicmp to test binary compatibility with the latest released version. This version of the PR introduces only minute binary compatibility-breaking changes, namely, I didn't bridge the supports method of concrete operation metrics classes, like what I did in 2fd5d7c. E.g.:

***! MODIFIED CLASS: PUBLIC net.sourceforge.pmd.lang.java.metrics.impl.NpathMetric  (not serializable)
  ===  CLASS FILE FORMAT VERSION: 51.0 <- 51.0
  ---! REMOVED METHOD: PUBLIC(-) double computeFor(net.sourceforge.pmd.lang.java.ast.ASTMethodOrConstructorDeclaration, net.sourceforge.pmd.lang.metrics.MetricOptions)

Bridging preserves binary compatibility but compels clients to explicitly add a cast to the method argument to link to the recommended method instead of the deprecated one. Without bridging, a simple recompilation links correctly to the new API. Under those conditions, I'm not sure whether adding bridges like in 2fd5d7c is the best idea, but at least it's binary compatible now, except for those corner cases which I think should be rare enough.

jsotuyod · 2018-03-19T04:24:44Z

pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/design/CyclomaticComplexityRule.java

+                    ? "method"
+                    : node.getKind() == MethodLikeKind.CONSTRUCTOR
+                            ? "constructor"
+                            : "lambda";


sounds like a name for MethodLikeKind, something that should be part of the enum itself possibly

I think I can open another PR for this small changeset

jsotuyod · 2018-03-19T04:28:57Z

@oowekyala I'm merging this as is, so as to not block you anymore. Please, look into the remaining comments in upcoming PRs. Sorry for the delay, the changeset resulted to be a little less straightforward than anticipated.

oowekyala added 11 commits January 31, 2018 22:44

Refactor ASTAnyTypeDeclaration into an abstract class.

55d3bbe

Remove getQualifiedName's copypasta.

Make JavaQualifiedName represent packages like in the JLS

f7636f2

The unnamed package is not represented as a lone full stop anymore. Rename QualifiedNameTest into JavaQualifiedNameTest

Documentation

a3c0f7d

Add qualified name support for anonymous classes

ece8c7e

* Simplify the API of JavaQualifiedName * The name of an anonymous class is for now only available via JavaQualifiedName factories, not from the nodes

Formatting and code rearrangement

5fcfb0a

Cleanup JavaQualifiedName to make it really immutable

332dadc

Use an immutable list to avoid array copies everywhere

Optimise format regex

72a9010

Use possessive quantifiers to make it fail faster

Split JavaQualifiedName into static factory + class

1f6fc29

Add JavaQualifiedName support for lambdas.

ba10a60

Add MethodLike superclass to ASTMethodDecl, ASTConstructorDecl, ASTLambdaExpr

Adapt the metrics framework to consider MethodLike

7e2930b

Make cyclo count lambdas separately

b30d64d

oowekyala added the in:pmd-internals Affects PMD's internals label Feb 4, 2018

oowekyala added 5 commits February 7, 2018 23:14

Update JavaParserVisitorReducedAdapter to consider MethodLike

8d520b6

Fix two bugs with qname regex parsing

3ede676

Fix the bugfix

a039788

Qualified names now use structural comparison to determine equality. Using toString hid away some bugs. We still use toString to shortcut the comparison, since it's cached.

Merge branch 'master' into improve-javaqualifiedname

40a5e3c

Fix merge fail + checkstyle

1d9a2f1

oowekyala mentioned this pull request Feb 14, 2018

[java] Add rule to detect missing @Override annotations #912

Merged

oowekyala added this to the 6.2.0 milestone Mar 1, 2018

jsotuyod self-assigned this Mar 3, 2018

jsotuyod reviewed Mar 3, 2018

View reviewed changes

oowekyala added 3 commits March 4, 2018 00:36

Use abstract class + interface for MethodLike hierarchy

0ef1955

Removes a breaking API change

Rename MethodLike to MethodLikeNode

c9cfb1f

Use abstract class + interface for AnyTypeDeclaration hierarchy

ba8aeaf

Remove one breaking API change

oowekyala added 3 commits March 5, 2018 22:25

checkstyle

f60a64a

Remove breaking binary changes with metrics framework

2fd5d7c

Spotted using japicmp

Remove breaking API changes of JavaQualifiedName

9766e63

Checked with japicmp

oowekyala requested a review from jsotuyod March 10, 2018 18:29

oowekyala added 3 commits March 12, 2018 00:36

Merge branch 'master'

e643da2

Fix pmd warnings

92da9fa

Merge branch 'master', fix merge conflicts

c967e63

jsotuyod reviewed Mar 19, 2018

View reviewed changes

jsotuyod merged commit c967e63 into pmd:master Mar 19, 2018

jsotuyod added a commit that referenced this pull request Mar 19, 2018

Update changelog, refs #895

1301a38

oowekyala deleted the improve-javaqualifiedname branch March 19, 2018 13:34

oowekyala mentioned this pull request Mar 19, 2018

[java] Qualified name resolver #986

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[java] Multiple improvements to JavaQualifiedName #895

[java] Multiple improvements to JavaQualifiedName #895

oowekyala commented Jan 31, 2018 •

edited

Loading

oowekyala commented Mar 1, 2018

jsotuyod commented Mar 2, 2018

oowekyala commented Mar 2, 2018

jsotuyod left a comment

jsotuyod Mar 3, 2018

oowekyala Mar 3, 2018

jsotuyod Mar 3, 2018

oowekyala Mar 3, 2018 •

edited

Loading

jsotuyod Mar 3, 2018

jsotuyod Mar 3, 2018

jsotuyod Mar 3, 2018

jsotuyod Mar 3, 2018

oowekyala Mar 3, 2018

jsotuyod Mar 12, 2018

oowekyala Mar 12, 2018

jsotuyod Mar 19, 2018 •

edited

Loading

jsotuyod Mar 3, 2018

jsotuyod Mar 3, 2018

jsotuyod Mar 3, 2018

jsotuyod Mar 3, 2018

oowekyala Mar 10, 2018

jsotuyod Mar 12, 2018

oowekyala Mar 12, 2018

jsotuyod Mar 19, 2018

oowekyala Mar 19, 2018

oowekyala commented Mar 10, 2018

jsotuyod Mar 19, 2018

oowekyala Mar 19, 2018

jsotuyod commented Mar 19, 2018


		public class ASTConstructorDeclaration extends AbstractJavaAccessNode implements ASTMethodOrConstructorDeclaration {
		public class ASTConstructorDeclaration extends ASTMethodOrConstructorDeclaration {

		@@ -4,379 +4,156 @@

		package net.sourceforge.pmd.lang.java.ast;

[java] Multiple improvements to JavaQualifiedName #895

[java] Multiple improvements to JavaQualifiedName #895

Conversation

oowekyala commented Jan 31, 2018 • edited Loading

oowekyala commented Mar 1, 2018

jsotuyod commented Mar 2, 2018

oowekyala commented Mar 2, 2018

jsotuyod left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oowekyala Mar 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsotuyod Mar 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oowekyala commented Mar 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsotuyod commented Mar 19, 2018

oowekyala commented Jan 31, 2018 •

edited

Loading

oowekyala Mar 3, 2018 •

edited

Loading

jsotuyod Mar 19, 2018 •

edited

Loading