Skip to content
This repository was archived by the owner on Oct 12, 2022. It is now read-only.
/ druntime Public archive

Comments

Annotate pure functions with return/scope#3480

Merged
dlang-bot merged 1 commit intodlang:masterfrom
dkorpel:pure-scope-fixes
May 28, 2021
Merged

Annotate pure functions with return/scope#3480
dlang-bot merged 1 commit intodlang:masterfrom
dkorpel:pure-scope-fixes

Conversation

@dkorpel
Copy link
Contributor

@dkorpel dkorpel commented May 20, 2021

This PR is similar to dlang/phobos#8076, but for druntime and for dlang/dmd#12010 instead of dlang/dmd#10924
It's probably too big to merge in one go, this is a proof of concept that can later be split up into smaller chunks for easier review.

The offending functions were found with something I cobbled together in dmd: dkorpel/dmd@7d1f0c5
It outputs this when running druntime unittests:

src/core/internal/dassert.d(221): parameter `a` of `idup` is cheating `scope`
src/object.d(3410): parameter `value` of `getElement` is cheating `scope`
src/core/internal/dassert.d(164): parameter `val` of `atomicLoad` is cheating `scope`
src/core/atomic.d(1161): parameter `val` of `atomicFetchAdd` is cheating `scope`
src/core/atomic.d(1208): parameter `val` of `atomicFetchSub` is cheating `scope`
src/core/lifetime.d(1881): parameter `source` of `moveImpl` is cheating `scope`
src/core/memory.d(737): parameter `p` of `gc_query` is cheating `scope`
src/core/memory.d(1453): parameter `p` of `realloc` is cheating `scope`
src/core/internal/convert.d(479): parameter `a` of `dup` is cheating `scope`
src/core/internal/array/appending.d-mixin-42(50): parameter `px` of `_d_arrayappendcTX` is cheating `scope`
src/core/internal/array/casting.d(101): parameter `from` of `__ArrayCast` is cheating `scope`
src/core/internal/array/concatenation.d(41): parameter `arrs` of `_d_arraycatnTX` is cheating `scope`
src/core/internal/backtrace/dwarf.d(153): parameter `dst` of `demangle` is cheating `scope`
src/core/internal/hash.d(213): parameter `arr` of `toUbyte` is cheating `scope`
src/rt/aaA.d(802): parameter `cti` of `unqualify` is cheating `scope`

One of the harder ones is:

src/core/lifetime.d(1881): parameter `source` of `moveImpl` is cheating `scope`

It's really tricky, since it carefully uses @trusted blocks to nudge the @safe inference in the right direction, which now breaks. Maybe someone can help out with that.

@aG0aep6G
Copy link
Contributor

The offending functions were found with something I cobbled together in dmd: dkorpel/dmd@7d1f0c5

Awesome!

One of the harder ones is:

src/core/lifetime.d(1881): parameter `source` of `moveImpl` is cheating `scope`

I think that boils down to moveEmplaceImpl assigning its first parameter to its second parameter. For DIP 1000 it must be other way around. No, really, it's in the spec.

  1. https://dlang.org/spec/function.html#return-ref-parameters: "If the function returns void, and the first parameter is ref or out, then all subsequent return ref parameters are considered as being assigned to the first parameter for lifetime checking.".
  2. https://dlang.org/spec/function.html#param-storage: "[A return parameter] may be returned or copied to the first parameter, but otherwise does not escape from the function."

For internal functions like moveEmplaceImpl we can just reverse the parameters. But move(source, target) might simply not be compatible with DIP 1000.

Copy link
Contributor

@RazvanN7 RazvanN7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Why is this still a draft?

@dkorpel
Copy link
Contributor Author

dkorpel commented May 25, 2021

The remaining errors are with dup and move. dup should be fixed in #3474, move is still a blocker since the @safe unittests move rely on the pure bug. I've made a forum post about it.

This could be merged in the meanwhile, with a follow up PR for move eventually.

@dkorpel dkorpel marked this pull request as ready for review May 25, 2021 11:32
@RazvanN7 RazvanN7 added 72h no objection -> merge The PR will be merged if there are no objections raised. auto-merge and removed 72h no objection -> merge The PR will be merged if there are no objections raised. labels May 25, 2021
}

/// Ditto
TailShared!T atomicLoad(MemoryOrder ms = MemoryOrder.seq, T)(ref shared const T val) pure nothrow @nogc @trusted
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait a second, this is not right. ref should not be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remember ref return not compiling, I think it should be ref return scope. I removed this change, will look at it later today (or in a follow-up PR).

@dlang-bot
Copy link
Contributor

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 + druntime#3480"

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants