feat: Add an unused symbols pass (#4745)

* feat: Allow warnings to be reported from the flux compiler

cc #4414 #4741

* feat: Allow semantic visitors to be trait objects

* feat: Add an unused symbols pass

* feat: Add a feature flag for unused variable warnings

* chore: make generate

internal/feature/flags.yml

@@ -99,3 +99,9 @@
   key: optimizeSetTransformation
   default: false
   contact: Jonathan Sternberg
+
+- name: Unused Symbol Warnings
+  description: Enables warnings for unused symbols
+  key: unusedSymbolWarnings
+  default: false
+  contact: Markus Westerlind

libflux/flux-core/src/semantic/mod.rs

@@ -4,6 +4,7 @@ pub mod convert;
 
 mod fs;
 mod infer;
+mod symbols;
 mod vectorize;
 
 #[macro_use]
@@ -76,6 +77,7 @@ pub enum ErrorKind {
     #[error("{0}")]
     Inference(nodes::ErrorKind),
 }
+
 impl From<ast::check::Error> for Error {
     fn from(error: ast::check::Error) -> Self {
         Self {
@@ -84,6 +86,7 @@ impl From<ast::check::Error> for Error {
         }
     }
 }
+
 impl From<convert::Error> for Error {
     fn from(error: convert::Error) -> Self {
         Self {
@@ -92,6 +95,7 @@ impl From<convert::Error> for Error {
         }
     }
 }
+
 impl From<check::Error> for Error {
     fn from(error: check::Error) -> Self {
         Self {
@@ -100,6 +104,7 @@ impl From<check::Error> for Error {
         }
     }
 }
+
 impl From<nodes::Error> for Error {
     fn from(error: nodes::Error) -> Self {
         Self {
@@ -108,12 +113,24 @@ impl From<nodes::Error> for Error {
         }
     }
 }
+
 impl From<Errors<nodes::Error>> for Errors<Error> {
     fn from(error: Errors<nodes::Error>) -> Self {
         error.into_iter().map(Error::from).collect()
     }
 }
 
+/// `Warning` represents any warning emitted by the flux compiler
+pub type Warning = Located<WarningKind>;
+
+/// `WarningKind` exposes details about where in the type analysis process a warning occurred.
+#[derive(Error, Debug, PartialEq)]
+pub enum WarningKind {
+    /// An unused symbol was found in the source
+    #[error("symbol {0} is never used")]
+    UnusedSymbol(String),
+}
+
 /// An environment of values that are available outside of a package
 #[derive(Debug, Clone, PartialEq)]
 pub struct PackageExports {
@@ -291,12 +308,33 @@ pub struct FileErrors {
     /// The source for this error, if one exists
     // TODO Change the API such that we must always provide the source?
     pub source: Option<String>,
+
     #[source]
-    /// The errors the occurred in that file
-    pub errors: Errors<Located<ErrorKind>>,
+    /// The collection of diagnostics
+    pub diagnostics: Diagnostics<ErrorKind, WarningKind>,
 }
 
 impl fmt::Display for FileErrors {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        self.diagnostics.fmt(f)
+    }
+}
+
+/// A collection of diagnostics
+#[derive(Error, Debug, PartialEq)]
+pub struct Diagnostics<E, W> {
+    #[source]
+    /// The errors the occurred in that file
+    pub errors: Errors<Located<E>>,
+    /// The warnings the occurred in that file
+    pub warnings: Errors<Located<W>>,
+}
+
+impl<E, W> fmt::Display for Diagnostics<E, W>
+where
+    E: fmt::Display,
+    W: fmt::Display,
+{
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         // TODO Use codespan's formatting for errors
         self.errors.fmt(f)
@@ -358,7 +396,7 @@ impl FileErrors {
                 // Mirror println! by ignoring errors
                 let _ = self.print_config(&term::Config::default(), source, &mut stdout);
             }
-            None => println!("{}", self.errors),
+            None => println!("{}", self.diagnostics),
         }
     }
 
@@ -378,25 +416,29 @@ impl FileErrors {
         writer: &mut dyn WriteColor,
     ) -> Result<(), codespan_reporting::files::Error> {
         let files = codespan_reporting::files::SimpleFile::new(&self.file[..], source);
-        for err in &self.errors {
-            err.pretty_fmt(config, &files, writer)?;
+        for warn in &self.diagnostics.warnings {
+            pretty_fmt(warn, config, &files, writer)?;
+        }
+        for err in &self.diagnostics.errors {
+            pretty_fmt(err, config, &files, writer)?;
         }
         Ok(())
     }
 }
 
-impl Error {
-    fn pretty_fmt(
-        &self,
-        config: &term::Config,
-        files: &codespan_reporting::files::SimpleFile<&str, &str>,
-        writer: &mut dyn WriteColor,
-    ) -> Result<(), codespan_reporting::files::Error> {
-        let diagnostic = self.as_diagnostic(files);
-
-        term::emit(writer, config, files, &diagnostic)?;
-        Ok(())
-    }
+fn pretty_fmt<E>(
+    err: &Located<E>,
+    config: &term::Config,
+    files: &codespan_reporting::files::SimpleFile<&str, &str>,
+    writer: &mut dyn WriteColor,
+) -> Result<(), codespan_reporting::files::Error>
+where
+    E: AsDiagnostic,
+{
+    let diagnostic = err.as_diagnostic(files);
+
+    term::emit(writer, config, files, &diagnostic)?;
+    Ok(())
 }
 
 impl AsDiagnostic for ErrorKind {
@@ -410,6 +452,12 @@ impl AsDiagnostic for ErrorKind {
     }
 }
 
+impl AsDiagnostic for WarningKind {
+    fn as_diagnostic(&self, _source: &dyn Source) -> diagnostic::Diagnostic<()> {
+        diagnostic::Diagnostic::warning().with_message(self.to_string())
+    }
+}
+
 /// Analyzer provides an API for analyzing Flux code.
 #[derive(Default)]
 pub struct Analyzer<'env, I: import::Importer> {
@@ -433,6 +481,9 @@ pub enum Feature {
 
     /// Enables label polymorphism
     LabelPolymorphism,
+
+    /// Enables warnings for unused symbols
+    UnusedSymbolWarnings,
 }
 
 /// A set of configuration options for the behavior of an Analyzer.
@@ -536,12 +587,22 @@ impl<'env, I: import::Importer> Analyzer<'env, I> {
 
         let mut sem_pkg = nodes::inject_pkg_types(sem_pkg, sub);
 
+        let mut warnings = Errors::new();
+
+        if self
+            .config
+            .features
+            .contains(&Feature::UnusedSymbolWarnings)
+        {
+            warnings.extend(symbols::unused_symbols(&sem_pkg));
+        }
+
         if errors.has_errors() {
             return Err(Salvage {
                 error: FileErrors {
                     file: sem_pkg.package.clone(),
                     source: None,
-                    errors,
+                    diagnostics: Diagnostics { errors, warnings },
                 },
                 value: Some((env, sem_pkg)),
             });

libflux/flux-core/src/semantic/symbols.rs

@@ -0,0 +1,107 @@
+use crate::{
+    ast,
+    errors::located,
+    map::HashSet,
+    semantic::{
+        nodes::{Package, Statement},
+        walk::{walk, Node, Visitor},
+        Symbol, Warning, WarningKind,
+    },
+};
+
+pub struct DefinitionVisitor<F> {
+    consumer: F,
+}
+
+impl<F> DefinitionVisitor<F> {
+    pub fn new(consumer: F) -> Self {
+        DefinitionVisitor { consumer }
+    }
+}
+
+impl<'a, F> Visitor<'a> for DefinitionVisitor<F>
+where
+    F: FnMut(&'a Symbol, &'a ast::SourceLocation, Node<'a>),
+{
+    fn visit(&mut self, node: Node<'a>) -> bool {
+        match node {
+            Node::VariableAssgn(va) => (self.consumer)(&va.id.name, &va.id.loc, node),
+            Node::BuiltinStmt(builtin) => (self.consumer)(&builtin.id.name, &builtin.id.loc, node),
+            Node::ImportDeclaration(import) => {
+                (self.consumer)(&import.import_symbol, &import.loc, node)
+            }
+            Node::FunctionExpr(func) => {
+                for param in &func.params {
+                    (self.consumer)(&param.key.name, &param.key.loc, node);
+                }
+            }
+            _ => (),
+        }
+        true
+    }
+}
+
+pub struct UseVisitor<F> {
+    consumer: F,
+}
+
+impl<F> UseVisitor<F> {
+    pub fn new(consumer: F) -> Self {
+        UseVisitor { consumer }
+    }
+}
+
+impl<'a, F> Visitor<'a> for UseVisitor<F>
+where
+    F: FnMut(&Symbol, &ast::SourceLocation),
+{
+    fn visit(&mut self, node: Node<'a>) -> bool {
+        match node {
+            Node::IdentifierExpr(id) => (self.consumer)(&id.name, &id.loc),
+            Node::File(file) => {
+                // All top-level bindings are "used" through being exported
+                for stmt in &file.body {
+                    match stmt {
+                        Statement::Variable(va) => (self.consumer)(&va.id.name, &va.loc),
+                        Statement::Builtin(builtin) => {
+                            (self.consumer)(&builtin.id.name, &builtin.loc)
+                        }
+                        _ => (),
+                    }
+                }
+            }
+            _ => (),
+        }
+        true
+    }
+}
+
+pub fn unused_symbols(node: &Package) -> Vec<Warning> {
+    let mut uses = HashSet::new();
+
+    walk(
+        &mut UseVisitor::new(|symbol: &Symbol, _: &ast::SourceLocation| {
+            uses.insert(symbol.clone());
+        }),
+        node.into(),
+    );
+
+    let mut warnings = Vec::new();
+
+    walk(
+        &mut DefinitionVisitor::new(|id, loc: &ast::SourceLocation, node| {
+            // Function parameters are part of the function type so users may need to
+            // specify a parameter for the program to type check even if they do not use it.
+            // So we do not emit warnings for unused function parameters.
+            if !matches!(node, Node::FunctionExpr(_)) && !uses.contains(id) {
+                warnings.push(located(
+                    loc.clone(),
+                    WarningKind::UnusedSymbol(id.to_string()),
+                ));
+            }
+        }),
+        node.into(),
+    );
+
+    warnings
+}