Fix issue 15889 - Array bounds check should report index and length

[dkorpel]

Uses the ArrayIndexError and ArraySliceError instead of RangeError for array index / slice operations.
Depends on this druntime PR: dlang/druntime#3517

I still need to decide when to use them:

• always
• with -checkaction=context
• in debug builds
• in unoptimized builds
• ...?

[dlang-bot]

Thanks for your pull request and interest in making D better, @dkorpel! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

• My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
• My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
• I have provided a detailed rationale explaining my changes
• New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

---

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	15889	enhancement	Array bounds check should report index and length

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#12871"

[MoonlightSentinel]

I still need to decide when to use them:

IMO the bounds should always be reported when a RangeError is thrown. They can provide vital information for a neglegible cost (in contrast to the whole RangeError construction + throwing).

[RazvanN7]

The vision document for druntime states that implementing druntime hooks as templates is a major objective. Would it make sense for these hooks to be implemented as templates? (the main advantage would be that the associated druntime functions will not be part of the executable if not used.)

[dkorpel]

The vision document for druntime states that implementing druntime hooks as templates is a major objective. Would it make sense for these hooks to be implemented as templates?

I thought the idea was to replace functions taking TypeInfo with functions templated on the type. In this case, since range errors don't depend on the array type, a normal function works fine. The linker can still strip _d_arraybounds_indexp if it's not used (though I think very few D projects don't use bounds checks).

[dkorpel]

/home/runner/work/dmd/dmd/dmd/test/test_results/runnable_cxx/cppa_0: symbol lookup error: /home/runner/work/dmd/dmd/dmd/test/test_results/runnable_cxx/cppa_0: undefined symbol: _d_arraybounds_slicep

Looks like the C++ interop tests with GCC aren't linking with the newest druntime, does anyone know how to fix it?

[dkorpel]

Looks like the C++ interop tests with GCC aren't linking with the newest druntime, does anyone know how to fix it?

It turns out the checkout is fine, but it doesn't seem to be linking with druntime to begin with. I worked around it by adding -checkaction=C to the cppa.d test. Thanks to @Geod24 for his assistance!

[Geod24]

Can you merge the second commit ("Add runnable test...") with the fix ? A fix / new feature should come with tests in the same commit. Also, could you swap the order of the commits so that the last commit comes first ? Each commit should be atomic, which means they should compile and pass the test-suite on their own, and the last commit is required for your feature to pass the test suite (but the last commit makes sense on its own, as the same problem would arise if, e.g. -checkaction=context was made the default, so 👍 for having it as a separate commit).

The code LGTM, and so does the changelog. Thanks for putting time to make it both informative and appealing, and for tackling that feature, it'll make a huge difference in usability.

[Geod24]

POV: "fluent" is a bit vague, I would advise to either phrase it as "now includes indices" or mention "better" or "improved" error message. But that's POV, I think the message is good enough as-is.

[dkorpel]

I borrowed the term 'fluent' from the common 'fluent asserts' you find in unit testing packages, but I agree it's not the best word. I'm thinking changing it to "informative" or, indeed, "better".

[Geod24]

Not sure why this mention was warranted ?

[dkorpel]

I figured people might wonder if all range errors are improved, so I'm stating a known limitation here.

[Geod24]

The changelog doesn't mention the two new classes derived from Error, except for the implicit mentions in the code sample. Not sure if you want to add it here or not.

[dkorpel]

I mentioned them at first, but I removed it because I don't want to encourage D users to catch range errors.

[dkorpel]

Each commit should be atomic, which means they should compile and pass the test-suite on their own

Thanks for the advice, I don't know a good commit separation policy so I was just winging it. I will change it.

[ibuclaw]

This PR made RangeErrors with pointer slices more confusing, not less.

void main()
{
    int* a = [1,2,3,4,5,6,7,8,9,10].ptr;
    a[1..5] = a[3..7];
}

core.exception.ArrayIndexError@range.d(4): index [4] exceeds array of length 4

Array length hasn't been exceeded, it just overlaps.

Edit: Same is with overlapping arrays too.

    int[] b = [1,2,3,4,5,6,7,8,9,10];
    b[1..5] = b[3..7];

[dkorpel]

I'll look into it

[dkorpel]

This makes no sense, this is not an index error but a slice copy error.