Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Regression: geoarrow crate does not compile in release mode on 1.82 #131960

Open
theemathas opened this issue Oct 20, 2024 · 7 comments
Open

Regression: geoarrow crate does not compile in release mode on 1.82 #131960

theemathas opened this issue Oct 20, 2024 · 7 comments
Labels
A-mir-opt-inlining Area: MIR inlining C-bug Category: This is a bug. P-high High priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-types Relevant to the types team, which will review and decide on the PR/issue. WG-mir-opt Working group: MIR optimizations

Comments

@theemathas
Copy link
Contributor

This issue is originally reported by @kylebarron at #128887 (comment)

The geoarrow crate does not compile in release mode on rust 1.82.0, despite compiling fine in debug mode or in rust 1.81.0.

The code can be obtained by:

git clone https://github.com/geoarrow/geoarrow-rs
cd geoarrow-rs
git checkout 0b6715c6a56f0115f9078803fae945700713b22f

The following commands give compilation errors:

rustup run 1.82 cargo build --release
rustup run nightly cargo build --release

The following commands compile fine without errors:

rustup run 1.81 cargo build --release
rustup run 1.81 cargo build
rustup run 1.82 cargo build
rustup run nightly cargo build
RUSTFLAGS='-Zinline-mir=no' rustup run nightly cargo build --release

Note that this is issue is distinct from the previous 1.80-to-nightly geoarrow regression, which is fixed in #129714 and tested in #129757.

@rustbot labels regression-from-stable-to-stable

@theemathas theemathas added the C-bug Category: This is a bug. label Oct 20, 2024
@rustbot rustbot added needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. regression-from-stable-to-stable Performance or correctness regression from one stable version to another. I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Oct 20, 2024
@saethlin saethlin added E-needs-bisection Call for participation: This issue needs bisection: https://github.com/rust-lang/cargo-bisect-rustc A-mir-opt-inlining Area: MIR inlining labels Oct 20, 2024
@saethlin
Copy link
Member

saethlin commented Oct 20, 2024

Note that this is issue is distinct from the previous 1.80-to-nightly geoarrow regression, which is fixed in #129714 and tested in #129757.

No, I'm pretty sure this is exactly the same problem. I see 128 recursive calls to mir_callgraph_reachable::process with -Ztreat-err-as-bug.

@saethlin
Copy link
Member

We would also need a magic -3 adjustment on top of the existing recursion limit hack to make this compile. I could speculate but I don't think I understand why. I think it's pretty clear though that's the regression test we checked in doesn't cover the scenario that geoarrow is hitting.

@lqd
Copy link
Member

lqd commented Oct 20, 2024

Bisects to nightly-2024-08-02, seemingly #127159 within rollup #128461. But it makes no sense, so there may be some other weirdness going on.

@lqd lqd removed the E-needs-bisection Call for participation: This issue needs bisection: https://github.com/rust-lang/cargo-bisect-rustc label Oct 20, 2024
@saethlin saethlin added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-types Relevant to the types team, which will review and decide on the PR/issue. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Oct 20, 2024
@saethlin
Copy link
Member

cc @compiler-errors

At the current time, I'm not working on any fix for this regression. I'm really not sure what a reasonable change would be; we can add some more magic numbers to the cycle avoidance recursion limit, but I'm not sure what number we can add that's reasonable. I suspect that whatever we do, there will be some type that someone can write that will run into this error anyway.

It's my understanding that this error can be fixed in the new trait solver, but that won't land for so long that it's kind of off topic for this issue.

I feel like there should be a better way to write the cycle avoidance call graph code, but I don't know if this scenario can be avoided by that.

@apiraino
Copy link
Contributor

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-high

@rustbot rustbot added P-high High priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Oct 21, 2024
@theemathas
Copy link
Contributor Author

theemathas commented Oct 21, 2024

Somewhat minimized reproduction code: geoarrow-repro.zip (around 80 lines of actual rust code)

The issue appears to be sensitive to the exact module paths and the exact dependencies used.

   Compiling geoarrow v0.0.0 (/Users/timch/geoarrow-rs)
error[E0275]: overflow evaluating the requirement `impl GeometryCollectionTrait<T = f64>: GeometryCollectionTrait`
   |
   = help: consider increasing the recursion limit by adding a `#![recursion_limit = "256"]` attribute to your crate (`geoarrow`)
note: required for `GeometryCollectionIterator<'_, f64, <<<... as GeometryCollectionTrait>::ItemType<'_> as GeometryTrait>::GeometryCollection<'_> as GeometryCollectionTrait>::ItemType<'_>, ...>` to implement `Iterator`
  --> src/lib.rs:45:7
   |
43 |         ItemType: 'a + GeometryTrait<T = T>,
   |                                      ----- unsatisfied trait bound introduced here
44 |         G: GeometryCollectionTrait<T = T, ItemType<'a> = ItemType>,
45 |     > Iterator for GeometryCollectionIterator<'a, T, ItemType, G>
   |       ^^^^^^^^     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   = note: required for `GeometryCollectionIterator<'_, f64, <<<... as GeometryCollectionTrait>::ItemType<'_> as GeometryTrait>::GeometryCollection<'_> as GeometryCollectionTrait>::ItemType<'_>, ...>` to implement `IntoIterator`
   = note: the full name for the type has been written to '/Users/timch/geoarrow-rs/target/release/deps/geoarrow-c9e0255bc8538bba.long-type-18348558354101559122.txt'
   = note: consider using `--verbose` to print the full type name to the console
   = note: the full name for the type has been written to '/Users/timch/geoarrow-rs/target/release/deps/geoarrow-c9e0255bc8538bba.long-type-18348558354101559122.txt'
   = note: consider using `--verbose` to print the full type name to the console

For more information about this error, try `rustc --explain E0275`.
error: could not compile `geoarrow` (lib) due to 1 previous error

@theemathas
Copy link
Contributor Author

theemathas commented Oct 21, 2024

This version of the code reproduces the issue on nightly (tested with rustc 1.84.0-nightly (662180b34 2024-10-20)), but not on stable. The difference is probably due to #130455, which makes MIR-inlining not depend on hashes.

Code
pub trait GeometryCollectionTrait: Sized {
    type T;

    type ItemType<'a>: 'a + GeometryTrait<T = Self::T>
    where
        Self: 'a;

    fn geometries(&self) -> GeometryCollectionIterator<'_, Self::T, Self::ItemType<'_>, Self> {
        unimplemented!()
    }
}
pub trait GeometryTrait {
    type T;

    type GeometryCollection<'a>: 'a + GeometryCollectionTrait<T = Self::T>
    where
        Self: 'a;

    fn as_type(&self) -> GeometryType<'_, Self::GeometryCollection<'_>>;
}

pub enum GeometryType<'a, GC>
where
    GC: GeometryCollectionTrait,
{
    GeometryCollection(&'a GC),
}

pub struct GeometryCollectionIterator<
    'a,
    T,
    ItemType: 'a + GeometryTrait<T = T>,
    G: GeometryCollectionTrait<T = T, ItemType<'a> = ItemType>,
> {
    _a: &'a G,
}

impl<
        'a,
        T,
        ItemType: 'a + GeometryTrait<T = T>,
        G: GeometryCollectionTrait<T = T, ItemType<'a> = ItemType>,
    > Iterator for GeometryCollectionIterator<'a, T, ItemType, G>
{
    type Item = ItemType;
    fn next(&mut self) -> Option<Self::Item> {
        unimplemented!()
    }
}
pub fn add_geometry(geometry: &impl GeometryTrait<T = f64>) {
    match geometry.as_type() {
        GeometryType::GeometryCollection(g) => add_geometry_collection(g),
    }
}

pub fn add_geometry_collection(geometry_collection: &impl GeometryCollectionTrait<T = f64>) {
    for geometry in geometry_collection.geometries() {
        add_geometry(&geometry);
    }
}

pub fn bounding_rect_geometry_collection(geom: &impl GeometryCollectionTrait<T = f64>) {
    add_geometry_collection(geom);
}

@wesleywiser wesleywiser added fixed-by-next-solver Fixed by the next-generation trait solver, `-Znext-solver`. WG-mir-opt Working group: MIR optimizations labels Nov 15, 2024
@jieyouxu jieyouxu added could-be-fixed-by-next-solver and removed fixed-by-next-solver Fixed by the next-generation trait solver, `-Znext-solver`. WG-mir-opt Working group: MIR optimizations labels Nov 15, 2024
@wesleywiser wesleywiser added the WG-mir-opt Working group: MIR optimizations label Nov 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-mir-opt-inlining Area: MIR inlining C-bug Category: This is a bug. P-high High priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-types Relevant to the types team, which will review and decide on the PR/issue. WG-mir-opt Working group: MIR optimizations
Projects
None yet
Development

No branches or pull requests

7 participants