-
-
Notifications
You must be signed in to change notification settings - Fork 739
Improve error message for to!string when used on infinite ranges #10744
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.
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
Improve error message for to!string when used on infinite ranges #10744
Conversation
Thanks for your pull request and interest in making D better, @DeterminedSage! 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#10744" |
Resolve DScanner warning by using the unused parameter in the new infinite range toImpl overload
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not convinced that this is a good idea, since it's not something that we can do in the general case. In general, the template constraint is how this kind of information needs to be messaged, though it's certainly true that it's worse than normal with std.conv.to
, because it takes almost anything.
In general, having a template constraint pass but then have compilation fail is something that we should not be doing. It does not have the same effect as the template constraint failing, since you can have a template constraint fail and then overload it with another template or template function which succeeds, but if you have a template constraint which succeeds, and then the template fails to compile internally, then you already have a match, and so you can't overload it with a match that works.
That being said, in this particular case, to
already doesn't have a template constraint at its top level, which makes overloading it externally impossible anyway, and it has so many overloads that figuring out why a particular call is failing is far more difficult than it arguably should be. So, this does arguably make the situation better without causing additional problems. But this is not an approach that we can use with most templates.
Either way, the message needs work as I indicated, and the overload should have a test.
std/conv.d
Outdated
if (isInputRange!S && isInfinite!S && isExactSomeString!T) | ||
{ | ||
auto _ = value; | ||
static assert(0, "Cannot convert infinite range to string. Use `.take(n)` or `.array` to make it finite."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
array
requires a finite range (since like to
, it's converting the entire range). If you want another suggestion, then takeExactly
would be appropriate.
Also, you should say std.range.take
, not .take
. Having a period in front of the symbol indicates that it's supposed to be taken from the outer scope, which makes no sense here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, there should be a test that tests that using to
on an infinite range doesn't compile. Arguably, there already should have been one, but if you're specifically adding an overload like this, then it needs one even more.
Improve the error message to: static assert(0, "Cannot convert infinite range to string. Use `std.range.take` or `std.range.takeExactly` to make it finite."); Also add a unittest to verify that compilation fails when an infinite range is passed for string conversion.
Format the changes made as per D Style
Format changes as per D Style
Format changes as per D Style
@jmdavis I have made the proposed changes . Please check . |
Error message Formatting improved Co-authored-by: Paul Backus <[email protected]>
@pbackus updated the changes as recommended . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good aside from the extraneous variable.
std/conv.d
Outdated
private T toImpl(T, S)(S value) | ||
if (isInputRange!S && isInfinite!S && isExactSomeString!T) | ||
{ | ||
auto _ = value; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this here? It should not be necessary to have anything other than the static assertion within the function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jmdavis It was failing one of the tests without the extra variable ,
std/conv.d(546:26)[warn]: Parameter value is never used .
Test Link : https://buildkite.com/dlang/phobos/builds/9649#019611b0-47b6-48fd-9641-688a83bf2a1f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, we're running dscanner with the CI. Great. And we have yet another place where dscanner complains when it shouldn't.
Well, that's stupid, but I guess that we'll have to deal with it for now. Please put a comment on the useless variable to indicate that it's there to make dscanner in the style check in the CI happy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does dscanner still complain if the parameter has no name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Herringway That should work in my experience.
Add comment regarding the extra variable
@jmdavis , appropriate comment has been included in the overload regarding the extra variable, please check . |
It looks good, but I'd suggest that you try Herringway's suggestion of removing the parameter name, since if that works, it would be better. If it doesn't, then we can go with this. |
Remove extra parameter name
@jmdavis @Herringway Correct me if I’m wrong, but I believe the change you both suggested is this:
This version does successfully pass all the unittest blocks, but it unfortunately Test Link : https://github.com/dlang/phobos/actions/runs/14461008664/job/40553498967 |
I'd be very surprised if the failure had anything to do with these changes. It's likely one of the periodic failures that we unfortunately get with the CI. Just push your changes again (which may require amending without any actual changes so that you get a different hash and then force pushing it). The CI will then run again, and it'll probably pass. Either way, the style check passed, so the problem there seems to be fixed with these changes. |
Re-trigger previous change
@jmdavis @Herringway The changes seem to be working fine now after the re-test. |
…ng#10744) Partial Fix: dlang#10559 Improve the error message to: static assert(0, "Cannot convert infinite range to string. Use `std.range.take` or `std.range.takeExactly` to make it finite."); Also add a unittest to verify that compilation fails when an infinite range is passed for string conversion. Co-authored-by: Paul Backus <[email protected]>
Partial Fix: #10559
Summary
This PR improves compile-time diagnostics when attempting to convert an infinite input range to a string using to!string.
Root Cause
Currently, the following code fails with a long and unhelpful template constraint error:
Error :
The reason is that Repeat!int is an infinite range and does not meet any of the constraints for string conversion — but the compiler doesn’t provide a meaningful message.
Fix
This patch adds a specialized overload of toImpl in std.conv to detect the pattern of:
And explicitly raises a meaningful compile-time error:
Example Reproducer
Now output :
###Notes
Fix added in std.conv via an early overload of toImpl.
Helps developers identify misuse with a clear and actionable message.
Verified manually via unittests on custom Phobos build