Upstreaming: Not inheriting annotations, somehow avoiding stub-file recursion

[cpovirk]

f3e9f21

Note that that links to jspecify/jspecify-reference-checker@2bab27e, which describes another problem solved by that commit. It doesn't seem that "not inheriting annotations" is necessarily a sufficient fix for that problem in general, but it works here. More to the point, if we someday restore inheriting annotations, we'll still need a way to avoid the recursion. (The linked commit does discuss some other options.)

[cpovirk]

And I forgot to link 719b6f0, which came right after f3e9f21. Note that, after a rename upstream, that code is now in AnnotationFileElementTypes.

[cpovirk]

Recursion (deep, though I don't think actual infinite) contributed to complicating my investigation of jspecify/jdk@fdf7954.

[cpovirk]

I think netdpb may have noted at one point that there was some particular API that all checkers look up from their constructors, and that may be the cause of some infinite recursion. But I'm not sure what the API is, and I haven't looked into how easy it would be to avoid (though I'm hoping "not hard"), and I don't know if it's the main problem here (notably, I still don't know if the recursion is actually infinite or just keep) or if the problem would just resurface whenever we actually have to parse the stub files later.

We're hoping that we can sidestep this by reducing our need for reading stub files, but we'll see how that goes.

[cpovirk]

It's also possible that typetools PR 5581 would be of help here. Again, though, we hope to move away from stub files enough that we no longer have to worry about this.

[cpovirk]

• I was seeing more recursion (and later NPE) from stub-file parsing while looking at internal bug 202004399.
• eisop redid some of the code in this area. Maybe it will help us.

[wmdietl]

eisop/checker-framework#305 and eisop/checker-framework#269 fixed some stub file problems.
Do you have test cases that will allow us to reproduce this issue?

Regarding inheriting decl annos: I see no problem making the methods you disable in xxx protected to allow changing the behavior in a checker.
Do you remember whether there is a test for this issue?

[cpovirk]

(dodging the questions about test cases until later in this post...)

The first bit of good news is that we turned off the use of stubs entirely (JDK and otherwise) for our integration of the JSpecify checker inside Google. So the use of stubs isn't necessary for that particular use case, which I think is where many of the problems were discovered.

It's also possible that some of the problems come up only in code that makes "significant use" of JDK classes. If so, the problems might not come up in the existing samples directory (especially the copy in the main branch) or the new conformance tests that are being filled in. Or, if they do come up, we might be able to make them go away without significant side effects by simply disabling the JDK stubs for our test runs.

Now, if an average person wants to try the reference checker, then that person is likely to want the JDK stubs. So we might not be able to just give up on stubs entirely. But... maybe we could for now? In the longer term, maybe we'll find a way to let external users put annotations into their JDK APIs more directly. And today, it's not as if we recommend using the reference checker for more than experimentation. Granted, it would be nice if experimentation with JDK APIs were to work as expected....

I guess that my suggestion would be to back out all these changes and see what happens with the existing tests. That's probably enough testing: If they work, we're probably fine. Or at least that's enough to put the ball back in my court to test things inside Google.

As for a test for inheriting annotations: I doubt there's a test specifically for that: I don't think we recognize any of the inherited annotations, especially because we treat everything as @Pure. So I think my real motivation for disabling them was the second problem listed in f3e9f21. The part about "but we don't actually want inheritance" was probably more of a reason to feel OK about making the change to fix the "real" problem.