On Mon, 4 Apr 2022 23:46:57 GMT, Jonathan Gibbons <j...@openjdk.org> wrote:
> This is a redo of a recent fix, which passed all tier1 tests but broke "make
> docs".
>
> The reason for the crash was a combination of a number of factors, such that
> fixing any one (or more) would make the problem go away, making it hard(er)
> to determine the correct path forward.
>
> 1. In a doc comment in JDK code, the author had mistakenly written a number
> of tags like `{@link byte}` instead of `{@code byte}` and `{@link byte[]}`
> instead of `{@code byte[]}`
>
> 2. `DocTrees.getElement` returns the javac-internal symbol for primitive
> types (instead of `null`) and the base type for arrays. So for both `{@link
> byte}` and `{@link byte[]}` the `LinkFactory` was given a javac-internal
> symbol (instead of null). Filed as JDK-8284315.
>
> 3. Thinking the value was a declared type, and so in a named or unnamed
> package, the `LinkFactory` attempts to create a link based on the package of
> the value. The package for the java-internal symbols for primitive type is
> a special `rootPackage` which has no equivalent in the Language Model world,
> and which has a `null` owner.
>
> 4. The new code in `JavacElements.getModuleOf` assumes that (as is generally
> standard in javac) all symbols have a non-`null` owner, this giving an NPE
> when called with `rootPackage`.
>
> So, there are various possible ways to fix this.
>
> 1. put a broad null check for `sym.owner` in `JavacElements.getModuleOf`
> 2. put a specific check for `rootPackage` in `JavacElements.getMOduleOf`
> 3. fix `DocTrees.getElement` to not return javac-internal symbols
> 4. change javadoc to workaround the issue with `DocTrees.getElement`
> 5. fix the javadoc comments to not use bad forms, like `{@link byte}` etc.
>
> 1 and 2 would both be atypical idioms in the code.
>
> 3 is desirable, but may cause additional work/updates/etc. It would be a
> change to the behavior of a public API, and so compatibility concerns would
> need to be addressed, although arguably the change in behavior is a bug fix,
> not a spec change. (The current behavior is under-specified!)
>
> By itself, 5 would just be ignoring the underlying problem.
>
> That leaves 4, which is the solution adopted here. It localizes the
> change/check to javadoc internal code. It is an interim measure until we can
> fix 3. (JDK-8284315)
>
> * The javac part of the fix is essentially the same as before (a redundant
> import has been removed, this time.) By itself the code was not wrong, when
> used as intended.
>
> * `DocTrees.getElement` is called from a single place in javadoc, in
> `CommentHelper` and so a check is made on the result such that `null` is
> returned when the result would otherwise not be a declared type.
>
> * A new (javadoc) test is added to verify the new behavior for `{@link byte}`
> etc. The prior behavior (even before the original fix) was to display the
> type in monospace font. The new behavior is for javadoc to fall back on the
> recently-new mechanism to display a "invalid tag" block.
>
> With these fixes, "make docs" works, and does not crash(!) Comparing the
> result of "make docs" before and after this fix, the only significant
> differences arise from the 5 bad `{@link ...}` tags in `MemorySegment.java`.
> These should (separately) be fixed.
Looks reasonable.
-------------
Marked as reviewed by jlahoda (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/8101
