Fix #10559 convert infinite range to string

[eslamsamy226]

No description provided.

[dlang-bot]

Thanks for your pull request and interest in making D better, @eslamsamy226! 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#10660"

[ljmf00]

Seems about right but not sure if this is the way we want to fix it. Perhaps needs more comments here.

[pbackus]

One potential benefit of the current behavior (compile error) is that it can help catch mistakes. How likely is it that someone trying to convert an infinite range to a string is doing it on purpose, rather than by mistake?

[eslamsamy226]

The issue consists for two parts, compilation error with to!string and infinite loop with format("%s", ...), the fix now covers both by converting them to string as suggested in the issues, But I'm open to any other suggestions to go about it.

[pbackus]

The way I see it there are two possibilities:

• Have both calls succeed somehow (e.g., by truncating the range).
• Have both calls fail, with a meaningful error message.

I'm not sure which is better. It might be a good idea to try and gather feedback on which option people would find less surprising.

[eslamsamy226]

I'm new to the D community and this actually is my first pull request here, so I'd appreciate if you suggest how to gather feedback, Thanks

[rikkimax]

Short of adding the infinity symbol (I do not recommend this), I don't see how this could be substantially improved.

Printing data, and with that pretty printing, is not an easy task to get right and there are plenty of opinions readily available.

I've enabled CI.

[pbackus]

@eslamsamy226 I've posted a thread on the D forums:

https://forum.dlang.org/post/bnwiqwctksnukdylrzxb@forum.dlang.org

[eslamsamy226]

@rikkimax I think the infinity symbol ∞ or any thing that indicates that this is an infinite sequence will be nice to be provided by the standard library, but this my subjective opinion.

[schveiguy]

I do not like that you are assuming that the user wants a specific behavior, and that the behavior is not configurable.

My recommendation is to add an overload to to!string that accepts a truncateAfter and what to write after the last element.

So to demonstrate my recommendation:

    import std.range : repeat;
    import std.conv : to;

    auto infiniteRange = repeat(42);
    static assert(!__traits(compiles, to!string(infiniteRange))); // doesn't compile as is
    assert(to!string(infiniteRange, truncateAfter: 3) == "[42, 42, 42, ...]");
    assert(to!string(infiniteRange, truncateAfter: 3,
                     truncateText: "and so on") == "[42, 42, 42, and so on]");

For reference, this is similar to CSS text-overflow feature: https://developer.mozilla.org/en-US/docs/Web/CSS/text-overflow

The difference being the trigger is an element width instead of a number of elements.

This also allows nice mechanisms even for non-infinite ranges.

[schveiguy]

For format, I think you need to use take and put in the ellipsis yourself. All the tools are already there.

I would actually be OK with format doing something like the current PR by default, as you have tools to adjust, and it currently already compiles.

[pbackus]

My recommendation is to add an overload to to!string that accepts a truncateAfter and what to write after the last element.

This is poor separation of concerns IMO. If the user wants to truncate the range to a custom length before converting to string, they can use take or something similar. There is no reason for std.conv to get involved in the business of range truncation.

Likewise for "what to write after"—anyone who wants a string formatted in a specific way should use std.format.

[schveiguy]

Yeah, actually this is not as easy as I was thinking, because format doesn't make this easy, and doesn't help for types that do not have a length.

Instead, a type that does the truncation output would be better.

[jmdavis]

In general, the correct thing to do with infinite ranges if a function can't handle them is for the template constraint to reject them, and then the user can use something like take or takeExactly if they want to use only part of the range. The only real problem I see with that is that it's possible to have an infinite range which is a member variable of a struct that has no toString, and if you want to print that out, then getting a compilation for a type that you potentially don't control could be annoying.

Testing this out right now, to!string fails to compile with an infinite range. I think that this is the correct behavior (and it looks like that's exactly what this PR is looking to change).

However, to!string on a struct that contains an infinite range compiles and attempts the conversion, which really should be fixed so that it fails the template constraint like attempting to convert the range directly does.

format!"%s" on both the range and the struct compiles and attempts the conversion, which also really should be fixed. The same goes for writeln and writefln!"%s". So, clearly whatever sharing of implementation exists between std.format and std.conv, they don't behave the same with regards to infinite ranges. I'd have to dig through the code though to see what sharing is going on there. I've always assumed that it was all the same code that did the conversion to string, but maybe it isn't given the current behavior.

So, I would say that we don't want the changes in this PR. The caller can just use take with however elements they want and then pass that to to!string. However, we should look at fixing what happens with to!string and structs that contain infinite ranges as well as how format, writeln, and writefln behave with both infinite ranges and structs that contain infinite ranges. It should fail a template constraint in all cases, just like to!string currently does. And if we want to make it more user-friendly to print out a struct that has a member that's an infinite range but does not implement toString, then maybe we can provide a wrapper type which has a toString which takes care of that. Either way, I don't think that we want the behavior proposed in this PR.

[schveiguy]

My attempt at starting the type. This doesn't work with formatting individual elements, and it doesn't forward other things like lenth, so it will have to be augmented and further tested.

UPDATE: realized we don't need to customize the string type, format will handle it.

struct LimitedRange(R)
{
    private import std.range;
    private enum oneType = is(ElementType!R == string);
    static if(oneType)
        alias Elem = string;
    else
    {
    	private import std.sumtype;
        alias Elem = SumType!(ElementType!R, string);
    }
    private R src;
    private long left;
    private string msg = "...";
    bool empty() => src.empty || left < 0;
    Elem front() {
        static if(oneType)
            static Elem get(Elem val) => val;
        else
            static Elem get(T)(T val) => Elem(val);
            
        if(left)
            return get(src.front);
        else
            return get(msg);
    }
    void popFront() {
        --left;
        if(!src.empty) src.popFront;
    }
}
auto limitedRange(R)(R range, size_t elements = 3, string msg = "...")
{
    return LimitedRange!(R)(range, elements, msg);
}

void main()
{
    auto rng = [1, 2, 3, 4, 5];
    import std.stdio;
    import std.range;
    writeln(rng.limitedRange(4));
    writeln(repeat(42).limitedRange(4, "etc."));
    writeln(repeat("hi").limitedRange(4, "etc."));

    import std.conv;
    assert(rng.limitedRange.to!string == "[1, 2, 3, ...]");
}

[pbackus]

It looks like the consensus on the forum is that trying to convert an infinite range to a string with to!string or formattedWrite should fail at compile time, with an error message that suggests an alternative.

For example:

Can't convert RangeType to a string because it is an infinite range. Consider using std.range.take to limit the range to a finite length.

...where RangeType is the replaced with the name of the range's actual type, obtained using T.stringof.

[0xEAB]

On a more general note, it’s a bit unfortunate when the brainstorming/vetting happens only after a patch has already been implemented and a corresponding PR is submitted.

---

Sure, a thousand files on the issue tracker is quite a lot and it would be a more than unrealistic expectation to expect anyone to look into (or remember) all of them.

In this particular case, however or at least, the issue was only about 6 months old and also got labelled “Good First Issue”. Either way, I’m not sure whether it leaves a “Good First Impression” when people seemingly only bother reading feature requests after someone already spent time implementing them.

This has already bothered me a bit with #10625 that was kinda treated as if it were a novel feature addition rather than the implementation of a feature request from more than 11 years ago – one that, by the way, had only had accumulated positive feedback and no rejection.

[pbackus]

@0xEAB You raise a good point. I think most people follow PRs more closely than issues because the volume of issues is much higher and harder to keep up with. Still, the earlier in the process we can handle things, the better, and we can definitely make more of an effort that we do now.

As of today, I've updated my Github settings to notify me of new issues in this repository, and will try to drop in and give feedback if/when appropriate. Hopefully other members of @dlang/team-phobos will do the same.

[atilaneves]

I think failing to compile is probably the right thing to do.

[LightBender]

@pbackus Sounds like we should close this PR then?

[pbackus]

Closing this PR since we've decided against this solution to issue #10559.