November 01, 2014 Re: std.experimental.logger formal review round 3 | ||||
---|---|---|---|---|
| ||||
Posted in reply to Dicebot | There is still a critical issue with std.experimental.logger that would prevent it from being merged right now: The abuse of @trusted all over the code. For example, the following piece of code compiles with burner/logger@c87e1032: --- import std.experimental.logger.core; struct Dangerous { string toString() { *(cast(int*)0xdeadbeef) = 0xcafebabe; return null; } } void main() @safe { logf("%s", Dangerous()); } --- To be quite honest, I'm rather disappointed that I was able to break @safe-ty with the first, most straightforward piece of code I tried. Sure, many people don't understand how dangerous @trusted applied to templates really is. But this issue has already been pointed out multiple times in Robert's code, both in this pull request and other unrelated ones. David |
November 01, 2014 Re: std.experimental.logger formal review round 3 | ||||
---|---|---|---|---|
| ||||
Posted in reply to David Nadlinger | On Saturday, 1 November 2014 at 17:35:37 UTC, David Nadlinger wrote:
> There is still a critical issue with std.experimental.logger that would prevent it from being merged right now: The abuse of @trusted all over the code.
>
> For example, the following piece of code compiles with burner/logger@c87e1032:
> ---
> import std.experimental.logger.core;
>
> struct Dangerous {
> string toString() {
> *(cast(int*)0xdeadbeef) = 0xcafebabe;
> return null;
> }
> }
>
> void main() @safe {
> logf("%s", Dangerous());
> }
> ---
>
> To be quite honest, I'm rather disappointed that I was able to break @safe-ty
> with the first, most straightforward piece of code I tried. Sure, many people don't understand how dangerous @trusted applied to templates really is. But this issue has already been pointed out multiple times in Robert's code, both in this pull request and other unrelated ones.
>
> David
Oh, I have actually completely missed that. It is critical indeed. Will try having a look myself and possibly providing patches.
|
November 02, 2014 Re: std.experimental.logger formal review round 3 | ||||
---|---|---|---|---|
| ||||
Posted in reply to David Nadlinger | On Saturday, 1 November 2014 at 17:35:37 UTC, David Nadlinger wrote:
> There is still a critical issue with std.experimental.logger that would prevent it from being merged right now: The abuse of @trusted all over the code.
Thank you. I was afraid I'd have to harp on it for the fourth time...
|
November 02, 2014 Re: std.experimental.logger formal review round 3 | ||||
---|---|---|---|---|
| ||||
Posted in reply to Jakob Ovrum | On Sunday, 2 November 2014 at 00:29:10 UTC, Jakob Ovrum wrote:
> On Saturday, 1 November 2014 at 17:35:37 UTC, David Nadlinger wrote:
>> There is still a critical issue with std.experimental.logger that would prevent it from being merged right now: The abuse of @trusted all over the code.
>
> Thank you. I was afraid I'd have to harp on it for the fourth time...
Sorry, it is really hard to follow important bits in many discussions spawned :( I'll take a go at @safe-ty issue personally this time.
|
November 02, 2014 Re: std.experimental.logger formal review round 3 | ||||
---|---|---|---|---|
| ||||
Posted in reply to David Nadlinger | I had initial go at cleaning safety attributes : https://github.com/Dicebot/phobos/tree/logger-fix-trust It still needs one druntime PR merged (https://github.com/D-Programming-Language/druntime/pull/1009) to be done properly but should be ready for destruction. David snipet results in this after all changes: std/experimental/logger/core.d(1675): Error: safe function 'std.experimental.logger.core.Logger.logf!(11, "aaa.d", "aaa.main", "void aaa.main() @safe", "aaa", Dangerous).logf' cannot call system function 'std.format.formattedWrite!(MsgRange, char, Dangerous).formattedWrite' |
November 02, 2014 Re: std.experimental.logger formal review round 3 | ||||
---|---|---|---|---|
| ||||
Posted in reply to Dicebot | On Sunday, 2 November 2014 at 13:06:24 UTC, Dicebot wrote:
> I had initial go at cleaning safety attributes : https://github.com/Dicebot/phobos/tree/logger-fix-trust
>
> It still needs one druntime PR merged (https://github.com/D-Programming-Language/druntime/pull/1009) to be done properly but should be ready for destruction.
>
> David snipet results in this after all changes:
> std/experimental/logger/core.d(1675): Error: safe function 'std.experimental.logger.core.Logger.logf!(11, "aaa.d", "aaa.main", "void aaa.main() @safe", "aaa", Dangerous).logf' cannot call system function 'std.format.formattedWrite!(MsgRange, char, Dangerous).formattedWrite'
I don't recall off the top of my head some non-template innards actually might require @safe, but apart from that, why not just leave the job to template attribute inference entirely? If somebody wants to log a type with a @system toString in non-@safe code, why not just let them?
David
|
November 02, 2014 Re: std.experimental.logger formal review round 3 | ||||
---|---|---|---|---|
| ||||
Posted in reply to David Nadlinger |
> I don't recall off the top of my head some non-template innards actually might require @safe, but apart from that, why not just leave the job to template attribute inference entirely? If somebody wants to log a type with a @system toString in non-@safe code, why not just let them?
>
> David
In my opinion inference is better choice for small building blocks (like algorithms). For complete system like logging API forcing @safe makes more sense as whatever its internals are, exposed API should never be @system
|
November 02, 2014 Re: std.experimental.logger formal review round 3 | ||||
---|---|---|---|---|
| ||||
Posted in reply to Dicebot | On Sunday, 2 November 2014 at 18:29:09 UTC, Dicebot wrote:
> In my opinion inference is better choice for small building blocks (like algorithms). For complete system like logging API forcing @safe makes more sense as whatever its internals are, exposed API should never be @system
This isn't about the library internals. These should be presented in a safe way, of course. The point here is that you restrict what your users can do by forcing templates to be @safe.
Imagine somebody has a type that cannot be @trusted because of whatever reason. Maybe because it's legacy code, maybe it uses resources it does not manage, … If you forcibly make logf @safe, then this type cannot be used with logf without some crazy workaround (simply using to!string might produce an unneeded allocation if the type uses the sink-delegate signature for toString).
Why not leave this up to the compiler and support more use cases without degrading the experience for @safe clients?
David
|
November 02, 2014 Re: std.experimental.logger formal review round 3 | ||||
---|---|---|---|---|
| ||||
Posted in reply to David Nadlinger | On Sunday, 2 November 2014 at 18:42:20 UTC, David Nadlinger wrote:
> Imagine somebody has a type that cannot be @trusted because of whatever reason. Maybe because it's legacy code, maybe it uses resources it does not manage, … If you forcibly make logf @safe, then this type cannot be used with logf without some crazy workaround (simply using to!string might produce an unneeded allocation if the type uses the sink-delegate signature for toString).
>
> Why not leave this up to the compiler and support more use cases without degrading the experience for @safe clients?
>
> David
You mean something like user type toString() which is legitimately @system and can't be made @trusted? Yes, this makes sense. Will need to also add tests for that.
Consider me convinced :)
|
November 03, 2014 Re: std.experimental.logger formal review round 3 | ||||
---|---|---|---|---|
| ||||
Posted in reply to Dicebot | On 11/2/14 10:07 PM, Dicebot wrote:
> On Sunday, 2 November 2014 at 18:42:20 UTC, David Nadlinger wrote:
>> Imagine somebody has a type that cannot be @trusted because of
>> whatever reason. Maybe because it's legacy code, maybe it uses
>> resources it does not manage, … If you forcibly make logf @safe, then
>> this type cannot be used with logf without some crazy workaround
>> (simply using to!string might produce an unneeded allocation if the
>> type uses the sink-delegate signature for toString).
>>
>> Why not leave this up to the compiler and support more use cases
>> without degrading the experience for @safe clients?
>>
>> David
>
> You mean something like user type toString() which is legitimately
> @system and can't be made @trusted? Yes, this makes sense. Will need to
> also add tests for that.
>
> Consider me convinced :)
Fantastic, thanks! -- Andrei
|
Copyright © 1999-2021 by the D Language Foundation