www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - All asserts need to have messages attached! Explicit as possible!

reply FoxyBrown <Foxy Brown.IPT> writes:
I came across some error in heap sort. Was erroring out on the 
wrong. A few lines below the assert existed but no error message 
associated, why is it so hard to not write a few extra EXTREMELY 
helpful error messages?

assert(isHeap(r), "This is an ERROR AT THIS 
LOCATION"~__FILE__~"("~__LINE__~")");

etc?

It should be mandatory that all asserts, throws, etc provide 
correct information about not only the point of the error but 
also the location and what caused it. These things are not 
irrelevant but affect all those that use it... imagine the real 
human man hours that could be saved if such things were done.

It would be easy to find all the bad asserts?
Jul 07
next sibling parent reply Vladimir Panteleev <thecybershadow.lists gmail.com> writes:
On Saturday, 8 July 2017 at 00:55:46 UTC, FoxyBrown wrote:
 It would be easy to find all the bad asserts?
Does Dscanner have a rule to detect assert and enforce calls without a message? We should have it enabled for Phobos.
Jul 07
parent reply Seb <seb wilzba.ch> writes:
On Saturday, 8 July 2017 at 01:01:38 UTC, Vladimir Panteleev 
wrote:
 On Saturday, 8 July 2017 at 00:55:46 UTC, FoxyBrown wrote:
 It would be easy to find all the bad asserts?
Does Dscanner have a rule to detect assert and enforce calls without a message? We should have it enabled for Phobos.
No, but now there's: https://github.com/dlang-community/D-Scanner/pull/495 I can always add this to the phobos branch at DScanner if you want to test this immediately.
Jul 07
parent Seb <seb wilzba.ch> writes:
On Saturday, 8 July 2017 at 01:31:54 UTC, Seb wrote:
 On Saturday, 8 July 2017 at 01:01:38 UTC, Vladimir Panteleev 
 wrote:
 On Saturday, 8 July 2017 at 00:55:46 UTC, FoxyBrown wrote:
 It would be easy to find all the bad asserts?
Does Dscanner have a rule to detect assert and enforce calls without a message? We should have it enabled for Phobos.
No, but now there's: https://github.com/dlang-community/D-Scanner/pull/495 I can always add this to the phobos branch at DScanner if you want to test this immediately.
We can improve the status quo: https://github.com/dlang/phobos/pull/5578 Once this is integrated, everyone can help out and improve Phobos by just removing a single module from the blacklist - it's a PR away.
Jul 08
prev sibling next sibling parent Timon Gehr <timon.gehr gmx.ch> writes:
On 08.07.2017 02:55, FoxyBrown wrote:
 I came across some error in heap sort. Was erroring out on the wrong. A 
 few lines below the assert existed but no error message associated, why 
 is it so hard to not write a few extra EXTREMELY helpful error messages?
 
 assert(isHeap(r), "This is an ERROR AT THIS 
 LOCATION"~__FILE__~"("~__LINE__~")");
 
 etc?
 
 It should be mandatory that all asserts, throws, etc provide correct 
 information about not only the point of the error but also the location 
 and what caused it. These things are not irrelevant but affect all those 
 that use it... imagine the real human man hours that could be saved if 
 such things were done.
 
 It would be easy to find all the bad asserts?
It think the main issue is that the assertion failure had the wrong location information. This is not supposed to happen. Do you have a small example that demonstrates the issue? I think that as long as the location is correct, the rest can be reconstructed without wasting a lot of time. (Of course, this is no excuse for Phobos not providing a more informative error message.)
Jul 07
prev sibling next sibling parent reply Moritz Maxeiner <moritz ucworks.org> writes:
On Saturday, 8 July 2017 at 00:55:46 UTC, FoxyBrown wrote:
 [...]
 It should be mandatory that all asserts, throws, etc provide 
 correct information about not only the point of the error but 
 also the location and what caused it. [...]
Error messages are sensible when they provide context (e.g. for complex conditions). In my experience, though, most asserts exist to cover fairly obvious cases, where an additional message would just add noise: --- struct SomeRange { bool empty() { ... } void popFront() { assert (!empty); ... } } ---
Jul 07
parent reply Jonathan M Davis via Digitalmars-d <digitalmars-d puremagic.com> writes:
On Saturday, July 8, 2017 1:51:34 AM MDT Moritz Maxeiner via Digitalmars-d 
wrote:
 On Saturday, 8 July 2017 at 00:55:46 UTC, FoxyBrown wrote:
 [...]
 It should be mandatory that all asserts, throws, etc provide
 correct information about not only the point of the error but
 also the location and what caused it. [...]
Error messages are sensible when they provide context (e.g. for complex conditions). In my experience, though, most asserts exist to cover fairly obvious cases, where an additional message would just add noise: --- struct SomeRange { bool empty() { ... } void popFront() { assert (!empty); ... } } ---
I tend to agree - in many cases, the error message is completely redundant - but a number of folks seem to think that you should never have an assertion without a message, and it tends to get complained about if you have an assertion without a message if it's not in a unit test, so I'm a bit surprised that the OP ran into one in Phobos. - Jonathan M Davis
Jul 07
parent reply Jacob Carlborg <doob me.com> writes:
On 2017-07-08 04:13, Jonathan M Davis via Digitalmars-d wrote:

 I tend to agree - in many cases, the error message is completely redundant -
 but a number of folks seem to think that you should never have an assertion
 without a message, and it tends to get complained about if you have an
 assertion without a message if it's not in a unit test, so I'm a bit
 surprised that the OP ran into one in Phobos.
I think it's extremely useful to have, especially in a code base you're not familiar with or is complicated. It has happened to me quite often that I hit asserts in DMD when I develop on that code base. It would help me a lot if all those asserts had descriptive messages. -- /Jacob Carlborg
Jul 10
parent reply Jonathan M Davis via Digitalmars-d <digitalmars-d puremagic.com> writes:
On Monday, July 10, 2017 1:28:44 PM MDT Jacob Carlborg via Digitalmars-d 
wrote:
 On 2017-07-08 04:13, Jonathan M Davis via Digitalmars-d wrote:
 I tend to agree - in many cases, the error message is completely
 redundant - but a number of folks seem to think that you should never
 have an assertion without a message, and it tends to get complained
 about if you have an assertion without a message if it's not in a unit
 test, so I'm a bit surprised that the OP ran into one in Phobos.
I think it's extremely useful to have, especially in a code base you're not familiar with or is complicated. It has happened to me quite often that I hit asserts in DMD when I develop on that code base. It would help me a lot if all those asserts had descriptive messages.
That only helps if there's something to describe. Often, it's something like assert(!range.empty); in which case, there really isn't much to say. I totally agree that assertion messages can be helpful when there's something to explain, but there often really isn't, and all a message is going to do is repeat the condition being checked in English, which is just pointless text. - Jonathan M Davis
Jul 10
parent reply Jacob Carlborg <doob me.com> writes:
On 2017-07-10 15:45, Jonathan M Davis via Digitalmars-d wrote:

 That only helps if there's something to describe. Often, it's something like
 
 assert(!range.empty);
 
 in which case, there really isn't much to say. I totally agree that
 assertion messages can be helpful when there's something to explain, but
 there often really isn't, and all a message is going to do is repeat the
 condition being checked in English, which is just pointless text.
Well, in this case it could include why the range cannot be empty. But in the case of ranges it's pretty clear, if you know how the whole range concept works. -- /Jacob Carlborg
Jul 10
parent Moritz Maxeiner <moritz ucworks.org> writes:
On Monday, 10 July 2017 at 13:52:22 UTC, Jacob Carlborg wrote:
 On 2017-07-10 15:45, Jonathan M Davis via Digitalmars-d wrote:

 [...]
Well, in this case it could include why the range cannot be empty.
If either the name of the function was non-descriptive or the function's documentation string (and assert messages are also just documentation) doesn't make it obvious why, sure.
 But in the case of ranges it's pretty clear, if you know how 
 the whole range concept works.
Even if you don't know about ranges, the function's name (pop*, remove*, extract*, etc.) virtually always implies a state mutating operation removing something.
Jul 10
prev sibling parent reply Jonathan M Davis via Digitalmars-d <digitalmars-d puremagic.com> writes:
On Saturday, July 8, 2017 12:55:46 AM MDT FoxyBrown via Digitalmars-d wrote:
 I came across some error in heap sort. Was erroring out on the
 wrong. A few lines below the assert existed but no error message
 associated, why is it so hard to not write a few extra EXTREMELY
 helpful error messages?

 assert(isHeap(r), "This is an ERROR AT THIS
 LOCATION"~__FILE__~"("~__LINE__~")");

 etc?

 It should be mandatory that all asserts, throws, etc provide
 correct information about not only the point of the error but
 also the location and what caused it. These things are not
 irrelevant but affect all those that use it... imagine the real
 human man hours that could be saved if such things were done.

 It would be easy to find all the bad asserts?
AssertError already provides the location of the assertion, and it uses __FILE__ and __LINE__ to do it (_all_ Exception types do that unless the person who wrote the class screwed up the constructor), so adding them to the message it pointless and redundant. If the assertion failure is printing out the wrong line, then it's likely either because you're looking at the wrong version of the code or because a string mixin is screwing with the line numbers (which IMHO, shouldn't happen, but it at least used to be a problem). In addition, the AssertError should be giving you a stack trace, which _should_ have the file and line numbers in it of every call in the stack (though stupidly, the line numbers don't always work, depending on your OS). So, you _should_ have had that information by the simple fact that an AssertError was thrown, and unless the assertion itself needed additional explanation beyond the condition that failed, then a message wouldn't have helped anyway. So, if the file and line number was wrong, the question is why, and that should be fixed. And we really need to figure out how to make it so that the issue of not having line numbers with stack traces goes away and stays away. The fact that it's been a problem is ridiculous, because file and line numvbers in stack traces are critical for debugging. - Jonathan M Davis
Jul 07
parent FoxyBrown <Foxy Brown.IPT> writes:
On Saturday, 8 July 2017 at 02:23:53 UTC, Jonathan M Davis wrote:
 On Saturday, July 8, 2017 12:55:46 AM MDT FoxyBrown via 
 Digitalmars-d wrote:
 I came across some error in heap sort. Was erroring out on the 
 wrong. A few lines below the assert existed but no error 
 message associated, why is it so hard to not write a few extra 
 EXTREMELY helpful error messages?

 assert(isHeap(r), "This is an ERROR AT THIS
 LOCATION"~__FILE__~"("~__LINE__~")");

 etc?

 It should be mandatory that all asserts, throws, etc provide 
 correct information about not only the point of the error but 
 also the location and what caused it. These things are not 
 irrelevant but affect all those that use it... imagine the 
 real human man hours that could be saved if such things were 
 done.

 It would be easy to find all the bad asserts?
AssertError already provides the location of the assertion, and it uses __FILE__ and __LINE__ to do it (_all_ Exception types do that unless the person who wrote the class screwed up the constructor), so adding them to the message it pointless and redundant. If the assertion failure is printing out the wrong line, then it's likely either because you're looking at the wrong version of the code or because a string mixin is screwing with the line numbers (which IMHO, shouldn't happen, but it at least used to be a problem). In addition, the AssertError should be giving you a stack trace, which _should_ have the file and line numbers in it of every call in the stack (though stupidly, the line numbers don't always work, depending on your OS). So, you _should_ have had that information by the simple fact that an AssertError was thrown, and unless the assertion itself needed additional explanation beyond the condition that failed, then a message wouldn't have helped anyway. So, if the file and line number was wrong, the question is why, and that should be fixed. And we really need to figure out how to make it so that the issue of not having line numbers with stack traces goes away and stays away. The fact that it's been a problem is ridiculous, because file and line numvbers in stack traces are critical for debugging. - Jonathan M Davis
But you miss the complete point, You focused on what D does rather than what it doesn't, which is what lead me to the problem in the first place. You are seeing the glass half full, rather than half empty, my friend. Shall I explain it so it makes sense? I was writing a natural sorting routine so I could get some string array sorted. This is because D is missing the capabilities to do a natural sort correctly. It does not compare the the integer in string a with the matching integer in string B correctly. If they happen to be a different length then it assumes the shorter one was smaller. At least, when I tried it, that is what I remember being the problem, or some other weird behavior, but it was not correct. Therefore, I decided to write the natural sorting routine to fix that(This is because I am a person that wants to drink a full glass of water rather than an empty glass). To implement the algorithm is rather simple in some ways and complicated in others. string[] naturalSort(string[] arr) /*pure safe*/ { alias myComp = (x, y) { return ComplexPart(x,y); }; auto x = arr.sort!(myComp).release; return x; } bool ComplexPart(string x, string y) { auto ml = cast(int)(min(x.length, y.length)-1); auto ofsX = -1; auto ofsY = -1; auto numX_found = -1; auto numY_found = -1; for(int i = 0; i <= ml; i++) { // If one string is left slice of the other, it wins if (i >= ml || ofsX >= ml || ofsY >= ml) { if (x.length < y.length) return true; return false; } ofsX++; ofsY++; // If characters are not a digit, use default comparer if (!isDigit(x[ofsX]) || !isDigit(y[ofsY])) { if (x[ofsX] != y[ofsY]) return x[ofsX] < y[ofsY]; continue; } // We now know that x and y are identical up to the digit sequence. Extract these sequences then compare, if identical *in value*(000 = 00000, etc) we continue the compare, else we compare the numbers and use that to specify the comparision quality while (ofsX < ml && isDigit(x[ofsX])) { ofsX++; }; while (ofsY < ml && isDigit(y[ofsY])) { ofsY++; }; auto numX = x[i..ofsX]; auto numY = y[i..ofsY]; auto res = compareStringNum(numX, numY); if (res != 0) return (res != 1); } return x > y; }; } (I've taken a brief time away from my life to explain the problem in a bit more detail that I normally would. My real code manually inlines the ComplexPart function. Actually, I came up with that function just now and specially for you.) As you can see, there are only really two logical parts to the naturalSort. In fact, one can say there is only one part and that is `arr.sort`! Which is a function that sorts the string array list `arr`. The lines of code of simple importance is: auto x = arr.sort!(myComp).release; return x; `arr.sort` takes a special kind of string comparer (binary) function that informs us if a string(x in this case) is greater or lesser than another string(y). That is, MyComp is a total order on the array of strings. It is irrelevant, only for sake of argument since we both have lives to attend to outside of this `playground` activity we have taken up as adults, because what happens when I actually run my code(Actually, not right now, as that would be redundant, but the first time I did it(like all first times, it never goes quite right)) is quite a curious thing. Very intriguing. Visual D provides a pretty generic and ugly assert message(Not the kind you want to take home to your mother). [***ATTENTION: What follows is a textural reduction for visualizing what really happened, instead of making a movie, which would contain about 10B(at least) different pieces of information to show what exactly happened(which is now impossible due to causality), you must pretend you are watching that video... better yet, to really get in to it pretend you are having the experience... then eventually you will experience it for real(if you haven't already)***. For clarity, I will mark the action points with ******* and they are sort of like annotations and meta data to understand what I'm talking about with more clarity. You must use your brain to the upmost potential to understand this well.] //template because of 12410 void heapSort()(Range r) { // If true, there is nothing to do if (r.length < 2) return; // Build Heap buildHeap(r); // Sort for (size_t i = r.length - 1; i > 0; --i) { r.swapAt(0, i); ****** BAM! Visual D plomps this on my lap. See [1] percolate(r, 0, i); } } //template because of 12410 void buildHeap()(Range r) { immutable n = r.length; for (size_t i = n / 2; i-- > 0; ) { siftDown(r, i, n); } assert(isHeap(r), "Build Heap Error"); } bool isHeap()(Range r) { size_t parent = 0; foreach (child; 1 .. r.length) { if (lessFun(r[parent], r[child])) return false; // Increment parent every other pass parent += !(child & 1); } return true; } [1] This is the line that visual D highlights as the error; and pops up the standard exception popup window that all visual D users know about. The assert gave the typical nonsense that takes far too long to really read and understand because it tends to be abstract gibberish(IP address, dll module that caused the crash, file that caused the crash, line number, and possibly, if you stand just right and balance on your nose using only a single white feather, you will get the almighty glorious WARNING MESSAGE!! No, the real thing, but the actual information that we computer programmers have encoded in to electrical bits(transistor states) by mechanical lever action operating at about 10000 Billion <AAIIFPP7BOITTCR>, to help they fellow other human beings out. It's a dangerous world out there and we must all help each other fill the glass up! These `Warning Messages` inform us on a more conceptual level allowing us to build faster conceptual devices so we can increase our speed. Look out fast your average human programmer can type, my friend, to his average closest cousin, the chimpanzee.. or is that ape? It doesn't matter because those two are both closely related too(Just like natural sort, natural selection has it's own total ordering(of on DNA, if you care)). It's all about speed! The time is running out for all of us. ..And just like ant's, we have to build it for them to come(But we usually go after ants with the most deadly poisons, cause they wouldn't understand love... Hopefully the ones coming to us won't destroy us like we do the ants, or birds, or rain forest, or whatever. Or maybe we will attack them, what only amounts to bites, which case it is Kum ba yah... just, we won't be the ones singing it. But back to the specifics. Lets dive a little bit more in to the code: Being the patient observer I am[*I'm actually quite impatient(I spend all my time trying to get the glass filled, it's hard work, but someone has to do it), but I must then have patients for getting things filled, since I'm so good at it], I observed the warning popup[the one we talked about earlier] and saw that it had a line number 2230, and being of single mind and body, and quick witted, and having a memory like a turnip, I had a unique moment of genius inspiration where I realized that the actual line the error cursor was at in visual D, was not 2130 but 2117!! That was the chink in the armor. (Actually I think I just scrolled down a few lines looking for the assert, I'm not really that smart to get it by genius.) But, not being a heap expert, after all, who's an expert on all things, I did not know why the error was occurring and had to spend time learn about heap, the heap property(hey, just a total ordering! What do you know!), the D implementation, and so forth. The exact amount of time spent learning about this area of knowledge could have been nanoseconds or lightyears, no body really knows. Regardless of time, it is safe to say that time was wasted. If I am the only human that experiences such bursts of time wasting activities such as having to learn about where an error in the D code actually is, what the D error actually is telling me, and how to solve the said D error(and for that matter, any error, An error in a C program, an operating system, a society, a government, a specifies, abrain, any annoying bug that rots away the essence of our souls by draining our glasses), then be it so. But if there are millions, nay, billions, like me, then what a waste of time! Is it not the only way to solve the problem is to solve the problem? If so, then surely any logical being will conclude the if errors and computer bugs are problems(again, we can extend the problem set to all problem sets and our logic still applies:See category theory), and that they must be solved, then the only way to solve them is to solve them. But if these problems are related to all other problems(and they are), then it is important that we solve them correctly, to the best of our ability, as to not introduce more problems, else, the human race spends its entire existence solving the same old problem, which is just a problem of another `N` problem... broken in to `N` choose `M` smaller problems(like ants and dirt, we build our castles to the sky). So, is information not one way to solve problems? Surely the knowing of a problem is bad information, if we didn't know about any problems then there couldn't be any, could there? Don't take me literally friend, I only mean it as an abstraction. So, to make a long story short: Why can't we just get better damn error messages? Can we not be absolutely serious for one minute? module std.algorithm.sorting; has 402 assert error statements and only 402 of them have absolutely no error messages added! Seriously, only 402! That's like 43%, right?!? Actually I might have overestimated because I have no way to actually count D assert statements since modern search and destroy(seek/find) do not have the ability to parse a string of letters as as if it were part of some grammar, which they always are. If it did, then I could easily count the exact and true number of error-less message asserts. I think the results would be different but similar. is that 402 roaches, 402 ants, maybe 402 golden tickets(if your a 'glass is half full' kinda person), I mean, not solving the real politicians, that's kinda been like the last 10-20 of our presidents, look how great it's been! ***In the meanwhile, 4 other programmers die of a stroke...
Jul 07