From 6ad140ca197a1147e476f34908c2b69a60cc6d2f Mon Sep 17 00:00:00 2001 From: Bastian Kauschke Date: Fri, 23 Oct 2020 12:13:44 +0200 Subject: [PATCH 1/2] const_eval_checked: deal with unused nodes + div --- .../src/traits/const_evaluatable.rs | 63 +++++++++++++++---- .../const_evaluatable_checked/division.rs | 11 ++++ .../const_evaluatable_checked/unused_expr.rs | 25 ++++++++ .../unused_expr.stderr | 26 ++++++++ 4 files changed, 113 insertions(+), 12 deletions(-) create mode 100644 src/test/ui/const-generics/const_evaluatable_checked/division.rs create mode 100644 src/test/ui/const-generics/const_evaluatable_checked/unused_expr.rs create mode 100644 src/test/ui/const-generics/const_evaluatable_checked/unused_expr.stderr diff --git a/compiler/rustc_trait_selection/src/traits/const_evaluatable.rs b/compiler/rustc_trait_selection/src/traits/const_evaluatable.rs index 1e1eb16faf4..cb0950e3627 100644 --- a/compiler/rustc_trait_selection/src/traits/const_evaluatable.rs +++ b/compiler/rustc_trait_selection/src/traits/const_evaluatable.rs @@ -227,7 +227,12 @@ struct AbstractConstBuilder<'a, 'tcx> { tcx: TyCtxt<'tcx>, body: &'a mir::Body<'tcx>, /// The current WIP node tree. - nodes: IndexVec>, + /// + /// We require all nodes to be used in the final abstract const, + /// so we store this here. Note that we also consider nodes as used + /// if they are mentioned in an assert, so some used nodes are never + /// actually reachable by walking the [`AbstractConst`]. + nodes: IndexVec, bool)>, locals: IndexVec, /// We only allow field accesses if they access /// the result of a checked operation. @@ -274,6 +279,27 @@ impl<'a, 'tcx> AbstractConstBuilder<'a, 'tcx> { Ok(Some(builder)) } + fn add_node(&mut self, n: Node<'tcx>) -> NodeId { + // Mark used nodes. + match n { + Node::Leaf(_) => (), + Node::Binop(_, lhs, rhs) => { + self.nodes[lhs].1 = true; + self.nodes[rhs].1 = true; + } + Node::UnaryOp(_, input) => { + self.nodes[input].1 = true; + } + Node::FunctionCall(func, nodes) => { + self.nodes[func].1 = true; + nodes.iter().for_each(|&n| self.nodes[n].1 = true); + } + } + + // Nodes start as unused. + self.nodes.push((n, false)) + } + fn place_to_local( &mut self, span: Span, @@ -311,7 +337,7 @@ impl<'a, 'tcx> AbstractConstBuilder<'a, 'tcx> { let local = self.place_to_local(span, p)?; Ok(self.locals[local]) } - mir::Operand::Constant(ct) => Ok(self.nodes.push(Node::Leaf(ct.literal))), + mir::Operand::Constant(ct) => Ok(self.add_node(Node::Leaf(ct.literal))), } } @@ -348,7 +374,7 @@ impl<'a, 'tcx> AbstractConstBuilder<'a, 'tcx> { Rvalue::BinaryOp(op, ref lhs, ref rhs) if Self::check_binop(op) => { let lhs = self.operand_to_node(stmt.source_info.span, lhs)?; let rhs = self.operand_to_node(stmt.source_info.span, rhs)?; - self.locals[local] = self.nodes.push(Node::Binop(op, lhs, rhs)); + self.locals[local] = self.add_node(Node::Binop(op, lhs, rhs)); if op.is_checkable() { bug!("unexpected unchecked checkable binary operation"); } else { @@ -358,13 +384,13 @@ impl<'a, 'tcx> AbstractConstBuilder<'a, 'tcx> { Rvalue::CheckedBinaryOp(op, ref lhs, ref rhs) if Self::check_binop(op) => { let lhs = self.operand_to_node(stmt.source_info.span, lhs)?; let rhs = self.operand_to_node(stmt.source_info.span, rhs)?; - self.locals[local] = self.nodes.push(Node::Binop(op, lhs, rhs)); + self.locals[local] = self.add_node(Node::Binop(op, lhs, rhs)); self.checked_op_locals.insert(local); Ok(()) } Rvalue::UnaryOp(op, ref operand) if Self::check_unop(op) => { let operand = self.operand_to_node(stmt.source_info.span, operand)?; - self.locals[local] = self.nodes.push(Node::UnaryOp(op, operand)); + self.locals[local] = self.add_node(Node::UnaryOp(op, operand)); Ok(()) } _ => self.error(Some(stmt.source_info.span), "unsupported rvalue")?, @@ -415,13 +441,9 @@ impl<'a, 'tcx> AbstractConstBuilder<'a, 'tcx> { .map(|arg| self.operand_to_node(terminator.source_info.span, arg)) .collect::, _>>()?, ); - self.locals[local] = self.nodes.push(Node::FunctionCall(func, args)); + self.locals[local] = self.add_node(Node::FunctionCall(func, args)); Ok(Some(target)) } - // We only allow asserts for checked operations. - // - // These asserts seem to all have the form `!_local.0` so - // we only allow exactly that. TerminatorKind::Assert { ref cond, expected: false, target, .. } => { let p = match cond { mir::Operand::Copy(p) | mir::Operand::Move(p) => p, @@ -430,7 +452,15 @@ impl<'a, 'tcx> AbstractConstBuilder<'a, 'tcx> { const ONE_FIELD: mir::Field = mir::Field::from_usize(1); debug!("proj: {:?}", p.projection); - if let &[mir::ProjectionElem::Field(ONE_FIELD, _)] = p.projection.as_ref() { + if let Some(p) = p.as_local() { + debug_assert!(!self.checked_op_locals.contains(p)); + // Mark locals directly used in asserts as used. + // + // This is needed because division does not use `CheckedBinop` but instead + // adds an explicit assert for `divisor != 0`. + self.nodes[self.locals[p]].1 = true; + return Ok(Some(target)); + } else if let &[mir::ProjectionElem::Field(ONE_FIELD, _)] = p.projection.as_ref() { // Only allow asserts checking the result of a checked operation. if self.checked_op_locals.contains(p.local) { return Ok(Some(target)); @@ -457,7 +487,16 @@ impl<'a, 'tcx> AbstractConstBuilder<'a, 'tcx> { if let Some(next) = self.build_terminator(block.terminator())? { block = &self.body.basic_blocks()[next]; } else { - return Ok(self.tcx.arena.alloc_from_iter(self.nodes)); + assert_eq!(self.locals[mir::Local::from_usize(0)], self.nodes.last().unwrap()); + self.nodes[self.locals[mir::Local::from_usize(0)]].1 = true; + if !self.nodes.iter().all(|n| n.1) { + self.error(None, "unused node")?; + } + + return Ok(self + .tcx + .arena + .alloc_from_iter(self.nodes.into_iter().map(|(n, _used)| n))); } } } diff --git a/src/test/ui/const-generics/const_evaluatable_checked/division.rs b/src/test/ui/const-generics/const_evaluatable_checked/division.rs new file mode 100644 index 00000000000..71a5f2d3472 --- /dev/null +++ b/src/test/ui/const-generics/const_evaluatable_checked/division.rs @@ -0,0 +1,11 @@ +// run-pass +#![feature(const_generics, const_evaluatable_checked)] +#![allow(incomplete_features)] + +fn with_bound() where [u8; N / 2]: Sized { + let _: [u8; N / 2] = [0; N / 2]; +} + +fn main() { + with_bound::<4>(); +} diff --git a/src/test/ui/const-generics/const_evaluatable_checked/unused_expr.rs b/src/test/ui/const-generics/const_evaluatable_checked/unused_expr.rs new file mode 100644 index 00000000000..9c603c57a48 --- /dev/null +++ b/src/test/ui/const-generics/const_evaluatable_checked/unused_expr.rs @@ -0,0 +1,25 @@ +#![feature(const_generics, const_evaluatable_checked)] +#![allow(incomplete_features)] + +fn add() -> [u8; { N + 1; 5 }] { + //~^ ERROR overly complex generic constant + todo!() +} + +fn div() -> [u8; { N / 1; 5 }] { + //~^ ERROR overly complex generic constant + todo!() +} + +const fn foo(n: usize) {} + +fn fn_call() -> [u8; { foo(N); 5 }] { + //~^ ERROR overly complex generic constant + todo!() +} + +fn main() { + add::<12>(); + div::<9>(); + fn_call::<14>(); +} diff --git a/src/test/ui/const-generics/const_evaluatable_checked/unused_expr.stderr b/src/test/ui/const-generics/const_evaluatable_checked/unused_expr.stderr new file mode 100644 index 00000000000..73c6d9393ce --- /dev/null +++ b/src/test/ui/const-generics/const_evaluatable_checked/unused_expr.stderr @@ -0,0 +1,26 @@ +error: overly complex generic constant + --> $DIR/unused_expr.rs:4:34 + | +LL | fn add() -> [u8; { N + 1; 5 }] { + | ^^^^^^^^^^^^ unused node + | + = help: consider moving this anonymous constant into a `const` function + +error: overly complex generic constant + --> $DIR/unused_expr.rs:9:34 + | +LL | fn div() -> [u8; { N / 1; 5 }] { + | ^^^^^^^^^^^^ unused node + | + = help: consider moving this anonymous constant into a `const` function + +error: overly complex generic constant + --> $DIR/unused_expr.rs:16:38 + | +LL | fn fn_call() -> [u8; { foo(N); 5 }] { + | ^^^^^^^^^^^^^ unused node + | + = help: consider moving this anonymous constant into a `const` function + +error: aborting due to 3 previous errors + From 47cb871f14b48653df2f42082cf93b6c16e2b2f1 Mon Sep 17 00:00:00 2001 From: Bastian Kauschke Date: Fri, 23 Oct 2020 15:04:12 +0200 Subject: [PATCH 2/2] review --- .../src/traits/const_evaluatable.rs | 68 ++++++++++--------- .../unused_expr.stderr | 12 +++- 2 files changed, 45 insertions(+), 35 deletions(-) diff --git a/compiler/rustc_trait_selection/src/traits/const_evaluatable.rs b/compiler/rustc_trait_selection/src/traits/const_evaluatable.rs index cb0950e3627..c79b2624f8c 100644 --- a/compiler/rustc_trait_selection/src/traits/const_evaluatable.rs +++ b/compiler/rustc_trait_selection/src/traits/const_evaluatable.rs @@ -223,6 +223,13 @@ impl AbstractConst<'tcx> { } } +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +struct WorkNode<'tcx> { + node: Node<'tcx>, + span: Span, + used: bool, +} + struct AbstractConstBuilder<'a, 'tcx> { tcx: TyCtxt<'tcx>, body: &'a mir::Body<'tcx>, @@ -232,7 +239,7 @@ struct AbstractConstBuilder<'a, 'tcx> { /// so we store this here. Note that we also consider nodes as used /// if they are mentioned in an assert, so some used nodes are never /// actually reachable by walking the [`AbstractConst`]. - nodes: IndexVec, bool)>, + nodes: IndexVec>, locals: IndexVec, /// We only allow field accesses if they access /// the result of a checked operation. @@ -279,25 +286,25 @@ impl<'a, 'tcx> AbstractConstBuilder<'a, 'tcx> { Ok(Some(builder)) } - fn add_node(&mut self, n: Node<'tcx>) -> NodeId { + fn add_node(&mut self, node: Node<'tcx>, span: Span) -> NodeId { // Mark used nodes. - match n { + match node { Node::Leaf(_) => (), Node::Binop(_, lhs, rhs) => { - self.nodes[lhs].1 = true; - self.nodes[rhs].1 = true; + self.nodes[lhs].used = true; + self.nodes[rhs].used = true; } Node::UnaryOp(_, input) => { - self.nodes[input].1 = true; + self.nodes[input].used = true; } Node::FunctionCall(func, nodes) => { - self.nodes[func].1 = true; - nodes.iter().for_each(|&n| self.nodes[n].1 = true); + self.nodes[func].used = true; + nodes.iter().for_each(|&n| self.nodes[n].used = true); } } // Nodes start as unused. - self.nodes.push((n, false)) + self.nodes.push(WorkNode { node, span, used: false }) } fn place_to_local( @@ -337,7 +344,7 @@ impl<'a, 'tcx> AbstractConstBuilder<'a, 'tcx> { let local = self.place_to_local(span, p)?; Ok(self.locals[local]) } - mir::Operand::Constant(ct) => Ok(self.add_node(Node::Leaf(ct.literal))), + mir::Operand::Constant(ct) => Ok(self.add_node(Node::Leaf(ct.literal), span)), } } @@ -362,19 +369,19 @@ impl<'a, 'tcx> AbstractConstBuilder<'a, 'tcx> { fn build_statement(&mut self, stmt: &mir::Statement<'tcx>) -> Result<(), ErrorReported> { debug!("AbstractConstBuilder: stmt={:?}", stmt); + let span = stmt.source_info.span; match stmt.kind { StatementKind::Assign(box (ref place, ref rvalue)) => { - let local = self.place_to_local(stmt.source_info.span, place)?; + let local = self.place_to_local(span, place)?; match *rvalue { Rvalue::Use(ref operand) => { - self.locals[local] = - self.operand_to_node(stmt.source_info.span, operand)?; + self.locals[local] = self.operand_to_node(span, operand)?; Ok(()) } Rvalue::BinaryOp(op, ref lhs, ref rhs) if Self::check_binop(op) => { - let lhs = self.operand_to_node(stmt.source_info.span, lhs)?; - let rhs = self.operand_to_node(stmt.source_info.span, rhs)?; - self.locals[local] = self.add_node(Node::Binop(op, lhs, rhs)); + let lhs = self.operand_to_node(span, lhs)?; + let rhs = self.operand_to_node(span, rhs)?; + self.locals[local] = self.add_node(Node::Binop(op, lhs, rhs), span); if op.is_checkable() { bug!("unexpected unchecked checkable binary operation"); } else { @@ -382,18 +389,18 @@ impl<'a, 'tcx> AbstractConstBuilder<'a, 'tcx> { } } Rvalue::CheckedBinaryOp(op, ref lhs, ref rhs) if Self::check_binop(op) => { - let lhs = self.operand_to_node(stmt.source_info.span, lhs)?; - let rhs = self.operand_to_node(stmt.source_info.span, rhs)?; - self.locals[local] = self.add_node(Node::Binop(op, lhs, rhs)); + let lhs = self.operand_to_node(span, lhs)?; + let rhs = self.operand_to_node(span, rhs)?; + self.locals[local] = self.add_node(Node::Binop(op, lhs, rhs), span); self.checked_op_locals.insert(local); Ok(()) } Rvalue::UnaryOp(op, ref operand) if Self::check_unop(op) => { - let operand = self.operand_to_node(stmt.source_info.span, operand)?; - self.locals[local] = self.add_node(Node::UnaryOp(op, operand)); + let operand = self.operand_to_node(span, operand)?; + self.locals[local] = self.add_node(Node::UnaryOp(op, operand), span); Ok(()) } - _ => self.error(Some(stmt.source_info.span), "unsupported rvalue")?, + _ => self.error(Some(span), "unsupported rvalue")?, } } // These are not actually relevant for us here, so we can ignore them. @@ -441,7 +448,7 @@ impl<'a, 'tcx> AbstractConstBuilder<'a, 'tcx> { .map(|arg| self.operand_to_node(terminator.source_info.span, arg)) .collect::, _>>()?, ); - self.locals[local] = self.add_node(Node::FunctionCall(func, args)); + self.locals[local] = self.add_node(Node::FunctionCall(func, args), fn_span); Ok(Some(target)) } TerminatorKind::Assert { ref cond, expected: false, target, .. } => { @@ -458,7 +465,7 @@ impl<'a, 'tcx> AbstractConstBuilder<'a, 'tcx> { // // This is needed because division does not use `CheckedBinop` but instead // adds an explicit assert for `divisor != 0`. - self.nodes[self.locals[p]].1 = true; + self.nodes[self.locals[p]].used = true; return Ok(Some(target)); } else if let &[mir::ProjectionElem::Field(ONE_FIELD, _)] = p.projection.as_ref() { // Only allow asserts checking the result of a checked operation. @@ -487,16 +494,13 @@ impl<'a, 'tcx> AbstractConstBuilder<'a, 'tcx> { if let Some(next) = self.build_terminator(block.terminator())? { block = &self.body.basic_blocks()[next]; } else { - assert_eq!(self.locals[mir::Local::from_usize(0)], self.nodes.last().unwrap()); - self.nodes[self.locals[mir::Local::from_usize(0)]].1 = true; - if !self.nodes.iter().all(|n| n.1) { - self.error(None, "unused node")?; + assert_eq!(self.locals[mir::RETURN_PLACE], self.nodes.last().unwrap()); + self.nodes[self.locals[mir::RETURN_PLACE]].used = true; + if let Some(&unused) = self.nodes.iter().find(|n| !n.used) { + self.error(Some(unused.span), "dead code")?; } - return Ok(self - .tcx - .arena - .alloc_from_iter(self.nodes.into_iter().map(|(n, _used)| n))); + return Ok(self.tcx.arena.alloc_from_iter(self.nodes.into_iter().map(|n| n.node))); } } } diff --git a/src/test/ui/const-generics/const_evaluatable_checked/unused_expr.stderr b/src/test/ui/const-generics/const_evaluatable_checked/unused_expr.stderr index 73c6d9393ce..1687dbbcbe3 100644 --- a/src/test/ui/const-generics/const_evaluatable_checked/unused_expr.stderr +++ b/src/test/ui/const-generics/const_evaluatable_checked/unused_expr.stderr @@ -2,7 +2,9 @@ error: overly complex generic constant --> $DIR/unused_expr.rs:4:34 | LL | fn add() -> [u8; { N + 1; 5 }] { - | ^^^^^^^^^^^^ unused node + | ^^-----^^^^^ + | | + | dead code | = help: consider moving this anonymous constant into a `const` function @@ -10,7 +12,9 @@ error: overly complex generic constant --> $DIR/unused_expr.rs:9:34 | LL | fn div() -> [u8; { N / 1; 5 }] { - | ^^^^^^^^^^^^ unused node + | ^^-----^^^^^ + | | + | dead code | = help: consider moving this anonymous constant into a `const` function @@ -18,7 +22,9 @@ error: overly complex generic constant --> $DIR/unused_expr.rs:16:38 | LL | fn fn_call() -> [u8; { foo(N); 5 }] { - | ^^^^^^^^^^^^^ unused node + | ^^------^^^^^ + | | + | dead code | = help: consider moving this anonymous constant into a `const` function