error introduced in geo v0.22.1 by "rustdoc: Add item_template macro" #113908
Labels
A-mir-opt
Area: MIR optimizations
A-mir-opt-inlining
Area: MIR inlining
C-bug
Category: This is a bug.
P-medium
Medium 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.
Comments
coffinmatician
added
C-bug
rustbot
added
the
needs-triage
|
Assuming https://github.com/georust/geo/tree/geo-0.22.1/geo is the crate and version with the issue, I'm not able to compile it as-is, because of a searched commit range: 5bd28f5...3307274 bisected with cargo-bisect-rustc v0.6.6Host triple: x86_64-unknown-linux-gnu cargo bisect-rustc --preserve --start 2023-06-01 --end 2023-07-17 --prompt -- build --release which seems more likely as it changes the type system. Removing the ICE label as this just seems to be a query recursion overflow, please let me know if this is incorrect |
clubby789
removed
the
I-ICE
|
Actually, this doesn't seem to error any more on the current nightly. Can you verify if the issue still reproduces after updating? |
|
Here is a further minified test case. I am very confused on why changing the module names of things (too much, but minor changes seem to still trigger the issue) seems to stop triggering the issue. It seems very odd. |
|
Thank you for providing code to reproduce the issue - this one does indeed produce an error on current nightly. Having bisected a couple of times and gotten a different result every time, it seems a little inconsistent to track down. However, both the error you posted and the one I'm getting are a regular query overflow error (although still likely a bug). An ICE is a specific error where the compiler panics. |
|
I strongly suspect that this regressed due to my MIR inliner change #109247, but I do not know how/why it was subsequently fixed (if it even was). Probably the only way to understand this is to brute-force instead of bisecting this because of the number of times this has probably changed between being an error and not. |
GuillaumeGomez
added
the
T-rustdoc
|
@rustbot label regression-untriaged |
rustbot
added
regression-untriaged
I was under the impression that ICE means any compiler error that keeps (reasonable) code that is expect to compile from compiling. Thank you for explaining that to me. @clubby789 Also sorry that I was grumpy yesterday. |
|
I have further minified it again. I think someone should be able to pull in the |
|
I'm not sure if this was noticed in the above output, but this appears to be a regression in the stable compiler. 1.71.0 fails on this crate, where 1.70.0 succeeded. |
|
Possibly related to #110475? |
|
Tried building the MCVE provide in this comment. |
|
WG-prioritization assigning priority (Zulip discussion). @rustbot label -I-prioritize +P-medium |
rustbot
added
P-medium
workingjubilee
changed the title
Noratrieb
removed
the
needs-triage
|
Hi, what should we do to work around this issue? |
Your question seems ambiguous to me. If you mean the issue when specifically affecting |
|
Sorry for the confusion, my crate was depending on specifically a version of |
I just tested building from source on 8ede3aa and I can confirm that the issue does not reproducible in the default configuration. BUT the issue does reproduce with the following patch. The steps I used to build, and test where: ./x.py build
rustup toolchain link 'coffin' build/host/stage2
cd /path/to/geo && cargo +coffin rustc -rI'm going to think more about this later, and possibly do more testing. I suspect that I'm doing something stupid (I tried this patch/change on a whim as I was thinking about what could affect the rmeta files) and that this isn't actually making a difference, but it seems after poking for a bit I can't figure out else what it could be and it seems to be fine without this patch, and produce the error with it. |
|
So I'm pretty sure I see the issue at this point, but I'm not really sure what the intended functionality of the code is. What is happening is that Now what really confuses me is that the The next questionable decision to me is Lastly, I there is the issue at the heart of this problem which I will fully admit to not understanding -- either the the MIR graph is dynamic and potentially infinite in size (at least for
- https://doc.rust-lang.org/1.71.0/nightly-rustc/rustc_middle/ty/struct.Instance.html OR this code, or something it is calling are creating different idential versions of the node other than the difference in the I WILL FULLY ADMIT TO NOT BEING FAMILIAR WITH THE RUST COMPILER CODE BASE. Everything I said above is taken from my quick observations, and usage of the rust compiler for the first time for this issue. I may be missing obvious details, making incorrect statements, or accusing code of being incorrect that is fine. I'm just writing down everything I think and didn't both to preface every sentence above with "I think". |
|
This issue is very old at this point, and I have investigated what I would consider heavily into this issue and took it as far as I felt comfortable. Possibly with some guidance I could go further, but at this point: I identified the issue, somewhat minified the example (admittedly not as much as I would like, but also I assume that if anyone here really cares that they are more familiar with automated minifaction tools than I am), semi-bisected the issue (it appears to me at this time that something is seeding randomness from the compiler version which seems like a bad idea to me if I'm correct), and even dug in the Rust compiler itself found the offending function (or at least what I believe is the offender and no one has commented further), and made a full write up. I recognize that this is an open source project, and I shouldn't expect people to work on this issue, but saddens me that the Rust project does not value the correctness of their compiler as highly as I believed. If people feel like I should be doing something to push this issue further forward I would like to know. If I should close this issue, and create one, or more new issues that are more streamlined than this somewhat organic issue I would like to know. |
saethlin
added
regression-from-stable-to-stable
rustbot
removed
the
T-rustdoc
|
#116896 maybe helps also here |
|
This problem was discussed in #110475 (which I dug up by searching the issue tracker for Adding this test to the minified example and trying to run it with #[cfg(test)]
mod tests {
use super::map_coords::deprecated::*;
use super::map_coords::modern::{Geometry, GeometryCollection};
use geo_types::point;
#[test]
fn test() {
let p = point!(x: 1.0, y: 1.0);
let pe = Geometry::Point(p);
let mut gc = GeometryCollection::new_from(vec![pe]);
let _ = gc.try_map_coords_inplace(|p| Ok::<_, ()>(p));
}
}As far as I can tell, any attempt to call This code flakes back and forth between compiling and not compiling across versions because the MIR inliner's cycle detection strategy involves sometimes just giving up on inlining within a crate, and this polymorphic recursion is entirely within a crate. So depending on the crate hash which can depend on the rustc version, the inliner doesn't even look at this function and the library builds. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Please see #112202 for the PR introducing the regression.
Code
geo v0.22.1 with the following Cargo.lock file. I can try to minify it, but I don't really want to.
Cargo.lock.txt
Meta
See the bisect below (I originally bisected over a larger range, but I wanted to recreate the message quickly):
Error output
The same error appears without the
RUSTFLAGS=-Awarningsit just has a lot of spam.The text was updated successfully, but these errors were encountered: