Recursive supertype and superinterface validation

[Leland-Takamine]

Fixes #660

This change adds recursive validation for supertypes and superinterfaces to SuperficialValidation.

[googlebot]

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

---

What to do if you already signed the CLA

Individual signers

• It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

• Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
• The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
• The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

[Leland-Takamine]

I signed it!

[googlebot]

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

[ronshapiro]

I'll try to run this through google-wide presubmit to check if anything breaks.

[ronshapiro]

If you want, e -> validateElement(e) is a little shorter than the method reference

[Leland-Takamine]

Would prefer to leave as is - e conflicts with the existing method parameter and the IDE produces a warning with the lambda.

[ronshapiro]

Unfortunately this is introducing some cycles. Here's a repro:

@Component
class Foo extends java.lang.ref.Reference {}

Looks like there's a cycle between java.lang.ref.Reference and java.lang.ref.ReferenceQueue.

I'm going to think more, but I think we may want to use something like com.google.common.graph.Traverser instead so that we can ensure we never infinitely recur.

When I ran this through google's global tests, the test framework failed it's smoke tests with a 50% failure rate. Seems like enough things refer to java.lang.Thread that in turn refers to Reference. So it wouldn't even run more than a few hundred tests

[Leland-Takamine]

@ronshapiro Would it be sufficient to just keep track of visited Elements and skip those we've already seen?

[ronshapiro]

We could try that. I think passing those around may be icky, so that's why I suggested Traverser, which can handle that for us. It abstracts "given this isntance, these are the other things that need to be checked" from "check this individual thing".

[Leland-Takamine]

Wouldn't we need to handle cycles manually when building to graph in order to leverage Traverser?

[Leland-Takamine]

Wouldn't we need to handle cycles manually when building to graph in order to leverage Traverser?

Ah never mind - I see now that Traverser.forGraph accepts a function not a graph.

[Leland-Takamine]

Updated to handle cycles by keeping track of visited Elements at the instance level. This requires making all internal logic non-static. Public static API is unchanged.

[Leland-Takamine]

@ronshapiro Mind taking another look?

[ronshapiro]

I'll send this through our tests again and report back

[ronshapiro]

I'm seeing a few cases of this come up now:

java.lang.IllegalArgumentException: <nulltype> cannot be represented as a Class<?>.

From within

SuperficialValidation$3.defaultAction

[Leland-Takamine]

@ronshapiro Are you able to track down which tests are failing? I'd like to figure out a repro case so I can write a test for that.

[ronshapiro]

I haven't had time yet to investigate

…

On Thu, May 23, 2019, 4:51 AM Leland Takamine ***@***.***> wrote: @ronshapiro <https://github.com/ronshapiro> Are you able to track down which tests are failing? I'd like to figure out a repro case so I can write a test for that. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#733?email_source=notifications&email_token=AAGBRXLHA5C26RDLQXPH6RTPWZLKLA5CNFSM4HM5QQFKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWBQYPQ#issuecomment-495127614>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAGBRXMXHSFODZPBXGQ3VA3PWZLKLANCNFSM4HM5QQFA> .

[Leland-Takamine]

@ronshapiro I've taken a look at the code but I'm still unclear on how we'd be visiting a null type. It would be really helpful to see the failing tests to help debug.

[Leland-Takamine]

Ping on this :)