Tuple toString #10645
Merged
Tuple toString #10645
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The tests are still working. Is it safe to assume that the workaround would've only been merged if there is a test-case checking it?
For shared classes, it prints the mangled name, for struct pointers it will print the adress. This version of Tuple.toString won't work anymore in @safe code in general if the tuple contains anything descended from Object. I think this is correct even if a breaking change, since dealing with this issue should be the responsibility of the caller.
|
Thanks for your pull request and interest in making D better, @Inkrementator! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
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 referencesYour 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 locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub run digger -- build "master + phobos#10645" |
thewilsonator
approved these changes
Feb 20, 2025
|
does this fix #9811 ? |
|
No, it doesn't. It would need to remove |
|
5fa226e actually put the surprising behaviour in a unittest (as documentation), before I understood why it happened |
It actually already partly fixes it. If |
Inkrementator
added a commit
to Inkrementator/phobos
that referenced
this pull request
Feb 21, 2025
Now, member structs won't become const due to transitiveness and the code in `std.format` can choose (erroneously) non-cost toString() functions of member structs. An expert has to decide whether this is worth the breaking change. Now, code that uses `const` tuples and calls toString directly on them will break. Alternatively, we could use [Template this parameters](https://dlang.org/spec/template.html#template_this_parameter). That way, we get a template parameter with the real type of this. Then we can cast away constness of `this` when we now for certain that it isn't, since `is(T != typeof(this))` (where T is the template this parameter). Of course, this implies making toString `@trusted` too. This might also lead to unforeseen bugs when `const` is cast away but the member objects are actually const. I'm not sure how this works. Fwiw, currently std.format and `std.conv: to` already intercepts const tuples, thus it at least won't call toString directly. The breaking change will only effect code that calls toString on const tuples directly. That's why I have added non-prescritive tests. If something in std.format changes, they'll be alerted and can then decide whether to change the tests or whether this module also needs work, in case this would lead a bigger breaking change. Followup to dlang#10645.
thewilsonator
pushed a commit
that referenced
this pull request
Feb 22, 2025
Now, member structs won't become const due to transitiveness and the code in `std.format` can choose (erroneously) non-cost toString() functions of member structs. An expert has to decide whether this is worth the breaking change. Now, code that uses `const` tuples and calls toString directly on them will break. Alternatively, we could use [Template this parameters](https://dlang.org/spec/template.html#template_this_parameter). That way, we get a template parameter with the real type of this. Then we can cast away constness of `this` when we now for certain that it isn't, since `is(T != typeof(this))` (where T is the template this parameter). Of course, this implies making toString `@trusted` too. This might also lead to unforeseen bugs when `const` is cast away but the member objects are actually const. I'm not sure how this works. Fwiw, currently std.format and `std.conv: to` already intercepts const tuples, thus it at least won't call toString directly. The breaking change will only effect code that calls toString on const tuples directly. That's why I have added non-prescritive tests. If something in std.format changes, they'll be alerted and can then decide whether to change the tests or whether this module also needs work, in case this would lead a bigger breaking change. Followup to #10645.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I'm not entirely sure why these workaround have been added, but I suspect that #6207 fixed this in
std.format. The unittests still work.This is a breaking change. See line 1787 (in the new version), If a class is null, the Tuple.toString will now print null instead of the mangled typename (because this is what std.format does)