Jump to page: 1 25  
Page
Thread overview
What should happen when the assert message expression throws?
Nov 18, 2022
RazvanN
Nov 18, 2022
Meta
Nov 18, 2022
Patrick Schluter
Nov 20, 2022
Ali Çehreli
Nov 18, 2022
Dennis
Nov 24, 2022
bauss
Nov 24, 2022
Dennis
Nov 18, 2022
Walter Bright
Nov 18, 2022
H. S. Teoh
Nov 18, 2022
Walter Bright
Nov 20, 2022
Ali Çehreli
Nov 19, 2022
JG
Nov 19, 2022
Dukc
Nov 19, 2022
JG
Nov 20, 2022
Ali Çehreli
Nov 24, 2022
Salih Dincer
Nov 25, 2022
Dukc
Nov 25, 2022
Quirin Schroll
Nov 25, 2022
kdevel
Dec 08, 2022
Dukc
Nov 25, 2022
Ali Çehreli
Nov 25, 2022
kdevel
Nov 25, 2022
Ali Çehreli
Nov 25, 2022
kdevel
Nov 25, 2022
Ali Çehreli
Nov 25, 2022
kdevel
Nov 26, 2022
Ali Çehreli
Nov 27, 2022
kdevel
Dec 07, 2022
Ali Çehreli
Dec 08, 2022
Timon Gehr
Dec 08, 2022
H. S. Teoh
Dec 09, 2022
kdevel
Dec 09, 2022
H. S. Teoh
Dec 09, 2022
Timon Gehr
Dec 08, 2022
Dukc
Jan 09, 2023
Quirin Schroll
Dec 08, 2022
Dukc
Dec 08, 2022
Ali Çehreli
Nov 19, 2022
Timon Gehr
Nov 19, 2022
Timon Gehr
Jan 10, 2023
Iain Buclaw
November 18, 2022

I have stumbled upon: https://issues.dlang.org/show_bug.cgi?id=17226

I want to fix it, however, the solution is not obvious.

Take this code:

import std.string;

void foo(int i)
{
    // In this case a %s is forgotten but it could be any other trivial error.
    assert(i == 42, format("Bad parameter:", i));
}

void main()
{
    foo(43);
}

If format throws, then the Exception is thrown masking the assert error.
Unfortunately, I don't see a clear way to rewrite this to valid D code as to catch the exception and then assert. Ideally, we could do something along the lines of:

assert(i == 42,
       (const(char)[] msg,
        try { msg = format("Bad parameter:", i); },
        catch (Exception e) { msg = "Assert message evaluation has failed";},
        msg)
       );

This rewrite would be done only if it is determined that the assert message may throw. However, the current dmd-fe implementation does not allow for such a construction and I think that it might be overkill to implement the necessary machinery just to support this case.

The try catch block can also be generated outside of the assert expression:

auto msg;
try { msg = format("Bad parameter:", i);}
catch (Exception e) { msg = "Assert message evaluation has failed";}
assert(i == 42, msg);

The difference between the 2 is that in this case we are evaluating the msg regardless of whether the assert condition is true or false.

Also, we need to take into account the case where the user tries to catch the exception by himself:

void foo(int i)
{
    try
    {
        // In this case a %s is forgotten but it could be any other trivial error.
        assert(i == 42, format("Bad parameter:", i));
    }
    catch(Exception e) { /* do some sort of fix up with the message */}
}

void main()
{
    foo(43);
}

Today, this code runs successfully and no AssertError is thrown. If we automatically catch the exception we might break such code (although I would argue it would not be too dramatic).

An alternative solution would be to deprecate having an assert message that may throw. This has the advantage that it avoids complexity inside the compiler and the user is forced to write:


auto msg = /* do whatever you want with throwing code */
assert(cond, msg);

If the user wants to catch the exception or not, it's his/hers business, but then the compiler has defined semantics in all situations.

What do you think? Is deprecating having an assert message that may throw a severe restriction? Are there other rewrites that I am missing?

Cheers,
RazvanN

November 18, 2022

On Friday, 18 November 2022 at 12:36:14 UTC, RazvanN wrote:

>

Unfortunately, I don't see a clear way to rewrite this to valid D code as to catch the exception and then assert. Ideally, we could do something along the lines of:

assert(i == 42,
       (const(char)[] msg,
        try { msg = format("Bad parameter:", i); },
        catch (Exception e) { msg = "Assert message evaluation has failed";},
        msg)
       );

Why not rewrite it like so?

assert(i == 42, ()
{
    try
    {
        return format("Bad parameter:", i);
    }
    catch (Exception e)
    {
        return "Assert message evaluation has failed";
    }
}());
November 18, 2022

On Friday, 18 November 2022 at 12:36:14 UTC, RazvanN wrote:

>

I have stumbled upon: https://issues.dlang.org/show_bug.cgi?id=17226

[...]

I question if a fix is even required. It's a bug in the program that has to be corrected in any case. When it is corrected, the assert() will manifest itself and can then be corrected also. When debugging a program, I always concentrate on the first error/bug/exception as very often subsequent errors/bugs/exceptions were more often than not just consequences of undefined behaviour of the initial error.

So a "won't fix" is an appropriate approach imho if a correction involves a much to costly fix.

November 18, 2022

On Friday, 18 November 2022 at 12:36:14 UTC, RazvanN wrote:

>

I have stumbled upon: https://issues.dlang.org/show_bug.cgi?id=17226

I want to fix it, however, the solution is not obvious.

Take this code:

import std.string;

void foo(int i)
{
    // In this case a %s is forgotten but it could be any other trivial error.
    assert(i == 42, format("Bad parameter:", i));
}

void main()
{
    foo(43);
}

If format throws, then the Exception is thrown masking the assert error.
Unfortunately, I don't see a clear way to rewrite this to valid D code as to catch the exception and then assert. Ideally, we could do something along the lines of:

assert(i == 42,
       (const(char)[] msg,
        try { msg = format("Bad parameter:", i); },
        catch (Exception e) { msg = "Assert message evaluation has failed";},
        msg)
       );

This rewrite would be done only if it is determined that the assert message may throw. However, the current dmd-fe implementation does not allow for such a construction and I think that it might be overkill to implement the necessary machinery just to support this case.

The try catch block can also be generated outside of the assert expression:

auto msg;
try { msg = format("Bad parameter:", i);}
catch (Exception e) { msg = "Assert message evaluation has failed";}
assert(i == 42, msg);

The difference between the 2 is that in this case we are evaluating the msg regardless of whether the assert condition is true or false.

Also, we need to take into account the case where the user tries to catch the exception by himself:

void foo(int i)
{
    try
    {
        // In this case a %s is forgotten but it could be any other trivial error.
        assert(i == 42, format("Bad parameter:", i));
    }
    catch(Exception e) { /* do some sort of fix up with the message */}
}

void main()
{
    foo(43);
}

Today, this code runs successfully and no AssertError is thrown. If we automatically catch the exception we might break such code (although I would argue it would not be too dramatic).

An alternative solution would be to deprecate having an assert message that may throw. This has the advantage that it avoids complexity inside the compiler and the user is forced to write:


auto msg = /* do whatever you want with throwing code */
assert(cond, msg);

If the user wants to catch the exception or not, it's his/hers business, but then the compiler has defined semantics in all situations.

What do you think? Is deprecating having an assert message that may throw a severe restriction? Are there other rewrites that I am missing?

Cheers,
RazvanN

Perhaps the assert hook in druntime could have additional overloads:

// pseudocode

// existing assert hook (without `-checkaction=context`)
void _d_assert(bool cond, string msg);

// new hook overloads
void _d_assert(bool cond, string function() /* infer attributes */ lazyMessage);
void _d_assert(bool cond, string delegate() /* infer attributes */ lazyMessage);

The compiler would select the lazyMessage overloads if the the expression could throw, or perhaps deemed more computationally expensive (based on some heuristic).

That way, we delay the evaluation of msg until we know for sure that the condition is false and then we could wrap that evaluation in try-catch and throw a proper nested Throwable.

If anything, making the msg parameter lazy evaluated would make the asserts more consistent with enforce.

November 18, 2022

My thoughts:

  • A wrong format string passed to format should be a runtime error (if not caught at compile time), not an exception
  • The current behavior is 'correct' if you consider assert a function call, where the argument list is evaluated before calling the function. I'm not a fan of adding more magic to assert, especially considering:
  • This issue is not limited to exceptions, but any bottom value generated in the assert message. 'Fixing' it for exceptions does not 'fix' it for other cases:
assert(x, (){while(1){} return "";}()); // endless loop before assert
assert(x, new char[2UL ^^ 60]); // Out of Memory before assert
assert(x, assert(0)); // assert before assert

On Friday, 18 November 2022 at 12:36:14 UTC, RazvanN wrote:

>

This rewrite would be done only if it is determined that the assert message may throw. However, the current dmd-fe implementation does not allow for such a construction and I think that it might be overkill to implement the necessary machinery just to support this case.

You could add a minimal implementation of std.exception.collectException to druntime and use that.

>

An alternative solution would be to deprecate having an assert message that may throw.

That could be annoying to users since Phobos' string functions often may throw.

November 18, 2022
On 11/18/2022 4:36 AM, RazvanN wrote:
> I have stumbled upon: https://issues.dlang.org/show_bug.cgi?id=17226

Yes, an interesting problem.

I suggest that have the format() template do the check at compile time, analogously to the way printf() formats are statically checked.

The signature of format() is:

  immutable(Char)[] format(Char, Args...)(in Char[] fmt, Args args)

so it should be doable as a template constraint.

This would be a good initiative anyway, as it will benefit all uses of format(). And if this enables format() itself to be nothrow, even better!

Of course, this does not solve the *general* problem of arguments to assert() throwing, but as others suggested, that can be dealt with by:

1. won't fix - too bad, so sad!

2. require such arguments to be nothrow, which is a more robust solution
November 18, 2022
On Fri, Nov 18, 2022 at 12:41:54PM -0800, Walter Bright via Digitalmars-d wrote:
> On 11/18/2022 4:36 AM, RazvanN wrote:
> > I have stumbled upon: https://issues.dlang.org/show_bug.cgi?id=17226
>
> Yes, an interesting problem.
>
> I suggest that have the format() template do the check at compile time,

It already does this. This works:

        string s = format!"blah %f blah"(123.45);

and this will produce a compile error:

        string s = format!"blah %d blah"(123.45);

So just write:

        assert(myAssumption, format!"%s doesn't work!"(blah));

Problem solved.


T

--
Life begins when you can spend your spare time programming instead of watching television. -- Cal Keegan

November 18, 2022
On 11/18/2022 1:33 PM, H. S. Teoh wrote:
> On Fri, Nov 18, 2022 at 12:41:54PM -0800, Walter Bright via Digitalmars-d wrote:
>> On 11/18/2022 4:36 AM, RazvanN wrote:
>> > I have stumbled upon: > https://issues.dlang.org/show_bug.cgi?id=17226
>>
>> Yes, an interesting problem.
>>
>> I suggest that have the format() template do the check at compile time,
> 
> It already does this. This works:
> 
>          string s = format!"blah %f blah"(123.45);
> 
> and this will produce a compile error:
> 
>          string s = format!"blah %d blah"(123.45);
> 
> So just write:
> 
>          assert(myAssumption, format!"%s doesn't work!"(blah));
> 
> Problem solved.

You should post that to the bug report!

November 19, 2022

On Friday, 18 November 2022 at 12:36:14 UTC, RazvanN wrote:

>

I have stumbled upon: https://issues.dlang.org/show_bug.cgi?id=17226

[...]

What about rewriting

assert(false,format("oops",1));

to


    assert(false,(){
        try {
        return format("oops",1);
        }
        catch(Exception e)
        {
            //throw equivalent error (this should be improved)
            throw new Error(e.msg);
        }
    }());
November 19, 2022

On Friday, 18 November 2022 at 12:36:14 UTC, RazvanN wrote:

>

What do you think? Is deprecating having an assert message that may throw a severe restriction? Are there other rewrites that I am missing?

In my opinion the fix is clear. When the assertion handler at DRuntime determines the assertion has failed, it should call the error message generator (it is lazy IIRC) in a try/catch block. If the generator throws an exception, the error message of assertion failure should be something like <FormatException thrown while writing error message>.

Should the error generation exception be the .next member of AssertError? Probably, but I'm not sure.

If the message generator throws an unrecoverable error, that should probably still mask the assert error. If a RangeError masks AssertError it's unlikely to be a problem since neither is likely to be caught and at least in my mind they're essentially the same - a "bug detected" error.

« First   ‹ Prev
1 2 3 4 5