RFR [9] 8177738: Runtime.Version must be a value-based class

[Pavel Rappo]

Hello,

Please review the following change:

    http://cr.openjdk.java.net/~prappo/8177738/webrev.00/

This is an attempt to resolve a group of issues related to Version-String API
and its implementation:

    8177738: Runtime.Version must be a value-based class
    8148822: (spec) Regex in Runtime.Version and JEP 223 should match
    8160954: (spec) Runtime.Version regex and $PRE/$OPT issues
    8148877: (spec) Specify when an empty '+' is required in a version string

A good amount of work has been already done here and we just need to finish it.
Let me explain what this patch does starting from the most trivial changes.

- Fixes certain javadoc issues [a] (e.g. dangling html tags, overly qualified
symbol references in @link tags, etc.)

- Refactors Basic.java test, making it a little bit easier to read/modify this
test in the future.

- Addresses [2] in Runtime.java:963,1050. In addition I've rephrased Runtime.java:951
to reflect the fact a version string contains only the mentioned components and
nothing else. In other words, $VSTR *matches* a valid version-string rather than
*finds* itself somewhere in this version-string.

- Makes Runtime.Version a value-based class [1][5]. The class has gotten `final`
modifier, the only constructor has become private and the usual (in this case)
verbiage has been added to the javadoc. Before that, Runtime.Version had ticked
all other the boxes in the value-based class checklist already.

- Improves $VNUM list mechanics in VersionProps.java.template and Runtime.java:1149-1153
by providing immutable list instead of unmodifiable. This change has become
possible due to the fact Runtime.Version constructor has become private and,
thus, all the callers are known and trusted.

- Removes redundant/explicit javadoc declarations in `hashCode`, `equals` and
`compareTo` methods. These methods are documented needlessly explicit. The only
truly justified comment sits inside `compareToIgnoreOptional` method.
Since `compareToIgnoreOptional` and `equalsIgnoreOptional` are not inherited,
I marked the "consistent-with" relationship between them explicitly [c].

- Now a few words about a not-so-pleasant part which addresses [3][4]. In a
nutshell the regex-style format ($VSTR) which is used in the spec to define the
version-string, matches all valid version strings and *some more*.

Here's the format:

    $VNUM(-$PRE)?(\+($BUILD)?(-$OPT)?)?

If we expand all optional elements we will get the following 8 possibilities:

  ##     $VSTR                      $PRE $BUILD $OPT    EXAMPLE
--------------------------------------------------------------------------------
  1      $VNUM                         0      0    0    9.0.1
  2      $VNUM+-$OPT                   0      0    1    9.0.1+-longcat.dev
  3      $VNUM+$BUILD                  0      1    0    9.0.1+256
  4      $VNUM+$BUILD-$OPT             0      1    1    9.0.1+256-longcat.dev
  5      $VNUM-$PRE                    1      0    0    9.0.1-ea
* 6      $VNUM-$PRE+-$OPT              1      0    1    9.0.1-ea+-longcat.dev
  7      $VNUM-$PRE+$BUILD             1      1    0    9.0.1-ea+256
  8      $VNUM-$PRE+$BUILD-$OPT        1      1    1    9.0.1-ea+256-longcat.dev

Now, ##6 was disallowed by design. In the case when $BUILD is not present, but
both $PRE and $OPT are, the $VSTR will instead look like this:

  6      $VNUM-$PRE-$OPT               1      0    1    9.0.1-ea-longcat.dev

(i.e. without the "+" separator).

This is a problem because it means that in addition to the format it needs a
clear description of exceptional cases which are to be subtracted from the set
of matched version-strings. Such a description has been proposed in [6] (you can
have a look at the webrev that thread refers to):

    * <p> If the build number is unset, then {@code '+'} may only be present
    * when {@code $OPT} is present and {@code $PRE} is not.

It indeed may be a satisfactory solution. However I would argue that it's
probably not the best one. I suspect spec readers will jump straight to the
format completely missing "the special case" paragraph simply because the format
attracts attention and neatly defines the version-string. Neatly but wrongly.

Another option would be to split the $VSTR format into a minimal number of
sub-cases (e.g. Runtime:1032-1034). This will require no extra "special case"
paragraph and will draw reader's attention to all the subtleties on the spot.

  #                                       ##
--------------------------------------------------------------------------------
  1      $VNUM(-$PRE)?\+$BUILD(-$OPT)?    3, 4, 7, 8
  2      $VNUM-$PRE(-$OPT)?               5, 6
  3      $VNUM(+-$OPT)?                   1, 2

One more option would be to require "+" to be there in the case where any
component to the right from $PRE is present (i.e. $BUILD or $OPT or both).
This will greatly simplify the rules and allow the $VSTR to stay as it is today.

This option would affect only the case ##6:

  6      $VNUM-$PRE+-$OPT              1      0    1    9.0.1-ea+-longcat.dev
                   ^^
Yes, it looks a bit ugly, but we already have this ugliness in case 2 (and we
can't get rid of it because of [3]):

  2      $VNUM+-$OPT                   0      0    1    9.0.1+-longcat.dev
              ^^
I suspect both 2 and 6 will be pretty rare anyway as it would mean optional
build information is there, but the build number is not.
Sounds like a corner case to me.

Thanks,
-Pavel

--------------------------------------------------------------------------------
[1] https://bugs.openjdk.java.net/browse/JDK-8177738
[2] https://bugs.openjdk.java.net/browse/JDK-8148822
[3] https://bugs.openjdk.java.net/browse/JDK-8148877
[4] https://bugs.openjdk.java.net/browse/JDK-8160954
[5] http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-March/046813.html
[6] http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-October/044277.html

[a] If you build locally or use promoted builds, please keep in mind there's a
    known issue with rendering documentation for Runtime.Version in
    some browsers: https://bugs.openjdk.java.net/browse/JDK-8177727
[b] Startup time/memory footprint might benefit as well (it will definitely
    require some benchmarking to determine if it is the case)
[c] Even though I'm fully aware I do not have the full knowledge the design
    decisions were based upon, I must say I fail to see why equalsIgnoreOptional
    and compareToIgnoreOptional methods have been introduced from the day 1.
    It looks like something that could be neatly solved by providing a custom
    Comparator if needed.