-
-
Notifications
You must be signed in to change notification settings - Fork 700
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
Better std.algorithm.comparison.equal error messages #8745
Conversation
Thanks for your pull request, @burner! 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#8745" |
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 don't think this needs a changelog, but w/e.
@CyberShadow with this change building Dustmite now fails with the following error message:
Any ideas? @burner you should be able to reproduce this locally like so: cd $DLANG
git clone https://github.com/dlang/tools
make -C tools -f posix.mak RELEASE=1 --jobs=4 |
Yeah, it's picking up the wrong overload now. There is an I don't think this is going to work :( Didn't we have proposals to add explanations to constraints so that we can have both cakes? |
static assert(allSatisfy!(isInputRange, Ranges), "All passed ranges " | ||
, "must by InputRanges"); | ||
static assert(!allSatisfy!(isInfinite, Ranges), "No passed range " | ||
, "must by an InfiniteRange"); |
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.
s/by/be/ BTW
Yes it was mine. I presume it must have been a DIP because it changed grammar, I remember I had trouble trying to implement it which I think was the reason it didn't go anywhere, will have a look for it tomorrow. |
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.
Any ideas?
Never mind, I figured it out. This is the culprit:
https://github.com/dlang/tools/blob/v2.104.0-beta.1/DustMite/dustmite.d#L1489-L1500
Essentially, some of the reasons for having template constraints in the language are:
- To document the requirements for the template parameters
- To fail compilation early if the one or more given template arguments doesn't meet the requirements
- To help with overload resolution by filtering out the list of candidates
- To make the documentation and error messages more difficult to parse for muggles 😄
Dustmite is relying on 3. in order to overload the equal
function, which is a valid design and not an uncommon idiom. By transforming the template constraints to static assert
s, it looks like the compiler can no longer discard std.algorithm.comparison.equal
as a valid candidate during overload resolution and instead starts analysing the function, and then hits the static assert
which aborts the whole compilation.
This is a breaking change and until it is resolved we can't merge it.
@burner My guess is that you want to solve 4. and you're not too attached to your current solution. In that case, here are some possible alternatives:
- Add https://github.com/atilaneves/concepts (or a similar technique) to Phobos and use it here
- Change the language, so during overload resolution
static assert
s act sort of like SFINAE does in C++ - they don't abort the compilation, unless none of the overload candidates match
In the longer term, considering that template constraints haven't received any improvement in the language for at least the past 10 years (AFAIK) it maybe useful to review C++20 concepts and see if there's something we can adopt to improve the status quo in D.
Why can't we just do if (/// More than one range is required
rs.length > 1 &&
/// All passed ranges must be input ranges
allSatisfy!(isInputRange, Ranges) &&
/// None of the passed ranges may be an `InfiniteRange`
!allSatisfy!(isInfinite, Ranges) &&
/// The first and second range's elements must be comparable with `binaryFun!pred`
is(typeof(binaryFun!pred(rs[0].front, rs[1].front))) &&
/// All successive range's elements must be comparable with`binaryFun!pred`
(rs.length == 2 || is(typeof(equal!pred(rs[1 .. $])) == bool))) It is syntactically unambiguous, doesn't require any language changes, and doesn't need a DIP (because it doesn't change grammar and only affects compiler output). I.e. just print any doc node attached to the expression that failed. |
That's the point of these PR's Closing this for now |
thank you for all the review work |
No description provided.