Fix 23216 - Better error message for foreach_reverse without Bidirectional Range

[dkorpel]

No description provided.

[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
✓	23216	enhancement	Better Error Message For foreach_reverse Without Bidirectional Range

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#14254"

[RazvanN7]

It seems that inferForeachAggregate only checks for back/front methods to be present, but not for popBack/Front. This makes this code:

  struct Range
  {
      bool empty = true;
      int front = 0;
      void popFront() { } 
      int back = 1;                                                                                                     
  }
  
  void main()
  {
      Range r;
      foreach_reverse (word; r) { } 
  }

issue: "Error: no property popBack for type test.Range"

whereas, this code:

  struct Range
  {
      bool empty = true;
      int front = 0;
      void popFront() { } 
      int popBack = 1;                                                                                                     
  }
  
  void main()
  {
      Range r;
      foreach_reverse (word; r) { } 
  }

issues: "test.d(12): Error: invalid foreach_reverse aggregate r of type Range
test.d(12): foreach_reverse requires a bidirectional range (implementing back and popBack)
test.d(12): https://dlang.org/phobos/std_range_primitives.html#isBidirectionalRange"

Although it's a minor concern, I think that this inconsistency should be fixed.

[mdparker]

Thanks!

[dkorpel]

It seems that inferForeachAggregate only checks for back/front methods to be present, but not for popBack/Front.

It doesn't check for the correct function signatures either, it's only guessing whether it's intended to use the range interface instead of opApply.

[RazvanN7]

There is no need for the 2 paths. You can combine them and use a ternary condition to select the appropriate string between foreach and foreach_reverse.

[RazvanN7]

ping @dkorpel

[dkorpel]

What's the advantage of a ternary here? I think it's more readable like this.

[RazvanN7]

Fewer lines of code, consistency with how this sort of case is treated in the rest of the code.

[RazvanN7]

At the minimum, the error supplemental should not be duplicated. Nvm.