Let safety of cartesianProduct.Result.popFront be inferred

[John-Colvin]

I don't believe #7440 was the correct thing to do. @Geod24

[dlang-bot]

Thanks for your pull request and interest in making D better, @John-Colvin! 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
✓	20943	regression	std.algorithm.setops.cartesianProduct fails for ranges with @System popFront

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 "stable + phobos#7519"

[PetarKirov]

Is there a @safe unit test to verify that this can be used from @safe code? If yes, than I'm fine with merging, though I wonder if you hit some problem in practice which prompted you to propose this change.

[Geod24]

Indeed, #7440 was only applying a pre-existing comment, but on second look, that comment was wrong. There are plenty of @safe unittest, but no @system one. Could you add one @John-Colvin ? From what I see, having a @system popFront would do it.

[John-Colvin]

Indeed, #7440 was only applying a pre-existing comment, but on second look, that comment was wrong. There are plenty of @safe unittest, but no @system one. Could you add one @John-Colvin ? From what I see, having a @system popFront would do it.

done

Also re-targeted to stable because it's a regression

[Geod24]

Style check failed:

generated/dscanner-9364d6f15f4a610fda49a693dbc18608bfc701bb/dsc --config .dscanner.ini --styleCheck etc std -I.
--
  | std/algorithm/setops.d(557:1)[warn]: A unittest should be annotated with at least @safe or @system
  | std/algorithm/setops.d(566:13)[warn]: Zero-parameter '@property' function should be marked 'const', 'inout', or 'immutable'.
  | std/algorithm/setops.d(571:14)[warn]: Zero-parameter '@property' function should be marked 'const', 'inout', or 'immutable'.

[John-Colvin]

Style check failed:

generated/dscanner-9364d6f15f4a610fda49a693dbc18608bfc701bb/dsc --config .dscanner.ini --styleCheck etc std -I.
--
  | std/algorithm/setops.d(557:1)[warn]: A unittest should be annotated with at least @safe or @system
  | std/algorithm/setops.d(566:13)[warn]: Zero-parameter '@property' function should be marked 'const', 'inout', or 'immutable'.
  | std/algorithm/setops.d(571:14)[warn]: Zero-parameter '@property' function should be marked 'const', 'inout', or 'immutable'.

The first one seems fair enough.

The fact that the second 2 apply to unittests is madness, because it means phobos can't test functions like them!

[Geod24]

True, but why would you use @property in the first place ?

[John-Colvin]

True, but why would you use @property in the first place ?

That's a reasonable question, but even if we assume one shouldn't use it, it's all over people's code for historical reasons or because of misunderstanding. You can't easily remove it because it affects typeof and __traits(compiles, ...).

[Geod24]

it's all over people's code

Yeah, maybe we should add a check that no new usage is introduced.

You can't easily remove it

Is there any code in Phobos that requires the usage of @property ? I know of the minor difference it introduces, but I'd imagine any reasonable generic utility would not depend on @property. Perhaps I am being too hopeful.

[n8sh]

Is there any code in Phobos that requires the usage of @property? I

There was at least some code that didn't work unless front was @property.