RFR: 8320234: Merge doclint.Env.AccessKind with tool.AccessKind

[Jonathan Gibbons]

On Mon, 20 Nov 2023 16:41:08 GMT, Jonathan Gibbons <jjg at openjdk.org> wrote:

>>> How much is the order actually relied on?
>> 
>> Clients of `doclint.Env.AccessKind` use `compareTo()`, which is effectively defined through `ordinal()`.
>> 
>>> A different solution, if one is really needed, would be a regression test to verify the expected order.
>> 
>> There are such tests already. For example, DocLintReferencesTest.
>> 
>>> Yet another alternate suggestion would be to use something like
>>> 
>>> ```
>>> assert List.of(AccessLevel.values())
>>>     .equals(List.of(PRIVATE, PACKAGE, PROTECTED, PUBLIC)
>>> ```
>>> 
>> 
>> This is certainly shorter. However, my IDE no longer flags that assert statement as "always true".
>> 
>>> I still think it is paranoid (and non-standard) to assert the order of enum members for any enum that is used as `Comparable`
>> 
>> For better or worse, every enum exposes its constants' declaration order. That order may unexpectedly become relied upon elsewhere, and the author of the enum cannot do anything about it. To me, this is still a gotcha moment.
>> 
>> When I found that doclint.Env.AccessKind.compareTo was used, my initial reaction (after Yuck!) was to introduce explicit construction AccessKind(int level) and boolean instance methods, such as lessLimiting(AccessLevel), equallyLimiting(AccessLevel) and moreLimiting(AccessLevel), that would compare int levels that the constants were constructed with.
>> 
>> But on second thought, it felt like working against the language, which gives us all these features for free. So I decided to embrace that part of enums design and aid the next person who will look at this code, by adding a test, a comment or an assertion.
>> 
>> There were already a few tests, so I decided not to add more. An assertion and a comment together are better than just an assertion, which is better than just a comment. Assertion is a checked comment: it jumps out at you, it fails fast, and works nicely with a sufficiently smart IDE.
>
>> For better or worse, every enum exposes its constants' declaration order. That order may unexpectedly become relied upon elsewhere, and the author of the enum cannot do anything about it. T
> 
> Since any `enum` implements `Comparable`, it is not reasonable to say that the order may be relied on "unexpectedly".  It is an intentionally defined feature of the language design.   And the author _can_ do something about it: the author can choose not to reorder constants when the order is significant.
> 
> A more interesting design, back in the day, might have been to make `Comparable` an opt-in super type, but that's 20-20 hindsight and not the case.

While it is not _necessary_ to indicate that an enum implements `Comparable`, it is permissible to state it explicitly, such as in `enum Foo implements Comparable<Foo> { foo1, foo2 }`.  which is a somewhat more linguistic way of writing `/** The order of the constants is important. */ enum Foo { foo1, foo2 }`

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/16714#discussion_r1399513007