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

Avoid stack overflow when printing unevaluated const #72341

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 40 additions & 18 deletions src/librustc_middle/ty/print/pretty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ thread_local! {
static FORCE_IMPL_FILENAME_LINE: Cell<bool> = Cell::new(false);
static SHOULD_PREFIX_WITH_CRATE: Cell<bool> = Cell::new(false);
static NO_QUERIES: Cell<bool> = Cell::new(false);
static NO_CONST_PATH: Cell<bool> = Cell::new(false);
}

/// Avoids running any queries during any prints that occur
Expand All @@ -72,6 +73,15 @@ pub fn with_no_queries<F: FnOnce() -> R, R>(f: F) -> R {
})
}

pub fn with_no_const_path<F: FnOnce() -> R, R>(f: F) -> R {
NO_CONST_PATH.with(|no_const_path| {
let old = no_const_path.replace(true);
let result = f();
no_const_path.set(old);
result
})
}

/// Force us to name impls with just the filename/line number. We
/// normally try to use types. But at some points, notably while printing
/// cycle errors, this can result in extra or suboptimal error output,
Expand Down Expand Up @@ -858,7 +868,9 @@ pub trait PrettyPrinter<'tcx>:
) -> Result<Self::Const, Self::Error> {
define_scoped_cx!(self);

if self.tcx().sess.verbose() {
// See the call to `with_no_const_path` inside the
// `ty::ConstKind::Unevaluated` for why we check `NO_CONST_PATH`
if self.tcx().sess.verbose() || NO_CONST_PATH.with(|q| q.get()) {
p!(write("Const({:?}: {:?})", ct.val, ct.ty));
return Ok(self);
}
Expand All @@ -882,29 +894,39 @@ pub trait PrettyPrinter<'tcx>:

match ct.val {
ty::ConstKind::Unevaluated(did, substs, promoted) => {
if let Some(promoted) = promoted {
p!(print_value_path(did, substs));
p!(write("::{:?}", promoted));
} else {
match self.tcx().def_kind(did) {
DefKind::Static | DefKind::Const | DefKind::AssocConst => {
p!(print_value_path(did, substs))
}
_ => {
if did.is_local() {
let span = self.tcx().def_span(did);
if let Ok(snip) = self.tcx().sess.source_map().span_to_snippet(span)
{
p!(write("{}", snip))
// Don't re-entrantly enter this code path: that is,
// don't try to print the DefPath of an unevaluated const
// while we're already printing the DefPath of an unevaluated
// const. This ensures that we never end up trying to recursively
// print a const that we're already printing.
// See issue #68104 for more details
self = with_no_const_path(|| {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you try with_forced_impl_filename_line instead? It should also work since I believe the problem here is the pretty-printing of impl paths.

Wait. Hang on. Why would this ever print paths?!

The bug is probably in metadata encoding/decoding. AnonConsts shouldn't turn into DefKind::Const.

if let Some(promoted) = promoted {
p!(print_value_path(did, substs));
p!(write("::{:?}", promoted));
} else {
match self.tcx().def_kind(did) {
DefKind::Static | DefKind::Const | DefKind::AssocConst => {
p!(print_value_path(did, substs))
}
_ => {
if did.is_local() {
let span = self.tcx().def_span(did);
if let Ok(snip) =
self.tcx().sess.source_map().span_to_snippet(span)
{
p!(write("{}", snip))
} else {
print_underscore!()
}
} else {
print_underscore!()
}
} else {
print_underscore!()
}
}
}
}
Ok(self)
})?;
}
ty::ConstKind::Infer(..) => print_underscore!(),
ty::ConstKind::Param(ParamConst { name, .. }) => p!(write("{}", name)),
Expand Down
9 changes: 9 additions & 0 deletions src/test/ui/const-generics/auxiliary/impl-const.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
#![feature(const_generics)]

pub struct Num<const N: usize>;

// Braces around const expression causes crash
impl Num<{5}> {
Comment on lines +5 to +6
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The braces shouldn't matter, I wonder if this is a bug (cc @varkor).
What you want is to use an expression, like 2+3, so that it definitely requires evaluation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We currently don't look into blocks for literals, It seems like we only do it for params at the moment

Copy link
Contributor

@lcnr lcnr May 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will get a PR ready which cleans this up.

edit: we may not actually want this for now, as being more aggressive here may mask some otherwise found issues with ConstKind::Unevaluated.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't need braces here: arbitrary expressions should work, and {5} is an expression.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The braces are needed to trigger the original ICE, so I included them in the test

pub fn five(&self) {
}
}
14 changes: 14 additions & 0 deletions src/test/ui/const-generics/issue-68104-print-stack-overflow.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
// aux-build:impl-const.rs
// run-pass

#![feature(const_generics)]
#![allow(incomplete_features)]

extern crate impl_const;

use impl_const::*;

pub fn main() {
let n = Num::<5>;
n.five();
}