From 22b5705f07b986f11101075cfd9fc87e69611c9f Mon Sep 17 00:00:00 2001 From: Patrick LaFontaine <32135464+Pat-Lafon@users.noreply.github.com> Date: Mon, 15 Aug 2022 23:43:59 -0400 Subject: [PATCH 1/7] Augment Rust tooling with Enhanced Source Positions --- bril-rs/bril2json/src/bril_grammar.lalrpop | 18 +- bril-rs/bril2json/src/cli.rs | 3 + bril-rs/bril2json/src/lib.rs | 42 ++- bril-rs/bril2json/src/main.rs | 2 +- bril-rs/brild/src/lib.rs | 11 +- bril-rs/rs2bril/src/cli.rs | 3 + bril-rs/rs2bril/src/lib.rs | 420 +++++++++++++-------- bril-rs/rs2bril/src/main.rs | 13 +- bril-rs/src/abstract_program.rs | 10 +- bril-rs/src/conversion.rs | 19 +- bril-rs/src/lib.rs | 2 + bril-rs/src/program.rs | 29 +- brilirs/src/basic_block.rs | 4 +- brilirs/src/cli.rs | 8 - brilirs/src/error.rs | 52 ++- brilirs/src/interp.rs | 12 +- brilirs/src/lib.rs | 4 +- brilirs/src/main.rs | 66 ++-- 18 files changed, 465 insertions(+), 253 deletions(-) diff --git a/bril-rs/bril2json/src/bril_grammar.lalrpop b/bril-rs/bril2json/src/bril_grammar.lalrpop index 92caccebb..7126eb6ca 100644 --- a/bril-rs/bril2json/src/bril_grammar.lalrpop +++ b/bril-rs/bril2json/src/bril_grammar.lalrpop @@ -75,12 +75,12 @@ Alias : String = { } AbstractFunction : AbstractFunction = { - "{" )*> "}" => {let a = a.unwrap_or_default(); AbstractFunction { + "{" )*> "}" => {let a = a.unwrap_or_default(); AbstractFunction { name : f, args : a, return_type : t, instrs: c, - pos : lines.get_position(loc), + pos : lines.get_position(loc, loc2), }} } @@ -100,19 +100,19 @@ AbstractArgument : AbstractArgument = { } AbstractCode : AbstractCode = { - ":" => AbstractCode::Label{ label : l, pos : lines.get_position(loc)}, + ":" => AbstractCode::Label{ label : l, pos : lines.get_position(loc, loc2)}, => AbstractCode::Instruction(i), } AbstractInstruction : AbstractInstruction = { - )?> "=" ";" => AbstractInstruction::Constant { + )?> "=" ";" => AbstractInstruction::Constant { op : c, dest : i, const_type : t, value : l, - pos : lines.get_position(loc), + pos : lines.get_position(loc, loc2), }, - )?> "=" )*> ";" => { + )?> "=" )*> ";" => { let mut a_vec = Vec::new(); let mut f_vec = Vec::new(); let mut l_vec = Vec::new(); @@ -130,10 +130,10 @@ AbstractInstruction : AbstractInstruction = { args: a_vec, funcs: f_vec, labels: l_vec, - pos : lines.get_position(loc), + pos : lines.get_position(loc, loc2), } }, - )*> ";" => { + )*> ";" => { let mut a_vec = Vec::new(); let mut f_vec = Vec::new(); let mut l_vec = Vec::new(); @@ -149,7 +149,7 @@ AbstractInstruction : AbstractInstruction = { args: a_vec, funcs: f_vec, labels: l_vec, - pos : lines.get_position(loc), + pos : lines.get_position(loc, loc2), } } diff --git a/bril-rs/bril2json/src/cli.rs b/bril-rs/bril2json/src/cli.rs index dc07f32db..14359898c 100644 --- a/bril-rs/bril2json/src/cli.rs +++ b/bril-rs/bril2json/src/cli.rs @@ -3,6 +3,9 @@ use clap::Parser; #[derive(Parser)] #[clap(about, version, author)] // keeps the cli synced with Cargo.toml pub struct Cli { + /// The bril file to statically link. stdin is assumed if file is not provided. + #[clap(short, long, action)] + pub file: Option, /// Flag for whether position information should be included #[clap(short, action)] pub position: bool, diff --git a/bril-rs/bril2json/src/lib.rs b/bril-rs/bril2json/src/lib.rs index 40f4daf5f..8f4bb8034 100644 --- a/bril-rs/bril2json/src/lib.rs +++ b/bril-rs/bril2json/src/lib.rs @@ -8,19 +8,23 @@ pub mod bril_grammar; #[doc(hidden)] pub mod cli; -use bril_rs::{AbstractProgram, Position}; +use std::fs::File; + +use bril_rs::{AbstractProgram, ColRow, Position}; #[doc(hidden)] #[derive(Clone)] pub struct Lines { use_pos: bool, new_lines: Vec, + src_name: Option, } impl Lines { - fn new(input: &str, use_pos: bool) -> Self { + fn new(input: &str, use_pos: bool, src_name: Option) -> Self { Self { use_pos, + src_name, new_lines: input .as_bytes() .iter() @@ -30,7 +34,19 @@ impl Lines { } } - fn get_position(&self, index: usize) -> Option { + fn get_position(&self, starting_index: usize, ending_index: usize) -> Option { + if self.use_pos { + Some(Position { + pos: self.get_row_col(starting_index).unwrap(), + pos_end: self.get_row_col(ending_index), + src: self.src_name.clone(), + }) + } else { + None + } + } + + fn get_row_col(&self, index: usize) -> Option { if self.use_pos { Some( self.new_lines @@ -38,13 +54,13 @@ impl Lines { .enumerate() .map(|(i, j)| (i + 1, j)) .fold( - Position { + ColRow { col: index as u64, row: 1, }, |current, (line_num, idx)| { if *idx < index { - Position { + ColRow { row: (line_num + 1) as u64, col: (index - idx) as u64, } @@ -66,17 +82,25 @@ impl Lines { pub fn parse_abstract_program_from_read( mut input: R, use_pos: bool, + file_name: Option, ) -> AbstractProgram { let mut buffer = String::new(); input.read_to_string(&mut buffer).unwrap(); let parser = bril_grammar::AbstractProgramParser::new(); parser - .parse(&Lines::new(&buffer, use_pos), &buffer) + .parse(&Lines::new(&buffer, use_pos, file_name), &buffer) .unwrap() } #[must_use] -/// A wrapper around [`parse_abstract_program_from_read`] which assumes [`std::io::Stdin`] -pub fn parse_abstract_program(use_pos: bool) -> AbstractProgram { - parse_abstract_program_from_read(std::io::stdin(), use_pos) +/// A wrapper around [`parse_abstract_program_from_read`] which assumes [`std::io::Stdin`] if `file_name` is [`None`] +/// # Panics +/// Will panic if the input is not well-formed Bril text or if `file_name` does not exist +pub fn parse_abstract_program(use_pos: bool, file_name: Option) -> AbstractProgram { + let input: Box = match file_name.clone() { + Some(f) => Box::new(File::open(f).unwrap()), + None => Box::new(std::io::stdin()), + }; + + parse_abstract_program_from_read(input, use_pos, file_name) } diff --git a/bril-rs/bril2json/src/main.rs b/bril-rs/bril2json/src/main.rs index f7631607e..78b8f9445 100644 --- a/bril-rs/bril2json/src/main.rs +++ b/bril-rs/bril2json/src/main.rs @@ -5,5 +5,5 @@ use clap::Parser; fn main() { let args = Cli::parse(); - output_abstract_program(&parse_abstract_program(args.position)) + output_abstract_program(&parse_abstract_program(args.position, args.file)) } diff --git a/bril-rs/brild/src/lib.rs b/bril-rs/brild/src/lib.rs index dac777e05..2987b2806 100644 --- a/bril-rs/brild/src/lib.rs +++ b/bril-rs/brild/src/lib.rs @@ -204,9 +204,14 @@ pub fn do_import( path_map.insert(canonical_path.clone(), None); // Find the correct parser for this path based on the extension - let f = match canonical_path.extension().and_then(std::ffi::OsStr::to_str) { - Some("bril") => |s| parse_abstract_program_from_read(s, true), - Some("json") => load_abstract_program_from_read, + let f: Box AbstractProgram> = match canonical_path + .extension() + .and_then(std::ffi::OsStr::to_str) + { + Some("bril") => Box::new(|s| { + parse_abstract_program_from_read(s, true, Some(canonical_path.display().to_string())) + }), + Some("json") => Box::new(load_abstract_program_from_read), Some(_) | None => { return Err(BrildError::MissingOrUnknownFileExtension( canonical_path.clone(), diff --git a/bril-rs/rs2bril/src/cli.rs b/bril-rs/rs2bril/src/cli.rs index dc07f32db..14359898c 100644 --- a/bril-rs/rs2bril/src/cli.rs +++ b/bril-rs/rs2bril/src/cli.rs @@ -3,6 +3,9 @@ use clap::Parser; #[derive(Parser)] #[clap(about, version, author)] // keeps the cli synced with Cargo.toml pub struct Cli { + /// The bril file to statically link. stdin is assumed if file is not provided. + #[clap(short, long, action)] + pub file: Option, /// Flag for whether position information should be included #[clap(short, action)] pub position: bool, diff --git a/bril-rs/rs2bril/src/lib.rs b/bril-rs/rs2bril/src/lib.rs index 50ff371a2..f2d59f6d4 100644 --- a/bril-rs/rs2bril/src/lib.rs +++ b/bril-rs/rs2bril/src/lib.rs @@ -8,17 +8,17 @@ pub mod cli; use bril_rs::{ - Argument, Code, ConstOps, EffectOps, Function, Instruction, Literal, Position, Program, Type, - ValueOps, + Argument, Code, ColRow, ConstOps, EffectOps, Function, Instruction, Literal, Position, Program, + Type, ValueOps, }; use syn::punctuated::Punctuated; use syn::{ BinOp, Block, Expr, ExprArray, ExprAssign, ExprAssignOp, ExprBinary, ExprBlock, ExprCall, - ExprCast, ExprIf, ExprIndex, ExprLit, ExprMacro, ExprParen, ExprPath, ExprReference, - ExprRepeat, ExprReturn, ExprUnary, ExprWhile, File, FnArg, Ident, Item, ItemFn, Lit, Local, - Macro, Pat, PatIdent, PatType, Path, PathSegment, ReturnType, Signature, Stmt, Type as SType, - TypeArray, TypePath, TypeReference, TypeSlice, UnOp, + ExprCast, ExprIf, ExprIndex, ExprLet, ExprLit, ExprLoop, ExprMacro, ExprParen, ExprPath, + ExprReference, ExprRepeat, ExprReturn, ExprUnary, ExprWhile, File, FnArg, Ident, Item, ItemFn, + Lit, Local, Macro, Pat, PatIdent, PatType, Path, PathSegment, ReturnType, Signature, Stmt, + Type as SType, TypeArray, TypePath, TypeReference, TypeSlice, UnOp, }; use proc_macro2::Span; @@ -30,15 +30,17 @@ use std::collections::HashMap; struct State { is_pos: bool, + src: Option, temp_var_count: u64, ident_type_map: HashMap, func_context_map: HashMap, Option)>, } impl State { - fn new(is_pos: bool) -> Self { + fn new(is_pos: bool, src: Option) -> Self { Self { is_pos, + src, temp_var_count: 0, ident_type_map: HashMap::new(), func_context_map: HashMap::new(), @@ -77,11 +79,179 @@ impl State { } } -fn from_span_to_position(span: Span) -> Position { - let position = span.start(); +fn from_expr_to_span(expr: &Expr) -> Span { + match expr { + Expr::Array(ExprArray { + attrs: _, + bracket_token, + elems: _, + }) + | Expr::Repeat(ExprRepeat { + attrs: _, + bracket_token, + expr: _, + semi_token: _, + len: _, + }) => bracket_token.span, + Expr::Assign(ExprAssign { + attrs: _, + left, + eq_token: _, + right, + }) + | Expr::AssignOp(ExprAssignOp { + attrs: _, + left, + op: _, + right, + }) + | Expr::Binary(ExprBinary { + attrs: _, + left, + op: _, + right, + }) => from_expr_to_span(left) + .join(from_expr_to_span(right)) + .unwrap(), + Expr::Block(ExprBlock { + attrs: _, + label: _, + block, + }) => block.brace_token.span, + Expr::Call(ExprCall { + attrs: _, + func, + paren_token, + args: _, + }) => from_expr_to_span(func).join(paren_token.span).unwrap(), + Expr::Cast(ExprCast { + attrs: _, + expr, + as_token: _, + ty: _, + }) + | Expr::Reference(ExprReference { + attrs: _, + and_token: _, + raw: _, + mutability: _, + expr, + }) => from_expr_to_span(expr), + Expr::If(ExprIf { + attrs: _, + if_token, + cond: _, + then_branch: _, + else_branch: Some((_, else_branch)), + }) => if_token.span.join(from_expr_to_span(else_branch)).unwrap(), + Expr::If(ExprIf { + attrs: _, + if_token, + cond: _, + then_branch, + else_branch: None, + }) => if_token.span.join(then_branch.brace_token.span).unwrap(), + Expr::Index(ExprIndex { + attrs: _, + expr, + bracket_token, + index: _, + }) => from_expr_to_span(expr).join(bracket_token.span).unwrap(), + Expr::Let(ExprLet { + attrs: _, + let_token, + pat: _, + eq_token: _, + expr, + }) => let_token.span.join(from_expr_to_span(expr)).unwrap(), + Expr::Lit(ExprLit { attrs: _, lit }) => lit.span(), + Expr::Loop(ExprLoop { + attrs: _, + label: _, + loop_token, + body, + }) => loop_token.span.join(body.brace_token.span).unwrap(), + Expr::Macro(ExprMacro { + attrs: _, + mac: + Macro { + path: _, + bang_token, + delimiter, + tokens: _, + }, + }) => bang_token + .span + .join(match delimiter { + syn::MacroDelimiter::Paren(p) => p.span, + syn::MacroDelimiter::Brace(b) => b.span, + syn::MacroDelimiter::Bracket(b) => b.span, + }) + .unwrap(), + Expr::Paren(ExprParen { + attrs: _, + paren_token, + expr: _, + }) => paren_token.span, + Expr::Path(ExprPath { + attrs: _, + qself: _, + path: Path { + leading_colon: _, + segments, + }, + }) => segments + .first() + .unwrap() + .ident + .span() + .join(segments.last().unwrap().ident.span()) + .unwrap(), + Expr::Return(ExprReturn { + attrs: _, + return_token, + expr: None, + }) => return_token.span, + Expr::Return(ExprReturn { + attrs: _, + return_token, + expr: Some(expr), + }) => return_token.span.join(from_expr_to_span(expr)).unwrap(), + Expr::Unary(ExprUnary { attrs: _, op, expr }) => match op { + UnOp::Deref(d) => d.span, + UnOp::Not(n) => n.span, + UnOp::Neg(n) => n.span, + } + .join(from_expr_to_span(expr)) + .unwrap(), + Expr::While(ExprWhile { + attrs: _, + label: _, + while_token, + cond: _, + body, + }) => while_token.span.join(body.brace_token.span).unwrap(), + _ => todo!(), + } +} + +fn from_span_to_position( + starting_span: Span, + ending_span: Option, + src: Option, +) -> Position { + let start = starting_span.start(); + let end = ending_span.map_or(starting_span.end(), |s| s.end()); Position { - col: position.column as u64, - row: position.line as u64, + pos: ColRow { + col: start.column as u64, + row: start.line as u64, + }, + pos_end: Some(ColRow { + col: end.column as u64, + row: end.line as u64, + }), + src, } } @@ -156,10 +326,10 @@ fn from_signature_to_function( asyncness, unsafety, abi, - fn_token: _, + fn_token, ident, generics, - paren_token: _, + paren_token, inputs, variadic, output, @@ -204,7 +374,11 @@ fn from_signature_to_function( Function { name: ident.to_string(), pos: if state.is_pos { - Some(from_span_to_position(ident.span())) + Some(from_span_to_position( + fn_token.span, + Some(paren_token.span), + state.src.clone(), + )) } else { None }, @@ -269,6 +443,15 @@ fn array_init_helper( } fn from_expr_to_bril(expr: Expr, state: &mut State) -> (Option, Vec) { + let pos = if state.is_pos { + Some(from_span_to_position( + from_expr_to_span(&expr), + None, + state.src.clone(), + )) + } else { + None + }; match expr { Expr::Array(ExprArray { attrs, @@ -288,7 +471,7 @@ fn from_expr_to_bril(expr: Expr, state: &mut State) -> (Option, Vec { let (arg, mut code) = from_expr_to_bril(*right, state); @@ -306,11 +489,7 @@ fn from_expr_to_bril(expr: Expr, state: &mut State) -> (Option, Vec (Option, Vec { let (arg1, mut code1) = from_expr_to_bril(*expr, state); @@ -334,11 +513,7 @@ fn from_expr_to_bril(expr: Expr, state: &mut State) -> (Option, Vec (Option, Vec (Option, Vec (Option, Vec (Option, Vec (ValueOps::Add, Type::Int, x.spans[0]), - (BinOp::Add(x), Type::Float) => (ValueOps::Fadd, Type::Float, x.spans[0]), - (BinOp::Sub(x), Type::Int) => (ValueOps::Sub, Type::Int, x.spans[0]), - (BinOp::Sub(x), Type::Float) => (ValueOps::Fsub, Type::Float, x.spans[0]), - (BinOp::Mul(x), Type::Int) => (ValueOps::Mul, Type::Int, x.spans[0]), - (BinOp::Mul(x), Type::Float) => (ValueOps::Fmul, Type::Float, x.spans[0]), - (BinOp::Div(x), Type::Int) => (ValueOps::Div, Type::Int, x.spans[0]), - (BinOp::Div(x), Type::Float) => (ValueOps::Fdiv, Type::Float, x.spans[0]), - (BinOp::And(x), _) => (ValueOps::And, Type::Bool, x.spans[0]), - (BinOp::Or(x), _) => (ValueOps::Or, Type::Bool, x.spans[0]), - (BinOp::Eq(x), Type::Int) => (ValueOps::Eq, Type::Bool, x.spans[0]), - (BinOp::Eq(x), Type::Float) => (ValueOps::Feq, Type::Bool, x.spans[0]), - (BinOp::Lt(x), Type::Int) => (ValueOps::Lt, Type::Bool, x.spans[0]), - (BinOp::Lt(x), Type::Float) => (ValueOps::Flt, Type::Bool, x.spans[0]), - (BinOp::Le(x), Type::Int) => (ValueOps::Le, Type::Bool, x.spans[0]), - (BinOp::Le(x), Type::Float) => (ValueOps::Fle, Type::Bool, x.spans[0]), - (BinOp::Ge(x), Type::Int) => (ValueOps::Ge, Type::Bool, x.spans[0]), - (BinOp::Ge(x), Type::Float) => (ValueOps::Fge, Type::Bool, x.spans[0]), - (BinOp::Gt(x), Type::Int) => (ValueOps::Gt, Type::Bool, x.spans[0]), - (BinOp::Gt(x), Type::Float) => (ValueOps::Fgt, Type::Bool, x.spans[0]), - (_, _) => unimplemented!("{op:?}"), - }; + let (value_op, op_type) = match (op, state.get_type_for_ident(arg1.as_ref().unwrap())) { + (BinOp::Add(_), Type::Int) => (ValueOps::Add, Type::Int), + (BinOp::Add(_), Type::Float) => (ValueOps::Fadd, Type::Float), + (BinOp::Sub(_), Type::Int) => (ValueOps::Sub, Type::Int), + (BinOp::Sub(_), Type::Float) => (ValueOps::Fsub, Type::Float), + (BinOp::Mul(_), Type::Int) => (ValueOps::Mul, Type::Int), + (BinOp::Mul(_), Type::Float) => (ValueOps::Fmul, Type::Float), + (BinOp::Div(_), Type::Int) => (ValueOps::Div, Type::Int), + (BinOp::Div(_), Type::Float) => (ValueOps::Fdiv, Type::Float), + (BinOp::And(_), _) => (ValueOps::And, Type::Bool), + (BinOp::Or(_), _) => (ValueOps::Or, Type::Bool), + (BinOp::Eq(_), Type::Int) => (ValueOps::Eq, Type::Bool), + (BinOp::Eq(_), Type::Float) => (ValueOps::Feq, Type::Bool), + (BinOp::Lt(_), Type::Int) => (ValueOps::Lt, Type::Bool), + (BinOp::Lt(_), Type::Float) => (ValueOps::Flt, Type::Bool), + (BinOp::Le(_), Type::Int) => (ValueOps::Le, Type::Bool), + (BinOp::Le(_), Type::Float) => (ValueOps::Fle, Type::Bool), + (BinOp::Ge(_), Type::Int) => (ValueOps::Ge, Type::Bool), + (BinOp::Ge(_), Type::Float) => (ValueOps::Fge, Type::Bool), + (BinOp::Gt(_), Type::Int) => (ValueOps::Gt, Type::Bool), + (BinOp::Gt(_), Type::Float) => (ValueOps::Fgt, Type::Bool), + (_, _) => unimplemented!("{op:?}"), + }; let dest = state.fresh_var(op_type.clone()); @@ -447,11 +617,7 @@ fn from_expr_to_bril(expr: Expr, state: &mut State) -> (Option, Vec (Option, Vec { let f = match *func { @@ -491,11 +657,7 @@ fn from_expr_to_bril(expr: Expr, state: &mut State) -> (Option, Vec (Option, Vec (Option, Vec (Option, Vec (Option, Vec (Option, Vec { let (arg1, mut code1) = from_expr_to_bril(*expr, state); @@ -631,11 +785,7 @@ fn from_expr_to_bril(expr: Expr, state: &mut State) -> (Option, Vec (Option, Vec (Option, Vec().unwrap()), })], @@ -679,11 +821,7 @@ fn from_expr_to_bril(expr: Expr, state: &mut State) -> (Option, Vec().unwrap()), })], @@ -696,11 +834,7 @@ fn from_expr_to_bril(expr: Expr, state: &mut State) -> (Option, Vec (Option, Vec (Option, Vec (Option, Vec { let (args, mut code) = match expr { @@ -804,11 +934,7 @@ fn from_expr_to_bril(expr: Expr, state: &mut State) -> (Option, Vec (Option, Vec ( - ValueOps::Id, - if state.is_pos { - Some(from_span_to_position(x.spans[0])) - } else { - None - }, - state.get_type_for_ident(&arg.unwrap()), - ), - UnOp::Not(x) => ( - ValueOps::Not, - if state.is_pos { - Some(from_span_to_position(x.spans[0])) - } else { - None - }, - Type::Bool, - ), - UnOp::Neg(x) => { + let (op, op_type) = match op { + UnOp::Deref(_) => (ValueOps::Id, state.get_type_for_ident(&arg.unwrap())), + UnOp::Not(_) => (ValueOps::Not, Type::Bool), + UnOp::Neg(_) => { let ty = state.get_type_for_ident(&arg.unwrap()); ( match ty { @@ -866,11 +976,6 @@ fn from_expr_to_bril(expr: Expr, state: &mut State) -> (Option, Vec panic!("can't handle negation of non-int/float"), }, - if state.is_pos { - Some(from_span_to_position(x.spans[0])) - } else { - None - }, ty, ) } @@ -902,7 +1007,7 @@ fn from_expr_to_bril(expr: Expr, state: &mut State) -> (Option, Vec (Option, Vec (Option, Vec Vec { let_token, pat, init, - semi_token: _, + semi_token, }) => { assert!(attrs.is_empty(), "can't handle attributes in function body"); assert!(init.is_some(), "must initialize all assignments"); @@ -968,7 +1073,11 @@ fn from_stmt_to_vec_code(s: Stmt, state: &mut State) -> Vec { labels: Vec::new(), op: ValueOps::Id, pos: if state.is_pos { - Some(from_span_to_position(let_token.span)) + Some(from_span_to_position( + let_token.span, + Some(semi_token.span), + state.src.clone(), + )) } else { None }, @@ -1029,12 +1138,13 @@ pub fn from_file_to_program( items, }: File, is_pos: bool, + src: Option, ) -> Program { assert!(shebang.is_none(), "can't handle shebang items in Rust file"); assert!(attrs.is_empty(), "can't handle attributes in Rust file"); - let mut state = State::new(is_pos); + let mut state = State::new(is_pos, src); // The processing of Functions is separated into two parts to get global information like type signatures for functions before processing function bodies let sigs_processed: Vec<(Function, Block)> = items diff --git a/bril-rs/rs2bril/src/main.rs b/bril-rs/rs2bril/src/main.rs index e58e98094..caf1b7d78 100644 --- a/bril-rs/rs2bril/src/main.rs +++ b/bril-rs/rs2bril/src/main.rs @@ -10,8 +10,17 @@ use clap::Parser; fn main() { let args = Cli::parse(); let mut src = String::new(); - std::io::stdin().read_to_string(&mut src).unwrap(); + let source_name = if let Some(f) = args.file { + let path = std::fs::canonicalize(f).unwrap(); + let mut file = std::fs::File::open(path.clone()).unwrap(); + file.read_to_string(&mut src).unwrap(); + Some(path.display().to_string()) + } else { + std::io::stdin().read_to_string(&mut src).unwrap(); + None + }; + let syntax = syn::parse_file(&src).unwrap(); - output_program(&from_file_to_program(syntax, args.position)); + output_program(&from_file_to_program(syntax, args.position, source_name)); } diff --git a/bril-rs/src/abstract_program.rs b/bril-rs/src/abstract_program.rs index a43e199bb..07d53a40a 100644 --- a/bril-rs/src/abstract_program.rs +++ b/bril-rs/src/abstract_program.rs @@ -48,7 +48,7 @@ pub struct AbstractFunction { pub name: String, /// The position of this function in the original source code #[cfg(feature = "position")] - #[serde(skip_serializing_if = "Option::is_none")] + #[serde(flatten, skip_serializing_if = "Option::is_none")] pub pos: Option, /// The possible return type of this function #[serde(rename = "type")] @@ -110,7 +110,7 @@ pub enum AbstractCode { label: String, /// Where the label is located in source code #[cfg(feature = "position")] - #[serde(skip_serializing_if = "Option::is_none")] + #[serde(flatten, skip_serializing_if = "Option::is_none")] pos: Option, }, /// @@ -142,7 +142,7 @@ pub enum AbstractInstruction { op: ConstOps, /// The source position of the instruction if provided #[cfg(feature = "position")] - #[serde(skip_serializing_if = "Option::is_none")] + #[serde(flatten, skip_serializing_if = "Option::is_none")] pos: Option, /// Type of variable #[serde(rename = "type")] @@ -167,7 +167,7 @@ pub enum AbstractInstruction { op: String, /// The source position of the instruction if provided #[cfg(feature = "position")] - #[serde(skip_serializing_if = "Option::is_none")] + #[serde(flatten, skip_serializing_if = "Option::is_none")] pos: Option, /// Type of variable #[serde(rename = "type")] @@ -188,7 +188,7 @@ pub enum AbstractInstruction { op: String, /// The source position of the instruction if provided #[cfg(feature = "position")] - #[serde(skip_serializing_if = "Option::is_none")] + #[serde(flatten, skip_serializing_if = "Option::is_none")] pos: Option, }, } diff --git a/bril-rs/src/conversion.rs b/bril-rs/src/conversion.rs index b2ba1cff8..f488615ba 100644 --- a/bril-rs/src/conversion.rs +++ b/bril-rs/src/conversion.rs @@ -73,7 +73,7 @@ impl Display for PositionalConversionError { match self { #[cfg(feature = "position")] PositionalConversionError { e, pos: Some(pos) } => { - write!(f, "Line {}, Column {}: {e}", pos.row, pos.col) + write!(f, "Line {}, Column {}: {e}", pos.pos.row, pos.pos.col) } #[cfg(not(feature = "position"))] PositionalConversionError { e: _, pos: Some(_) } => { @@ -121,7 +121,7 @@ impl TryFrom for Function { .into_iter() .map(std::convert::TryInto::try_into) .collect::, _>>() - .map_err(|e| e.add_pos(pos))?, + .map_err(|e| e.add_pos(pos.clone()))?, instrs: instrs .into_iter() .map(std::convert::TryInto::try_into) @@ -129,7 +129,10 @@ impl TryFrom for Function { name, return_type: match return_type { None => None, - Some(t) => Some(t.try_into().map_err(|e: ConversionError| e.add_pos(pos))?), + Some(t) => Some( + t.try_into() + .map_err(|e: ConversionError| e.add_pos(pos.clone()))?, + ), }, #[cfg(feature = "position")] pos, @@ -183,10 +186,10 @@ impl TryFrom for Instruction { op, const_type: const_type .try_into() - .map_err(|e: ConversionError| e.add_pos(pos))?, + .map_err(|e: ConversionError| e.add_pos(pos.clone()))?, value, #[cfg(feature = "position")] - pos, + pos: pos, }, AbstractInstruction::Value { args, @@ -204,9 +207,9 @@ impl TryFrom for Instruction { labels, op_type: op_type .try_into() - .map_err(|e: ConversionError| e.add_pos(pos))?, + .map_err(|e: ConversionError| e.add_pos(pos.clone()))?, #[cfg(feature = "position")] - pos, + pos: pos.clone(), op: match op.as_ref() { "add" => ValueOps::Add, "mul" => ValueOps::Mul, @@ -266,7 +269,7 @@ impl TryFrom for Instruction { funcs, labels, #[cfg(feature = "position")] - pos, + pos: pos.clone(), op: match op.as_ref() { "jmp" => EffectOps::Jump, "br" => EffectOps::Branch, diff --git a/bril-rs/src/lib.rs b/bril-rs/src/lib.rs index 161b0d168..7292eee37 100644 --- a/bril-rs/src/lib.rs +++ b/bril-rs/src/lib.rs @@ -4,6 +4,8 @@ #![allow(clippy::too_many_lines)] // https://github.com/rust-lang/rust-clippy/issues/6902 #![allow(clippy::use_self)] +// Buggy in that it wants Eq to be derived on Literal which has an f64 field +#![allow(clippy::derive_partial_eq_without_eq)] /// Provides the unstructured representation of Bril programs pub mod abstract_program; diff --git a/bril-rs/src/program.rs b/bril-rs/src/program.rs index d4a9a2204..0ac35c77e 100644 --- a/bril-rs/src/program.rs +++ b/bril-rs/src/program.rs @@ -90,7 +90,7 @@ pub struct Function { pub name: String, /// The position of this function in the original source code #[cfg(feature = "position")] - #[serde(skip_serializing_if = "Option::is_none")] + #[serde(flatten, skip_serializing_if = "Option::is_none")] pub pos: Option, /// The possible return type of this function #[serde(rename = "type")] @@ -152,7 +152,7 @@ pub enum Code { label: String, /// Where the label is located in source code #[cfg(feature = "position")] - #[serde(skip_serializing_if = "Option::is_none")] + #[serde(flatten, skip_serializing_if = "Option::is_none")] pos: Option, }, /// @@ -184,7 +184,7 @@ pub enum Instruction { op: ConstOps, #[cfg(feature = "position")] /// The source position of the instruction if provided - #[serde(skip_serializing_if = "Option::is_none")] + #[serde(flatten, skip_serializing_if = "Option::is_none")] pos: Option, /// Type of variable #[serde(rename = "type")] @@ -209,7 +209,7 @@ pub enum Instruction { op: ValueOps, /// The source position of the instruction if provided #[cfg(feature = "position")] - #[serde(skip_serializing_if = "Option::is_none")] + #[serde(flatten, skip_serializing_if = "Option::is_none")] pos: Option, /// Type of variable #[serde(rename = "type")] @@ -230,7 +230,7 @@ pub enum Instruction { op: EffectOps, /// The source position of the instruction if provided #[cfg(feature = "position")] - #[serde(skip_serializing_if = "Option::is_none")] + #[serde(flatten, skip_serializing_if = "Option::is_none")] pos: Option, }, } @@ -239,11 +239,11 @@ pub enum Instruction { impl Instruction { /// A helper function to extract the position value if it exists from an instruction #[must_use] - pub const fn get_pos(&self) -> Option { + pub fn get_pos(&self) -> Option { match self { Instruction::Constant { pos, .. } | Instruction::Value { pos, .. } - | Instruction::Effect { pos, .. } => *pos, + | Instruction::Effect { pos, .. } => pos.clone(), } } } @@ -570,8 +570,21 @@ impl Literal { } /// -#[derive(Serialize, Deserialize, Debug, Clone, Copy, PartialEq)] +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq)] pub struct Position { + /// Starting position + pub pos: ColRow, + /// Optional ending position + #[serde(skip_serializing_if = "Option::is_none")] + pub pos_end: Option, + /// Optional absolute path to source file + #[serde(skip_serializing_if = "Option::is_none")] + pub src: Option, +} + +/// +#[derive(Serialize, Deserialize, Debug, Clone, Copy, PartialEq, Eq)] +pub struct ColRow { /// Column pub col: u64, /// Row diff --git a/brilirs/src/basic_block.rs b/brilirs/src/basic_block.rs index 8b805c0df..4d1fcae4c 100644 --- a/brilirs/src/basic_block.rs +++ b/brilirs/src/basic_block.rs @@ -142,7 +142,7 @@ impl NumifiedInstruction { func_map .get(f) .copied() - .ok_or_else(|| InterpError::FuncNotFound(f.to_string()).add_pos(*pos)) + .ok_or_else(|| InterpError::FuncNotFound(f.to_string()).add_pos(pos.clone())) }) .collect::, PositionalInterpError>>()?, }, @@ -160,7 +160,7 @@ impl NumifiedInstruction { func_map .get(f) .copied() - .ok_or_else(|| InterpError::FuncNotFound(f.to_string()).add_pos(*pos)) + .ok_or_else(|| InterpError::FuncNotFound(f.to_string()).add_pos(pos.clone())) }) .collect::, PositionalInterpError>>()?, }, diff --git a/brilirs/src/cli.rs b/brilirs/src/cli.rs index 7e41e04be..248883ef0 100644 --- a/brilirs/src/cli.rs +++ b/brilirs/src/cli.rs @@ -23,12 +23,4 @@ pub struct Cli { /// Arguments for the main function #[clap(action)] pub args: Vec, - - /// This is the original source file that the file input was generated from. - /// You would want to provide this if the bril file you are providing to the - /// interpreter is in JSON form with source positions and you what brilirs to - /// include sections of the source file in its error messages. - /// If --text/-t is provided, that will be assumed to be the source file if none are provided. - #[clap(short, long, action)] - pub source: Option, } diff --git a/brilirs/src/error.rs b/brilirs/src/error.rs index 094ed4e3e..7ff7a5a0a 100644 --- a/brilirs/src/error.rs +++ b/brilirs/src/error.rs @@ -79,10 +79,58 @@ pub struct PositionalInterpError { impl Display for PositionalInterpError { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { - PositionalInterpError { e, pos: Some(pos) } => { + Self { + e, + pos: + Some(Position { + pos, + pos_end: Some(end), + src: Some(s), + }), + } => { + write!( + f, + "{s}:{}:{} to {s}:{}:{} \n\t {e}", + pos.row, pos.col, end.row, end.col + ) + } + Self { + e, + pos: + Some(Position { + pos, + pos_end: None, + src: Some(s), + }), + } => { + write!(f, "{s}:{}:{} \n\t {e}", pos.row, pos.col) + } + Self { + e, + pos: + Some(Position { + pos, + pos_end: Some(end), + src: None, + }), + } => { + write!( + f, + "Line {}, Column {} to Line {}, Column {}: {e}", + pos.row, pos.col, end.row, end.col + ) + } + Self { + e, + pos: Some(Position { + pos, + pos_end: None, + src: None, + }), + } => { write!(f, "Line {}, Column {}: {e}", pos.row, pos.col) } - PositionalInterpError { e, pos: None } => write!(f, "{e}"), + Self { e, pos: None } => write!(f, "{e}"), } } } diff --git a/brilirs/src/interp.rs b/brilirs/src/interp.rs index 4e379ed31..d27b072d8 100644 --- a/brilirs/src/interp.rs +++ b/brilirs/src/interp.rs @@ -493,7 +493,7 @@ fn execute_effect_op<'a, T: std::io::Write>( // In the typical case, users only print out one value at a time // So we can usually avoid extra allocations by providing that string directly if args.len() == 1 { - optimized_val_output(&mut state.out, state.env.get(args.get(0).unwrap()))?; + optimized_val_output(&mut state.out, state.env.get(args.first().unwrap()))?; // Add new line state.out.write_all(&[b'\n'])?; } else { @@ -600,7 +600,7 @@ fn execute<'a, T: std::io::Write>( &numified_code.funcs, last_label, ) - .map_err(|e| e.add_pos(*pos))?; + .map_err(|e| e.add_pos(pos.clone()))?; } Instruction::Effect { op, @@ -618,7 +618,7 @@ fn execute<'a, T: std::io::Write>( &mut next_block_idx, &mut result, ) - .map_err(|e| e.add_pos(*pos))?; + .map_err(|e| e.add_pos(pos.clone()))?; } } } @@ -732,21 +732,21 @@ pub fn execute_main( if main_func.return_type.is_some() { return Err(InterpError::NonEmptyRetForFunc(main_func.name.clone())) - .map_err(|e| e.add_pos(main_func.pos)); + .map_err(|e| e.add_pos(main_func.pos.clone())); } let mut env = Environment::new(main_func.num_of_vars); let heap = Heap::default(); env = parse_args(env, &main_func.args, &main_func.args_as_nums, input_args) - .map_err(|e| e.add_pos(main_func.pos))?; + .map_err(|e| e.add_pos(main_func.pos.clone()))?; let mut state = State::new(prog, env, heap, out); execute(&mut state, main_func)?; if !state.heap.is_empty() { - return Err(InterpError::MemLeak).map_err(|e| e.add_pos(main_func.pos)); + return Err(InterpError::MemLeak).map_err(|e| e.add_pos(main_func.pos.clone())); } state.out.flush().map_err(InterpError::IoError)?; diff --git a/brilirs/src/lib.rs b/brilirs/src/lib.rs index 08cc5e4b5..ed13049b7 100644 --- a/brilirs/src/lib.rs +++ b/brilirs/src/lib.rs @@ -4,6 +4,7 @@ #![allow(clippy::similar_names)] #![allow(clippy::too_many_lines)] #![allow(clippy::module_name_repetitions)] +#![allow(clippy::too_many_arguments)] #![doc = include_str!("../README.md")] use basic_block::BBProgram; @@ -30,12 +31,13 @@ pub fn run_input( profiling_out: U, check: bool, text: bool, + src_name: Option, ) -> Result<(), PositionalInterpError> { // It's a little confusing because of the naming conventions. // - bril_rs takes file.json as input // - bril2json takes file.bril as input let prog: Program = if text { - bril2json::parse_abstract_program_from_read(input, true).try_into()? + bril2json::parse_abstract_program_from_read(input, true, src_name).try_into()? } else { bril_rs::load_abstract_program_from_read(input).try_into()? }; diff --git a/brilirs/src/main.rs b/brilirs/src/main.rs index 1287a63f2..9c0c5eb10 100644 --- a/brilirs/src/main.rs +++ b/brilirs/src/main.rs @@ -1,3 +1,4 @@ +use bril_rs::Position; use brilirs::cli::Cli; use brilirs::error::PositionalInterpError; use clap::Parser; @@ -7,62 +8,59 @@ use std::io::Read; fn main() { let args = Cli::parse(); - let mut input: Box = match args.file { + let input: Box = match args.file.clone() { None => Box::new(std::io::stdin()), Some(input_file) => Box::new(File::open(input_file).unwrap()), }; - // Here we are reading out the input into a string so that we have it for error reporting - // This will be done again during parsing inside of run_input because of the current interface - // This is inefficient but probably not meaningfully so since benchmarking has shown that parsing quickly gets out weighted by program execution - // If this does matter to you in your profiling, split up parse_abstract_program_from_read/load_abstract_program_from_read so that they can take a string instead of a `Read` - let mut input_string = String::new(); - input.read_to_string(&mut input_string).unwrap(); - /* todo should you be able to supply output locations from the command line interface? Instead of builtin std::io::stdout()/std::io::stderr() */ if let Err(e) = brilirs::run_input( - input_string.as_bytes(), + input, std::io::BufWriter::new(std::io::stdout()), &args.args, args.profile, std::io::stderr(), args.check, args.text, + args.file, ) { - let mut source_file = None; - if args.text { - source_file = Some(&input_string); - } - - let mut tmp_string = String::new(); - if let Some(s) = args.source { - File::open(s) - .unwrap() - .read_to_string(&mut tmp_string) - .unwrap(); - source_file = Some(&tmp_string); - } - - if let ( - Some(f), - PositionalInterpError { - e: _, - pos: Some(pos), - }, - ) = (source_file, &e) + eprintln!("error: {e}"); + if let PositionalInterpError { + pos: Some(Position { + pos, + pos_end, + src: Some(src), + }), + .. + } = e { - // TODO delegate this to a crate that uses spans? + let mut f = String::new(); + File::open(src).unwrap().read_to_string(&mut f).unwrap(); + let mut lines = f.split('\n'); - eprintln!("error: {e}"); + + // print the first line eprintln!("{}", lines.nth((pos.row - 1) as usize).unwrap()); eprintln!("{:>width$}", "^", width = pos.col as usize); - } else { - eprintln!("error: {e}"); + + // Then check if there is more + if let Some(end) = pos_end { + if pos.row != end.row { + let mut row = pos.row + 1; + while row < end.row { + eprintln!("{}", lines.nth((row - 1) as usize).unwrap()); + eprintln!("^"); + row += 1; + } + eprintln!("{}", lines.nth((end.row - 1) as usize).unwrap()); + eprintln!("{:>width$}", "^", width = end.col as usize); + } + } } std::process::exit(2) } From 2d09e786f1770b12a6c1143664a2f01de485850a Mon Sep 17 00:00:00 2001 From: Patrick LaFontaine <32135464+Pat-Lafon@users.noreply.github.com> Date: Tue, 16 Aug 2022 00:06:59 -0400 Subject: [PATCH 2/7] Have bril2json use full path for source position --- bril-rs/bril2json/src/lib.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/bril-rs/bril2json/src/lib.rs b/bril-rs/bril2json/src/lib.rs index 8f4bb8034..b6a10e7d3 100644 --- a/bril-rs/bril2json/src/lib.rs +++ b/bril-rs/bril2json/src/lib.rs @@ -87,8 +87,11 @@ pub fn parse_abstract_program_from_read( let mut buffer = String::new(); input.read_to_string(&mut buffer).unwrap(); let parser = bril_grammar::AbstractProgramParser::new(); + + let src_name = file_name.map(|f| std::fs::canonicalize(f).unwrap().display().to_string()); + parser - .parse(&Lines::new(&buffer, use_pos, file_name), &buffer) + .parse(&Lines::new(&buffer, use_pos, src_name), &buffer) .unwrap() } From ad7f474ae0929d141057b7300b55e6b7db7a2b22 Mon Sep 17 00:00:00 2001 From: Patrick LaFontaine <32135464+Pat-Lafon@users.noreply.github.com> Date: Tue, 16 Aug 2022 00:11:50 -0400 Subject: [PATCH 3/7] Update test with ending position --- test/parse/positions.json | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/test/parse/positions.json b/test/parse/positions.json index d2c30591c..7f02951f9 100644 --- a/test/parse/positions.json +++ b/test/parse/positions.json @@ -9,6 +9,10 @@ "col": 3, "row": 3 }, + "pos_end": { + "col": 21, + "row": 3 + }, "type": "int", "value": 1 }, @@ -19,6 +23,10 @@ "col": 3, "row": 4 }, + "pos_end": { + "col": 21, + "row": 4 + }, "type": "int", "value": 2 }, @@ -30,6 +38,10 @@ "pos": { "col": 3, "row": 5 + }, + "pos_end": { + "col": 14, + "row": 5 } }, { @@ -37,6 +49,10 @@ "pos": { "col": 1, "row": 6 + }, + "pos_end": { + "col": 8, + "row": 6 } }, { @@ -50,6 +66,10 @@ "col": 3, "row": 7 }, + "pos_end": { + "col": 23, + "row": 7 + }, "type": "int" }, { @@ -60,6 +80,10 @@ "pos": { "col": 3, "row": 8 + }, + "pos_end": { + "col": 12, + "row": 8 } } ], @@ -67,6 +91,10 @@ "pos": { "col": 1, "row": 2 + }, + "pos_end": { + "col": 7, + "row": 2 } } ] From 9157c53eb066164335d439c30f7a0a3f79d67938 Mon Sep 17 00:00:00 2001 From: Patrick LaFontaine <32135464+Pat-Lafon@users.noreply.github.com> Date: Fri, 26 Aug 2022 20:38:50 -0400 Subject: [PATCH 4/7] doc fix --- docs/tools/rust.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/tools/rust.md b/docs/tools/rust.md index 4ba4c4c6b..812ba1921 100644 --- a/docs/tools/rust.md +++ b/docs/tools/rust.md @@ -24,7 +24,7 @@ Tools This library supports fully compatible Rust implementations of `bril2txt` and `bril2json`. This library also implements the [import][] extension with a static linker called `brild`. -This library is used in a Rust compiler called `rs2bril` which supports generating [core], [float], and [mem] Bril from a subset of valid Rust. +This library is used in a Rust compiler called `rs2bril` which supports generating [core], [float], and [memory] Bril from a subset of valid Rust. For ease of use, these tools can be installed and added to your path by running the following in `bril-rs/`: From 6c308b7b5aa099107778117b11e83091e7868996 Mon Sep 17 00:00:00 2001 From: Patrick LaFontaine <32135464+Pat-Lafon@users.noreply.github.com> Date: Sun, 2 Oct 2022 19:35:44 -0400 Subject: [PATCH 5/7] New clippy lints and Clap 4.0 --- bril-rs/bril2json/Cargo.toml | 2 +- bril-rs/bril2json/src/cli.rs | 6 +++--- bril-rs/brild/Cargo.toml | 2 +- bril-rs/brild/src/cli.rs | 6 +++--- bril-rs/rs2bril/Cargo.toml | 2 +- bril-rs/rs2bril/src/cli.rs | 6 +++--- brilirs/Cargo.toml | 6 +++--- brilirs/build.rs | 2 +- brilirs/src/basic_block.rs | 17 +++++++++-------- brilirs/src/check.rs | 13 +++++++------ brilirs/src/cli.rs | 14 +++++++------- brilirs/src/interp.rs | 16 ++++++++-------- 12 files changed, 47 insertions(+), 45 deletions(-) diff --git a/bril-rs/bril2json/Cargo.toml b/bril-rs/bril2json/Cargo.toml index d6eebf084..e268e37ba 100644 --- a/bril-rs/bril2json/Cargo.toml +++ b/bril-rs/bril2json/Cargo.toml @@ -14,7 +14,7 @@ keywords = ["compiler", "bril", "parser", "data-structures", "language"] # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html [dependencies] -clap = { version = "3.2", features = ["derive"] } +clap = { version = "4.0", features = ["derive"] } lalrpop-util = { version = "0.19", features = ["lexer"] } regex = "1.5" diff --git a/bril-rs/bril2json/src/cli.rs b/bril-rs/bril2json/src/cli.rs index 14359898c..417e8db63 100644 --- a/bril-rs/bril2json/src/cli.rs +++ b/bril-rs/bril2json/src/cli.rs @@ -1,12 +1,12 @@ use clap::Parser; #[derive(Parser)] -#[clap(about, version, author)] // keeps the cli synced with Cargo.toml +#[command(about, version, author)] // keeps the cli synced with Cargo.toml pub struct Cli { /// The bril file to statically link. stdin is assumed if file is not provided. - #[clap(short, long, action)] + #[arg(short, long, action)] pub file: Option, /// Flag for whether position information should be included - #[clap(short, action)] + #[arg(short, action)] pub position: bool, } diff --git a/bril-rs/brild/Cargo.toml b/bril-rs/brild/Cargo.toml index cfbf910c9..a8d718087 100644 --- a/bril-rs/brild/Cargo.toml +++ b/bril-rs/brild/Cargo.toml @@ -13,7 +13,7 @@ keywords = ["compiler", "bril", "parser", "data-structures", "language"] # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html [dependencies] -clap = { version = "3.2", features = ["derive"] } +clap = { version = "4.0", features = ["derive"] } thiserror = "1.0" [dependencies.bril2json] diff --git a/bril-rs/brild/src/cli.rs b/bril-rs/brild/src/cli.rs index 9a508adeb..156e03134 100644 --- a/bril-rs/brild/src/cli.rs +++ b/bril-rs/brild/src/cli.rs @@ -2,12 +2,12 @@ use clap::Parser; use std::path::PathBuf; #[derive(Parser)] -#[clap(about, version, author)] // keeps the cli synced with Cargo.toml +#[command(about, version, author)] // keeps the cli synced with Cargo.toml pub struct Cli { /// The bril file to statically link. stdin is assumed if file is not provided. - #[clap(short, long, action)] + #[arg(short, long, action)] pub file: Option, /// A list of library paths to look for Bril files. - #[clap(short, long, action, multiple_values = true)] + #[arg(short, long, action, num_args=1..)] pub libs: Vec, } diff --git a/bril-rs/rs2bril/Cargo.toml b/bril-rs/rs2bril/Cargo.toml index 5cc05056a..8c059ccbc 100644 --- a/bril-rs/rs2bril/Cargo.toml +++ b/bril-rs/rs2bril/Cargo.toml @@ -16,7 +16,7 @@ keywords = ["compiler", "bril", "parser", "data-structures", "language"] [dependencies] syn = {version = "1.0", features = ["full", "extra-traits"]} proc-macro2 = {version = "1.0", features = ["span-locations"]} -clap = { version = "3.2", features = ["derive"] } +clap = { version = "4.0", features = ["derive"] } [dependencies.bril-rs] version = "0.1.0" diff --git a/bril-rs/rs2bril/src/cli.rs b/bril-rs/rs2bril/src/cli.rs index 14359898c..417e8db63 100644 --- a/bril-rs/rs2bril/src/cli.rs +++ b/bril-rs/rs2bril/src/cli.rs @@ -1,12 +1,12 @@ use clap::Parser; #[derive(Parser)] -#[clap(about, version, author)] // keeps the cli synced with Cargo.toml +#[command(about, version, author)] // keeps the cli synced with Cargo.toml pub struct Cli { /// The bril file to statically link. stdin is assumed if file is not provided. - #[clap(short, long, action)] + #[arg(short, long, action)] pub file: Option, /// Flag for whether position information should be included - #[clap(short, action)] + #[arg(short, action)] pub position: bool, } diff --git a/brilirs/Cargo.toml b/brilirs/Cargo.toml index b891b712f..6e5afee1d 100644 --- a/brilirs/Cargo.toml +++ b/brilirs/Cargo.toml @@ -13,12 +13,12 @@ keywords = ["compiler", "bril", "interpreter", "data-structures", "language"] # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html [build-dependencies] -clap = { version = "3.2", features = ["derive"] } -clap_complete= "3.2" +clap = { version = "4.0", features = ["derive"] } +clap_complete= "4.0" [dependencies] thiserror = "1.0" -clap = { version = "3.2", features = ["derive"] } +clap = { version = "4.0", features = ["derive"] } fxhash = "0.2" mimalloc = "0.1" itoa = "1.0" diff --git a/brilirs/build.rs b/brilirs/build.rs index 3ccf73d3d..1f7760567 100644 --- a/brilirs/build.rs +++ b/brilirs/build.rs @@ -1,6 +1,6 @@ // https://kbknapp.dev/shell-completions/ -use clap::IntoApp; +use clap::CommandFactory; use clap_complete::{ shells::{Bash, Elvish, Fish, PowerShell, Zsh}, Generator, diff --git a/brilirs/src/basic_block.rs b/brilirs/src/basic_block.rs index 4d1fcae4c..bcd20c00a 100644 --- a/brilirs/src/basic_block.rs +++ b/brilirs/src/basic_block.rs @@ -97,14 +97,15 @@ fn get_num_from_map( // A map from variables to numbers num_var_map: &mut FxHashMap, ) -> usize { - match num_var_map.get(variable_name) { - Some(i) => *i, - None => { - let x = *num_of_vars; - num_var_map.insert(variable_name.to_string(), x); - *num_of_vars += 1; - x - } + // https://github.com/rust-lang/rust-clippy/issues/8346 + #[allow(clippy::option_if_let_else)] + if let Some(i) = num_var_map.get(variable_name) { + *i + } else { + let x = *num_of_vars; + num_var_map.insert(variable_name.to_string(), x); + *num_of_vars += 1; + x } } diff --git a/brilirs/src/check.rs b/brilirs/src/check.rs index 95d1dcd99..c6a030cef 100644 --- a/brilirs/src/check.rs +++ b/brilirs/src/check.rs @@ -43,12 +43,13 @@ fn update_env<'a>( dest: &'a str, typ: &'a Type, ) -> Result<(), InterpError> { - match env.get(dest) { - Some(current_typ) => check_asmt_type(current_typ, typ), - None => { - env.insert(dest, typ); - Ok(()) - } + // https://github.com/rust-lang/rust-clippy/issues/8346 + #[allow(clippy::option_if_let_else)] + if let Some(current_typ) = env.get(dest) { + check_asmt_type(current_typ, typ) + } else { + env.insert(dest, typ); + Ok(()) } } diff --git a/brilirs/src/cli.rs b/brilirs/src/cli.rs index 248883ef0..b9167bb77 100644 --- a/brilirs/src/cli.rs +++ b/brilirs/src/cli.rs @@ -1,26 +1,26 @@ use clap::Parser; #[derive(Parser)] -#[clap(about, version, author)] // keeps the cli synced with Cargo.toml -#[clap(allow_hyphen_values(true))] +#[command(about, version, author)] // keeps the cli synced with Cargo.toml +#[command(allow_hyphen_values(true))] pub struct Cli { /// Flag to output the total number of dynamic instructions - #[clap(short, long, action)] + #[arg(short, long, action)] pub profile: bool, /// The bril file to run. stdin is assumed if file is not provided - #[clap(short, long, action)] + #[arg(short, long, action)] pub file: Option, /// Flag to only typecheck/validate the bril program - #[clap(short, long, action)] + #[arg(short, long, action)] pub check: bool, /// Flag for when the bril program is in text form - #[clap(short, long, action)] + #[arg(short, long, action)] pub text: bool, /// Arguments for the main function - #[clap(action)] + #[arg(action)] pub args: Vec, } diff --git a/brilirs/src/interp.rs b/brilirs/src/interp.rs index d27b072d8..f7604871c 100644 --- a/brilirs/src/interp.rs +++ b/brilirs/src/interp.rs @@ -44,16 +44,16 @@ impl Environment { } } - pub fn get(&self, ident: &usize) -> &Value { + pub fn get(&self, ident: usize) -> &Value { // A bril program is well formed when, dynamically, every variable is defined before its use. // If this is violated, this will return Value::Uninitialized and the whole interpreter will come crashing down. - self.env.get(self.current_pointer + *ident).unwrap() + self.env.get(self.current_pointer + ident).unwrap() } // Used for getting arguments that should be passed to the current frame from the previous one - pub fn get_from_last_frame(&self, ident: &usize) -> &Value { + pub fn get_from_last_frame(&self, ident: usize) -> &Value { let past_pointer = self.stack_pointers.last().unwrap().0; - self.env.get(past_pointer + *ident).unwrap() + self.env.get(past_pointer + ident).unwrap() } pub fn set(&mut self, ident: usize, val: Value) { @@ -161,7 +161,7 @@ impl Heap { // you just want the underlying value(like a f64). // Or can just be used to get a owned version of the Value fn get_arg<'a, T: From<&'a Value>>(vars: &'a Environment, index: usize, args: &[usize]) -> T { - T::from(vars.get(&args[index])) + T::from(vars.get(args[index])) } #[derive(Debug, Default, Clone, Copy)] @@ -290,7 +290,7 @@ fn make_func_args<'a>(callee_func: &'a BBFunction, args: &[usize], vars: &mut En .iter() .zip(callee_func.args_as_nums.iter()) .for_each(|(arg_name, expected_arg)| { - let arg = vars.get_from_last_frame(arg_name); + let arg = vars.get_from_last_frame(*arg_name); vars.set(*expected_arg, *arg); }); } @@ -493,7 +493,7 @@ fn execute_effect_op<'a, T: std::io::Write>( // In the typical case, users only print out one value at a time // So we can usually avoid extra allocations by providing that string directly if args.len() == 1 { - optimized_val_output(&mut state.out, state.env.get(args.first().unwrap()))?; + optimized_val_output(&mut state.out, state.env.get(*args.first().unwrap()))?; // Add new line state.out.write_all(&[b'\n'])?; } else { @@ -502,7 +502,7 @@ fn execute_effect_op<'a, T: std::io::Write>( "{}", args .iter() - .map(|a| state.env.get(a).to_string()) + .map(|a| state.env.get(*a).to_string()) .collect::>() .join(" ") )?; From 4614676c19ded28dd20beb992e49086368afc39d Mon Sep 17 00:00:00 2001 From: Patrick LaFontaine <32135464+Pat-Lafon@users.noreply.github.com> Date: Sun, 2 Oct 2022 20:39:01 -0400 Subject: [PATCH 6/7] missing undef check in brilck? --- brilck.ts | 38 ++++++++++++++++++++------------------ 1 file changed, 20 insertions(+), 18 deletions(-) diff --git a/brilck.ts b/brilck.ts index a3d51a671..4411b20a6 100644 --- a/brilck.ts +++ b/brilck.ts @@ -281,14 +281,14 @@ const INSTR_CHECKS: {[key: string]: CheckFunc} = { } return; }, - + phi: (env, instr) => { let args = instr.args ?? []; if (!('type' in instr)) { err(`phi needs a result type`, instr.pos); return; } - + // Construct a signature with uniform argument types. let argTypes: bril.Type[] = []; for (let i = 0; i < args.length; ++i) { @@ -353,25 +353,27 @@ function checkFunc(funcs: FuncEnv, func: bril.Function) { } // Gather up all the types of the local variables and all the label names. - for (let instr of func.instrs) { - if ('dest' in instr) { - addType(vars, instr.dest, instr.type, instr.pos); - } else if ('label' in instr) { - if (labels.has(instr.label)) { - err(`multiply defined label .${instr.label}`, instr.pos); - } else { - labels.add(instr.label); + if (func.instrs){ + for (let instr of func.instrs) { + if ('dest' in instr) { + addType(vars, instr.dest, instr.type, instr.pos); + } else if ('label' in instr) { + if (labels.has(instr.label)) { + err(`multiply defined label .${instr.label}`, instr.pos); + } else { + labels.add(instr.label); + } } } - } - // Check each instruction. - for (let instr of func.instrs) { - if ('op' in instr) { - if (instr.op === 'const') { - checkConst(instr); - } else { - checkOp({vars, labels, funcs, ret: func.type}, instr); + // Check each instruction. + for (let instr of func.instrs) { + if ('op' in instr) { + if (instr.op === 'const') { + checkConst(instr); + } else { + checkOp({vars, labels, funcs, ret: func.type}, instr); + } } } } From 658de4e162e37bf261f27a8f6d58ad41a3ee9152 Mon Sep 17 00:00:00 2001 From: Patrick LaFontaine <32135464+Pat-Lafon@users.noreply.github.com> Date: Sun, 2 Oct 2022 21:02:15 -0400 Subject: [PATCH 7/7] only enable end positions with -p -p --- bril-rs/bril2json/src/cli.rs | 6 +++--- bril-rs/bril2json/src/lib.rs | 21 ++++++++++++++++----- bril-rs/bril2json/src/main.rs | 6 +++++- bril-rs/brild/src/lib.rs | 31 +++++++++++++++++-------------- brilirs/src/lib.rs | 2 +- test/parse/positions.json | 28 ---------------------------- 6 files changed, 42 insertions(+), 52 deletions(-) diff --git a/bril-rs/bril2json/src/cli.rs b/bril-rs/bril2json/src/cli.rs index 417e8db63..255443cfb 100644 --- a/bril-rs/bril2json/src/cli.rs +++ b/bril-rs/bril2json/src/cli.rs @@ -1,4 +1,4 @@ -use clap::Parser; +use clap::{ArgAction::Count, Parser}; #[derive(Parser)] #[command(about, version, author)] // keeps the cli synced with Cargo.toml @@ -7,6 +7,6 @@ pub struct Cli { #[arg(short, long, action)] pub file: Option, /// Flag for whether position information should be included - #[arg(short, action)] - pub position: bool, + #[arg(short, action = Count)] + pub position: u8, } diff --git a/bril-rs/bril2json/src/lib.rs b/bril-rs/bril2json/src/lib.rs index b6a10e7d3..41d45d644 100644 --- a/bril-rs/bril2json/src/lib.rs +++ b/bril-rs/bril2json/src/lib.rs @@ -16,14 +16,16 @@ use bril_rs::{AbstractProgram, ColRow, Position}; #[derive(Clone)] pub struct Lines { use_pos: bool, + with_end: bool, new_lines: Vec, src_name: Option, } impl Lines { - fn new(input: &str, use_pos: bool, src_name: Option) -> Self { + fn new(input: &str, use_pos: bool, with_end: bool, src_name: Option) -> Self { Self { use_pos, + with_end, src_name, new_lines: input .as_bytes() @@ -38,7 +40,11 @@ impl Lines { if self.use_pos { Some(Position { pos: self.get_row_col(starting_index).unwrap(), - pos_end: self.get_row_col(ending_index), + pos_end: if self.with_end { + self.get_row_col(ending_index) + } else { + None + }, src: self.src_name.clone(), }) } else { @@ -82,6 +88,7 @@ impl Lines { pub fn parse_abstract_program_from_read( mut input: R, use_pos: bool, + with_end: bool, file_name: Option, ) -> AbstractProgram { let mut buffer = String::new(); @@ -91,7 +98,7 @@ pub fn parse_abstract_program_from_read( let src_name = file_name.map(|f| std::fs::canonicalize(f).unwrap().display().to_string()); parser - .parse(&Lines::new(&buffer, use_pos, src_name), &buffer) + .parse(&Lines::new(&buffer, use_pos, with_end, src_name), &buffer) .unwrap() } @@ -99,11 +106,15 @@ pub fn parse_abstract_program_from_read( /// A wrapper around [`parse_abstract_program_from_read`] which assumes [`std::io::Stdin`] if `file_name` is [`None`] /// # Panics /// Will panic if the input is not well-formed Bril text or if `file_name` does not exist -pub fn parse_abstract_program(use_pos: bool, file_name: Option) -> AbstractProgram { +pub fn parse_abstract_program( + use_pos: bool, + with_end: bool, + file_name: Option, +) -> AbstractProgram { let input: Box = match file_name.clone() { Some(f) => Box::new(File::open(f).unwrap()), None => Box::new(std::io::stdin()), }; - parse_abstract_program_from_read(input, use_pos, file_name) + parse_abstract_program_from_read(input, use_pos, with_end, file_name) } diff --git a/bril-rs/bril2json/src/main.rs b/bril-rs/bril2json/src/main.rs index 78b8f9445..b6d9ff8f1 100644 --- a/bril-rs/bril2json/src/main.rs +++ b/bril-rs/bril2json/src/main.rs @@ -5,5 +5,9 @@ use clap::Parser; fn main() { let args = Cli::parse(); - output_abstract_program(&parse_abstract_program(args.position, args.file)) + output_abstract_program(&parse_abstract_program( + args.position >= 1, + args.position >= 2, + args.file, + )) } diff --git a/bril-rs/brild/src/lib.rs b/bril-rs/brild/src/lib.rs index 2987b2806..5742567d3 100644 --- a/bril-rs/brild/src/lib.rs +++ b/bril-rs/brild/src/lib.rs @@ -204,20 +204,23 @@ pub fn do_import( path_map.insert(canonical_path.clone(), None); // Find the correct parser for this path based on the extension - let f: Box AbstractProgram> = match canonical_path - .extension() - .and_then(std::ffi::OsStr::to_str) - { - Some("bril") => Box::new(|s| { - parse_abstract_program_from_read(s, true, Some(canonical_path.display().to_string())) - }), - Some("json") => Box::new(load_abstract_program_from_read), - Some(_) | None => { - return Err(BrildError::MissingOrUnknownFileExtension( - canonical_path.clone(), - )) - } - }; + let f: Box AbstractProgram> = + match canonical_path.extension().and_then(std::ffi::OsStr::to_str) { + Some("bril") => Box::new(|s| { + parse_abstract_program_from_read( + s, + true, + true, + Some(canonical_path.display().to_string()), + ) + }), + Some("json") => Box::new(load_abstract_program_from_read), + Some(_) | None => { + return Err(BrildError::MissingOrUnknownFileExtension( + canonical_path.clone(), + )) + } + }; // Get the AbstractProgram representation of the file let program = f(File::open(&canonical_path)?); diff --git a/brilirs/src/lib.rs b/brilirs/src/lib.rs index ed13049b7..4c72bd395 100644 --- a/brilirs/src/lib.rs +++ b/brilirs/src/lib.rs @@ -37,7 +37,7 @@ pub fn run_input( // - bril_rs takes file.json as input // - bril2json takes file.bril as input let prog: Program = if text { - bril2json::parse_abstract_program_from_read(input, true, src_name).try_into()? + bril2json::parse_abstract_program_from_read(input, true, true, src_name).try_into()? } else { bril_rs::load_abstract_program_from_read(input).try_into()? }; diff --git a/test/parse/positions.json b/test/parse/positions.json index 7f02951f9..d2c30591c 100644 --- a/test/parse/positions.json +++ b/test/parse/positions.json @@ -9,10 +9,6 @@ "col": 3, "row": 3 }, - "pos_end": { - "col": 21, - "row": 3 - }, "type": "int", "value": 1 }, @@ -23,10 +19,6 @@ "col": 3, "row": 4 }, - "pos_end": { - "col": 21, - "row": 4 - }, "type": "int", "value": 2 }, @@ -38,10 +30,6 @@ "pos": { "col": 3, "row": 5 - }, - "pos_end": { - "col": 14, - "row": 5 } }, { @@ -49,10 +37,6 @@ "pos": { "col": 1, "row": 6 - }, - "pos_end": { - "col": 8, - "row": 6 } }, { @@ -66,10 +50,6 @@ "col": 3, "row": 7 }, - "pos_end": { - "col": 23, - "row": 7 - }, "type": "int" }, { @@ -80,10 +60,6 @@ "pos": { "col": 3, "row": 8 - }, - "pos_end": { - "col": 12, - "row": 8 } } ], @@ -91,10 +67,6 @@ "pos": { "col": 1, "row": 2 - }, - "pos_end": { - "col": 7, - "row": 2 } } ]