From a5e505e6b09dabe05ae6e9353023ce9a3207f340 Mon Sep 17 00:00:00 2001 From: KtorZ Date: Thu, 16 Mar 2023 16:51:30 +0100 Subject: [PATCH] Remove unused let-binding from type-checking The typed-AST produced as a result of type-checking the program will no longer contain unused let-bindings. They still raise warnings in the code so that developers are aware that they are being ignore. This is mainly done to prevent mistakes for people coming from an imperative background who may think that things like: ``` let _ = foo(...) ``` should have some side-effects. It does not, and it's similar to assigned variables that are never used / evaluated. We now properly strip those elements from the AST when encountered and raise proper warnings, even for discarded values. --- crates/aiken-lang/src/tests/check.rs | 58 ++++++++++- crates/aiken-lang/src/tipo/expr.rs | 133 ++++++++++++++------------ crates/aiken-lang/src/tipo/pattern.rs | 24 +++-- 3 files changed, 146 insertions(+), 69 deletions(-) diff --git a/crates/aiken-lang/src/tests/check.rs b/crates/aiken-lang/src/tests/check.rs index 40227927..0bf369a7 100644 --- a/crates/aiken-lang/src/tests/check.rs +++ b/crates/aiken-lang/src/tests/check.rs @@ -1,6 +1,8 @@ use crate::{ - ast::{ModuleKind, Tracing, TypedModule, UntypedModule}, - builtins, parser, + ast::{Definition, ModuleKind, Tracing, TypedModule, UntypedModule}, + builtins, + expr::TypedExpr, + parser, tipo::error::{Error, Warning}, IdGenerator, }; @@ -319,3 +321,55 @@ fn utf8_hex_literal_warning() { Warning::Utf8ByteArrayIsValidHexString { .. } )) } + +#[test] +fn discarded_let_bindings() { + let source_code = r#" + fn foo() { + let result = when 42 is { + 1 -> { + let unused = "foo" + Void + } + _ -> { + Void + } + } + + let _ = "foo" + + result + } + "#; + + let (warnings, ast) = check(parse(source_code)).unwrap(); + + assert!(matches!(warnings[0], Warning::UnusedVariable { ref name, .. } if name == "unused")); + assert!(matches!(warnings[1], Warning::UnusedVariable { ref name, .. } if name == "_")); + + // Controls that unused let-bindings have been erased from the transformed AST. + match ast.definitions.first() { + Some(Definition::Fn(def)) => match &def.body { + TypedExpr::Sequence { expressions, .. } => { + assert_eq!(expressions.len(), 2); + assert!( + matches!(expressions[1], TypedExpr::Var { .. }), + "last expression isn't return variable" + ); + match &expressions[0] { + TypedExpr::Assignment { value, .. } => match **value { + TypedExpr::When { ref clauses, .. } => { + assert!( + matches!(clauses[0].then, TypedExpr::Sequence { ref expressions, ..} if expressions.len() == 1) + ) + } + _ => unreachable!("first expression isn't when/is"), + }, + _ => unreachable!("first expression isn't assignment"), + } + } + _ => unreachable!("body isn't a Sequence"), + }, + _ => unreachable!("ast isn't a Fn"), + } +} diff --git a/crates/aiken-lang/src/tipo/expr.rs b/crates/aiken-lang/src/tipo/expr.rs index e98c17ce..2042ae7a 100644 --- a/crates/aiken-lang/src/tipo/expr.rs +++ b/crates/aiken-lang/src/tipo/expr.rs @@ -912,8 +912,7 @@ impl<'a, 'b> ExprTyper<'a, 'b> { annotation: &Option, location: Span, ) -> Result { - let typed_value = - self.in_new_scope(|value_typer| value_typer.infer(untyped_value.clone()))?; + let typed_value = self.infer(untyped_value.clone())?; let mut value_typ = typed_value.tipo(); let value_is_data = value_typ.is_data(); @@ -938,6 +937,7 @@ impl<'a, 'b> ExprTyper<'a, 'b> { untyped_pattern.clone(), value_typ.clone(), Some(ann_typ), + kind.is_let(), )? } else { if value_is_data && !untyped_pattern.is_var() && !untyped_pattern.is_discard() { @@ -963,6 +963,7 @@ impl<'a, 'b> ExprTyper<'a, 'b> { untyped_pattern.clone(), value_typ.clone(), None, + kind.is_let(), )? }; @@ -1102,22 +1103,17 @@ impl<'a, 'b> ExprTyper<'a, 'b> { location, } = clause; - let (guard, then, typed_pattern, typed_alternatives) = - self.in_new_scope(|clause_typer| { - // Check the types - let (typed_pattern, typed_alternatives) = clause_typer.infer_clause_pattern( - pattern, - alternative_patterns, - subjects, - &location, - )?; + let (guard, then, typed_pattern, typed_alternatives) = self.in_new_scope(|scope| { + // Check the types + let (typed_pattern, typed_alternatives) = + scope.infer_clause_pattern(pattern, alternative_patterns, subjects, &location)?; - let guard = clause_typer.infer_optional_clause_guard(guard)?; + let guard = scope.infer_optional_clause_guard(guard)?; - let then = clause_typer.infer(then)?; + let then = scope.infer(then)?; - Ok::<_, Error>((guard, then, typed_pattern, typed_alternatives)) - })?; + Ok::<_, Error>((guard, then, typed_pattern, typed_alternatives)) + })?; Ok(Clause { location, @@ -1421,7 +1417,7 @@ impl<'a, 'b> ExprTyper<'a, 'b> { false, )?; - let body = self.in_new_scope(|body_typer| body_typer.infer(first.body.clone()))?; + let body = self.infer(first.body.clone())?; let tipo = body.tipo(); @@ -1441,7 +1437,7 @@ impl<'a, 'b> ExprTyper<'a, 'b> { false, )?; - let body = self.in_new_scope(|body_typer| body_typer.infer(branch.body.clone()))?; + let body = self.infer(branch.body.clone())?; self.unify( tipo.clone(), @@ -1457,7 +1453,7 @@ impl<'a, 'b> ExprTyper<'a, 'b> { }); } - let typed_final_else = self.in_new_scope(|body_typer| body_typer.infer(final_else))?; + let typed_final_else = self.infer(final_else)?; self.unify( tipo.clone(), @@ -1507,30 +1503,28 @@ impl<'a, 'b> ExprTyper<'a, 'b> { ) -> Result<(Vec, TypedExpr), Error> { self.assert_no_assignment(&body)?; - let (body_rigid_names, body_infer) = self.in_new_scope(|body_typer| { - for (arg, t) in args.iter().zip(args.iter().map(|arg| arg.tipo.clone())) { - match &arg.arg_name { - ArgName::Named { name, .. } => { - body_typer.environment.insert_variable( - name.to_string(), - ValueConstructorVariant::LocalVariable { - location: arg.location, - }, - t, - ); + for (arg, t) in args.iter().zip(args.iter().map(|arg| arg.tipo.clone())) { + match &arg.arg_name { + ArgName::Named { name, .. } => { + self.environment.insert_variable( + name.to_string(), + ValueConstructorVariant::LocalVariable { + location: arg.location, + }, + t, + ); - body_typer.environment.init_usage( - name.to_string(), - EntityKind::Variable, - arg.location, - ); - } - ArgName::Discarded { .. } => (), - }; - } + self.environment.init_usage( + name.to_string(), + EntityKind::Variable, + arg.location, + ); + } + ArgName::Discarded { .. } => (), + }; + } - (body_typer.hydrator.rigid_names(), body_typer.infer(body)) - }); + let (body_rigid_names, body_infer) = (self.hydrator.rigid_names(), self.infer(body)); let body = body_infer.map_err(|e| e.with_unify_error_rigid_names(&body_rigid_names))?; @@ -1625,26 +1619,51 @@ impl<'a, 'b> ExprTyper<'a, 'b> { } fn infer_seq(&mut self, location: Span, untyped: Vec) -> Result { - let count = untyped.len(); + let sequence = self.in_new_scope(|scope| { + let count = untyped.len(); - let mut expressions = Vec::with_capacity(count); + let mut expressions = Vec::with_capacity(count); - for (i, expression) in untyped.into_iter().enumerate() { - match i.cmp(&(count - 1)) { - // When the expression is the last in a sequence, we enforce it is NOT - // an assignment (kind of treat assignments like statements). - Ordering::Equal => self.assert_no_assignment(&expression)?, + for (i, expression) in untyped.into_iter().enumerate() { + match i.cmp(&(count - 1)) { + // When the expression is the last in a sequence, we enforce it is NOT + // an assignment (kind of treat assignments like statements). + Ordering::Equal => scope.assert_no_assignment(&expression)?, - // This isn't the final expression in the sequence, so it *must* - // be a let-binding; we do not allow anything else. - Ordering::Less => self.assert_assignment(&expression)?, + // This isn't the final expression in the sequence, so it *must* + // be a let-binding; we do not allow anything else. + Ordering::Less => scope.assert_assignment(&expression)?, - // Can't actually happen - Ordering::Greater => (), + // Can't actually happen + Ordering::Greater => (), + } + + expressions.push(scope.infer(expression)?); } - expressions.push(self.infer(expression)?); - } + Ok(expressions) + })?; + + let unused = self + .environment + .warnings + .iter() + .filter_map(|w| match w { + Warning::UnusedVariable { location, .. } => Some(*location), + _ => None, + }) + .collect::>(); + + let expressions = sequence + .into_iter() + .filter(|expr| { + if let TypedExpr::Assignment { pattern, .. } = expr { + !unused.contains(&pattern.location()) + } else { + true + } + }) + .collect::>(); Ok(TypedExpr::Sequence { location, @@ -1874,11 +1893,7 @@ impl<'a, 'b> ExprTyper<'a, 'b> { let return_type = self.new_unbound_var(); for subject in subjects { - let subject = self.in_new_scope(|subject_typer| { - let subject = subject_typer.infer(subject)?; - - Ok::<_, Error>(subject) - })?; + let subject = self.infer(subject)?; subject_types.push(subject.tipo()); diff --git a/crates/aiken-lang/src/tipo/pattern.rs b/crates/aiken-lang/src/tipo/pattern.rs index ad3d5b1f..57f5ee86 100644 --- a/crates/aiken-lang/src/tipo/pattern.rs +++ b/crates/aiken-lang/src/tipo/pattern.rs @@ -145,7 +145,7 @@ impl<'a, 'b> PatternTyper<'a, 'b> { // Unify each pattern in the multi-pattern with the corresponding subject let mut typed_multi = Vec::with_capacity(multi_pattern.len()); for (pattern, subject_type) in multi_pattern.into_iter().zip(subjects) { - let pattern = self.unify(pattern, subject_type.clone(), None)?; + let pattern = self.unify(pattern, subject_type.clone(), None, false)?; typed_multi.push(pattern); } Ok(typed_multi) @@ -159,9 +159,17 @@ impl<'a, 'b> PatternTyper<'a, 'b> { pattern: UntypedPattern, tipo: Arc, ann_type: Option>, + is_assignment: bool, ) -> Result { match pattern { - Pattern::Discard { name, location } => Ok(Pattern::Discard { name, location }), + Pattern::Discard { name, location } => { + if is_assignment { + // Register declaration for the unused variable detection + self.environment + .init_usage(name.to_string(), EntityKind::Variable, location); + }; + Ok(Pattern::Discard { name, location }) + } Pattern::Var { name, location } => { self.insert_variable(&name, ann_type.unwrap_or(tipo), location, location)?; @@ -181,7 +189,7 @@ impl<'a, 'b> PatternTyper<'a, 'b> { pattern.location(), )?; - let pattern = self.unify(*pattern, tipo, ann_type)?; + let pattern = self.unify(*pattern, tipo, ann_type, false)?; Ok(Pattern::Assign { name, @@ -209,11 +217,11 @@ impl<'a, 'b> PatternTyper<'a, 'b> { let elements = elements .into_iter() - .map(|element| self.unify(element, tipo.clone(), None)) + .map(|element| self.unify(element, tipo.clone(), None, false)) .try_collect()?; let tail = match tail { - Some(tail) => Some(Box::new(self.unify(*tail, list(tipo), None)?)), + Some(tail) => Some(Box::new(self.unify(*tail, list(tipo), None, false)?)), None => None, }; @@ -246,7 +254,7 @@ impl<'a, 'b> PatternTyper<'a, 'b> { let mut patterns = vec![]; for (pattern, typ) in elems.into_iter().zip(type_elems) { - let typed_pattern = self.unify(pattern, typ.clone(), None)?; + let typed_pattern = self.unify(pattern, typ.clone(), None, false)?; patterns.push(typed_pattern); } @@ -268,7 +276,7 @@ impl<'a, 'b> PatternTyper<'a, 'b> { let mut patterns = vec![]; for (pattern, type_) in elems.into_iter().zip(elems_types) { - let typed_pattern = self.unify(pattern, type_, None)?; + let typed_pattern = self.unify(pattern, type_, None, false)?; patterns.push(typed_pattern); } @@ -408,7 +416,7 @@ impl<'a, 'b> PatternTyper<'a, 'b> { label, } = arg; - let value = self.unify(value, typ.clone(), None)?; + let value = self.unify(value, typ.clone(), None, false)?; Ok::<_, Error>(CallArg { value,