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

Compile-time stack overflow when trait impl contains extern crate #55779

Closed
dtolnay opened this issue Nov 8, 2018 · 23 comments · Fixed by #89738
Closed

Compile-time stack overflow when trait impl contains extern crate #55779

dtolnay opened this issue Nov 8, 2018 · 23 comments · Fixed by #89738
Assignees
Labels
C-bug Category: This is a bug. I-crash Issue: The compiler crashes (SIGSEGV, SIGABRT, etc). Use I-ICE instead when the compiler panics. P-high High priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@dtolnay
Copy link
Member

dtolnay commented Nov 8, 2018

I have two crates:

helper/src/lib.rs

pub trait Trait {
    fn method();
}

repro/src/main.rs

struct Local;

impl helper::Trait for Local {
    fn method() {
        extern crate helper;
        unimplemented!()
    }
}

fn main() {
    <Local as helper::Trait>::method();
}

Notice that the implementation of helper::Trait contains an extern crate helper. Something is not happy about that.

$ ./repro.sh 
     Created library `helper` package
     Created binary (application) `repro` package
   Compiling helper v0.1.0
   Compiling repro v0.1.0

thread 'main' has overflowed its stack
fatal runtime error: stack overflow
error: Could not compile `repro`.

To learn more, run the command again with --verbose.

This currently affects Serde trait impls that use a private helper type with a Serde derive, which is a common pattern.

use serde::{Serialize, Serializer};
use serde_derive::Serialize;

struct _Local {
    range: std::ops::Range<usize>,
}

impl Serialize for _Local {
    fn serialize<S: Serializer>(&self, serializer: S) -> Result<S::Ok, S::Error> {
        #[derive(Serialize)]
        struct Helper {
            min: usize,
            max: usize,
        }

        let helper = Helper {
            min: self.range.start,
            max: self.range.end,
        };
        helper.serialize(serializer)
    }
}

fn main() {}

The following script reproduces the issue as of rustc 1.31.0-beta.4 (04da282 2018-11-01) as well as rustc 1.32.0-nightly (25a42b2 2018-11-07).

#!/bin/bash

cargo new --lib helper
cargo new --bin repro

echo >helper/src/lib.rs '
pub trait Trait {
    fn method();
}
'

echo >>repro/Cargo.toml '
helper = { path = "../helper" }
'

echo >repro/src/main.rs '
struct Local;

impl helper::Trait for Local {
    fn method() {
        extern crate helper;
        unimplemented!()
    }
}

fn main() {
    <Local as helper::Trait>::method();
}
'

cargo build --manifest-path repro/Cargo.toml
@dtolnay
Copy link
Member Author

dtolnay commented Nov 8, 2018

Mentioning @petrochenkov who seems to know about extern_prelude.

@petrochenkov petrochenkov self-assigned this Nov 8, 2018
@petrochenkov
Copy link
Contributor

The issue is caused by fn push_item_path entering infinite recursion when trying to print the impl path.
If @eddyb returns and finishes #56655, then this issue should be fixed as well.

@petrochenkov
Copy link
Contributor

(Also, the issue reproduces on 2015 edition as well.)

@Arnavion
Copy link

I'm hitting this on stable 1.32.0 with a #[derive(Deserialize)], and I confirmed that replacing it with a manual impl works. Is there any other workaround?

@jonas-schievink jonas-schievink added the I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ label Jan 27, 2019
@dtolnay dtolnay changed the title Compile-time stack overflow when trait impl contains extern crate and edition is 2018 Compile-time stack overflow when trait impl contains extern crate Jan 29, 2019
@dbeckwith
Copy link

Just ran into this on nightly-2019-02-27 with the serde helper pattern.

@jonas-schievink jonas-schievink added C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 18, 2019
@dtolnay dtolnay added I-crash Issue: The compiler crashes (SIGSEGV, SIGABRT, etc). Use I-ICE instead when the compiler panics. I-nominated and removed I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ labels Jan 4, 2020
@dtolnay
Copy link
Member Author

dtolnay commented Jan 4, 2020

Copying @jonas-schievink's I-nominated from #67817.

@pnkfelix
Copy link
Member

pnkfelix commented Jan 9, 2020

pre-triage: P-high. Leaving nominated for discussion at compiler triage meeting tomorrow.

@pnkfelix pnkfelix added the P-high High priority label Jan 9, 2020
@pnkfelix
Copy link
Member

From what I can tell, this was fixed sometime between nightly-2020-01-04 and nightly-2020-01-05.

Here's what landed during that time:

% git log c5840f9d2..760ce94c6 --author=bors --format=oneline
760ce94c69ca510d44087291c311296f6d9ccdf5 Auto merge of #67874 - Dylan-DPC:rollup-xy6bkoe, r=Dylan-DPC
cd8377d37e9bc47f9a5a982c41705a7800cbb51d Auto merge of #67866 - GuillaumeGomez:rollup-32vsg5b, r=GuillaumeGomez
79cf5e4fe23991ab281413623e3b50ba3deb24b2 Auto merge of #67788 - cjgillot:delint-day, r=Zoxc
abf2e00e38ad404d563f03acbcf06b08813fd086 Auto merge of #67853 - Centril:rollup-sx5zi9n, r=Centril
e845e691c988303ea54cb9de9639a8577b5a9d6b Auto merge of #67829 - michaelwoerister:try-to-fix-pgo-branch-weights-test, r=Mark-Simulacrum

@pnkfelix
Copy link
Member

I just discovered a detail about this bug that I did not previously appreciate: It needed the unimplemented!() invocation (or some similar call to e.g. panic!) to replicate.

The bug as described here ended up being fixed by PR #67874. I suspect that's due to that rollup PR's inclusion of PR #67137, but I have not yet verified this.

The question I have remaining: Is the bug here really fixed, or is it merely masked by the changes from PR #67874? E.g. PR #67137 makes heavy revisions to the expansion of panic!; can one manually write code that emulates the previous expansion, and then replicate the original bug atop that?

Anyway, even if the bug here is truly fixed, we should add the code from the description here as a regression test.

@dtolnay
Copy link
Member Author

dtolnay commented Jan 10, 2020

It's not fixed, the repro at the top still overflows rustc's stack as of 1.42.0-nightly (72b2bd5 2020-01-09). We would need to minimize again to get a different minimal repro post-#67874.

// [dependencies]
// serde = "1.0"
// serde_derive = "1.0"

use serde::{Serialize, Serializer};
use serde_derive::Serialize;

struct _Local {
    range: std::ops::Range<usize>,
}

impl Serialize for _Local {
    fn serialize<S: Serializer>(&self, serializer: S) -> Result<S::Ok, S::Error> {
        #[derive(Serialize)]
        struct Helper {
            min: usize,
            max: usize,
        }

        let helper = Helper {
            min: self.range.start,
            max: self.range.end,
        };
        helper.serialize(serializer)
    }
}

fn main() {}

@pnkfelix
Copy link
Member

pnkfelix commented Jan 13, 2020

Okay I've got a new reproduction now, thanks for the pointer @dtolnay

extern_trait/src/lib.rs:

pub trait Trait { fn no_op(&self); }

redo/src/main.rs:

use extern_trait::Trait;

struct Local;
struct Helper;

impl Trait for Local {
    fn no_op(&self)
    {
        // (Unused) extern crate declaration necessary to reproduce bug
        extern crate extern_trait;

        // This one works
        // impl Trait for Helper { fn no_op(&self) { } }

        // This one infinite-loops
        const _IMPL_SERIALIZE_FOR_HELPER: () = {
            // (extern crate can also appear here to reproduce bug,
            // as in originating example from serde)
            impl Trait for Helper { fn no_op(&self) { } }
        };

    }
}

fn main() { }

@pnkfelix
Copy link
Member

removing nomination tag since I do not think this needs explicit discussion.

However, it should get assignment. I'm not yet sure if I will take it on myself or try to get someone else to investigate it.

@lambda-fairy
Copy link
Contributor

For posterity, here's the current backtrace (since the item_path module no longer exists):

#43398 0x00007ffff6b53362 in rustc::ty::print::Printer::default_print_def_path () from /home/chris/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/bin/../lib/librustc_driver-6128a9623e6ddecc.so
#43399 0x00007ffff6b4af29 in <rustc::ty::print::pretty::FmtPrinter<F> as rustc::ty::print::Printer>::print_def_path () from /home/chris/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/bin/../lib/librustc_driver-6128a9623e6ddecc.so
#43400 0x00007ffff6b51f77 in <rustc::ty::print::pretty::FmtPrinter<F> as rustc::ty::print::pretty::PrettyPrinter>::generic_delimiters () from /home/chris/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/bin/../lib/librustc_driver-6128a9623e6ddecc.so
#43401 0x00007ffff6b5326a in rustc::ty::print::Printer::default_print_def_path () from /home/chris/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/bin/../lib/librustc_driver-6128a9623e6ddecc.so
#43402 0x00007ffff6b4af29 in <rustc::ty::print::pretty::FmtPrinter<F> as rustc::ty::print::Printer>::print_def_path () from /home/chris/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/bin/../lib/librustc_driver-6128a9623e6ddecc.so
#43403 0x00007ffff6b52ca2 in rustc::ty::print::Printer::default_print_def_path () from /home/chris/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/bin/../lib/librustc_driver-6128a9623e6ddecc.so
#43404 0x00007ffff6b4af29 in <rustc::ty::print::pretty::FmtPrinter<F> as rustc::ty::print::Printer>::print_def_path () from /home/chris/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/bin/../lib/librustc_driver-6128a9623e6ddecc.so
#43405 0x00007ffff6b52ca2 in rustc::ty::print::Printer::default_print_def_path () from /home/chris/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/bin/../lib/librustc_driver-6128a9623e6ddecc.so
#43406 0x00007ffff6b4af29 in <rustc::ty::print::pretty::FmtPrinter<F> as rustc::ty::print::Printer>::print_def_path () from /home/chris/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/bin/../lib/librustc_driver-6128a9623e6ddecc.so
#43407 0x00007ffff6b52ca2 in rustc::ty::print::Printer::default_print_def_path () from /home/chris/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/bin/../lib/librustc_driver-6128a9623e6ddecc.so
#43408 0x00007ffff6b4af29 in <rustc::ty::print::pretty::FmtPrinter<F> as rustc::ty::print::Printer>::print_def_path () from /home/chris/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/bin/../lib/librustc_driver-6128a9623e6ddecc.so
#43409 0x00007ffff6b3c59c in rustc::ty::print::pretty::PrettyPrinter::try_print_visible_def_path_recur () from /home/chris/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/bin/../lib/librustc_driver-6128a9623e6ddecc.so
#43410 0x00007ffff6b3c3ed in rustc::ty::print::pretty::PrettyPrinter::try_print_visible_def_path_recur () from /home/chris/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/bin/../lib/librustc_driver-6128a9623e6ddecc.so
#43411 0x00007ffff6b4af54 in <rustc::ty::print::pretty::FmtPrinter<F> as rustc::ty::print::Printer>::print_def_path () from /home/chris/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/bin/../lib/librustc_driver-6128a9623e6ddecc.so
#43412 0x00007ffff6b53362 in rustc::ty::print::Printer::default_print_def_path () from /home/chris/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/bin/../lib/librustc_driver-6128a9623e6ddecc.so

@estebank
Copy link
Contributor

The issue is here

return Ok((
if !span.is_dummy() {
self.print_def_path(def_id, &[])?
} else {
self.path_crate(cnum)?
},
true,
));

Removing the !span.is_dummy() check makes the repro case compile successfully, but it degrades some existing test output:

failures:
    [ui] ui/issues/issue-1920-1.rs
    [ui] ui/issues/issue-1920-2.rs
    [ui] ui/lint/lint-stability-deprecated.rs
    [ui] ui/privacy/private-inferred-type-2.rs
    [ui] ui/privacy/private-inferred-type-3.rs
    [ui] ui/privacy/private-type-in-interface.rs
    [ui] ui/structs/struct-field-privacy.rs
    [ui] ui/suggestions/suggest-private-fields.rs
    [ui] ui/traits/trait-bounds-same-crate-name.rs
    [ui] ui/type/type-mismatch-same-crate-name.rs

an example of the changes:

diff --git a/src/test/ui/privacy/private-inferred-type-3.stderr b/src/test/ui/privacy/private-inferred-type-3.stderr
index 39ef6472526..a54fb8f7a19 100644
--- a/src/test/ui/privacy/private-inferred-type-3.stderr
+++ b/src/test/ui/privacy/private-inferred-type-3.stderr
@@ -1,4 +1,4 @@
-error: type `fn() {ext::priv_fn}` is private
+error: type `fn() {private_inferred_type::priv_fn}` is private
   --> $DIR/private-inferred-type-3.rs:16:5
    |
 LL |     ext::m!();
@@ -14,7 +14,7 @@ LL |     ext::m!();
    |
    = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)

-error: type `ext::PrivEnum` is private
+error: type `private_inferred_type::PrivEnum` is private
   --> $DIR/private-inferred-type-3.rs:16:5
    |
 LL |     ext::m!();
@@ -22,7 +22,7 @@ LL |     ext::m!();
    |
    = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)

I'll see what can be done.

@eddyb
Copy link
Member

eddyb commented Apr 16, 2020

@estebank Just checking, did you see this? #55779 (comment)

I don't remember all the details but I found that comment after looking at the context around the code you linked, which seemed vaguely familiar.

@estebank
Copy link
Contributor

@eddyb I'd seen that comment, but didn't grok the full meaning of it until I looked at this in more detail.

Is there any reason not to change the if to be if !span.is_dummy() && !callers.contains(&def_id) ? It seems to me that the entire point of callers is precisely as a way to avoid the problem at hand of unconditional recursion for other cases.

@CAD97
Copy link
Contributor

CAD97 commented Apr 29, 2020

I found this separately in #71157 (comment). Workaround: add an extern crate serde (or whichever crate) at the root of your crate.

@pnkfelix
Copy link
Member

pnkfelix commented May 6, 2020

triage: nominating for discussion as part of attempt to burn down set of unassigned P-high issues at an otherwise light triage meeting.

@pnkfelix
Copy link
Member

pnkfelix commented May 7, 2020

discussed at T-compiler meeting. Assigning to @eddyb

@jplatte
Copy link
Contributor

jplatte commented Jun 4, 2020

@DevinR528 just ran into this bug in their GSoC project when adding a trybuild test (ruma-events#108 (Files)).

EDIT: Rest of the comment was probably nonsense.

That test is disabled for now, but FWIW this doesn't seem to actually require an extern crate inside a fn.

We do have an extern crate self as ruma_events; that is used for the proc macros to work inside and outside the main crate, but that's it.

@aldanor
Copy link

aldanor commented Oct 20, 2020

Just to confirm, still there in 1.46.0 (same repro with serde as posted above).

@eddyb
Copy link
Member

eddyb commented Oct 10, 2021

@eddyb I'd seen that comment, but didn't grok the full meaning of it until I looked at this in more detail.

Is there any reason not to change the if to be if !span.is_dummy() && !callers.contains(&def_id) ? It seems to me that the entire point of callers is precisely as a way to avoid the problem at hand of unconditional recursion for other cases.

Didn't see this when it was posted, my bad, but I wanted to reply because it's not that simple because that recursion tracking is limited to try_print_visible_def_path_recur, but currently, print_def_path gets in between that, so that you have many independent callers sets, none of them containing the def_id of the extern crate item.

@LesnyRumcajs
Copy link

I ran into this as well with rustc 1.58.1 (db9d1b20b 2022-01-20) while trying to delete this workaround based on the status here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. I-crash Issue: The compiler crashes (SIGSEGV, SIGABRT, etc). Use I-ICE instead when the compiler panics. P-high High priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.