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

Fix #10538 - Cannot swap a std.typecons.Tuple #8864

Merged
merged 2 commits into from
Jan 17, 2025
Merged

Fix #10538 - Cannot swap a std.typecons.Tuple #8864

merged 2 commits into from
Jan 17, 2025

Conversation

ntrel
Copy link
Contributor

@ntrel ntrel commented Dec 18, 2023

Avoid CTFE unsafe cast by swapping elements of tupleof.

@dlang-bot
Copy link
Contributor

dlang-bot commented Dec 18, 2023 •
edited
Loading

Thanks for your pull request and interest in making D better, @ntrel! 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.

⚠️⚠️⚠️ Warnings ⚠️⚠️⚠️

  • In preparation for migrating from Bugzilla to GitHub Issues, the issue reference syntax has changed. Please add the word "Bugzilla" to issue references. For example, Fix Bugzilla Issue 12345 or Fix Bugzilla 12345.(Reminder: the edit needs to be done in the Git commit message, not the GitHub pull request.)

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

@schveiguy
Copy link
Member

this needs effort to make it pass CI. It should not be automerged by accident. Should be reviewed after being fixed.

@thewilsonator
Copy link
Contributor

Please retitle the commit message "Fix bugzilla issue 24285 - ..."

@LightBender
Copy link
Contributor

@ntrel typecons is coming up for Phobos 3. Is there any chance we can get this merged soon so that we don't have to duplicate this fix?

@0xEAB
Copy link
Member

0xEAB commented Jan 15, 2025 •
edited
Loading

Backup copy of the previous commits on the branch: https://github.com/0xEAB/phobos/tree/ntrel/swap-ctfe-backup
See also, https://github.com/0xEAB/phobos/tree/0xEAB/swap-ctfe-backup for an intermediate state of my edits to the patch.

@0xEAB
Copy link
Member

0xEAB commented Jan 15, 2025

AFACIT, apart from the sumtype breakage that is pointed out by CI, this fails at least this test:

phobos/std/algorithm/sorting.d

Line 2145 in 336bed6

static assert(hasSwappableElements!R);

0xEAB
0xEAB reviewed Jan 15, 2025
View reviewed changes
std/algorithm/mutation.d Outdated
Comment on lines 3154 to 3159
/// ditto
void swap(T)(ref T lhs, ref T rhs)
if (is(typeof(lhs.proxySwap(rhs))))
{
lhs.proxySwap(rhs);
}
Copy link
Member

Choose a reason for hiding this comment

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

@ntrel
What was this overload supposed to solve?

Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately, I haven’t been able to figure that out myself.
All I can tell is that it breaks this code:

phobos/std/algorithm/sorting.d

Line 2145 in 336bed6

static assert(hasSwappableElements!R);

0xEAB
0xEAB reviewed Jan 15, 2025
View reviewed changes
std/typecons.d
Comment on lines +983 to +989
/*
This optimization caused compilation failures with no error message available:

> Error: unknown, please file report on issues.dlang.org
> std/sumtype.d(1262): Error: template instance `std.sumtype.SumType!(Flag, Tuple!(This*))` error instantiating
*/
version (none)
Copy link
Member

Choose a reason for hiding this comment

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

This was awful to debug. I’d love if someone could track this down and open a DMD bug report.

@0xEAB 0xEAB changed the title Fix Issue 24285 - Cannot swap a std.typecons.Tuple Fix #10538 - Cannot swap a std.typecons.Tuple Jan 16, 2025
@0xEAB 0xEAB requested a review from thewilsonator January 16, 2025 23:57
@thewilsonator thewilsonator merged commit 5f39350 into dlang:master Jan 17, 2025
8 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0xEAB 0xEAB 0xEAB left review comments

@thewilsonator thewilsonator thewilsonator approved these changes

@andralex andralex Awaiting requested review from andralex

@JackStouffer JackStouffer Awaiting requested review from JackStouffer

@wilzbach wilzbach Awaiting requested review from wilzbach

@PetarKirov PetarKirov Awaiting requested review from PetarKirov PetarKirov is a code owner

Assignees
No one assigned
Labels
Severity:Enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@ntrel @dlang-bot @schveiguy @thewilsonator @LightBender @0xEAB