refactor(tvix/eval): store spans instead of nodes in Warning/Error

Another step towards being able to report accurate errors. The codemap
spans contain strictly more accessible information, as they now retain
information about which input file something came from.

This required some shuffling around in the compiler to thread all the
right information to the right places.

Change-Id: I18ccfb20f07b0c33e1c4f51ca00cd09f7b2d19c6
Reviewed-on: https://cl.tvl.fyi/c/depot/+/6404
Reviewed-by: sterni <sternenseemann@systemli.org>
Tested-by: BuildkiteCI
This commit is contained in:
Vincent Ambo 2022-09-01 19:57:55 +03:00 committed by tazjin
parent 8033300900
commit 55d21a1389
5 changed files with 92 additions and 90 deletions

View file

@ -103,17 +103,18 @@ impl Compiler<'_> {
&mut self.context_mut().scope &mut self.context_mut().scope
} }
fn span_for<N: AstNode>(&self, node: &N) -> codemap::Span {
let rowan_span = node.syntax().text_range();
self.file.span.subspan(
u32::from(rowan_span.start()) as u64,
u32::from(rowan_span.end()) as u64,
)
}
/// Push a single instruction to the current bytecode chunk and /// Push a single instruction to the current bytecode chunk and
/// track the source span from which it was compiled. /// track the source span from which it was compiled.
fn push_op<T: AstNode>(&mut self, data: OpCode, node: &T) -> CodeIdx { fn push_op<T: AstNode>(&mut self, data: OpCode, node: &T) -> CodeIdx {
let span: codemap::Span = { let span = self.span_for(node);
let rowan_span = node.syntax().text_range();
self.file.span.subspan(
u32::from(rowan_span.start()) as u64,
u32::from(rowan_span.end()) as u64,
)
};
self.chunk().push_op(data, span) self.chunk().push_op(data, span)
} }
@ -168,8 +169,9 @@ impl Compiler<'_> {
ast::LiteralKind::Integer(i) => { ast::LiteralKind::Integer(i) => {
self.emit_constant(Value::Integer(i.value().unwrap()), &node); self.emit_constant(Value::Integer(i.value().unwrap()), &node);
} }
ast::LiteralKind::Uri(u) => { ast::LiteralKind::Uri(u) => {
self.emit_warning(node.syntax().clone(), WarningKind::DeprecatedLiteralURL); self.emit_warning(self.span_for(&node), WarningKind::DeprecatedLiteralURL);
self.emit_constant(Value::String(u.syntax().text().into()), &node); self.emit_constant(Value::String(u.syntax().text().into()), &node);
} }
} }
@ -187,7 +189,7 @@ impl Compiler<'_> {
Some(buf) => buf, Some(buf) => buf,
None => { None => {
self.emit_error( self.emit_error(
node.syntax().clone(), self.span_for(&node),
ErrorKind::PathResolution("failed to determine home directory".into()), ErrorKind::PathResolution("failed to determine home directory".into()),
); );
return; return;
@ -645,7 +647,7 @@ impl Compiler<'_> {
// scope is a no-op *if* the identifier can be // scope is a no-op *if* the identifier can be
// statically resolved. // statically resolved.
None if !self.scope().has_with() => { None if !self.scope().has_with() => {
self.emit_warning(inherit.syntax().clone(), WarningKind::UselessInherit); self.emit_warning(self.span_for(&inherit), WarningKind::UselessInherit);
continue; continue;
} }
@ -659,15 +661,12 @@ impl Compiler<'_> {
.resolve_local(ident.ident_token().unwrap().text()), .resolve_local(ident.ident_token().unwrap().text()),
LocalPosition::Known(_) LocalPosition::Known(_)
) { ) {
self.emit_warning(ident.syntax().clone(), WarningKind::UselessInherit); self.emit_warning(self.span_for(&ident), WarningKind::UselessInherit);
continue; continue;
} }
self.compile_ident(slot, ident.clone()); self.compile_ident(slot, ident.clone());
let idx = self.declare_local( let idx = self.declare_local(&ident, ident.ident_token().unwrap().text());
ident.syntax().clone(),
ident.ident_token().unwrap().text(),
);
self.scope_mut().mark_initialised(idx); self.scope_mut().mark_initialised(idx);
} }
} }
@ -679,10 +678,7 @@ impl Compiler<'_> {
self.emit_literal_ident(&ident); self.emit_literal_ident(&ident);
self.push_op(OpCode::OpAttrsSelect, &ident); self.push_op(OpCode::OpAttrsSelect, &ident);
let idx = self.declare_local( let idx = self.declare_local(&ident, ident.ident_token().unwrap().text());
ident.syntax().clone(),
ident.ident_token().unwrap().text(),
);
self.scope_mut().mark_initialised(idx); self.scope_mut().mark_initialised(idx);
} }
} }
@ -704,7 +700,7 @@ impl Compiler<'_> {
// required for resolving recursion. // required for resolving recursion.
let mut entries: Vec<(LocalIdx, ast::Expr)> = vec![]; let mut entries: Vec<(LocalIdx, ast::Expr)> = vec![];
for entry in node.attrpath_values() { for entry in node.attrpath_values() {
let mut path = match normalise_ident_path(entry.attrpath().unwrap().attrs()) { let mut path = match self.normalise_ident_path(entry.attrpath().unwrap().attrs()) {
Ok(p) => p, Ok(p) => p,
Err(err) => { Err(err) => {
self.errors.push(err); self.errors.push(err);
@ -716,10 +712,7 @@ impl Compiler<'_> {
todo!("nested bindings in let expressions :(") todo!("nested bindings in let expressions :(")
} }
let idx = self.declare_local( let idx = self.declare_local(&entry.attrpath().unwrap(), path.pop().unwrap());
entry.attrpath().unwrap().syntax().clone(),
path.pop().unwrap(),
);
entries.push((idx, entry.value().unwrap())); entries.push((idx, entry.value().unwrap()));
} }
@ -787,7 +780,7 @@ impl Compiler<'_> {
} }
if !self.scope().has_with() { if !self.scope().has_with() {
self.emit_error(node.syntax().clone(), ErrorKind::UnknownStaticVariable); self.emit_error(self.span_for(&node), ErrorKind::UnknownStaticVariable);
return; return;
} }
@ -827,7 +820,8 @@ impl Compiler<'_> {
self.compile(slot, node.namespace().unwrap()); self.compile(slot, node.namespace().unwrap());
self.emit_force(&node.namespace().unwrap()); self.emit_force(&node.namespace().unwrap());
let local_idx = self.scope_mut().declare_phantom(); let span = self.span_for(&node.namespace().unwrap());
let local_idx = self.scope_mut().declare_phantom(span);
let with_idx = self.scope().stack_index(local_idx); let with_idx = self.scope().stack_index(local_idx);
self.scope_mut().push_with(); self.scope_mut().push_with();
@ -859,7 +853,7 @@ impl Compiler<'_> {
.text() .text()
.to_string(); .to_string();
let idx = self.declare_local(param.syntax().clone(), &name); let idx = self.declare_local(&param, &name);
self.scope_mut().mark_initialised(idx); self.scope_mut().mark_initialised(idx);
} }
} }
@ -1047,14 +1041,11 @@ impl Compiler<'_> {
// accessed while it existed and emit a warning to the // accessed while it existed and emit a warning to the
// user otherwise. // user otherwise.
if let Some(Local { if let Some(Local {
node: Some(node), span, used, name, ..
used,
name,
..
}) = self.scope_mut().locals.pop() }) = self.scope_mut().locals.pop()
{ {
if !used && !name.starts_with('_') { if !used && !name.starts_with('_') {
self.emit_warning(node, WarningKind::UnusedBinding); self.emit_warning(span, WarningKind::UnusedBinding);
} }
} }
} }
@ -1067,7 +1058,7 @@ impl Compiler<'_> {
/// Declare a local variable known in the scope that is being /// Declare a local variable known in the scope that is being
/// compiled by pushing it to the locals. This is used to /// compiled by pushing it to the locals. This is used to
/// determine the stack offset of variables. /// determine the stack offset of variables.
fn declare_local<S: Into<String>>(&mut self, node: rnix::SyntaxNode, name: S) -> LocalIdx { fn declare_local<S: Into<String>, N: AstNode>(&mut self, node: &N, name: S) -> LocalIdx {
let name = name.into(); let name = name.into();
let depth = self.scope().scope_depth; let depth = self.scope().scope_depth;
@ -1079,7 +1070,10 @@ impl Compiler<'_> {
}; };
if let Some(global_ident) = key { if let Some(global_ident) = key {
self.emit_warning(node.clone(), WarningKind::ShadowedGlobal(global_ident)); self.emit_warning(
self.span_for(node),
WarningKind::ShadowedGlobal(global_ident),
);
self.scope_mut().poison(global_ident, depth); self.scope_mut().poison(global_ident, depth);
} }
@ -1093,12 +1087,13 @@ impl Compiler<'_> {
if shadowed { if shadowed {
self.emit_error( self.emit_error(
node.clone(), self.span_for(node),
ErrorKind::VariableAlreadyDefined(name.clone()), ErrorKind::VariableAlreadyDefined(name.clone()),
); );
} }
self.scope_mut().declare_local(name, node) let span = self.span_for(node);
self.scope_mut().declare_local(name, span)
} }
fn resolve_upvalue( fn resolve_upvalue(
@ -1217,58 +1212,64 @@ impl Compiler<'_> {
self.push_op(OpCode::OpForce, node); self.push_op(OpCode::OpForce, node);
} }
fn emit_warning(&mut self, node: rnix::SyntaxNode, kind: WarningKind) { fn emit_warning(&mut self, span: codemap::Span, kind: WarningKind) {
self.warnings.push(EvalWarning { node, kind }) self.warnings.push(EvalWarning { kind, span })
} }
fn emit_error(&mut self, node: rnix::SyntaxNode, kind: ErrorKind) { fn emit_error(&mut self, span: codemap::Span, kind: ErrorKind) {
self.errors.push(Error { self.errors.push(Error {
node: Some(node),
kind, kind,
span: Some(span),
}) })
} }
}
/// Convert a non-dynamic string expression to a string if possible, /// Convert a non-dynamic string expression to a string if possible,
/// or raise an error. /// or raise an error.
fn expr_str_to_string(expr: ast::Str) -> EvalResult<String> { fn expr_str_to_string(&self, expr: ast::Str) -> EvalResult<String> {
if expr.normalized_parts().len() == 1 { if expr.normalized_parts().len() == 1 {
if let ast::InterpolPart::Literal(s) = expr.normalized_parts().pop().unwrap() { if let ast::InterpolPart::Literal(s) = expr.normalized_parts().pop().unwrap() {
return Ok(s); return Ok(s);
}
}
return Err(Error {
kind: ErrorKind::DynamicKeyInLet(expr.syntax().clone()),
span: Some(self.span_for(&expr)),
});
}
/// Convert a single identifier path fragment to a string if possible,
/// or raise an error about the node being dynamic.
fn attr_to_string(&self, node: ast::Attr) -> EvalResult<String> {
match node {
ast::Attr::Ident(ident) => Ok(ident.ident_token().unwrap().text().into()),
ast::Attr::Str(s) => self.expr_str_to_string(s),
// The dynamic node type is just a wrapper. C++ Nix does not
// care about the dynamic wrapper when determining whether the
// node itself is dynamic, it depends solely on the expression
// inside (i.e. `let ${"a"} = 1; in a` is valid).
ast::Attr::Dynamic(ref dynamic) => match dynamic.expr().unwrap() {
ast::Expr::Str(s) => self.expr_str_to_string(s),
_ => Err(Error {
kind: ErrorKind::DynamicKeyInLet(node.syntax().clone()),
span: Some(self.span_for(&node)),
}),
},
} }
} }
return Err(Error { // Normalises identifier fragments into a single string vector for
node: Some(expr.syntax().clone()), // `let`-expressions; fails if fragments requiring dynamic computation
kind: ErrorKind::DynamicKeyInLet(expr.syntax().clone()), // are encountered.
}); fn normalise_ident_path<I: Iterator<Item = ast::Attr>>(
} &self,
path: I,
/// Convert a single identifier path fragment to a string if possible, ) -> EvalResult<Vec<String>> {
/// or raise an error about the node being dynamic. path.map(|node| self.attr_to_string(node)).collect()
fn attr_to_string(node: ast::Attr) -> EvalResult<String> {
match node {
ast::Attr::Ident(ident) => Ok(ident.ident_token().unwrap().text().into()),
ast::Attr::Str(s) => expr_str_to_string(s),
// The dynamic node type is just a wrapper. C++ Nix does not
// care about the dynamic wrapper when determining whether the
// node itself is dynamic, it depends solely on the expression
// inside (i.e. `let ${"a"} = 1; in a` is valid).
ast::Attr::Dynamic(ref dynamic) => match dynamic.expr().unwrap() {
ast::Expr::Str(s) => expr_str_to_string(s),
_ => Err(ErrorKind::DynamicKeyInLet(node.syntax().clone()).into()),
},
} }
} }
// Normalises identifier fragments into a single string vector for
// `let`-expressions; fails if fragments requiring dynamic computation
// are encountered.
fn normalise_ident_path<I: Iterator<Item = ast::Attr>>(path: I) -> EvalResult<Vec<String>> {
path.map(attr_to_string).collect()
}
/// Prepare the full set of globals from additional globals supplied /// Prepare the full set of globals from additional globals supplied
/// by the caller of the compiler, as well as the built-in globals /// by the caller of the compiler, as well as the built-in globals
/// that are always part of the language. /// that are always part of the language.

View file

@ -27,8 +27,8 @@ pub struct Local {
// of `let`-expressions. // of `let`-expressions.
pub name: String, pub name: String,
// Syntax node at which this local was declared. // Source span at which this local was declared.
pub node: Option<rnix::SyntaxNode>, pub span: codemap::Span,
// Scope depth of this local. // Scope depth of this local.
pub depth: usize, pub depth: usize,
@ -203,14 +203,14 @@ impl Scope {
/// Declare a local variable that occupies a stack slot and should /// Declare a local variable that occupies a stack slot and should
/// be accounted for, but is not directly accessible by users /// be accounted for, but is not directly accessible by users
/// (e.g. attribute sets used for `with`). /// (e.g. attribute sets used for `with`).
pub fn declare_phantom(&mut self) -> LocalIdx { pub fn declare_phantom(&mut self, span: codemap::Span) -> LocalIdx {
let idx = self.locals.len(); let idx = self.locals.len();
self.locals.push(Local { self.locals.push(Local {
span,
depth: self.scope_depth, depth: self.scope_depth,
initialised: true, initialised: true,
needs_finaliser: false, needs_finaliser: false,
name: "".into(), name: "".into(),
node: None,
phantom: true, phantom: true,
used: true, used: true,
}); });
@ -219,14 +219,14 @@ impl Scope {
} }
/// Declare an uninitialised local variable. /// Declare an uninitialised local variable.
pub fn declare_local(&mut self, name: String, node: rnix::SyntaxNode) -> LocalIdx { pub fn declare_local(&mut self, name: String, span: codemap::Span) -> LocalIdx {
let idx = self.locals.len(); let idx = self.locals.len();
self.locals.push(Local { self.locals.push(Local {
name, name,
span,
depth: self.scope_depth, depth: self.scope_depth,
initialised: false, initialised: false,
needs_finaliser: false, needs_finaliser: false,
node: Some(node),
phantom: false, phantom: false,
used: false, used: false,
}); });

View file

@ -52,13 +52,13 @@ pub enum ErrorKind {
#[derive(Clone, Debug)] #[derive(Clone, Debug)]
pub struct Error { pub struct Error {
pub node: Option<rnix::SyntaxNode>,
pub kind: ErrorKind, pub kind: ErrorKind,
pub span: Option<codemap::Span>,
} }
impl From<ErrorKind> for Error { impl From<ErrorKind> for Error {
fn from(kind: ErrorKind) -> Self { fn from(kind: ErrorKind) -> Self {
Error { node: None, kind } Error { span: None, kind }
} }
} }

View file

@ -43,18 +43,19 @@ pub fn interpret(code: &str, location: Option<PathBuf>) -> EvalResult<Value> {
for warning in result.warnings { for warning in result.warnings {
eprintln!( eprintln!(
"warning: {:?} at `{:?}`[{:?}]", "warning: {:?} at `{}`[line {}]",
warning.kind, warning.kind,
warning.node.text(), file.source_slice(warning.span),
warning.node.text_range().start() file.find_line(warning.span.low()) + 1
) )
} }
for error in &result.errors { for error in &result.errors {
eprintln!( eprintln!(
"compiler error: {:?} at {:?}", "compiler error: {:?} at `{}`[line {}]",
error.kind, error.kind,
error.node.as_ref().map(|node| node.text()) file.source_slice(error.span.expect("TODO: non-optional")),
file.find_line(error.span.unwrap().low()) + 1
); );
} }

View file

@ -11,6 +11,6 @@ pub enum WarningKind {
#[derive(Debug)] #[derive(Debug)]
pub struct EvalWarning { pub struct EvalWarning {
pub node: rnix::SyntaxNode,
pub kind: WarningKind, pub kind: WarningKind,
pub span: codemap::Span,
} }