Page MenuHomePhabricator

rust-dirs: address failing tests for `dirs` impl with a temporary fix
ClosedPublic

Authored by Alphare on Nov 22 2019, 4:46 AM.

Details

Summary

https://phab.mercurial-scm.org/D7252 (5d40317d42b7083b49467502549e25f144888cb3)
introduced a regression in Rust tests.

This is a temporary fix that replicates the behavior of the C and Python impl,
pending the resolution of the discussion (in the phabricator link) about how
we actually want to solve this problem.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Alphare created this revision.Nov 22 2019, 4:46 AM
marmoute added inline comments.
rust/hg-core/src/lib.rs
117

Should we also have the path included in that error ?

This revision was not accepted when it landed; it landed in state Needs Review.
This revision was automatically updated to reflect the committed changes.
Alphare added inline comments.Nov 22 2019, 12:46 PM
rust/hg-core/src/lib.rs
117

This patch is a minimal effort to stick to the C and Python implementations until a decision is reached. Should we decide to keep this code, I would be for having the path included in the error.

marmoute added inline comments.Nov 22 2019, 12:50 PM
rust/hg-core/src/lib.rs
117

This seems fine.

yuja added a subscriber: yuja.Nov 23 2019, 1:13 AM

Many unhandled results:

warning: unused `std::result::Result` that must be used
  --> hg-core/src/dirstate/dirs_multiset.rs:42:21
   |
42 |                     multiset.add_path(filename);
   |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: #[warn(unused_must_use)] on by default
   = note: this `Result` may be an `Err` variant, which should be handled

warning: unused `std::result::Result` that must be used
  --> hg-core/src/dirstate/dirs_multiset.rs:45:17
   |
45 |                 multiset.add_path(filename);
   |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: this `Result` may be an `Err` variant, which should be handled

warning: unused `std::result::Result` that must be used
  --> hg-core/src/dirstate/dirs_multiset.rs:59:13
   |
59 |             multiset.add_path(filename);
   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: this `Result` may be an `Err` variant, which should be handled

warning: unused `std::result::Result` that must be used
   --> hg-core/src/dirstate/dirstate_map.rs:129:17
    |
129 |                 all_dirs.add_path(filename);
    |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: this `Result` may be an `Err` variant, which should be handled

Might be better to do path.check_state() in cpython layer, and insert
debug_assert to hg-core.

In D7503#110345, @yuja wrote:

Many unhandled results:

Ah sorry, the cargo compilation cache hid those warnings from me, I usually catch them.

Might be better to do path.check_state() in cpython layer, and insert
debug_assert to hg-core.

That would be cleaner for the current purposes, but using debug_assert in hg-core indicates to me that we want the Rust code to not worry about checking for consecutive slashes in dirs, because we would have the pathauditor. Am I correct?

yuja added a comment.Nov 25 2019, 8:47 AM
> Might be better to do `path.check_state()` in cpython layer, and insert
> `debug_assert` to hg-core.
That would be cleaner for the current purposes, but using `debug_assert` in `hg-core` indicates to me that we want the Rust code to not worry about checking for consecutive slashes in `dirs`, because we would have the `pathauditor`. Am I correct?

I generally prefer adding safety checks at ABI boundary. If malicious input
makes Rust code crash or exhaust CPU/memory resource, I would add sanity
check to rust-cpython layer.

I generally prefer adding safety checks at ABI boundary. If malicious input
makes Rust code crash or exhaust CPU/memory resource, I would add sanity
check to rust-cpython layer.

Sure, that makes sense in our configuration, but we need to consider hg-core as its own standalone library when making decisions like this. Either the Dirs / dirs API has changed to this new behavior (which I'm not super happy about), either we revert the changes proposed by Augie with a change in the fuzzer instead.

Sorry for the warnings, I'll send a follow-up, I've been caught up in another project.

yuja added a comment.Nov 27 2019, 8:30 AM
> I generally prefer adding safety checks at ABI boundary. If malicious input
> makes Rust code crash or exhaust CPU/memory resource, I would add sanity
> check to rust-cpython layer.
Sure, that makes sense in our configuration, but we need to consider `hg-core` as its own standalone library when making decisions like this.

Okay, then using non-debug assert!() seems more appropriate. If we prefer
being stricter, "checked" HgPath type can be introduced.

Either the Dirs / dirs API has changed to this new behavior (which I'm not super happy about), either we revert the changes proposed by Augie with a change in the fuzzer instead.

Sorry for the warnings, I'll send a follow-up, I've been caught up in another project.

Actually I tried to suppress these warnings by propagating Result upwards,
and I got a feeling that we're doing wrong.

Okay, then using non-debug assert!() seems more appropriate. If we prefer
being stricter, "checked" HgPath type can be introduced.
...
Actually I tried to suppress these warnings by propagating Result upwards,
and I got a feeling that we're doing wrong.

I sent a followup as D7522 because that gets rid of the warnings. As explained in the commit message, I am unhappy about this change, as you seem to be.

We're doing it wrong, indeed, because the checks are happening at the wrong level, causing abstraction leakage all over the place. The Rust type-system makes it much more obvious than in C or Python.

I am of the opinion of backing out of the original patch (5d40317d42b7083b49467502549e25f144888cb3) that @durin42 introduced.

yuja added a comment.Nov 28 2019, 8:14 AM
> Okay, then using non-debug `assert!()` seems more appropriate. If we prefer
> being stricter, "checked" HgPath type can be introduced.
> ...
> Actually I tried to suppress these warnings by propagating Result upwards,
> and I got a feeling that we're doing wrong.
I sent a followup as D7522 <https://phab.mercurial-scm.org/D7522> because that gets rid of the warnings. As explained in the commit message, I am unhappy about this change, as you seem to be.

Thanks. Given Rust impl is still unstable, I think it's better to back out
the changes in Rust and leave the test failure until we find a right solution.

We're doing it wrong, indeed, because the checks are happening at the wrong level, causing abstraction leakage all over the place. The Rust type-system makes it much more obvious than in C or Python.
I am of the opinion of backing out of the original patch (5d40317d42b7083b49467502549e25f144888cb3 <https://phab.mercurial-scm.org/rHG5d40317d42b7083b49467502549e25f144888cb3>) that @durin42 introduced.

I don't agree with that since raising Python exception is the only way to
safely error out from C extension layer. It should be better than blocking
the entire process (or exhausting resources.)

In D7503#110631, @yuja wrote:
> Okay, then using non-debug `assert!()` seems more appropriate. If we prefer
> being stricter, "checked" HgPath type can be introduced.
> ...
> Actually I tried to suppress these warnings by propagating Result upwards,
> and I got a feeling that we're doing wrong.
I sent a followup as D7522 <https://phab.mercurial-scm.org/D7522> because that gets rid of the warnings. As explained in the commit message, I am unhappy about this change, as you seem to be.

Thanks. Given Rust impl is still unstable, I think it's better to back out
the changes in Rust and leave the test failure until we find a right solution.

I am -1 for this. Having the test broken is a big overhead for people actually using Rust and testing it. The rust support is experimental but not "unstable". We use it in production in a couple of places. Keeping the test suite

yuja added a comment.Nov 29 2019, 9:57 AM
>>   > Okay, then using non-debug `assert!()` seems more appropriate. If we prefer
>>   > being stricter, "checked" HgPath type can be introduced.
>>   > ...
>>   > Actually I tried to suppress these warnings by propagating Result upwards,
>>   > and I got a feeling that we're doing wrong.
>>   I sent a followup as D7522 <https://phab.mercurial-scm.org/D7522> because that gets rid of the warnings. As explained in the commit message, I am unhappy about this change, as you seem to be.
>
> Thanks. Given Rust impl is still unstable, I think it's better to back out
> the changes in Rust and leave the test failure until we find a right solution.
I am -1 for this. Having the test broken is a big overhead for people actually using Rust and testing it. The rust support is experimental but not "unstable". We use it in production in a couple of places. Keeping the test suite

Well, IMHO, it's as unstable as Python 3. Core features should work, but I
think it's okay to opt out some "known to be broken" tests so long as the
test failure makes sense.

I felt the changes were too much to make Rust implementation behaves exactly
in the same way as C one. If not, I would send the fix last weekend instead
of getting around the phabricator email.

I am fine with not having all test passing from the start. However here we have a test that used to pass that is now failing, so we regressed here. I would rather not regress in test coverage here. Without this, all mercurial test pass with the Rust code, I would like to keep it that way.

yuja added a comment.Nov 29 2019, 10:15 AM
I am fine with not having all test passing from the start. However here we have a test that used to pass that is now failing, so we regressed here. I would rather not regress in test coverage here. Without this, all mercurial test pass with the Rust code, I would like to keep it that way.

Isn't it a new test added at 5d40317d42b7, right?

As I said, I'm not against the change at 5d40317d42b7, but it seems not
fit well into Rust.