Eliminate redundant allocation in to!string for types with toString

[deveshidwivedi]

This PR fixes the issue where to!string creates an unnecessary allocation when used on types that already have a toString() method. By adding a specialized implementation for these types, we directly use the string returned by toString() instead of creating a new allocation. This improves performance and reduces memory usage in common type conversion scenarios.

[dlang-bot]

Thanks for your pull request and interest in making D better, @deveshidwivedi! 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
✓	24739	normal	to!string always allocates a new string

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

[0xEAB]

Please note that the commit message is wrong. This is not Bugzilla issue no.10560, but issue #10560 here on GitHub.

[0xEAB]

Please add a unittest similar to the assertion outlined in the bug report.

[deveshidwivedi]

Please note that the commit message is wrong. This is not Bugzilla issue no.10560, but issue #10560 here on GitHub.

Sorry for the confusion, I'll update it

[pbackus]

Error: return value value.toString() of type string does not match return type char[], and cannot be implicitly converted

Looks like we need to test whether the return type of toString matches the desired return type of to.

[deveshidwivedi]

Error: return value value.toString() of type string does not match return type char[], and cannot be implicitly converted

Looks like we need to test whether the return type of toString matches the desired return type of to.

I'll test this and update as needed

[pbackus]

This is the test that's failing in unit-threaded:

https://github.com/atilaneves/unit-threaded/blob/f06b64e8f60c2a109d6195c46299548564257e6f/tests/unit_threaded/ut/should.d#L533-L544

Looks like the problem is that we're attempting to call toString on a null class reference, and that's causing the program to crash.

To fix it, if value is a null class reference, we should return null instead of calling value.toString().

[deveshidwivedi]

I made three key changes to address the failing test cases:

• returning null if value is a null class reference
• added special handling for void and int return types

the failing test cases:

std/variant.d(502): Error: expression (*zis).opDispatch() is void and has no value target = (zis).toString(); ^ std/variant.d(731): Error: template instance std.variant.VariantN!32LU.VariantN.handler!(SWithNoLength) error instantiating fptr = &handler!(T); ^ std/variant.d(656): instantiated from here: opAssign!(SWithNoLength) opAssign(value); ^ std/variant.d(3107): instantiated from here: __ctor!(SWithNoLength) Variant v = sWithNoLength; ^ make: *** [Makefile:406: generated/linux/release/64/unittest/std/variant.o] Error 1

• added support for struct types with alias this the failing test case:

@safe unittest
{
    // Conversion representing class object with string
    class A
    {
        override string toString() @safe const { return "an A"; }
    }
    A a;
    assert(to!string(a) == "null");
    a = new A;
    assert(to!string(a) == "an A");

    // https://issues.dlang.org/show_bug.cgi?id=7660
    class C { override string toString() @safe const { return "C"; } }
    struct S { C c; alias c this; }
    S s; s.c = new C();
    assert(to!string(s) == "C");
}

Please let me know if any of my changes have issues or could be improved.

[pbackus]

Current failing test is here:

https://github.com/dlang-community/libdparse/blob/master/src/dparse/lexer.d#L2856

I'll be honest, this one is complex enough that it's not obvious to me what the problem is.

[pbackus]

I'm reluctant to approve this for three reasons:

1. The added code is very complex. I can't tell whether or not it's correct just by looking at it.
2. Previous revisions of this PR have caused unexpected regressions in existing code, which means there's a higher-than-normal chance that the current revision will too.
3. Proper test coverage would make me less concerned about both (1) and (2). However, only one new unittest has been added.

[pbackus]

As I suspected, there are several cases where this PR does not preserve the behavior of the original implementation, and would introduce regressions.

The current implementation relies on std.format.write.formatValue to convert struct and class values to strings. The actual implementation is in std.format.internal.write.formatValueImpl. Specifically, it is in these two overloads:

• formatValueImpl for class values
• formatValueImpl for struct and union values

In order to avoid regressions, any new code we introduce for calling toString needs to match the behavior of these functions exactly.

For structs, this is not terribly difficult, but for classes, it would require a large amount of code duplication. So for now, it may be a better idea to limit this fix to structs, and leave classes the way they are.

[deveshidwivedi]

Thank you @pbackus, updated to align with formatValueImpl behavior

[pbackus]

Edited to fix a mistake in one of my suggestions (missing )). Sorry for the trouble!

[pbackus]

Assertion failed: j == 61440 && k == 3840, file runnable\exe1.c, line 1449

Failure is an ~~ImportC~~ DMD backent test and appears to also be affecting other Phobos PRs, so it is almost certainly not related to the changes in this PR.

[pbackus]

The test failure appears to have been introduced in https://github.com/dlang/dmd/pull/21010.

[dkorpel]

That PR only touches dead code (the -arm switch isn't used yet), so I can't imagine it introducing the failure.

[pbackus]

Well, the test itself was introduced in commit dlang/dmd@4261907, and the earliest instance I could find of that test failing was in the linked PR, which was merged as commit dlang/dmd@0312e82. So either the test itself has been (nondeterministically) broken since its introduction, or the failure was introduced in some commit between those two.

Looking through git log --stat for commits that touch non-ARM backend code turns up the following suspects:

• dlang/dmd#20996
• dlang/dmd#20988
• dlang/dmd#20987

[dkorpel]

The failure is nondeterministic, since several PRs have been green since then. I also only see it on Windows x64 with LDC 1.40 as host compiler. That suggests it's more than a simple logic bug in dmd, but something much more devious like undefined behavior.

[0xEAB]

@dkorpel https://github.com/dlang/dmd/issues/20985

[LightBender]

This PR has had a broken CI for 6+ months, please reopen if you plan to continue working on this.