[15] RFR: 8239836: ZoneRules.of() doesn't check transitionList/standardOffsetTL arguments validity

[naoto.sato at oracle.com]

Thanks, Stephen.

Updated the webrev for those two suggestions:

http://cr.openjdk.java.net/~naoto/8239836/webrev.04/

Naoto

On 3/8/20 4:22 PM, Stephen Colebourne wrote:
>   standardOffsets[0] == wallOffsets[0] needs to use .equals()
> 
> Unrelated, there is a Javadoc/spec error at line 578 of the "new"
> file. It should be getValidOffsets, not getOffset.
> 
> Apart from that, it looks good.
> 
> thanks
> Stephen
> 
> 
> On Wed, 4 Mar 2020 at 19:06, <naoto.sato at oracle.com> wrote:
>>
>> Hi Stephen,
>>
>> Thank you for the detailed explanation and correcting the
>> implementation. I incorporated all the suggestions below, as well as
>> adding a new test for isFixedOffset() implementation (with some clean-up):
>>
>> https://cr.openjdk.java.net/~naoto/8239836/webrev.03/
>>
>> Please take a look at it.
>>
>> Naoto
>>
>> On 3/3/20 3:30 PM, Stephen Colebourne wrote:
>>> I fear more changes are needed here.
>>>
>>> 1) The code is isFixedOffset() is indeed wrong, but the fix is
>>> insufficient. The zone is fixed if baseStandardOffset=baseWallOffset
>>> and all three other lists are empty. A fixed offset rule is the
>>> equivalent of a ZoneOffset. See ZoneId.normalized() for the code that
>>> assumes that.
>>>
>>> 2) The code in getOffset(Instant) is wrong, but so is the proposed
>>> fix. The purpose of the if statement is to optimise the following few
>>> lines which refer to savingsInstantTransitions and wallOffsets. The
>>> code does have a bug as it should return the first wall offset.
>>> Corrected code:
>>>     public ZoneOffset getOffset(Instant instant) {
>>>       if (savingsInstantTransitions.length == 0) {
>>>         return wallOffsets[0];
>>>       }
>>> With the correct definition of isFixedOffset() it becomes apparent
>>> that this if statement is in fact not related to the fixed offset.
>>>
>>> Currently this test case fails (a zone in DST for all eternity):
>>>           ZoneRules rules = ZoneRules.of(
>>>                   ZoneOffset.ofHours(1),
>>>                   ZoneOffset.ofHours(2),
>>>                   Collections.emptyList(),
>>>                   Collections.emptyList(),
>>>                   Collections.emptyList());
>>>           assertEquals(rules.getStandardOffset(Instant.EPOCH),
>>> ZoneOffset.ofHours(1));
>>>           assertEquals(rules.getOffset(Instant.EPOCH),  ZoneOffset.ofHours(2));
>>>
>>> 3) The code in getStandardOffset(Instant) also has an error as it
>>> checks the wrong array. It should check standardTransitions as that is
>>> what is used in the following few lines. Corrected code:
>>>     public ZoneOffset getStandardOffset(Instant instant) {
>>>       if (standardTransitions.length == 0) {
>>>         return standardOffsets[0];
>>>       }
>>>
>>> 4) The code in getOffsetInfo(LocalDateTime dt) has a similar fault.
>>> Since it protects savingsLocalTransitions, it should return
>>> wallOffsets[0].
>>>
>>> 5) The code in getDaylightSavings(Instant instant) has an optimising
>>> if statement that should refer to isFixedOffset().
>>>
>>> 6) Also, in the test.
>>>    assertTrue(!rules.isFixedOffset());
>>> should be
>>>    assertFalse(rules.isFixedOffset());
>>>
>>> The class has worked so far as every normal case has
>>> baseStandardOffset = baseWallOffset, even though it is conceptually
>>> valid for this not to be true.
>>>
>>> Stephen
>>>
>>>
>>>
>>>
>>> On Thu, 27 Feb 2020 at 20:42, <naoto.sato at oracle.com> wrote:
>>>>
>>>> Hello,
>>>>
>>>> Please review the fix to the following issue:
>>>>
>>>> https://bugs.openjdk.java.net/browse/JDK-8239836
>>>>
>>>> The proposed changeset is located at:
>>>>
>>>> https://cr.openjdk.java.net/~naoto/8239836/webrev.00/
>>>>
>>>> It turned out that 'transitionList' is not necessarily a superset of
>>>> 'standardOffsetTransitionList', as in some occasions where a standard
>>>> offset change and a savings change happen at the same time (canceling
>>>> each other), resulting in no wall time transition. This means that the
>>>> "rules" in the sample code is a valid ZoneRules instance.
>>>>
>>>> However, there is an assumption in ZoneRules where no (wall time)
>>>> transition means the fixed offset zone. With that assumption, the
>>>> example rule is considered a fixed offset zone which is not correct. So
>>>> the fix proposed here is not to throw an IAE but to recognize the rule
>>>> as a valid, non-fixed offset ZoneRules instance.
>>>>
>>>> Naoto
>>>>
>>>>