use array interface to hashOf()#1595
Conversation
| } | ||
|
|
||
| override size_t getHash(in void* p) @safe pure nothrow const | ||
| override size_t getHash(in void* p) @trusted pure nothrow const |
There was a problem hiding this comment.
Because the slicing is an unsafe operation. The older code was unsafe as well.
There was a problem hiding this comment.
How can this interface (dereferencing a void*) even be @safe in the first place?
There was a problem hiding this comment.
getHash is supposed to take a pointer to the object matching the TypeInfo.
There was a problem hiding this comment.
"is supposed" is something entirely different than "is guaranteed to". You could of course argue that the interface was broken before, but it still remains a safety violation that should at least be recorded in Bugzilla (and subsequently be deprecated/made un-@safe).
There was a problem hiding this comment.
Please do add a bugzilla issue for it. Such is beyond the scope of this PR, however, and should not impede it.
There was a problem hiding this comment.
Please do add a bugzilla issue for it. Such is beyond the scope of this PR, however, and should not impede it.
The PR adds a @trusted annotation that is clearly wrong. It's barely acceptable because the code was already broken before, but now it is your responsibility to make sure it is eventually cleaned up. Previously, the place would have been flagged automatically once the mistaken @trusted annotation on hashOf is removed.
There was a problem hiding this comment.
The @safe was equally wrong. This PR does not address (or change) the interface to getHash(), the change was made to reflect the unsafe operation inside the function. The issue you brought up is with the interface to getHash(), not the interface to hashOf().
your responsibility
It is the responsibility of people who find bugs to post them to bugzilla. The bug you found has nothing to do with this PR, which is to fix the interface to hashOf(). You should take the credit for finding the issue by posting it to bugzilla.
Meanwhile, this PR is only concerned with the interface to hashOf.
There was a problem hiding this comment.
The @safe was equally wrong.
No; @safe can never be wrong as in breaking the type system, it can just cause code not to compile. You are adding a new, incorrect instance of @trusted, which does add an extra type system violation.
As I'm sure you realise, this is a huge deal because instances where things are mistakenly marked as @safe can be detected automatically, which is impossible for mistaken uses of @trusted. The only way to find those is to painstakingly go through all occurrences of @trusted and manually audit each one of them.
Just trust me on that one. I've found a large share of the Phobos safety bugs you went through recently, this PR would add another such bug, I point it out, you fix/document it before it goes in. That's how pull request review works. You can't justify knowingly introducing an issue without any documentation, etc. by pointing out that the code base contained bugs before.
WalterBright
force-pushed
the
useHashOf
branch
3 times, most recently
from
e7e2b5b to
67bf11a
Compare
|
The changes LGTM. Interfaces with |
WalterBright
force-pushed
the
useHashOf
branch
3 times, most recently
from
537cf8b to
077c4f6
Compare
| } | ||
|
|
||
| @trusted pure nothrow | ||
| size_t hashOf( const(void)* buf, size_t len, size_t seed = 0 ) |
There was a problem hiding this comment.
get rid of the default argument, it just causes bugs
WalterBright
force-pushed
the
useHashOf
branch
4 times, most recently
from
35f7183 to
01537a2
Compare
|
(This is currently blocked due to an incorrect addition of |
|
It is not incorrect. The revised code will not compile with @safe, even though the original, despite being marked @safe, was not detected as being unsafe. The unsafety was in the interface to hashOf(). By fixing the interface to hashOf(), the unsafety of the getHash() interface is exposed. This is similar to a discussion I had with @yebblies some time back about adding const correctness incrementally. Adding const tends to be viral, i.e. an all-or-nothing proposition. But trying to const-correct the entire program in on PR is thoroughly impractical, so I was doing it incrementally by adding unsafe casts here and there to satisfy the type system. As const spread through the program in successive PRs, the casts were removed. |
Are they all gone now? Of course, you could just do what every other contributor does and FINISH the changes in a branch before pushing little bits to master. |
|
Sure – I did acknowledge that a temporary fix might be valid in this situation, since we need to deprecate the type system-breaking API first. All I'm asking is that you add a comment there mentioning the fact that |
I just came here to report the same thing! So what's stopping you from pulling this? |
|
The correct marking of
Note that item 1 doesn't include any of the calls being modified here. Please, let's not obfuscate where the problem lies. If this breaks code, it's code worth breaking. Anyone who cares about |
|
Redoing the interface is |
|
@WalterBright: Yes, the ticket does exist, but you neither mentioned it in code, nor in the commit message (where I would really want to see it at the very least). The fact that the issue lay dormant in Bugzilla for more than two years with no duplicates being reported – even though many people have used and even worked on |
|
This PR does not change the interface to getHash. Trying to lump this PR in with another problem is a mistake and completely off-topic. If you want to raise the visibility of https://issues.dlang.org/show_bug.cgi?id=12516 that's fine, but it has nothing to do with this PR and should not block it. |
That's not what we do. Bug reports go in Bugzilla.
It has nothing to do with this PR. It is an entirely separate problem. |
Up to this, I've assumed that there a genuine misunderstanding here, but if you want to have a discussion on that level, I can also just close this PR and ask you to open a separate one for the single bug fix hidden among all the other refactorings.
You are adding an incorrect |
IMO, a |
Broken code was not introduced by this PR. If you believe it does, please re-examine it. Furthermore, this is a misunderstanding of @trusted. @trusted is a statement about the contents of the function, not the interface. The interface did not change, and the function was never safe, and it was never safe because of the unsafe interface to hashOf. Fixing hashOf, the lowest level code, is the first step to improving the safety of the call chain.
That is correct.
This is incorrect. It fixes the unsafe interface of
An all-or-nothing approach to fixing safety problems means that zero progress can be made. |
I think you mean something different, like that it avoids using the unsafe interface? But in any case, this doesn't push the problem to the right level. Passing a memory-invalid array to a
There is no reason to avoid calling the unsafe interface when the operation itself is actually unsafe. IMO, this PR is already achieving zero progress. It doesn't have to be all or nothing. Fix calls to |
I suppose it is only fair then to ask that you have another look at the proposed code yourself. The commit introduces the fragment In any case, would you mind separating out the actual one-line bug fix (zero length vs. seed) into a second PR so I can merge it right away? All the usual arguments you make for DMD PRs apply. |
Of course it does. There's no reason for hashOf to be trusted instead of safe, and this fixes the problem with hashOf.
The problem was always there, it was just hidden in the call to hashOf due to the unsafe interface to hashOf. This PR does not introduce a problem.
No, because it's time to settle this point, not defer it. The hashOf interface needs to be fixed, and this PR fixes it and does NOT degrade anything else. Adding a requirement for PRs to only be "full stack" fixes is unworkable in general. What needs to happen is progress, piece by piece. Otherwise druntime/phobos will simply never get fixed to be safe. It's exactly the same issue one runs into when trying to retrofit const-correctness into an existing code base. Doing it incrementally, bottom up, is the only practical way, and requires the introduction of temporary |
|
I have two questions about this:
|
src/rt/util/hash.d
Outdated
|
|
||
| @trusted pure nothrow | ||
| size_t hashOf( const(void)* buf, size_t len, size_t seed = 0 ) | ||
| @trusted pure nothrow @nogc |
There was a problem hiding this comment.
(Somewhat tangentially to the main discussion, this should be changed to @system right away. rt.util is a druntime-internal module, and there is nothing to be gained from keeping the broken attribute around.)
There was a problem hiding this comment.
(And unless you make this @system, you can hardly claim that it makes any piece of code more safe. More @safe, anyway – there is an argument to be made that removing the default value already makes client code more resilient against bugs.) (edited punctuation for clarity)
There was a problem hiding this comment.
I left the old function in there because removing it may break existing code, adding system to it may break existing code, and breaking existing code should be done separately if possible. Not lumped in with other things. Even if hashOf is an internal function, there's nothing stopping anyone from using it. There being another hashOf in object.d is unfortunate and further obfuscates things, but again, that is not the topic of this PR.
There was a problem hiding this comment.
Even if hashOf is an internal function, there's nothing stopping anyone from using it.
rt.* is not distributed to users.
There was a problem hiding this comment.
Anyone can import it - it's in import/core/internal/hash.d
Changing that is properly a separate PR.
Edit: Ah, you're right, I had it confused.
There was a problem hiding this comment.
Ah, you're right, I had it confused.
No worries – all the functions having (almost) identical names make it easy to mix them up.
There was a problem hiding this comment.
Ah, you're right, I had it confused.
No worries – all the functions having (almost) identical names make it easy to mix them up.
This is correct as far as the client (druntime user) is concerned. The function was mistakenly callable from safe code before, and continues to be so. However, this is completely besides the point – I already agreed that deprecating the old Where this PR does introduce a problem, however, is for maintaining druntime. While the interface is still broken for the user just the same, your change spreads out the mistakenly In case that was somehow lost in translation, this is literally what I have been asking for previously, and still am – adding a short comment describing why introducing a piece of code not that different from And besides, isn't the fact that it managed to spawn a discussion of this length between experts enough of a hint enough that there is something at odds with just "use array interface to hashOf()" as the sole explanation of this contribution? I'm honestly not sure how asking for more than five words of description for a subtle matter like this can even be controversial in the first place.
It was only a suggestion to resolve this impasse, as I find it silly to hold up a bug fix and spend absurd amounts of time on a discussion just because of your apparent unwillingness to even minimally document your work. If you want to continue this argument instead, fine by me. Unless you insist, I'm not going to comment on the (non-)applicability of your comparison to const-correctness or the technical merits of resorting to BOLD ALL CAPS for fallacious arguments, since I don't think those are related to what my point is. @andralex: |
No, it does not. It simply identifies code that should have been marked @trusted to begin with, because it was not @safe despite being marked @safe.
The consequences will be to avoid making a decision and never fix hashOf.
It's an apt comparison. Insisting on a "full stack" approach to fixing viral issues means nothing will get done.
I see no purpose to documenting things that are listed as bugs in bugzilla. You're putting forth a new policy from existing practice. You might as well require that all phobos bugs in bugzilla also get PRs to document them in the source code. Remember, this PR is not about fixing bugs in the interface to getHash, it is about making the interface to hashOf safe. Conflating this with other problems is not how PRs are supposed to work. Grepping a codebase for FIXME is the old way of doing things. We use bugzilla now. And besides, simply marking it as @trusted is supposed to be a flag for reviewers. It is not hidden. |
The @safe was misapplied to getHash, not hashOf. hashOf was marked @trusted although it did not provide a trusted interface to what it was doing. It's like marking
My plan was grepping the druntime sources for |
|
Keep in mind that this PR is very simple and straightforward: It adds an overload to It's a self-contained, incremental step towards improving the overall safety of druntime/Phobos. |
[…]
These are nonsensical statements. Also, the code shouldn't have been marked [1] In the sense that compilation will fail instead of it causing a type-system breaking bug. |
This PR does not do that. I already pulled #1593 |
src/rt/typeinfo/ti_Ag.d
Outdated
| { | ||
| byte[] s = *cast(byte[]*)p; | ||
| return rt.util.hash.hashOf(s.ptr, s.length * byte.sizeof); | ||
| const s = *cast(void[]*)p; |
There was a problem hiding this comment.
please cast to const void[] instead
You're right. This one fixes the calls to it.
This is incorrect. The problem was that hashOf was marked @trusted, even though hashOf did not have a safe interface. Trusted can be used to hide unsafety from the compiler, and was in this case, and there is no compiler technology that can detect that in the general case.
It is obvious to me. But adding @System to getHash is an API-breaking change, and needs to be in a separate PR and be evaluated for its breaking effects before doing so. Such is beyond the scope of this PR, which is solely about hashOf. |
|
This is a viable PR that marks progress on an issue. The intensity of the discussion is somewhat proportional; clearly something hanky-panky was going on in the virtual @WalterBright please add this just before the // TODO: fix https://issues.dlang.org/show_bug.cgi?id=12516 e.g. by changing this to a truly safe interface.I'll pull after @WalterBright addresses the nits. |
|
@andralex done |
|
alrighty then |
|
thanks! |
7 participants
Because the array interface is safer.