georust / rstar

R*-tree spatial index for the Rust ecosystem
https://docs.rs/rstar
Apache License 2.0
386 stars 67 forks source link

Impl Point for [RTreeNum; N] const generic N #115

Closed dominikWin closed 1 year ago

dominikWin commented 1 year ago

Point is now implemented for [RTreeNum; N] where N is a usize. Previously it was only for N values between 2 and 9. Const generics are stable in every supported version of Rust.

dominikWin commented 1 year ago

Fair point. Unfortunately this is a pain point of the current const generics spec. verify_parameters currently does an assert for this, so I added one to this PR with the same message. I think this is the best solution.

The only other way I can find is to use an associated trait:

trait AssertGt1 {
    const VALID: ();
}

impl<S, const N: usize> AssertGt1 for [S; N] where S: RTreeNum {
    const VALID: () = assert!(N >= 2);
}

Then one of the functions in impl<S, const N: usize> Point for [S; N] can start with _ = <Self as AssertGt1>::VALID;. This fails with this error:

error[E0080]: evaluation of `<[f64; 1] as rstar::point::AssertGt1>::VALID` failed
   --> <path>/rstar/src/point.rs:313:23
    |
313 |     const VALID: () = assert!(N >= 2);
    |                       ^^^^^^^^^^^^^^^ the evaluated program panicked at 'assertion failed: N >= 2', <path>/rstar/src/point.rs:313:23
    |
    = note: this error originates in the macro `assert` (in Nightly builds, run with -Z macro-backtrace for more info)

The error message is not terrible, but the code is hacky and would put an extra trait into the docs.

michaelkirk commented 1 year ago

That trait AssertGt1 error happens at compile time right? That's pretty cool, but I agree it's a little harder to follow.

I agree that the straight-forward runtime panic for this rare case is preferable.

Maybe once we get these two RFC's stabilized we could do something cleaner:

  1. panic in const fn
  2. const fn in trait (maybe not the best link... historical rustlang RFC discussions are sometimes hard for me to navigate)
dominikWin commented 1 year ago

Yes that error is at compile time.

michaelkirk commented 1 year ago

bors r+

bors[bot] commented 1 year ago

Build failed:

michaelkirk commented 1 year ago

Looks like we'll need to bump our MSRV (which I think is just fine - currently it's very old). @dominikWin - can you verify what minimum version of rust is required to build this branch?

dominikWin commented 1 year ago

1.55 is when array_map is stabilized, but 1.59 is when the project will compile since a few dependencies (rayon, which we use in criterion) want that version now.

adamreichold commented 1 year ago

1.55 is when array_map is stabilized, but 1.59 is when the project will compile since a few dependencies (rayon, which we use in criterion) want that version now.

We should still aim for 1.55 then and use cargo update --precise to pin compatible versions of our dependencies in the CI. (People building using the MSRV by definition do not have the latest toolchain and hence they are usually fine by not having the latest version of the dependencies as long as they are compatible.)

adamreichold commented 1 year ago

It appears that we have effectively bumped our MSRV to 1.63 at this point.

@dominikWin Could you rebase this? Thanks!

dominikWin commented 1 year ago

What moves rstar to rely on 1.63? I can't seem to find anything mentioned elsewhere.

adamreichold commented 1 year ago

What moves rstar to rely on 1.63? I can't seem to find anything mentioned elsewhere.

The changes to GitHub workflow, i.e. https://github.com/georust/rstar/commit/f5f0cb1a9c90c4e65b6948e3abee4f1ce1bc1ec9, which tests our MSRV. But indeed, there is no changelog entry and the rust-version property also does not match. Will try to fix that separately.

EDIT: #124 is that separate PR.

dominikWin commented 1 year ago

bors try

bors[bot] commented 1 year ago

:lock: Permission denied

Existing reviewers: click here to make dominikWin a reviewer

adamreichold commented 1 year ago

bors try

bors ATM does not check anything that the CI itself does, so if you rebased and the CI is happen, you're golden.

I think the changelog entry is misplaced though? It still refers to the already released 0.10 section instead of the unreleased section.

dominikWin commented 1 year ago

I think the changelog entry is misplaced though? It still refers to the already released 0.10 section instead of the unreleased section.

Whoops. Should be fixed now

adamreichold commented 1 year ago

bors r=michaelkirk

bors[bot] commented 1 year ago

Build succeeded: