"gradle precommit" no longer catches accidental package-private APIs? [LUCENE-9711]

[asfimport]

While backporting the new exciting deterministic multi-segment indexing tool, #10733, ant precommit failed, because the new DocumentSelector API was accidentally package private, missing its public modifier.

[Aside: I sometimes feel we should not put unit tests in the same package as the APIs they are testing.  We of course do this to make testing internal, package-private state, possible/easier.  But it then leads to API bugs like this, where we fail to make an API public when it should be.]

Anyways, luckily, the old crazy hacky Python javadoc linter in 8.x caught this issue, and I fixed it on backport, and will shortly fix it in mainline as well.  But gradle precommit on mainline failed to catch it, I think?

Is this a known regression in our gradle migration?  Do we have plans to somehow recover it?  It clearly sometimes catches important API bugs!  And this is not the first time it's caught such bugs...

---

Migrated from LUCENE-9711 by Michael McCandless (@mikemccand), updated Jan 31 2021 Attachments: look_ma_no_link.png

[asfimport]

Dawid Weiss (@dweiss) (migrated from JIRA)

I think it's the fact the python linter has been replaced by javadoc doclet, not gradle build itself.

[asfimport]

Robert Muir (@rmuir) (migrated from JIRA)

That isn't the case. I reverted locally the public on the interface, ran precommit with ancient java 8, and it fails in branch_8x because of checkJavaDocs.py:

     [exec] Verify...
     [exec] 
     [exec] file:///build/docs/misc/org/apache/lucene/misc/index/BinaryDocValueSelector.html
     [exec]   BROKEN LINK: file:///build/docs/core/org/apache/lucene/misc.index.IndexRearranger.DocumentSelector.html
     [exec]   BROKEN LINK: file:///build/docs/core/org/apache/lucene/misc.index.IndexRearranger.DocumentSelector.html
     [exec] 
     [exec] file:///build/docs/misc/org/apache/lucene/misc/index/IndexRearranger.html
     [exec]   BROKEN LINK: file:///build/docs/core/org/apache/lucene/misc.index.IndexRearranger.DocumentSelector.html
     [exec]   BROKEN LINK: file:///build/docs/core/org/apache/lucene/misc.index.IndexRearranger.DocumentSelector.html
     [exec]   BROKEN LINK: file:///build/docs/core/org/apache/lucene/misc.index.IndexRearranger.DocumentSelector.html
     [exec]   BROKEN LINK: file:///build/docs/core/org/apache/lucene/misc.index.IndexRearranger.DocumentSelector.html

We still have this same broken-links checker in master branch, too. I ran it: ./gradlew checkBrokenLinks and it passes.

It is just a side-effect of the newer java version used. In the newer version of java it no longer makes a broken link. There is just no link at all. So we can't rely on javadoc making a broken link if we screw up the APIs, ideally we add some explicit detection to the doclet to fail because a "link had to be greyed-out" ?

[asfimport]

Robert Muir (@rmuir) (migrated from JIRA)

I looked into this enough to know that I think we don't want to mess with it via our doclet stuff.

We want to detect in this example a public method with a return type that has package-private generics. The class file still knows this, so that the compiler can do what it needs to do. Seems like implementing a check for this kind of API screw up would be better implemented with ASM?

[asfimport]

Robert Muir (@rmuir) (migrated from JIRA)

I did try checkboxing the eclipse javadocs linter's "report non visible references" just for kicks, but that seems to be about something else. Its another avenue to explore, maybe ecj has some check that would trip here.

[asfimport]

Michael McCandless (@mikemccand) (migrated from JIRA)

So we can't rely on javadoc making a broken link if we screw up the APIs, ideally we add some explicit detection to the doclet to fail because a "link had to be greyed-out" ?

Ahh, OK, so we were relying on buggy behavior of older JDKs to help us catch these broken APIs.

It's crazy to me that javac does not already support this kind of static checking!

Thanks for digging @rmuir!

[asfimport]

Dawid Weiss (@dweiss) (migrated from JIRA)

Thanks for locating the problem, Robert.

I think the scope of all elements is available for doclets via the type tree.... but it feels like rewriting of what javadoc is already doing, eh.