[
https://issues.apache.org/jira/browse/GROOVY-12412?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18115591#comment-18115591
]
ASF GitHub Bot commented on GROOVY-12412:
-----------------------------------------
codecov-commenter commented on PR #2937:
URL: https://github.com/apache/groovy/pull/2937#issuecomment-5680729270
##
[Codecov](https://app.codecov.io/gh/apache/groovy/pull/2937?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 71.6104%. Comparing base
([`0041ce8`](https://app.codecov.io/gh/apache/groovy/commit/0041ce8c3a6364687a59aaad943a3264be4a2237?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache))
to head
([`86fa154`](https://app.codecov.io/gh/apache/groovy/commit/86fa154c37d84e187ce82ced86bbd828ce09e070?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
<details><summary>Additional details and impacted files</summary>
[](https://app.codecov.io/gh/apache/groovy/pull/2937?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
```diff
@@ Coverage Diff @@
## master #2937 +/- ##
==================================================
+ Coverage 71.5979% 71.6104% +0.0126%
- Complexity 38043 38060 +17
==================================================
Files 1588 1588
Lines 137046 137068 +22
Branches 25493 25505 +12
==================================================
+ Hits 98122 98155 +33
+ Misses 30018 30009 -9
+ Partials 8906 8904 -2
```
| [Files with missing
lines](https://app.codecov.io/gh/apache/groovy/pull/2937?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
| Coverage Δ | |
|---|---|---|
|
[src/main/java/groovy/lang/NumberRange.java](https://app.codecov.io/gh/apache/groovy/pull/2937?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Fgroovy%2Flang%2FNumberRange.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9ncm9vdnkvbGFuZy9OdW1iZXJSYW5nZS5qYXZh)
| `80.1980% <100.0000%> (+0.1980%)` | :arrow_up: |
|
[...rg/codehaus/groovy/runtime/ArrayGroovyMethods.java](https://app.codecov.io/gh/apache/groovy/pull/2937?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Fruntime%2FArrayGroovyMethods.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L3J1bnRpbWUvQXJyYXlHcm9vdnlNZXRob2RzLmphdmE=)
| `87.3247% <100.0000%> (+0.0549%)` | :arrow_up: |
|
[.../codehaus/groovy/runtime/DefaultGroovyMethods.java](https://app.codecov.io/gh/apache/groovy/pull/2937?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Fruntime%2FDefaultGroovyMethods.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L3J1bnRpbWUvRGVmYXVsdEdyb292eU1ldGhvZHMuamF2YQ==)
| `75.6757% <100.0000%> (+0.0186%)` | :arrow_up: |
|
[...g/codehaus/groovy/runtime/StringGroovyMethods.java](https://app.codecov.io/gh/apache/groovy/pull/2937?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Fruntime%2FStringGroovyMethods.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L3J1bnRpbWUvU3RyaW5nR3Jvb3Z5TWV0aG9kcy5qYXZh)
| `82.6456% <100.0000%> (+0.1916%)` | :arrow_up: |
... and [12 files with indirect coverage
changes](https://app.codecov.io/gh/apache/groovy/pull/2937/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
</details>
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test
Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests,
report on failures, and find test suite problems.
- :package: [JS Bundle
Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save
yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
> Improve consistency of numerous Range indexing edge cases
> ---------------------------------------------------------
>
> Key: GROOVY-12412
> URL: https://issues.apache.org/jira/browse/GROOVY-12412
> Project: Groovy
> Issue Type: Bug
> Reporter: Paul King
> Assignee: Paul King
> Priority: Major
>
> Range indexing behaves differently depending on the receiver, on which
> {{Range}} implementation
> the bounds happen to produce, and on whether the range is the only index or
> one of several. The
> differences are not documented anywhere and no test pins them. This issue
> brings all of them into
> line and adds the missing tests.
> Six receivers support subscripting by index: {{List}}, {{ListWithDefault}},
> object arrays,
> primitive arrays, {{CharSequence}} and {{Matcher}}. Measured over eighteen
> subscript forms, they
> disagree on twelve of them today.
> h3. The rule that should hold
> A range used as an index has its bounds resolved against the size of the
> receiver, whether it is
> the only index or one of several: a negative bound counts back from the end,
> both exclusive forms
> drop the right element, a range written high to low reverses the result, and
> an empty range
> contributes nothing. Anything that is not a range, including a nested list of
> indices, is walked
> element by element, with negative values still counting from the end.
> h3. Symptoms
> {code:groovy}
> def list = (1..8).toList()
> def ints = (1..8) as int[]
> def objs = (1..8) as Integer[]
> def m = 'a1b2c3d4' =~ /[a-z]\d/
> // 1. a nested range on a List ignores a negative bound (regression, 5.0.0)
> assert list[0..2, 3..-1] == [1, 2, 3, 4, 5, 6, 7, 8] // actual [1, 2, 3, 4,
> 3, 2, 1, 8]
> // 2. a primitive array resolves a sole range only when it is an IntRange
> assert ints[3..-1] == [4, 5, 6, 7, 8] // ok
> assert ints[3L..-1L] == [4, 5, 6, 7, 8] // actual [4, 3, 2, 1,
> 8]
> assert ints[0, 3..-1] == [1, 4, 5, 6, 7, 8] // actual [1, 4, 3, 2,
> 1, 8]
> // 3. a NumberRange cannot tell a negative bound from a reversed range
> assert list[3L..-1L] == [4, 5, 6, 7, 8] // actual [8, 7, 6, 5,
> 4]
> assert list[2L..0L] == [3, 2, 1] // actual [1, 2, 3]
> // 4. Matcher loses an element on every exclusive form, ignores reversal,
> // and rejects a nested collection of indices
> assert m[1..<3] == ['b2', 'c3'] // actual ['b2']
> assert m[3..1] == ['d4', 'c3', 'b2'] // actual ['b2', 'c3',
> 'd4']
> m[0, [1, 3]] // GroovyCastException
> // 5. a nested empty range adds a stray element on object arrays and strings
> assert objs[0..1, 2..<2] == [1, 2] // actual [1, 2, 3]
> assert 'abcdefgh'[0..2, 2..<2] == 'abc' // actual 'abcc'
> {code}
> h3. Root causes
> *1. {{DefaultGroovyMethods.getAt(List, Collection)}} lost its dynamic
> dispatch.* Commit
> 87b4109418 ("minor refactor - no need to funnel through InvokerHelper here",
> 2022-08-24, no JIRA
> reference) replaced {{InvokerHelper.invokeMethod(self, "getAt", value)}} with
> a direct call to the
> {{Collection}} overload. The dynamic call let the MOP select {{getAt(List,
> Range)}} for a nested
> range; the direct call recurses and walks the range's own elements as
> indices. This is the only
> regression in this issue: it is not on {{GROOVY_4_0_X}} and first shipped in
> 5.0.0. The sibling
> {{getAt(ListWithDefault, Collection)}} still uses the dynamic form, which is
> why
> {{ListWithDefault}} was never affected.
> *2. Primitive arrays resolve only an {{IntRange}}.* The sixteen
> {{getAt(prim[], Range<?>)}} and {{getAt(prim[], ObjectRange)}} overloads, and
> the shared helper
> behind the {{Collection}} overloads, walk the range literally. Only the typed
> {{getAt(prim[], IntRange)}} overloads resolve bounds. Present in every 4.x
> and 5.x release.
> *3. {{NumberRange.subListBorders}} reads its normalised bounds.* The
> constructor computes
> {{reverse}} and stores {{from}} and {{to}} in ascending order, so by the time
> borders are computed
> {{3L..-1L}} is indistinguishable from {{-1L..3L}}. {{IntRange}} keeps the raw
> order in its fields
> and presents the normalised view through {{getFrom()}} and {{getTo()}}, which
> is exactly why it
> gets negative indexing right. Both then share {{IntRange.subListBorders}}.
> Any bound that is not
> an {{Integer}} produces a {{NumberRange}}, so this covers long, short, byte,
> {{BigInteger}} and
> decimal bounds. Present in every 4.x and 5.x release.
> *4. {{StringGroovyMethods.getAt(Matcher, Collection)}} rebuilds a range from
> resolved borders.*
> It resolves borders only when the whole index argument is an {{IntRange}},
> then does
> {{new IntRange(((IntRange) indices).getInclusive(), info.from, info.to -
> 1)}}. Re-applying the
> original inclusivity to an already resolved bound drops an element, and
> {{info.reverse}} is
> discarded. It also has no branch for a nested collection. Present in every
> 4.x and 5.x release.
> *5. {{EmptyRange}} implements {{Range}}, so a nested empty range reaches
> {{subListBorders(int, Range)}}, which returns a one element window.* This
> affects object arrays
> and {{CharSequence}}. The {{List}} path avoided it only by accident of the
> code in cause 1, so the
> guard is needed there too once that is fixed.
> h3. Fix
> || file || change ||
> | {{groovy/lang/NumberRange.java}} | recover the written bound order from the
> {{reverse}} flag before computing borders |
> | {{runtime/ArrayGroovyMethods.java}} | new private {{primitiveArraySubList}}
> resolves a range against the array length and handles {{EmptyRange}}; the
> sixteen sole-index overloads and the nested path both use it |
> | {{runtime/DefaultGroovyMethods.java}} | {{getAt(List, Collection)}}
> dispatches a nested range to the {{Range}} overload, with an {{EmptyRange}}
> guard |
> | {{runtime/StringGroovyMethods.java}} | {{EmptyRange}} guard on the
> {{CharSequence}} path; {{getAt(Matcher, Collection)}} rewritten to read the
> resolved window and to accept nested collections |
> The eight {{getAt(prim[], IntRange)}} overloads are untouched: they rebuild a
> range through
> {{subListRange}}, which does correctly undo the inclusivity flags, and they
> handle an empty window
> correctly.
> The literal walk remains available on every receiver by passing a plain list
> of indices, for
> example {{a[(3..-1).toList()]}}, which still yields {{[4, 3, 2, 1, 8]}}.
> h3. Behaviour changes that are not regression fixes
> * A {{NumberRange}} index now behaves like the equivalent {{IntRange}} on
> every receiver. Code
> relying on {{list[3L..-1L]}} returning a reversed window changes.
> * A sole {{NumberRange}} or {{ObjectRange}} index on a primitive array is now
> resolved rather than
> walked.
> * {{matcher[0, [1, 2]]}} now returns a result. {{RegularExpressionsTest}}
> carried
> {{shouldFail \{ matcher[0, [1, 2]] \}}}, added in 2008 under GROOVY-3028,
> which reads as
> characterising the behaviour of the day rather than arguing the form should
> be rejected. It has
> been replaced with a positive assertion, since every other receiver accepts
> it.
> * The deprecated protected {{primitiveArrayGet}} helpers in
> {{DefaultGroovyMethods}} still hold
> the old literal logic. Nothing in Groovy calls them; they are kept for
> binary compatibility.
> h3. Verification
> * Eighteen subscript forms measured against all six receivers, results
> normalised to the positions
> selected: twelve forms disagreed before, none disagree after.
> * Core {{:test}}: 18342 passed, 68 skipped, 0 failed. All 37 module suites:
> 5691 passed, 0 failed.
> * New tests: {{GroovyMethodsTest.testGetAtMultipleIndicesIncludingRanges}} and
> {{testGetAtSingleRangeResolvesBoundsOnEveryReceiver}} cover {{List}},
> {{ListWithDefault}},
> {{Integer[]}}, all eight primitive array types and {{CharSequence}} over
> negative, exclusive,
> reversed, empty and {{ObjectRange}} bounds in both positions.
> {{RegularExpressionsTest.testMatcherRangeIndicesMatchOtherAggregates}} does
> the same for
> {{Matcher}} and asserts that each index expression selects the same
> positions as from a list of
> the same size.
> * New executed javadoc examples on {{getAt(int[], Collection)}},
> {{getAt(int[], Range)}} and
> {{getAt(Matcher, Collection)}}, so the corrected behaviour is documented as
> well as tested.
> h3. Why this belongs in 6.0.0
> Item 1 alone is a regression against 4.x and would justify a fix on its own.
> The remaining items
> are long standing, individually obscure, and each looked too marginal to
> disturb an RC. Taken
> together they are what makes the first item a regression rather than a quirk:
> the same subscript
> written three ways over the same eight elements returns three different
> answers, and which answer
> you get depends on whether a bound happened to be an {{int}} or a {{long}},
> and on whether the
> range stood alone or sat beside another index. Fixing them as one change is
> what produces a rule
> that can be stated in a sentence and documented. Deferring the rest to 7
> would mean shipping 6
> with the regression fixed and the surrounding inconsistencies intact, then
> changing the same
> methods again a major version later.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
