Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Issue 23101 - [std.sumtype] canMatch does not account ref #8457

Merged
merged 2 commits into from
May 12, 2022
Merged

Issue 23101 - [std.sumtype] canMatch does not account ref #8457

merged 2 commits into from
May 12, 2022

Conversation

iK4tsu
Copy link
Contributor

@iK4tsu iK4tsu commented May 10, 2022

We can safely assume all Ts values can be tested with ref because get always returns by ref.

@iK4tsu
fix(sumtype): template canMatch does not account for ref when matching
6069c44 
The template `canMatch` does not account for `ref`.
The template `valueTypes` stores all types of the member values and uses SumTypes's `get` function, which returns a `ref`.
However, ref does not persist and the type is not sent to `canMatch` as a `ref`.
Because of this, when matching, `canMatch` will fail as it will test for a copy, and returning a reference of that value results in escaping it.

Fix Issue 23101

Signed-off-by: João Lourenço <jlourenco5691@gmail.com>
@iK4tsu iK4tsu requested a review from pbackus as a code owner May 10, 2022 19:30
@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @iK4tsu! 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
✓ 23101 enhancement [std.sumtype] canMatch does not account ref

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

ljmf00
ljmf00 approved these changes May 10, 2022
View reviewed changes
Copy link
Member

@ljmf00 ljmf00 left a comment

Choose a reason for hiding this comment

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

Seems good. CC @pbackus

pbackus
pbackus reviewed May 11, 2022
View reviewed changes
Copy link
Contributor

@pbackus pbackus left a comment

Choose a reason for hiding this comment

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

Test could be a little cleaner, but looks good overall. Thanks for catching this!

std/sumtype.d Outdated Show resolved Hide resolved
std/sumtype.d Outdated Show resolved Hide resolved
@iK4tsu
Copy link
Contributor Author

iK4tsu commented May 11, 2022

I also used return ref because from what I understand ATM, it is the correct way of returning a reference and ensuring it doesn't outlive the original instance in dip1000.

@iK4tsu iK4tsu force-pushed the fix-sumtype-canMatch branch from d7fac1c to 5f67888 Compare May 11, 2022 18:49
@iK4tsu
test(sumtype): add unittests when returning the matched value's refer…
d39c4ea 
…ence

Signed-off-by: João Lourenço <jlourenco5691@gmail.com>
@iK4tsu iK4tsu force-pushed the fix-sumtype-canMatch branch from 5f67888 to d39c4ea Compare May 11, 2022 20:24
@dlang-bot dlang-bot merged commit a37fd5f into dlang:master May 12, 2022
pbackus added a commit to pbackus/sumtype that referenced this pull request May 12, 2022
@pbackus
Allow 'return ref' handlers that return pointers
47a0519 
Previously, canMatch would reject these handlers because calling them
would escape a reference to 'args' from the lambda
'(Ts args) => handler(args)'. Making 'args' a ref parameter allows the
compiler to instead infer it as 'return ref' in this situation.

Dlang issue: https://issues.dlang.org/show_bug.cgi?id=23101
Phobos PR: dlang/phobos#8457
@pbackus pbackus mentioned this pull request May 12, 2022
Merged
dkorpel
dkorpel reviewed May 13, 2022
View reviewed changes
std/sumtype.d
// issue: https://issues.dlang.org/show_bug.cgi?id=23101
@safe unittest
{
static assert(!__traits(compiles, () {
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the expected compile error here?

Copy link
Contributor

@dkorpel dkorpel May 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Okay, it's:

Error: returning match(st) escapes a reference to local variable st

Which is also an error in @system functions

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

@dkorpel dkorpel dkorpel left review comments

@pbackus pbackus pbackus approved these changes

@ljmf00 ljmf00 ljmf00 approved these changes

Assignees
No one assigned
Labels
Merge:auto-merge Severity:Enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@iK4tsu @dlang-bot @ljmf00 @pbackus @dkorpel @atilaneves