From 8a3e558cbd1bbe28610b9d9399962c22e5a5029d Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Tue, 29 Oct 2024 19:10:43 +0100 Subject: [PATCH] Don't produce redundant unreachable warnings When comments are included, we no longer lower these to anything in sequences of values. When used as single expressions (e.g. a value of a `let`), we lower them directly to Nil instead of Noop. The result is that we no longer produce redundant "this code is unreachable" diagnostics when a comment follows something that makes it unreachable (e.g. a return). This fixes https://github.com/inko-lang/inko/issues/772. Changelog: fixed --- compiler/src/hir.rs | 237 ++++++++++++++++++---- compiler/src/mir/passes.rs | 5 - compiler/src/test.rs | 9 + compiler/src/type_check/expressions.rs | 1 - std/fixtures/diagnostics/unreachable.inko | 21 ++ 5 files changed, 223 insertions(+), 50 deletions(-) create mode 100644 std/fixtures/diagnostics/unreachable.inko diff --git a/compiler/src/hir.rs b/compiler/src/hir.rs index 318f3005..044c1b1d 100644 --- a/compiler/src/hir.rs +++ b/compiler/src/hir.rs @@ -506,11 +506,6 @@ pub(crate) struct Try { pub(crate) return_type: types::TypeRef, } -#[derive(Clone, Debug, PartialEq, Eq)] -pub(crate) struct Noop { - pub(crate) location: Location, -} - #[derive(Clone, Debug, PartialEq, Eq)] pub(crate) struct SizeOf { pub(crate) argument: Type, @@ -555,7 +550,6 @@ pub(crate) enum Expression { TypeCast(Box), Recover(Box), Try(Box), - Noop(Box), SizeOf(Box), } @@ -597,7 +591,6 @@ impl Expression { Expression::TypeCast(ref n) => n.location, Expression::Recover(ref n) => n.location, Expression::Try(ref n) => n.location, - Expression::Noop(ref n) => n.location, Expression::SizeOf(ref n) => n.location, } } @@ -2116,6 +2109,42 @@ impl<'a> LowerToHir<'a> { ); } + let var_ref = Expression::IdentifierRef(Box::new(IdentifierRef { + name: ARRAY_LIT_VAR.to_string(), + kind: types::IdentifierKind::Unknown, + location: node.location, + })); + + let mut pushes = Vec::new(); + + for n in node.values { + if let ast::Expression::Comment(_) = n { + continue; + } + + let arg = self.expression(n); + let loc = arg.location(); + let push = Expression::Call(Box::new(Call { + kind: types::CallKind::Unknown, + receiver: Some(var_ref.clone()), + name: Identifier { + name: ARRAY_PUSH.to_string(), + location: node.location, + }, + parens: true, + in_mut: false, + arguments: vec![Argument::Positional(Box::new( + PositionalArgument { + value: arg, + expected_type: types::TypeRef::Unknown, + }, + ))], + location: loc, + })); + + pushes.push(push); + } + let def_var = Expression::DefineVariable(Box::new(DefineVariable { resolved_type: types::TypeRef::Unknown, variable_id: None, @@ -2145,7 +2174,7 @@ impl<'a> LowerToHir<'a> { arguments: vec![Argument::Positional(Box::new( PositionalArgument { value: Expression::Int(Box::new(IntLiteral { - value: node.values.len() as _, + value: pushes.len() as _, resolved_type: types::TypeRef::Unknown, location: node.location, })), @@ -2157,38 +2186,9 @@ impl<'a> LowerToHir<'a> { location: node.location, })); - let var_ref = Expression::IdentifierRef(Box::new(IdentifierRef { - name: ARRAY_LIT_VAR.to_string(), - kind: types::IdentifierKind::Unknown, - location: node.location, - })); - let mut body = vec![def_var]; - for n in node.values { - let arg = self.expression(n); - let loc = arg.location(); - let push = Expression::Call(Box::new(Call { - kind: types::CallKind::Unknown, - receiver: Some(var_ref.clone()), - name: Identifier { - name: ARRAY_PUSH.to_string(), - location: node.location, - }, - parens: true, - in_mut: false, - arguments: vec![Argument::Positional(Box::new( - PositionalArgument { - value: arg, - expected_type: types::TypeRef::Unknown, - }, - ))], - location: loc, - })); - - body.push(push); - } - + body.append(&mut pushes); body.push(var_ref); Expression::Scope(Box::new(Scope { @@ -2277,7 +2277,20 @@ impl<'a> LowerToHir<'a> { } fn values(&mut self, nodes: Vec) -> Vec { - nodes.into_iter().map(|n| self.expression(n)).collect() + nodes + .into_iter() + .filter_map(|n| { + // Comments in sequences of values aren't useful in HIR, and + // keeping them around somehow (e.g. by producing a Nil node) + // may result in redundant unreachable code warnings, so we get + // rid of comments here. + if let ast::Expression::Comment(_) = n { + None + } else { + Some(self.expression(n)) + } + }) + .collect() } fn expression(&mut self, node: ast::Expression) -> Expression { @@ -2400,9 +2413,10 @@ impl<'a> LowerToHir<'a> { ast::Expression::Tuple(node) => { Expression::Tuple(self.tuple_literal(*node)) } - ast::Expression::Comment(c) => { - Expression::Noop(Box::new(Noop { location: c.location })) - } + ast::Expression::Comment(c) => Expression::Nil(Box::new(Nil { + resolved_type: types::TypeRef::Unknown, + location: c.location, + })), } } @@ -2515,7 +2529,10 @@ impl<'a> LowerToHir<'a> { node.location, ); - Expression::Noop(Box::new(Noop { location: node.location })) + Expression::Nil(Box::new(Nil { + resolved_type: types::TypeRef::Unknown, + location: node.location, + })) } } @@ -3225,7 +3242,7 @@ impl<'a> LowerToHir<'a> { mod tests { use super::*; use crate::config::Config; - use crate::test::cols; + use crate::test::{cols, loc}; use ::ast::parser::Parser; use similar_asserts::assert_eq; use types::module_name::ModuleName; @@ -3240,6 +3257,16 @@ mod tests { ParsedModule { ast, name } } + #[track_caller] + fn parse_with_comments(input: &str) -> ParsedModule { + let name = ModuleName::new("std.foo"); + let ast = Parser::with_comments(input.into(), "test.inko".into()) + .parse() + .expect("failed to parse the module"); + + ParsedModule { ast, name } + } + #[track_caller] fn lower(input: &str) -> (Module, usize) { let mut state = State::new(Config::new()); @@ -3249,6 +3276,15 @@ mod tests { (hir.pop().unwrap(), state.diagnostics.iter().count()) } + #[track_caller] + fn lower_with_comments(input: &str) -> (Module, usize) { + let mut state = State::new(Config::new()); + let ast = parse_with_comments(input); + let mut hir = LowerToHir::run_all(&mut state, vec![ast]); + + (hir.pop().unwrap(), state.diagnostics.iter().count()) + } + #[track_caller] fn lower_top_expr(input: &str) -> (TopLevelExpression, usize) { let (mut module, diags) = lower(input); @@ -3286,6 +3322,21 @@ mod tests { } } + #[track_caller] + fn lower_expr_with_comments(input: &str) -> (Expression, usize) { + let (mut top, diags) = lower_with_comments(input); + let hir = top.expressions.remove(0); + + match hir { + TopLevelExpression::ModuleMethod(mut node) => { + (node.body.remove(0), diags) + } + _ => { + panic!("the top-level expression must be a module method") + } + } + } + #[test] fn test_lower_module() { let (hir, diags) = lower(" "); @@ -5420,6 +5471,104 @@ mod tests { ); } + #[test] + fn test_lower_array_with_comments() { + let hir = lower_expr_with_comments( + "fn a { + [ + # foo + ] + }", + ) + .0; + + assert_eq!( + hir, + Expression::Scope(Box::new(Scope { + resolved_type: types::TypeRef::Unknown, + body: vec![ + Expression::DefineVariable(Box::new(DefineVariable { + resolved_type: types::TypeRef::Unknown, + variable_id: None, + mutable: false, + name: Identifier { + name: "$array".to_string(), + location: loc(2, 4, 15, 15), + }, + value_type: None, + value: Expression::Call(Box::new(Call { + kind: types::CallKind::Unknown, + receiver: Some(Expression::ConstantRef(Box::new( + ConstantRef { + kind: types::ConstantKind::Unknown, + source: None, + name: "$Array".to_string(), + resolved_type: types::TypeRef::Unknown, + location: loc(2, 4, 15, 15), + } + ))), + name: Identifier { + name: "with_capacity".to_string(), + location: loc(2, 4, 15, 15), + }, + parens: true, + in_mut: false, + arguments: vec![Argument::Positional(Box::new( + PositionalArgument { + value: Expression::Int(Box::new( + IntLiteral { + value: 0, + resolved_type: + types::TypeRef::Unknown, + location: loc(2, 4, 15, 15), + } + )), + expected_type: types::TypeRef::Unknown, + } + ))], + location: loc(2, 4, 15, 15), + },)), + location: loc(2, 4, 15, 15), + })), + Expression::IdentifierRef(Box::new(IdentifierRef { + name: "$array".to_string(), + kind: types::IdentifierKind::Unknown, + location: loc(2, 4, 15, 15), + })), + ], + location: loc(2, 4, 15, 15), + })) + ); + } + + #[test] + fn test_lower_tuple_with_comments() { + let hir = lower_expr_with_comments( + "fn a { + ( + 10, + # foo + ) +}", + ) + .0; + + assert_eq!( + hir, + Expression::Tuple(Box::new(TupleLiteral { + class_id: None, + resolved_type: types::TypeRef::Unknown, + value_types: Vec::new(), + values: vec![Expression::Int(Box::new(IntLiteral { + resolved_type: types::TypeRef::Unknown, + value: 10, + location: loc(3, 3, 5, 6) + }))], + location: loc(2, 5, 3, 3) + })) + ); + } + #[test] fn test_lower_tuple() { let hir = lower_expr("fn a { (10,) }").0; diff --git a/compiler/src/mir/passes.rs b/compiler/src/mir/passes.rs index 6030f906..155fae97 100644 --- a/compiler/src/mir/passes.rs +++ b/compiler/src/mir/passes.rs @@ -1485,7 +1485,6 @@ impl<'a> LowerMethod<'a> { hir::Expression::TypeCast(n) => self.type_cast(*n), hir::Expression::Recover(n) => self.recover_expression(*n), hir::Expression::Try(n) => self.try_expression(*n), - hir::Expression::Noop(n) => self.noop(n.location), hir::Expression::SizeOf(n) => self.size_of(*n), } } @@ -2427,10 +2426,6 @@ impl<'a> LowerMethod<'a> { out_reg } - fn noop(&mut self, location: Location) -> RegisterId { - self.get_nil(InstructionLocation::new(location)) - } - fn size_of(&mut self, node: hir::SizeOf) -> RegisterId { let loc = InstructionLocation::new(node.location); let reg = self.new_register(TypeRef::int()); diff --git a/compiler/src/test.rs b/compiler/src/test.rs index 5ee3bc30..df795bc7 100644 --- a/compiler/src/test.rs +++ b/compiler/src/test.rs @@ -8,6 +8,15 @@ use types::{ DROP_TRAIT, }; +pub(crate) fn loc( + line_start: u32, + line_end: u32, + column_start: u32, + column_end: u32, +) -> Location { + Location { line_start, line_end, column_start, column_end } +} + pub(crate) fn cols(start: u32, stop: u32) -> Location { Location { line_start: 1, diff --git a/compiler/src/type_check/expressions.rs b/compiler/src/type_check/expressions.rs index ab2cf6a3..f2523fcd 100644 --- a/compiler/src/type_check/expressions.rs +++ b/compiler/src/type_check/expressions.rs @@ -1427,7 +1427,6 @@ impl<'a> CheckMethodBody<'a> { hir::Expression::Tuple(ref mut n) => self.tuple_literal(n, scope), hir::Expression::TypeCast(ref mut n) => self.type_cast(n, scope), hir::Expression::Try(ref mut n) => self.try_expression(n, scope), - hir::Expression::Noop(_) => TypeRef::nil(), hir::Expression::SizeOf(ref mut n) => self.size_of(n), } } diff --git a/std/fixtures/diagnostics/unreachable.inko b/std/fixtures/diagnostics/unreachable.inko new file mode 100644 index 00000000..5b47818b --- /dev/null +++ b/std/fixtures/diagnostics/unreachable.inko @@ -0,0 +1,21 @@ +class Example { + fn example1 { + return + 42 + } + + fn example2 { + return + self + } + + fn example3 { + loop {} + + 42 + } +} + +# unreachable.inko:4:5 warning(unreachable): this code is unreachable +# unreachable.inko:9:5 warning(unreachable): this code is unreachable +# unreachable.inko:15:5 warning(unreachable): this code is unreachable