Merge pull request #88 from symphorien/unbound-ident

error diagnostics for unbound identifiers

Author: Ma27

Cargo.lock

@@ -27,6 +27,7 @@ nixpkgs-fmt-rnix = "1.2.0"
 
 [dev-dependencies]
 stoppable_thread = "0.2.1"
+maplit = "1"
 
 [features]
 

Cargo.toml

@@ -1,11 +1,33 @@
+use std::error::Error;
+
 use gc::{Finalize, Trace};
+use rnix::TextRange;
 
 pub const ERR_PARSING: EvalError = EvalError::Internal(InternalError::Parsing);
 
+/// Used for rnix-lsp main functions
+#[derive(Debug, Clone, Trace, Finalize)]
+pub enum AppError {
+    /// Unspecified internal error
+    Internal(String),
+}
+
+impl std::error::Error for AppError {}
+
+impl std::fmt::Display for AppError {
+    fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
+        match self {
+            AppError::Internal(x) => write!(f, "internal error: {}", x),
+        }
+    }
+}
 
+/// Could not evaluate an AST node
 #[derive(Debug, Clone, Trace, Finalize)]
 pub enum EvalError {
+    /// failed to evaluate because of a limitation of our evaluator
     Internal(InternalError),
+    /// the code of the user definitely does not evaluate
     Value(ValueError),
 }
 
@@ -31,19 +53,45 @@ pub enum InternalError {
     Parsing,
 }
 
-#[derive(Debug, Clone, Trace, Finalize)]
+#[derive(Debug, Clone, Trace, Finalize, PartialEq, Eq)]
 /// Used when we're confident that the user/code is at fault, such as
 /// division by zero. We use the parser directly for error reporting,
 /// so the evaluator returns its copies of parsing errors as internal,
 /// silent errors (see InternalError above).
-// TODO: store error text ranges to help the user figure out
-//       exactly where an error is caused
 pub enum ValueError {
+    /// Division by zero
     DivisionByZero,
+    /// Type error
     TypeError(String),
+    /// An attribute name is present twice in the same attrset
     AttrAlreadyDefined(String),
+    /// An identifier is definitely not defined. Argument is the identifer.
+    ///
+    /// Note that this should not be used when one cannot know like
+    /// `with import <nixpkgs> {}; some_attr`
+    UnboundIdentifier(String),
+}
+
+#[derive(Debug, Clone, Trace, Finalize, PartialEq, Eq)]
+/// An error augmented with a location
+pub struct Located<T: Error + Clone + Trace + Finalize> {
+    /// Where the error is located
+    #[unsafe_ignore_trace]
+    pub range: TextRange,
+    /// The nature of the issue
+    pub kind: T
 }
 
+impl<T: Error + Clone + Trace + Finalize> std::error::Error for Located<T> {}
+
+impl<T: Error + Clone + Trace + Finalize> std::fmt::Display for Located<T> {
+    fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
+        write!(f, "{} at {:?}", &self.kind, &self.range)
+    }
+}
+
+impl std::error::Error for ValueError {}
+
 impl std::error::Error for EvalError {}
 
 impl std::fmt::Display for EvalError {
@@ -71,6 +119,7 @@ impl std::fmt::Display for ValueError {
             ValueError::DivisionByZero => write!(f, "division by zero"),
             ValueError::AttrAlreadyDefined(name) => write!(f, "attribute `{}` defined more than once", name),
             ValueError::TypeError(msg) => write!(f, "{}", msg),
+            ValueError::UnboundIdentifier(name) => write!(f, "identifier {} is unbound", name),
         }
     }
 }

src/error.rs

@@ -25,6 +25,7 @@ mod error;
 mod eval;
 mod lookup;
 mod parse;
+mod static_analysis;
 mod scope;
 mod tests;
 mod utils;
@@ -58,6 +59,8 @@ use std::{
     str::FromStr,
 };
 
+use crate::error::AppError;
+
 type Error = Box<dyn std::error::Error>;
 
 fn main() {
@@ -250,21 +253,26 @@ impl App {
         }
     }
 
+    /// Common code of handle_notification between DidOpenTextDocument and DidChangeTextDocument
+    fn handle_content(&mut self, uri: Url, content: String) -> Result<(), Error> {
+        let parsed = rnix::parse(&content);
+        let path = PathBuf::from_str(uri.path());
+        let path = path.unwrap_or_else(|_| PathBuf::from("<unnamed>"));
+        let gc_root = Gc::new(Scope::Root(path));
+        let parsed_root = parsed.root().inner().ok_or(ERR_PARSING);
+        let evaluated = parsed_root.and_then(|x| Expr::parse(x, gc_root));
+        self.files.insert(uri.clone(), (parsed, content, evaluated));
+        self.send_diagnostics(uri)?;
+        Ok(())
+    }
+
     // https://microsoft.github.io/language-server-protocol/specifications/specification-current/#textDocument_didChange
     fn handle_notification(&mut self, req: Notification) -> Result<(), Error> {
         match &*req.method {
             DidOpenTextDocument::METHOD => {
                 let params: DidOpenTextDocumentParams = serde_json::from_value(req.params)?;
                 let text = params.text_document.text;
-                let parsed = rnix::parse(&text);
-                self.send_diagnostics(params.text_document.uri.clone(), &text, &parsed)?;
-                if let Ok(path) = PathBuf::from_str(params.text_document.uri.path()) {
-                    let gc_root = Gc::new(Scope::Root(path));
-                    let parsed_root = parsed.root().inner().ok_or(ERR_PARSING);
-                    let evaluated = parsed_root.and_then(|x| Expr::parse(x, gc_root));
-                    self.files
-                        .insert(params.text_document.uri, (parsed, text, evaluated));
-                }
+                self.handle_content(params.text_document.uri, text)?;
             }
             DidChangeTextDocument::METHOD => {
                 // Per the language server spec (https://git.io/JcrvY), we should apply changes
@@ -319,15 +327,7 @@ impl App {
 
                     content = new_content;
                 }
-                let parsed = rnix::parse(&content);
-                self.send_diagnostics(uri.clone(), &content, &parsed)?;
-                if let Ok(path) = PathBuf::from_str(uri.path()) {
-                    let gc_root = Gc::new(Scope::Root(path));
-                    let parsed_root = parsed.root().inner().ok_or(ERR_PARSING);
-                    let evaluated = parsed_root.and_then(|x| Expr::parse(x, gc_root));
-                    self.files
-                        .insert(uri, (parsed, content.to_owned().to_string(), evaluated));
-                }
+                self.handle_content(uri, content)?;
             }
             _ => (),
         }
@@ -580,7 +580,14 @@ impl App {
 
         Some(lsp_links)
     }
-    fn send_diagnostics(&mut self, uri: Url, code: &str, ast: &AST) -> Result<(), Error> {
+    fn send_diagnostics(&mut self, uri: Url) -> Result<(), Error> {
+        let (ast, code, parsed) = self.files.get(&uri).ok_or_else(|| {
+            AppError::Internal(format!(
+                "send_diagnostics called on unregistered uri {}",
+                &uri
+            ))
+        })?;
+        // errors reported by rnix-parser
         let errors = ast.errors();
         let mut diagnostics = Vec::with_capacity(errors.len());
         for err in errors {
@@ -603,6 +610,26 @@ impl App {
                 });
             }
         }
+        // errors reported by crate::static_analysis
+        let errors = match parsed {
+            Ok(v) => static_analysis::check(v),
+            Err(EvalError::Value(e)) => {
+                let range = TextRange::up_to(TextSize::of(code));
+                vec![error::Located { range, kind: e.clone() }]
+            },
+            Err(EvalError::Internal(_)) => {
+                // don't report false positives
+                vec![]
+            },
+        };
+        for error in errors {
+                diagnostics.push(Diagnostic {
+                    range: utils::range(code, error.range),
+                    severity: Some(DiagnosticSeverity::ERROR),
+                    message: error.kind.to_string(),
+                    ..Diagnostic::default()
+                });
+        }
         self.notify(Notification::new(
             "textDocument/publishDiagnostics".into(),
             PublishDiagnosticsParams {

src/main.rs

@@ -307,9 +307,11 @@ impl Expr {
                     },
                 }
             }
-            ParsedType::Ident(ident) => ExprSource::Ident {
-                name: ident.as_str().to_string(),
-            },
+            ParsedType::Ident(ident) => {
+                ExprSource::Ident {
+                    name: ident.as_str().to_string(),
+                }
+            }
             ParsedType::Dynamic(dynamic) => ExprSource::Dynamic {
                 inner: recurse_box(dynamic.inner().ok_or(ERR_PARSING)?),
             },

src/parse.rs

@@ -0,0 +1,95 @@
+use std::borrow::Borrow;
+
+use rnix::TextRange;
+
+use crate::{
+    error::{EvalError, Located, ValueError},
+    eval::{Expr, ExprSource},
+};
+
+/// If this node is an identifier, should it be a variable name ?
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+enum Ident {
+    /// If this node is an identifier, it should be a variable name
+    IsVariable,
+    /// If this not is an identifier, it is not a variable name
+    ///
+    /// Example: foo in `bar.foo`
+    IsNotVariable,
+}
+
+/// If this is an error, maybe report it to acc. Otherwise, visit all children of this node
+/// to find evaluation errors and collect diagnostics on this accumulator
+fn visit_result<T: Borrow<Expr>>(
+    acc: &mut Vec<Located<ValueError>>,
+    result: &Result<T, EvalError>,
+    range: &Option<TextRange>,
+    ident_ctx: Ident,
+) {
+    match result {
+        Err(EvalError::Value(err)) => {
+            if let Some(range) = range {
+                acc.push(Located {
+                    range: range.clone(),
+                    kind: err.clone(),
+                })
+            }
+        }
+        Err(EvalError::Internal(_)) => {}
+        Ok(v) => visit(acc, v.borrow(), ident_ctx),
+    }
+}
+
+/// Visit all children of this node to find evaluation errors and collect
+/// diagnostics on this accumulator
+fn visit(acc: &mut Vec<Located<ValueError>>, node: &Expr, ident_ctx: Ident) {
+    use Ident::*;
+    match &node.source {
+        ExprSource::AttrSet { definitions } => {
+            for i in definitions.iter() {
+                visit_result(acc, i, &node.range, IsVariable)
+            }
+        }
+        ExprSource::KeyValuePair { key, value } => {
+            visit_result(acc, key, &node.range, IsNotVariable);
+            visit_result(acc, value, &node.range, IsVariable);
+        }
+        ExprSource::Select { from, index } => {
+            visit_result(acc, from, &node.range, IsVariable);
+            visit_result(acc, index, &node.range, IsNotVariable);
+        }
+        ExprSource::Ident { name } => {
+            if ident_ctx == IsVariable {
+                if let Some(range) = &node.range {
+                    // check that the variable is bound
+                    if node.scope.get_let(name).is_none() {
+                        acc.push(Located {
+                            range: range.clone(),
+                            kind: ValueError::UnboundIdentifier(name.clone()),
+                        });
+                    }
+                }
+            }
+        }
+        ExprSource::Literal { .. } => {}
+        ExprSource::UnaryInvert { value: inner }
+        | ExprSource::UnaryNegate { value: inner }
+        | ExprSource::Dynamic { inner }
+        | ExprSource::Paren { inner } => visit_result(acc, inner, &node.range, IsVariable),
+        ExprSource::BinOp { left, right, .. }
+        | ExprSource::BoolAnd { left, right }
+        | ExprSource::Implication { left, right }
+        | ExprSource::BoolOr { left, right } => {
+            visit_result(acc, left, &node.range, IsVariable);
+            visit_result(acc, right, &node.range, IsVariable);
+        }
+    }
+}
+/// Looks for errors in the user code. Attempts to return no false positives.
+///
+/// Current analysis can detect undefined variables.
+pub fn check(node: &Expr) -> Vec<Located<ValueError>> {
+    let mut res = Vec::new();
+    visit(&mut res, node, Ident::IsVariable);
+    res
+}