Annotate more of phobos with `return` and `scope` by dkorpel · Pull Request #8113 · dlang/phobos

dkorpel · 2021-05-28T19:50:40Z

Continuation of #8076, but for dlang/dmd#12010 instead of dlang/dmd#10924

I'm still figuring out what to do with range functions like array, text and toCase. scope fails to be inferred for simple char[] cases, but putting scope on them forces every InputRange it's called on to be scope compatible, while a generic InputRange can escape its innards through global variables or Exceptions. Not that I want to support such ranges, but it would make transitioning to -dip1000 more diffcult for users, so it may be better to leave them be and use work-arounds for current Phobos functions that need them to be scope, like I did in std.getopt by replacing split (not scope compatible, returns array of slices) with splitter (lazy, scope compatible).

dlang-bot · 2021-05-28T19:50:43Z

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

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

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

dkorpel · 2021-07-07T11:18:31Z

This should be good to go now: I can now locally build Phobos' unittests with dlang/dmd#12010

I undid some of the scope annotations of Per's PR because they were only done to satisfy the requirement that pure functions always take scope arguments, which I argued against.

Many range functions don't have correct scope inference yet, so I replaced some static arrays in @safe unittests with arrays in static data. They can be enhanced to work with scope ranges later, for now it's important to fix dip1000 first

thewilsonator

Pardon my ignorance here

std/algorithm/mutation.d

thewilsonator · 2021-07-07T14:04:47Z

std/array.d

        For a lazy version, see $(REF joiner, std,algorithm,iteration)
  +/
-ElementEncodingType!(ElementType!RoR)[] join(RoR, R)(RoR ror, scope R sep)
+ElementEncodingType!(ElementType!RoR)[] join(RoR, R)(RoR ror, R sep)


this removes scope I thought this PR was trying to add it, am I missing something?

Range functions should ideally infer scope based on empty(), popFront(), front(), copy constructors etc.
scope inference is not that good yet however (issue 20674) so sometimes scope is manually applied to help the compiler. With issue 20150 fixed, this scope R sep gives an error:

scope variable sep assigned to non-scope parameter r calling std.array.array!(Once).array

So the long term fix is to ensure scope inference works for both join and array, but that's an enhancement. For now it's important to get the dip1000 bug fix in.

Won't this break existing code if the inference fails?

All the accepts-invalid fixes for dip1000 are breaking changes. Luckily dip1000 is still experimental.

But these changes are neither specific to DIP1000 nor about actually invalid code. It would be better to first improve the inference s.t. there is no unecessary breakage.

What code do you think would break without -preview=dip1000?

I don't have aspecific example ATM, will post one if I find a small example.
But we should strive to not break valid code. -dip1000 has existed for quite some time and is certainly used in the wild.

dlang/dmd#12010 is going to break such dip1000 code in the wild undoubtedly, I just tested it on my own 37 KLOC dip1000 library and I had to add 3 scope annotations. Do you think we need a second switch preview=dip1000fixed?

Neither said nor implied that - some breakage will be unavoidable. I'm just concerned that these changes introduce a temporary regression that could be avoided by first improving the scope inference.

But the actual fallout might be neglegible given that your projects required minimal changes and buildkite passes as is.

thewilsonator · 2021-07-07T14:05:35Z

std/path.d

    See_Also:
        $(LREF asAbsolutePath) which does not allocate
 */
-string absolutePath(return scope string path, lazy string base = getcwd())


absoultePath calls chainPath(baseVar, path).array, and scope inference fails for .array currently

thewilsonator · 2021-07-07T14:05:42Z

std/path.d

    `Exception` if the specified _base directory is not absolute.
 */
 string relativePath(CaseSensitive cs = CaseSensitive.osDefault)
-    (scope return string path, lazy string base = getcwd())


relativePath calls asRelativePath which takes input ranges, and scope inference fails there

std/random.d

std/algorithm/mutation.d

thewilsonator · 2021-07-07T14:09:14Z

This otherwise looks OK

thewilsonator · 2021-07-07T14:33:28Z

Thanks for clearing that all up.

thewilsonator · 2021-07-07T22:49:37Z

please rebase or force push to start CircleCI

nordlow · 2021-07-11T19:23:14Z

std/algorithm/mutation.d

 void move(T)(ref T source, ref T target)
 {
-    moveImpl(source, target);
+    moveImpl(target, source);


Why the reversion of parameter order?

dlang/druntime#3480 (comment)

I probably should have made a comment in the source code as well

dkorpel force-pushed the more-pure-scope branch from 5be4da2 to 1bee1db Compare May 29, 2021 11:24

This was referenced May 29, 2021

Annotate more return/scope on bigint functions #8114

Merged

Use splitter instead of split in getopt #8115

Merged

Add return scope to realloc #8116

Merged

Add return scope to xml.toType #8117

Merged

dlang-bot added Merge:Needs Rebase Review:Needs Work labels May 30, 2021

This was referenced Jun 1, 2021

Use icmp instead of toUpper in getopt #8121

Merged

Add return scope to std.uni MultiArray ctor #8122

Merged

Use static foreach for correct scope inference #8123

Merged

dlang-bot added Merge:Needs Rebase Review:Needs Work and removed Merge:Needs Rebase Review:Needs Work labels Jun 4, 2021

dkorpel force-pushed the more-pure-scope branch from 65092e8 to f48aa19 Compare July 6, 2021 19:54

dlang-bot removed Merge:Needs Rebase Review:Needs Work labels Jul 6, 2021

ibuclaw closed this Jul 6, 2021

ibuclaw reopened this Jul 6, 2021

ibuclaw closed this Jul 6, 2021

ibuclaw reopened this Jul 6, 2021

dkorpel force-pushed the more-pure-scope branch 3 times, most recently from 495d4d8 to a023284 Compare July 6, 2021 22:54

ibuclaw closed this Jul 6, 2021

ibuclaw reopened this Jul 6, 2021

dkorpel force-pushed the more-pure-scope branch from a023284 to 71a3b5f Compare July 7, 2021 09:44

dkorpel marked this pull request as ready for review July 7, 2021 10:47

dkorpel requested a review from andralex as a code owner July 7, 2021 10:47

dkorpel requested review from CyberShadow, JackStouffer, burner, jmdavis, kyllingstad, schveiguy and wilzbach as code owners July 7, 2021 10:47

CyberShadow removed their request for review July 7, 2021 11:19

thewilsonator reviewed Jul 7, 2021

View reviewed changes

thewilsonator approved these changes Jul 7, 2021

View reviewed changes

thewilsonator added the Merge:auto-merge label Jul 7, 2021

thewilsonator removed the Merge:auto-merge label Jul 7, 2021

dkorpel force-pushed the more-pure-scope branch 2 times, most recently from 9d930b0 to a94213e Compare July 8, 2021 19:35

add explicit scope/return for pure functions

5f6f796

dkorpel force-pushed the more-pure-scope branch from a94213e to 5f6f796 Compare July 11, 2021 12:19

thewilsonator added the Merge:auto-merge label Jul 11, 2021

dlang-bot merged commit 9ec2419 into dlang:master Jul 11, 2021

dkorpel deleted the more-pure-scope branch July 11, 2021 13:35

dkorpel mentioned this pull request Jul 11, 2021

fix Issue 20150 - -dip1000 defeated by pure dlang/dmd#12010

Merged

nordlow reviewed Jul 11, 2021

View reviewed changes

Uh oh!

Comments

Conversation

dkorpel commented May 28, 2021

Uh oh!

dlang-bot commented May 28, 2021

Bugzilla references

Testing this PR locally

Uh oh!

dkorpel commented Jul 7, 2021

Uh oh!

thewilsonator left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

thewilsonator commented Jul 7, 2021

Uh oh!

thewilsonator commented Jul 7, 2021

Uh oh!

thewilsonator commented Jul 7, 2021

Uh oh!

nordlow Jul 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

nordlow Jul 11, 2021 •

edited

Loading