[java] Rewrite symbol table by oowekyala · Pull Request #2601 · pmd/pmd

oowekyala · 2020-06-18T15:01:00Z

Describe the PR

Rewrites the newer symbol table for maintainability and performance.
This extracts the following abstractions from the current symbol table implementation:

NameResolver: this is a strategy that can map a name to a symbol, eg get it from a map, or ask a classloader. These were implemented ad-hoc in each symbol table implementation, now they're factorized into reusable units
ShadowChain: this is the API that symbol tables offered (resolve a name), but broken down, one chain for each kind of name. This is a linked list of NameResolvers, each node shadows the next one. Symbol tables previously were a single linked list for all kind of names, so this was a possible performance bottleneck

This also unifies the API to resolve method, type and variable names (previously resolveMethod returned a List, the others returned a ResolveResult). Now all may return a List. This handles name ambiguity and duplicate declarations for variables and type names, so it is more general than the previous mechanism. It also reuses more code.

In this new framework, JSymbolTable is retained as just a simple tuple of ShadowChains, for ease of use (but it's not a central concept anymore).

This also adds a "MostlySingularMultimap" type, a kind of multimap that is optimised for the case where there are actually only at most one value per key. So the implementation is optimised for the most common case.

Possibly, the ShadowChain API could be lifted up to pmd-core. I'm waiting for a use case before doing this, but I implemented it as if it was in another module, so it has no dependencies on pmd-java

That was the "rewrite" part. The PR also adds support for inherited member resolution and a lot of tests to cover corner cases (hiding of inherited declarations).

This is the last piece needed before we proceed to the type resolution revamp.

Ready?

Added unit tests for fixed bug/feature
Passing all unit tests
Complete build ./mvnw clean verify passes (checked automatically by travis)
Added (in-code) documentation (if needed)

TODO implement

ghost · 2020-06-18T15:14:22Z

	1 Message
📖	No java rules are changed!

Generated by 🚫 Danger

adangel

Sorry that it took so long, will merge soon...

adangel · 2020-07-19T16:24:51Z

+package net.sourceforge.pmd.util;
+
+/** Represents a boolean that may not be present. Use as a non-null type. */
+public enum OptionalBool {


Should this be public? It sounds like an implementation helper....

Hm... seems to be used in the shadow chain API. Any reason, why you didn't use Optional<Boolean> instead?

Hm... seems to be used in the shadow chain API.

It's also used in some parts of the upcoming typeres API too. It's a common pattern I think, one that is sometimes replaced with a nullable boxed Boolean, which I think is... meh

Any reason, why you didn't use Optional instead?

All three values are known and there's no need to create a new optional every time

adangel · 2020-07-19T16:30:55Z

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


the package name "coreimpl" sounds a bit unusual. That's the part, you made independent of pmd-java and could potentially be moved to core, right?
Maybe we should rename the sub-package with something like "shadow" or "nameshadow" or so... or maybe something with "hierarchy" in it?

That's the part, you made independent of pmd-java and could potentially be moved to core, right?

Indeed, which is why the packaging is a bit awkward... Maybe chain would be a better name?

ShadowChain is a bit awkward, but I wanted to avoid the name clash with the current Scope classes we have. I think in a world where Scope doesn't exist, ShadowChain could be renamed to ScopeChain, and placed eg in the package nspmd.symbols.scopes?

adangel · 2020-07-19T18:21:23Z

@oowekyala I've merged (and pushed) pmd/7.0.x into java-grammar. There are the following failing tests:

[ERROR] Failures: 
[ERROR]   Not a child class of the current context (ReferenceCtx{packageName='com.foo', enclosingClass=ast:class(com.foo.Foo)}): ast:class(com.foo.Foo$7$AnonMember)
[ERROR]   Not a child class of the current context (ReferenceCtx{packageName='com.foo', enclosingClass=ast:class(com.foo.Foo)}): ast:class(com.foo.Foo$7$AnonMember)
[ERROR]   Not a child class of the current context (ReferenceCtx{packageName='com.foo', enclosingClass=ast:class(com.foo.Foo)}): ast:class(com.foo.Foo$1$1Locc)
[ERROR]   Not a child class of the current context (ReferenceCtx{packageName='com.foo', enclosingClass=ast:class(com.foo.Foo)}): ast:class(com.foo.Foo$1$1Locc)
[ERROR]   JavaQualifiedNameTest.testLocalInAnonymousClass:231->getNodes:27 Not a child class of the current context (ReferenceCtx{packageName='', enclosingClass=ast:class(Bzaz)}): ast:class(Bzaz$1$1FooRunnable)

Are these related to the symbol table? I've run out of time for today....

oowekyala · 2020-07-19T18:53:48Z

Thanks for doing this! They're not directly related, they happen because JavaVisitorBase doesn't delegate visit(ASTAnonymousClass) to visit(ASTAnyTypeDeclaration). I'll push a fix

oowekyala added 30 commits June 18, 2020 16:53

Make PMultimap type

0b7b397

Make shadowGroup

480ee17

Make a few utilities to replace import tables

4de86bd

Replace other tables

3d28990

Cleanup

eaecefe

Compat with old impl

1db3537

Fix rebase

691fced

Checkout newer sym table from typeres branch

c32dba7

Port

672718c

Checkout tests

fcb2bf6

Port tests, remove old impl

0cd4469

Extract augment function

41b0713

Move classes back into internal package

536eab3

Abstract stuff away for pmd-core

57ab1e4

Optimise singular map case even more

c4ffa79

Fix nextShadowGroup WIP

849d6dd

Make ShadowIterator to replace getNextGroup

5606d38

Use chain iterator

7bf9554

Add tests

bac76fc

Specify tests for ambiguity

46da94c

TODO implement

Finish impl

eb976a1

Add tests for supertype enumerator

5630372

Add tests for method shadowing in imports

841fd6b

Object methods should be in scope in interfaces

5134a33

Doc

1b48125

Change the name

ab9ec47

Doc

a5856e7

Split API

134c760

Remove recursion guard

e43eeea

Move ConsList into pmd-core

54c0b99

oowekyala added 10 commits June 18, 2020 16:53

Prune more empty nodes

4d75f64

Fix bug with shadow iterator

43a60c0

Move ScopeInfo into published package

a520d1d

Fix socping

36c9e97

Make disambiguation handle inherited types

2deef0b

Fix context accessibility

a7db782

Report ambiguity errors

a7051c4

PMD violation

268a3e4

Cleanup pcollections dependency

8f0b62f

doc

9054303

oowekyala added the in:symbol-table Affects the symbol table code label Jun 18, 2020

oowekyala added this to the 7.0.0 milestone Jun 18, 2020

oowekyala marked this pull request as ready for review June 18, 2020 16:12

oowekyala added 2 commits July 6, 2020 13:41

Merge branch 'java-grammar' into sym-optimize-symtables

ad6c976

Remove duplicate pcollection dep

c4c9ef3

adangel self-assigned this Jul 19, 2020

adangel reviewed Jul 19, 2020

View reviewed changes

adangel merged commit 6c3f8c4 into pmd:java-grammar Jul 19, 2020

oowekyala deleted the sym-optimize-symtables branch July 19, 2020 17:59

adangel mentioned this pull request Jan 23, 2023

PMD 7 Tracking Issue #3898

Closed

55 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[java] Rewrite symbol table#2601

[java] Rewrite symbol table#2601
adangel merged 42 commits into
pmd:java-grammarfrom
oowekyala:sym-optimize-symtables

oowekyala commented Jun 18, 2020

Uh oh!

ghost commented Jun 18, 2020

Uh oh!

adangel left a comment

Uh oh!

adangel Jul 19, 2020

Uh oh!

oowekyala Jul 19, 2020

Uh oh!

adangel Jul 19, 2020

Uh oh!

oowekyala Jul 19, 2020

Uh oh!

adangel commented Jul 19, 2020

Uh oh!

oowekyala commented Jul 19, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

oowekyala commented Jun 18, 2020

Describe the PR

Ready?

Uh oh!

ghost commented Jun 18, 2020

Uh oh!

adangel left a comment

Choose a reason for hiding this comment

Uh oh!

adangel Jul 19, 2020

Choose a reason for hiding this comment

Uh oh!

oowekyala Jul 19, 2020

Choose a reason for hiding this comment

Uh oh!

adangel Jul 19, 2020

Choose a reason for hiding this comment

Uh oh!

oowekyala Jul 19, 2020

Choose a reason for hiding this comment

Uh oh!

adangel commented Jul 19, 2020

Uh oh!

oowekyala commented Jul 19, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants