From 3ca28157d1be863861652c937349695e34da7d92 Mon Sep 17 00:00:00 2001 From: Justus Adam Date: Fri, 1 Mar 2024 17:42:37 -0500 Subject: [PATCH 01/20] Pull forward the error handling code --- Cargo.lock | 14 +++- crates/paralegal-flow/src/ana/mod.rs | 83 ++++++++++++++++++++ crates/paralegal-policy/Cargo.toml | 1 + crates/paralegal-policy/src/context.rs | 101 +++++++++++++++++++++++++ crates/paralegal-spdg/src/lib.rs | 45 ++++++++++- 5 files changed, 242 insertions(+), 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index c3c7711de2..18173ae11b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -283,6 +283,17 @@ version = "1.0.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "acbf1af155f9b9ef647e42cdc158db4b64a1b61f743629225fde6f3e0be2a7c7" +[[package]] +name = "colored" +version = "1.9.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5a5f741c91823341bebf717d4c71bda820630ce065443b58bd1b7451af008355" +dependencies = [ + "is-terminal", + "lazy_static", + "winapi", +] + [[package]] name = "colored" version = "2.0.4" @@ -821,6 +832,7 @@ version = "0.1.0" dependencies = [ "anyhow", "bitvec", + "colored 1.9.4", "indexical", "itertools 0.11.0", "lazy_static", @@ -1107,7 +1119,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "48047e77b528151aaf841a10a9025f9459da80ba820e425ff7eb005708a76dc7" dependencies = [ "atty", - "colored", + "colored 2.0.4", "log", "time", "winapi", diff --git a/crates/paralegal-flow/src/ana/mod.rs b/crates/paralegal-flow/src/ana/mod.rs index 105a416240..f22ed921c6 100644 --- a/crates/paralegal-flow/src/ana/mod.rs +++ b/crates/paralegal-flow/src/ana/mod.rs @@ -20,6 +20,7 @@ use flowistry::pdg::CallChanges; use flowistry::pdg::SkipCall::Skip; use paralegal_spdg::Node; use petgraph::visit::{GraphBase, IntoNodeReferences, NodeIndexable, NodeRef}; +use rustc_span::{FileNameDisplayPreference, Span}; use super::discover::FnToAnalyze; @@ -114,11 +115,68 @@ impl<'tcx> SPDGGenerator<'tcx> { .iter() .map(|id| (*id, def_info_for_item(*id, tcx))) .collect(); + + let call_sites: HashSet<&CallSite> = controllers + .values() + .flat_map(|ctrl| { + ctrl.all_sources() + .filter_map(|src| src.as_function_call()) + .chain( + ctrl.data_sinks() + .filter_map(|sink| Some(sink.as_argument()?.0)), + ) + .chain(ctrl.call_sites()) + }) + .collect(); + let src_locs: HashMap = call_sites + .into_iter() + .map(|call_site| { + let call_site_defid: DefId = call_site.function; + let locations: &[GlobalLocation] = call_site.location.as_slice(); + let call_loc: Vec = locations + .iter() + .filter_map(|loc| { + let body: &mir::Body<'_> = &tcx.body_for_def_id(loc.function).ok()?.body; + let expanded_span: Span = body.stmt_at(loc.location).either( + |statement| statement.source_info.span, + |terminator| terminator.source_info.span, + ); + let stmt_span = tcx.sess.source_map().stmt_span(expanded_span, body.span); + Some(CallSiteSpan { + loc: src_loc_for_span(stmt_span, tcx), + expanded_loc: src_loc_for_span(expanded_span, tcx), + }) + }) + .collect(); + let func_def_span = tcx.def_span(call_site_defid); + ( + call_site_defid, + SrcCodeInfo { + func_iden: tcx.def_path_str(call_site_defid), + func_header_loc: src_loc_for_span(func_def_span, tcx), + call_loc, + }, + ) + }) + .chain(controllers.keys().map(|ctrl_id| { + let ctrl_span = tcx.def_span(ctrl_id.to_def_id()); + ( + *ctrl_id, + SrcCodeInfo { + func_iden: tcx.def_path_str(*ctrl_id), + func_header_loc: src_loc_for_span(ctrl_span, tcx), + call_loc: Vec::::new(), + }, + ) + })) + .collect(); + ProgramDescription { type_info: self.collect_type_info(&controllers), instruction_info: self.collect_instruction_info(&controllers), controllers, def_info, + src_locs, } } @@ -203,6 +261,31 @@ impl<'tcx> SPDGGenerator<'tcx> { } } +fn src_loc_for_span(span: Span, tcx: TyCtxt) -> SrcCodeSpan { + let (source_file, start_line, start_col, end_line, end_col) = + tcx.sess.source_map().span_to_location_info(span); + let file_path = source_file + .expect("could not find source file") + .name + .display(FileNameDisplayPreference::Local) + .to_string(); + let abs_file_path = if !file_path.starts_with('/') { + std::env::current_dir() + .expect("failed to obtain current working directory") + .join(&file_path) + } else { + std::path::PathBuf::from(&file_path) + }; + SrcCodeSpan { + file_path, + abs_file_path, + start_line, + start_col, + end_line, + end_col, + } +} + fn default_index() -> ::NodeId { ::NodeId::end() } diff --git a/crates/paralegal-policy/Cargo.toml b/crates/paralegal-policy/Cargo.toml index 71461f6dd5..4852af9b78 100644 --- a/crates/paralegal-policy/Cargo.toml +++ b/crates/paralegal-policy/Cargo.toml @@ -16,6 +16,7 @@ simple_logger = "2" lazy_static = "1" bitvec = "1" petgraph = { workspace = true } +colored = "1" [dev-dependencies] paralegal-flow = { path = "../paralegal-flow", features = ["test"] } diff --git a/crates/paralegal-policy/src/context.rs b/crates/paralegal-policy/src/context.rs index 68b9e83698..7d81e9107d 100644 --- a/crates/paralegal-policy/src/context.rs +++ b/crates/paralegal-policy/src/context.rs @@ -8,6 +8,7 @@ use paralegal_spdg::{ }; use anyhow::{anyhow, bail, ensure, Result}; +use colored::*; use itertools::{Either, Itertools}; use petgraph::prelude::Bfs; use petgraph::visit::{Control, DfsEvent, EdgeFiltered, EdgeRef, Walker}; @@ -601,6 +602,92 @@ impl Context { } NodeCluster::new(src.controller_id(), start) } + + /// Prints a diagnostic message for a given problematic node, given the type and coloring + /// of said diagnostic and the message to be printed + fn print_node_diagnostic( + &self, + diag_type: &str, + coloring: impl Fn(&str) -> ColoredString, + node: GlobalNode, + msg: &str, + ) -> Result<()> { + let src_loc = self + .get_location(&node) + .ok_or(anyhow::Error::msg("node's location was not found in mapping"))?; + + let max_line_len = std::cmp::max( + src_loc.start_line.to_string().len(), + src_loc.end_line.to_string().len(), + ); + + println!("{}: {}", coloring(diag_type), msg); + let tab: String = " ".repeat(max_line_len); + println!( + "{}{} {}:{}:{}", + tab, + as_blue("-->"), + src_loc.file_path, + src_loc.start_line, + src_loc.start_col, + ); + println!("{} {}", tab, as_blue("|")); + let lines = std::io::BufReader::new(std::fs::File::open(&src_loc.abs_file_path)?) + .lines() + .skip(src_loc.start_line - 1) + .take(src_loc.end_line - src_loc.start_line + 1) + .enumerate(); + for (i, line) in lines { + let line_content: String = line?; + let line_num = src_loc.start_line + i; + let end: usize = if line_num == src_loc.end_line { + src_loc.end_col + } else { + line_content.len() + 1 + }; + let start: usize = if line_num == src_loc.start_line { + src_loc.start_col + } else { + line_content + .find(|c: char| !c.is_whitespace()) + .unwrap_or(end - 1) + + 1 + }; + let tab_len = max_line_len - line_num.to_string().len(); + + println!( + "{}{} {} {}", + " ".repeat(tab_len), + as_blue(&line_num.to_string()), + as_blue("|"), + line_content + ); + println!( + "{} {} {}{}", + tab, + as_blue("|"), + " ".repeat(start - 1), + coloring(&"^".repeat(end - start)) + ); + } + println!("{} {}", tab, as_blue("|")); + Ok(()) + } + + /// Prints an error message for a problematic node + pub fn print_node_error(&self, node: GlobalNode, msg: &str) -> () { + let _ = self.print_node_diagnostic("error", as_red, node, msg); + } + + /// Prints a warning message for a problematic node + pub fn print_node_warning(&self, node: GlobalNode, msg: &str) -> () { + let _ = self.print_node_diagnostic("warning", as_yellow, node, msg); + } + + /// Prints a note for a problematic node + pub fn print_node_note(&self, node: GlobalNode, msg: &str) -> () { + let _ = self.print_node_diagnostic("note", as_green, node, msg); + } } /// Provide display trait for DefId in a Context. @@ -817,6 +904,20 @@ fn test_happens_before() -> Result<()> { Ok(()) } +// For colored output for error printing +fn as_blue(input: &str) -> ColoredString { + input.blue() +} +fn as_green(input: &str) -> ColoredString { + input.green() +} +fn as_yellow(input: &str) -> ColoredString { + input.yellow() +} +fn as_red(input: &str) -> ColoredString { + input.red() +} + #[test] fn test_influencees() -> Result<()> { let ctx = crate::test_utils::test_ctx(); diff --git a/crates/paralegal-spdg/src/lib.rs b/crates/paralegal-spdg/src/lib.rs index ff9269ef31..582df821e7 100644 --- a/crates/paralegal-spdg/src/lib.rs +++ b/crates/paralegal-spdg/src/lib.rs @@ -31,7 +31,7 @@ use internment::Intern; use itertools::Itertools; use rustc_portable::DefId; use serde::{Deserialize, Serialize}; -use std::{fmt, hash::Hash}; +use std::{fmt, hash::Hash, path::PathBuf}; use utils::serde_map_via_vec; @@ -270,6 +270,7 @@ pub struct DefInfo { pub path: Vec, /// Kind of object pub kind: DefKind, + pub src_info: SrcCodeInfo, } /// Similar to `DefKind` in rustc but *not the same*! @@ -283,6 +284,48 @@ pub enum DefKind { Type, } +/// Encodes a source code location +#[derive(Clone, PartialEq, Eq, Serialize, Deserialize, Debug)] +pub struct SrcCodeSpan { + /// Printable location of the source code file - either an absolute path to library source code + /// or a path relative to within the compiled crate (e.g. `src/...`) + pub file_path: String, + /// Absolute path to source code file + pub abs_file_path: PathBuf, + /// The starting line of the location within the file (note: a one-based index) + pub start_line: usize, + /// The column of starting line that the location starts at within the file (note: a one-based index) + pub start_col: usize, + /// The ending line of the location within the file (note: a one-based index) + pub end_line: usize, + /// The column of ending line that the location ends at within the file (note: a one-based index) + pub end_col: usize, +} + +/// Encodes a location of a call site +#[derive(Clone, PartialEq, Eq, Serialize, Deserialize, Debug)] +pub struct CallSiteSpan { + /// The source code location of the call site - if the call site occurs within a macro, this + /// refers to the macro's call site + pub loc: SrcCodeSpan, + /// The expanded location of the call site - if the call site occurs within a macro, this + /// refers to its location within the macro's definition + pub expanded_loc: SrcCodeSpan, +} + +/// Encodes source code information for controllers and call site nodes in the SPDG +#[derive(Clone, PartialEq, Eq, Serialize, Deserialize, Debug)] +pub struct SrcCodeInfo { + /// Identifier of the function + pub func_iden: String, + /// Location of the header of the function's definition + pub func_header_loc: SrcCodeSpan, + /// Location of the function's call site, contains the source code locations of the + /// call chain from within the controller to the call site of said function (this field + /// is empty for a controller) + pub call_loc: Vec, +} + #[derive(Debug, Clone, Copy, Serialize, Deserialize, Eq, Ord, PartialOrd, PartialEq)] pub struct FunctionCallInfo { pub is_inlined: bool, From 0cbea5ed65a68c20e0f2a9be51e5acff3238fe80 Mon Sep 17 00:00:00 2001 From: Justus Adam Date: Fri, 1 Mar 2024 18:11:57 -0500 Subject: [PATCH 02/20] Fixed up the errors --- crates/paralegal-flow/src/ana/mod.rs | 121 +++++++++------------- crates/paralegal-flow/src/test_utils.rs | 1 + crates/paralegal-policy/src/context.rs | 21 ++-- crates/paralegal-policy/src/test_utils.rs | 6 +- crates/paralegal-spdg/src/dot.rs | 16 +-- crates/paralegal-spdg/src/lib.rs | 21 ++-- 6 files changed, 92 insertions(+), 94 deletions(-) diff --git a/crates/paralegal-flow/src/ana/mod.rs b/crates/paralegal-flow/src/ana/mod.rs index 6cf5cb8efd..d8a83e7a19 100644 --- a/crates/paralegal-flow/src/ana/mod.rs +++ b/crates/paralegal-flow/src/ana/mod.rs @@ -121,61 +121,6 @@ impl<'tcx> SPDGGenerator<'tcx> { .map(|id| (*id, def_info_for_item(*id, tcx))) .collect(); - let call_sites: HashSet<&CallSite> = controllers - .values() - .flat_map(|ctrl| { - ctrl.all_sources() - .filter_map(|src| src.as_function_call()) - .chain( - ctrl.data_sinks() - .filter_map(|sink| Some(sink.as_argument()?.0)), - ) - .chain(ctrl.call_sites()) - }) - .collect(); - let src_locs: HashMap = call_sites - .into_iter() - .map(|call_site| { - let call_site_defid: DefId = call_site.function; - let locations: &[GlobalLocation] = call_site.location.as_slice(); - let call_loc: Vec = locations - .iter() - .filter_map(|loc| { - let body: &mir::Body<'_> = &tcx.body_for_def_id(loc.function).ok()?.body; - let expanded_span: Span = body.stmt_at(loc.location).either( - |statement| statement.source_info.span, - |terminator| terminator.source_info.span, - ); - let stmt_span = tcx.sess.source_map().stmt_span(expanded_span, body.span); - Some(CallSiteSpan { - loc: src_loc_for_span(stmt_span, tcx), - expanded_loc: src_loc_for_span(expanded_span, tcx), - }) - }) - .collect(); - let func_def_span = tcx.def_span(call_site_defid); - ( - call_site_defid, - SrcCodeInfo { - func_iden: tcx.def_path_str(call_site_defid), - func_header_loc: src_loc_for_span(func_def_span, tcx), - call_loc, - }, - ) - }) - .chain(controllers.keys().map(|ctrl_id| { - let ctrl_span = tcx.def_span(ctrl_id.to_def_id()); - ( - *ctrl_id, - SrcCodeInfo { - func_iden: tcx.def_path_str(*ctrl_id), - func_header_loc: src_loc_for_span(ctrl_span, tcx), - call_loc: Vec::::new(), - }, - ) - })) - .collect(); - let type_info = self.collect_type_info(); type_info_sanity_check(&controllers, &type_info); ProgramDescription { @@ -183,7 +128,6 @@ impl<'tcx> SPDGGenerator<'tcx> { instruction_info: self.collect_instruction_info(&controllers), controllers, def_info, - src_locs, } } @@ -205,23 +149,51 @@ impl<'tcx> SPDGGenerator<'tcx> { all_instructions .into_iter() .map(|i| { + let tcx = self.tcx; let body = self.tcx.body_for_def_id(i.function).unwrap(); + let with_default_spans = |kind| { + let default_span = src_loc_for_span(tcx.def_span(i.function.to_def_id()), tcx); + InstructionInfo { + kind, + call_loc: CallSiteSpan { + loc: default_span.clone(), + expanded_loc: default_span, + }, + } + }; + let info = match i.location { - RichLocation::End => InstructionInfo::Return, - RichLocation::Start => InstructionInfo::Start, - RichLocation::Location(loc) => match body.body.stmt_at(loc) { - crate::Either::Right(term) => { - if let Ok((id, ..)) = term.as_fn_and_args(self.tcx) { - InstructionInfo::FunctionCall(FunctionCallInfo { - id, - is_inlined: id.is_local(), - }) - } else { - InstructionInfo::Terminator + RichLocation::End => with_default_spans(InstructionKind::Return), + RichLocation::Start => with_default_spans(InstructionKind::Start), + RichLocation::Location(loc) => { + let (kind, expanded_span) = match body.body.stmt_at(loc) { + crate::Either::Right(term) => { + let kind = if let Ok((id, ..)) = term.as_fn_and_args(self.tcx) { + InstructionKind::FunctionCall(FunctionCallInfo { + id, + is_inlined: id.is_local(), + }) + } else { + InstructionKind::Terminator + }; + (kind, term.source_info.span) + } + crate::Either::Left(stmt) => { + (InstructionKind::Statement, stmt.source_info.span) } + }; + let stmt_span = tcx + .sess + .source_map() + .stmt_span(expanded_span, body.body.span); + InstructionInfo { + kind, + call_loc: CallSiteSpan { + loc: src_loc_for_span(stmt_span, tcx), + expanded_loc: src_loc_for_span(expanded_span, tcx), + }, } - _ => InstructionInfo::Statement, - }, + } }; (i, info) }) @@ -812,7 +784,16 @@ fn def_info_for_item(id: DefId, tcx: TyCtxt) -> DefInfo { } })) .collect(); - DefInfo { name, path, kind } + let src_info = SrcCodeInfo { + func_iden: tcx.def_path_str(id), + func_header_loc: src_loc_for_span(tcx.def_span(id), tcx), + }; + DefInfo { + name, + path, + kind, + src_info, + } } /// A higher order function that increases the logging level if the `target` diff --git a/crates/paralegal-flow/src/test_utils.rs b/crates/paralegal-flow/src/test_utils.rs index 96c558889a..9c87583eb0 100644 --- a/crates/paralegal-flow/src/test_utils.rs +++ b/crates/paralegal-flow/src/test_utils.rs @@ -350,6 +350,7 @@ impl<'g> CtrlRef<'g> { .chain(self.ctrl.graph.node_weights().map(|info| info.at)) .filter(|m| { instruction_info[&m.leaf()] + .kind .as_function_call() .map_or(false, |i| i.id == fun.ident) }) diff --git a/crates/paralegal-policy/src/context.rs b/crates/paralegal-policy/src/context.rs index c86442c229..3539a76261 100644 --- a/crates/paralegal-policy/src/context.rs +++ b/crates/paralegal-policy/src/context.rs @@ -3,9 +3,9 @@ use std::{io::Write, process::exit, sync::Arc}; pub use paralegal_spdg::rustc_portable::{DefId, LocalDefId}; use paralegal_spdg::traverse::{generic_flows_to, EdgeSelection}; use paralegal_spdg::{ - CallString, DisplayNode, Endpoint, GlobalNode, HashMap, Identifier, InstructionInfo, - IntoIterGlobalNodes, Node as SPDGNode, NodeCluster, NodeInfo, ProgramDescription, SPDGImpl, - TypeId, SPDG, + CallSiteSpan, CallString, DisplayNode, Endpoint, GlobalNode, HashMap, Identifier, + InstructionInfo, IntoIterGlobalNodes, Node as SPDGNode, NodeCluster, NodeInfo, + ProgramDescription, SPDGImpl, TypeId, SPDG, }; use anyhow::{anyhow, bail, ensure, Result}; @@ -631,6 +631,11 @@ impl Context { NodeCluster::new(src.controller_id(), start) } + fn get_location(&self, node: GlobalNode) -> Option<&CallSiteSpan> { + let at = self.node_info(node).at; + Some(&self.desc().instruction_info.get(&at.leaf())?.call_loc) + } + /// Prints a diagnostic message for a given problematic node, given the type and coloring /// of said diagnostic and the message to be printed fn print_node_diagnostic( @@ -640,9 +645,13 @@ impl Context { node: GlobalNode, msg: &str, ) -> Result<()> { - let src_loc = self - .get_location(&node) - .ok_or(anyhow::Error::msg("node's location was not found in mapping"))?; + use std::io::BufRead; + let src_loc = &self + .get_location(node) + .ok_or(anyhow::Error::msg( + "node's location was not found in mapping", + ))? + .loc; let max_line_len = std::cmp::max( src_loc.start_line.to_string().len(), diff --git a/crates/paralegal-policy/src/test_utils.rs b/crates/paralegal-policy/src/test_utils.rs index 5c9f263c18..9c0c76441f 100644 --- a/crates/paralegal-policy/src/test_utils.rs +++ b/crates/paralegal-policy/src/test_utils.rs @@ -3,7 +3,7 @@ use crate::ControllerId; use paralegal_flow::test_utils::PreFrg; use paralegal_spdg::IntoIterGlobalNodes; use paralegal_spdg::NodeCluster; -use paralegal_spdg::{Identifier, InstructionInfo, Node as SPDGNode, SPDG}; +use paralegal_spdg::{Identifier, InstructionKind, Node as SPDGNode, SPDG}; use std::sync::Arc; use std::sync::OnceLock; @@ -51,8 +51,8 @@ fn is_at_function_call_with_name( let weight = ctrl.graph.node_weight(node).unwrap().at; let instruction = &ctx.desc().instruction_info[&weight.leaf()]; matches!( - instruction, - InstructionInfo::FunctionCall(call) if + instruction.kind, + InstructionKind::FunctionCall(call) if ctx.desc().def_info[&call.id].name == name ) } diff --git a/crates/paralegal-spdg/src/dot.rs b/crates/paralegal-spdg/src/dot.rs index bb12950e2c..e6c228f682 100644 --- a/crates/paralegal-spdg/src/dot.rs +++ b/crates/paralegal-spdg/src/dot.rs @@ -1,6 +1,6 @@ //! Display SPDGs as dot graphs -use crate::{GlobalEdge, InstructionInfo, Node, ProgramDescription}; +use crate::{GlobalEdge, InstructionKind, Node, ProgramDescription}; use dot::{CompassPoint, Edges, Id, LabelText, Nodes}; use flowistry_pdg::rustc_portable::LocalDefId; use flowistry_pdg::{CallString, RichLocation}; @@ -108,7 +108,7 @@ impl<'a, 'd> dot::Labeller<'a, CallString, GlobalEdge> for DotPrintableProgramDe fn node_label(&'a self, n: &CallString) -> LabelText<'a> { let (ctrl_id, nodes) = &self.call_sites[n]; let ctrl = &self.spdg.controllers[ctrl_id]; - let instruction = self.spdg.instruction_info[&n.leaf()]; + let instruction = &self.spdg.instruction_info[&n.leaf()]; let write_label = || { use std::fmt::Write; @@ -116,17 +116,17 @@ impl<'a, 'd> dot::Labeller<'a, CallString, GlobalEdge> for DotPrintableProgramDe write!(s, "{}|", self.format_call_string(*n))?; - match instruction { - InstructionInfo::Statement => s.push('S'), - InstructionInfo::FunctionCall(function) => { + match instruction.kind { + InstructionKind::Statement => s.push('S'), + InstructionKind::FunctionCall(function) => { let info = &self.spdg.def_info[&function.id]; write!(s, "{}", info.name)? } - InstructionInfo::Terminator => s.push('T'), - InstructionInfo::Start => { + InstructionKind::Terminator => s.push('T'), + InstructionKind::Start => { s.push('*'); } - InstructionInfo::Return => s.push_str("end"), + InstructionKind::Return => s.push_str("end"), }; for &n in nodes { diff --git a/crates/paralegal-spdg/src/lib.rs b/crates/paralegal-spdg/src/lib.rs index 448c306a0c..aaab50a045 100644 --- a/crates/paralegal-spdg/src/lib.rs +++ b/crates/paralegal-spdg/src/lib.rs @@ -125,6 +125,7 @@ pub struct DefInfo { pub path: Vec, /// Kind of object pub kind: DefKind, + /// Information about the span pub src_info: SrcCodeInfo, } @@ -179,10 +180,6 @@ pub struct SrcCodeInfo { pub func_iden: String, /// Location of the header of the function's definition pub func_header_loc: SrcCodeSpan, - /// Location of the function's call site, contains the source code locations of the - /// call chain from within the controller to the call site of said function (this field - /// is empty for a controller) - pub call_loc: Vec, } /// Metadata on a function call. @@ -199,7 +196,7 @@ pub struct FunctionCallInfo { #[derive( Debug, Clone, Copy, Serialize, Deserialize, Eq, Ord, PartialOrd, PartialEq, strum::EnumIs, )] -pub enum InstructionInfo { +pub enum InstructionKind { /// Some type of statement Statement, /// A function call @@ -212,11 +209,21 @@ pub enum InstructionInfo { Return, } -impl InstructionInfo { +/// Information about instructions +#[derive(Serialize, Deserialize, Debug)] +pub struct InstructionInfo { + /// The kind of instruction + pub kind: InstructionKind, + /// call chain from within the controller to the call site of said function (this field + /// is empty for a controller) + pub call_loc: CallSiteSpan, +} + +impl InstructionKind { /// If this identifies a function call, return the information inside. pub fn as_function_call(self) -> Option { match self { - InstructionInfo::FunctionCall(d) => Some(d), + InstructionKind::FunctionCall(d) => Some(d), _ => None, } } From 25e4846db392463f008a1736ffc1503300d3c960 Mon Sep 17 00:00:00 2001 From: Justus Adam Date: Sat, 2 Mar 2024 18:50:26 +0000 Subject: [PATCH 03/20] Move node printing top diagnostics and debug tab handling in line printing --- crates/paralegal-policy/src/context.rs | 106 +------------ crates/paralegal-policy/src/diagnostics.rs | 165 ++++++++++++++++++++- 2 files changed, 165 insertions(+), 106 deletions(-) diff --git a/crates/paralegal-policy/src/context.rs b/crates/paralegal-policy/src/context.rs index 3539a76261..17c8a15e8f 100644 --- a/crates/paralegal-policy/src/context.rs +++ b/crates/paralegal-policy/src/context.rs @@ -631,100 +631,10 @@ impl Context { NodeCluster::new(src.controller_id(), start) } - fn get_location(&self, node: GlobalNode) -> Option<&CallSiteSpan> { + pub fn get_location(&self, node: GlobalNode) -> Option<&CallSiteSpan> { let at = self.node_info(node).at; Some(&self.desc().instruction_info.get(&at.leaf())?.call_loc) } - - /// Prints a diagnostic message for a given problematic node, given the type and coloring - /// of said diagnostic and the message to be printed - fn print_node_diagnostic( - &self, - diag_type: &str, - coloring: impl Fn(&str) -> ColoredString, - node: GlobalNode, - msg: &str, - ) -> Result<()> { - use std::io::BufRead; - let src_loc = &self - .get_location(node) - .ok_or(anyhow::Error::msg( - "node's location was not found in mapping", - ))? - .loc; - - let max_line_len = std::cmp::max( - src_loc.start_line.to_string().len(), - src_loc.end_line.to_string().len(), - ); - - println!("{}: {}", coloring(diag_type), msg); - let tab: String = " ".repeat(max_line_len); - println!( - "{}{} {}:{}:{}", - tab, - as_blue("-->"), - src_loc.file_path, - src_loc.start_line, - src_loc.start_col, - ); - println!("{} {}", tab, as_blue("|")); - let lines = std::io::BufReader::new(std::fs::File::open(&src_loc.abs_file_path)?) - .lines() - .skip(src_loc.start_line - 1) - .take(src_loc.end_line - src_loc.start_line + 1) - .enumerate(); - for (i, line) in lines { - let line_content: String = line?; - let line_num = src_loc.start_line + i; - let end: usize = if line_num == src_loc.end_line { - src_loc.end_col - } else { - line_content.len() + 1 - }; - let start: usize = if line_num == src_loc.start_line { - src_loc.start_col - } else { - line_content - .find(|c: char| !c.is_whitespace()) - .unwrap_or(end - 1) - + 1 - }; - let tab_len = max_line_len - line_num.to_string().len(); - - println!( - "{}{} {} {}", - " ".repeat(tab_len), - as_blue(&line_num.to_string()), - as_blue("|"), - line_content - ); - println!( - "{} {} {}{}", - tab, - as_blue("|"), - " ".repeat(start - 1), - coloring(&"^".repeat(end - start)) - ); - } - println!("{} {}", tab, as_blue("|")); - Ok(()) - } - - /// Prints an error message for a problematic node - pub fn print_node_error(&self, node: GlobalNode, msg: &str) -> () { - let _ = self.print_node_diagnostic("error", as_red, node, msg); - } - - /// Prints a warning message for a problematic node - pub fn print_node_warning(&self, node: GlobalNode, msg: &str) -> () { - let _ = self.print_node_diagnostic("warning", as_yellow, node, msg); - } - - /// Prints a note for a problematic node - pub fn print_node_note(&self, node: GlobalNode, msg: &str) -> () { - let _ = self.print_node_diagnostic("note", as_green, node, msg); - } } /// Provide display trait for DefId in a Context. @@ -941,20 +851,6 @@ fn test_happens_before() -> Result<()> { Ok(()) } -// For colored output for error printing -fn as_blue(input: &str) -> ColoredString { - input.blue() -} -fn as_green(input: &str) -> ColoredString { - input.green() -} -fn as_yellow(input: &str) -> ColoredString { - input.yellow() -} -fn as_red(input: &str) -> ColoredString { - input.red() -} - #[test] fn test_influencees() -> Result<()> { let ctx = crate::test_utils::test_ctx(); diff --git a/crates/paralegal-policy/src/diagnostics.rs b/crates/paralegal-policy/src/diagnostics.rs index 380df871cc..961e5d8125 100644 --- a/crates/paralegal-policy/src/diagnostics.rs +++ b/crates/paralegal-policy/src/diagnostics.rs @@ -78,10 +78,11 @@ #![allow(clippy::arc_with_non_send_sync)] +use colored::*; use std::rc::Rc; use std::{io::Write, sync::Arc}; -use paralegal_spdg::{Identifier, SPDG}; +use paralegal_spdg::{GlobalNode, Identifier, SPDG}; use crate::{Context, ControllerId}; @@ -132,6 +133,10 @@ pub enum Severity { Fail, /// This could indicate that the policy does not operate as intended. Warning, + /// Additional information for a diagnostic + Note, + /// Some helpful hint + Help, } impl Severity { @@ -213,6 +218,164 @@ pub trait Diagnostics: HasDiagnosticsBase { fn warning(&self, msg: impl Into) { self.record(msg.into(), Severity::Warning, vec![]) } + + /// Prints a diagnostic message for a given problematic node, given the type and coloring + /// of said diagnostic and the message to be printed + fn node_diagnostic( + &self, + node: GlobalNode, + msg: &str, + severity: Severity, + ) -> anyhow::Result<()> { + use std::fmt::Write; + let (diag_type, coloring) = match severity { + Severity::Fail => ("error", as_red as fn(&str) -> ColoredString), + Severity::Warning => ("warning", as_yellow as _), + Severity::Note => ("note", as_blue as _), + Severity::Help => ("help", as_green as _), + }; + + let mut s = String::new(); + macro_rules! println { + ($($t:tt)*) => { + writeln!(s, $($t)*)?; + }; + } + use std::io::BufRead; + let src_loc = &self + .as_ctx() + .get_location(node) + .ok_or(anyhow::Error::msg( + "node's location was not found in mapping", + ))? + .loc; + + let max_line_len = std::cmp::max( + src_loc.start_line.to_string().len(), + src_loc.end_line.to_string().len(), + ); + + println!("{}: {}", coloring(diag_type), msg); + let tab: String = " ".repeat(max_line_len); + println!( + "{}{} {}:{}:{}", + tab, + as_blue("-->"), + src_loc.file_path, + src_loc.start_line, + src_loc.start_col, + ); + println!("{} {}", tab, as_blue("|")); + let lines = std::io::BufReader::new(std::fs::File::open(&src_loc.abs_file_path)?) + .lines() + .skip(src_loc.start_line - 1) + .take(src_loc.end_line - src_loc.start_line + 1) + .enumerate(); + for (i, line) in lines { + let line_content: String = line?; + let line_num = src_loc.start_line + i; + let end: usize = if line_num == src_loc.end_line { + line_length_while(&line_content[0..src_loc.end_col - 1], |_| true) + } else { + line_length_while(&line_content, |_| true) + }; + let start: usize = if line_num == src_loc.start_line { + line_length_while(&line_content[0..src_loc.start_col - 1], |_| true) + } else { + line_length_while(&line_content, char::is_whitespace) + }; + let tab_len = max_line_len - line_num.to_string().len(); + + println!( + "{}{} {} {}", + " ".repeat(tab_len), + as_blue(&line_num.to_string()), + as_blue("|"), + line_content.replace('\t', &" ".repeat(TAB_SIZE)) + ); + if start > end { + eprintln!("start: {start}\nend: {end}\nin {line_content:?}\nstart_line: {}\nline_num: {line_num}\nend_line: {}\nstart col: {}\nend col: {}", src_loc.start_line, src_loc.end_line, src_loc.start_col, src_loc.end_col); + } + println!( + "{} {} {}{}", + tab, + as_blue("|"), + " ".repeat(start), + coloring(&"^".repeat(end - start)) + ); + } + println!("{} {}", tab, as_blue("|")); + self.record(s, severity, vec![]); + Ok(()) + } + + /// Prints an error message for a problematic node + fn print_node_error(&self, node: GlobalNode, msg: &str) -> anyhow::Result<()> { + self.node_diagnostic(node, msg, Severity::Fail) + } + + /// Prints a warning message for a problematic node + fn print_node_warning(&self, node: GlobalNode, msg: &str) -> anyhow::Result<()> { + self.node_diagnostic(node, msg, Severity::Warning) + } + + /// Prints a note for a problematic node + fn print_node_note(&self, node: GlobalNode, msg: &str) -> anyhow::Result<()> { + self.node_diagnostic(node, msg, Severity::Note) + } + + fn print_node_hint(&self, node: GlobalNode, msg: &str) -> anyhow::Result<()> { + self.node_diagnostic(node, msg, Severity::Help) + } +} + +const TAB_SIZE: usize = 4; + +fn line_length_while(s: &str, mut cont: impl FnMut(char) -> bool) -> usize { + s.chars() + .fold((false, 0), |(found, num), c| { + if found || !cont(c) { + (true, num) + } else { + let more = if c == '\t' { TAB_SIZE } else { 1 }; + (false, num + more) + } + }) + .1 +} + +#[cfg(test)] +mod tests { + use crate::diagnostics::{line_length_while, TAB_SIZE}; + + #[test] + fn test_line_length() { + assert_eq!(line_length_while(" ", |_| true), 2); + assert_eq!(line_length_while(" . ", |_| true), 4); + assert_eq!(line_length_while(" . ", char::is_whitespace), 2); + assert_eq!(line_length_while("\t", |_| true), TAB_SIZE); + assert_eq!(line_length_while("\t . ", |_| true), TAB_SIZE + 3); + assert_eq!(line_length_while(" . \t", |_| true), TAB_SIZE + 3); + assert_eq!(line_length_while("\t. ", char::is_whitespace), TAB_SIZE); + assert_eq!( + line_length_while("\t \t. ", char::is_whitespace), + 2 * TAB_SIZE + 1 + ); + } +} + +// For colored output for error printing +fn as_blue(input: &str) -> ColoredString { + input.blue() +} +fn as_green(input: &str) -> ColoredString { + input.green() +} +fn as_yellow(input: &str) -> ColoredString { + input.yellow() +} +fn as_red(input: &str) -> ColoredString { + input.red() } impl Diagnostics for T {} From dbaf281ab67143ec93fe418e8d8a83404adecf3e Mon Sep 17 00:00:00 2001 From: Justus Adam Date: Sun, 3 Mar 2024 02:31:35 +0000 Subject: [PATCH 04/20] Expose other diagnostic types --- crates/paralegal-policy/src/diagnostics.rs | 52 ++++++++++------------ 1 file changed, 24 insertions(+), 28 deletions(-) diff --git a/crates/paralegal-policy/src/diagnostics.rs b/crates/paralegal-policy/src/diagnostics.rs index 961e5d8125..2650341fd1 100644 --- a/crates/paralegal-policy/src/diagnostics.rs +++ b/crates/paralegal-policy/src/diagnostics.rs @@ -219,6 +219,16 @@ pub trait Diagnostics: HasDiagnosticsBase { self.record(msg.into(), Severity::Warning, vec![]) } + /// Emit a message that provides additional information to the user. + fn note(&self, msg: impl Into) { + self.record(msg.into(), Severity::Note, vec![]) + } + + /// Emit a message that suggests something to the user. + fn hint(&self, msg: impl Into) { + self.record(msg.into(), Severity::Help, vec![]) + } + /// Prints a diagnostic message for a given problematic node, given the type and coloring /// of said diagnostic and the message to be printed fn node_diagnostic( @@ -229,10 +239,10 @@ pub trait Diagnostics: HasDiagnosticsBase { ) -> anyhow::Result<()> { use std::fmt::Write; let (diag_type, coloring) = match severity { - Severity::Fail => ("error", as_red as fn(&str) -> ColoredString), - Severity::Warning => ("warning", as_yellow as _), - Severity::Note => ("note", as_blue as _), - Severity::Help => ("help", as_green as _), + Severity::Fail => ("error", (|s| s.red()) as fn(&str) -> ColoredString), + Severity::Warning => ("warning", (|s: &str| s.yellow()) as _), + Severity::Note => ("note", (|s: &str| s.blue()) as _), + Severity::Help => ("help", (|s: &str| s.green()) as _), }; let mut s = String::new(); @@ -242,6 +252,8 @@ pub trait Diagnostics: HasDiagnosticsBase { }; } use std::io::BufRead; + let node_kind = self.as_ctx().node_info(node).kind; + let src_loc = &self .as_ctx() .get_location(node) @@ -258,14 +270,14 @@ pub trait Diagnostics: HasDiagnosticsBase { println!("{}: {}", coloring(diag_type), msg); let tab: String = " ".repeat(max_line_len); println!( - "{}{} {}:{}:{}", + "{}{} {}:{}:{} ({node_kind})", tab, - as_blue("-->"), + "-->".blue(), src_loc.file_path, src_loc.start_line, src_loc.start_col, ); - println!("{} {}", tab, as_blue("|")); + println!("{} {}", tab, "|".blue()); let lines = std::io::BufReader::new(std::fs::File::open(&src_loc.abs_file_path)?) .lines() .skip(src_loc.start_line - 1) @@ -289,22 +301,19 @@ pub trait Diagnostics: HasDiagnosticsBase { println!( "{}{} {} {}", " ".repeat(tab_len), - as_blue(&line_num.to_string()), - as_blue("|"), + &line_num.to_string().blue(), + "|".blue(), line_content.replace('\t', &" ".repeat(TAB_SIZE)) ); - if start > end { - eprintln!("start: {start}\nend: {end}\nin {line_content:?}\nstart_line: {}\nline_num: {line_num}\nend_line: {}\nstart col: {}\nend col: {}", src_loc.start_line, src_loc.end_line, src_loc.start_col, src_loc.end_col); - } println!( "{} {} {}{}", tab, - as_blue("|"), + "|".blue(), " ".repeat(start), coloring(&"^".repeat(end - start)) ); } - println!("{} {}", tab, as_blue("|")); + println!("{} {}", tab, "|".blue()); self.record(s, severity, vec![]); Ok(()) } @@ -324,6 +333,7 @@ pub trait Diagnostics: HasDiagnosticsBase { self.node_diagnostic(node, msg, Severity::Note) } + /// Print a hint with a node fn print_node_hint(&self, node: GlobalNode, msg: &str) -> anyhow::Result<()> { self.node_diagnostic(node, msg, Severity::Help) } @@ -364,20 +374,6 @@ mod tests { } } -// For colored output for error printing -fn as_blue(input: &str) -> ColoredString { - input.blue() -} -fn as_green(input: &str) -> ColoredString { - input.green() -} -fn as_yellow(input: &str) -> ColoredString { - input.yellow() -} -fn as_red(input: &str) -> ColoredString { - input.red() -} - impl Diagnostics for T {} /// A context for a named policy. From 494a5b70183ea4679735edbd9eba52b99ccdde0f Mon Sep 17 00:00:00 2001 From: Justus Adam Date: Sat, 2 Mar 2024 18:50:45 +0000 Subject: [PATCH 05/20] Make happens before print its nodes --- crates/paralegal-policy/src/context.rs | 76 +++++++++++++++----------- 1 file changed, 45 insertions(+), 31 deletions(-) diff --git a/crates/paralegal-policy/src/context.rs b/crates/paralegal-policy/src/context.rs index 17c8a15e8f..2f2b3fde46 100644 --- a/crates/paralegal-policy/src/context.rs +++ b/crates/paralegal-policy/src/context.rs @@ -1,4 +1,4 @@ -use std::{io::Write, process::exit, sync::Arc}; +use std::{collections::HashSet, io::Write, process::exit, sync::Arc}; pub use paralegal_spdg::rustc_portable::{DefId, LocalDefId}; use paralegal_spdg::traverse::{generic_flows_to, EdgeSelection}; @@ -9,7 +9,6 @@ use paralegal_spdg::{ }; use anyhow::{anyhow, bail, ensure, Result}; -use colored::*; use itertools::{Either, Itertools}; use petgraph::prelude::Bfs; use petgraph::visit::{Control, DfsEvent, EdgeFiltered, EdgeRef, Walker}; @@ -19,7 +18,7 @@ use super::flows_to::CtrlFlowsTo; use crate::Diagnostics; use crate::{ - assert_error, assert_warning, + assert_warning, diagnostics::{CombinatorContext, DiagnosticsRecorder, HasDiagnosticsBase}, }; @@ -484,26 +483,25 @@ impl Context { mut is_checkpoint: impl FnMut(GlobalNode) -> bool, mut is_terminal: impl FnMut(GlobalNode) -> bool, ) -> Result { - let mut num_reached = 0; - let mut num_checkpointed = 0; + let mut reached = HashSet::new(); + let mut checkpointed = HashSet::new(); let start_map = starting_points .into_iter() .map(|i| (i.controller_id(), i.local_node())) .into_group_map(); - let started_with = start_map.values().map(Vec::len).sum(); - for (ctrl_id, starts) in start_map { + for (ctrl_id, starts) in &start_map { let spdg = &self.desc.controllers[&ctrl_id]; let g = &spdg.graph; - petgraph::visit::depth_first_search(g, starts, |event| match event { + petgraph::visit::depth_first_search(g, starts.iter().copied(), |event| match event { DfsEvent::Discover(inner, _) => { - let as_node = GlobalNode::from_local_node(ctrl_id, inner); + let as_node = GlobalNode::from_local_node(*ctrl_id, inner); if is_checkpoint(as_node) { - num_checkpointed += 1; + checkpointed.insert(as_node); Control::<()>::Prune } else if is_terminal(as_node) { - num_reached += 1; + reached.insert(as_node); Control::Prune } else { Control::Continue @@ -512,10 +510,18 @@ impl Context { _ => Control::Continue, }); } + let started_with = start_map + .into_iter() + .flat_map(|(ctrl_id, nodes)| { + nodes + .into_iter() + .map(move |node| GlobalNode::from_local_node(ctrl_id, node)) + }) + .collect(); Ok(AlwaysHappensBefore { - num_reached, - num_checkpointed, + reached: reached.into_iter().collect(), + checkpointed: checkpointed.into_iter().collect(), started_with, }) } @@ -678,27 +684,25 @@ impl<'a> std::fmt::Display for DisplayDef<'a> { /// for-human-eyes-only. pub struct AlwaysHappensBefore { /// How many paths terminated at the end? - num_reached: i32, + reached: Vec, /// How many paths lead to the checkpoints? - num_checkpointed: i32, + checkpointed: Vec, /// How large was the set of initial nodes this traversal started with. - started_with: usize, + started_with: Vec, } impl std::fmt::Display for AlwaysHappensBefore { /// Format the results of this combinator, using the `def_info` to print /// readable names instead of ids fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - let Self { - num_reached, - num_checkpointed, - started_with, - } = self; write!( f, - "{num_reached} paths reached the terminal, \ - {num_checkpointed} paths reached the checkpoints, \ - started with {started_with} nodes" + "{} paths reached the terminal, \ + {} paths reached the checkpoints, \ + started with {} nodes", + self.reached.len(), + self.checkpointed.len(), + self.started_with.len(), ) } } @@ -714,14 +718,26 @@ impl AlwaysHappensBefore { /// nodes. pub fn report(&self, ctx: Arc) { let ctx = CombinatorContext::new(*ALWAYS_HAPPENS_BEFORE_NAME, ctx); - assert_warning!(ctx, self.started_with != 0, "Started with 0 nodes."); + assert_warning!(ctx, self.started_with.len() != 0, "Started with 0 nodes."); assert_warning!(ctx, !self.is_vacuous(), "Is vacuously true."); - assert_error!(ctx, self.holds(), format!("Violation detected: {}", self)); + if !self.holds() { + for &reached in &self.reached { + ctx.print_node_error(reached, "Reached this terminal") + .unwrap(); + } + for &start in &self.started_with { + ctx.print_node_note(start, "Started from here").unwrap(); + } + for &check in &self.checkpointed { + ctx.print_node_hint(check, "This checkpoint was reached") + .unwrap(); + } + } } /// Returns `true` if the property that created these statistics holds. pub fn holds(&self) -> bool { - self.num_reached == 0 + self.reached.is_empty() } /// Fails if [`Self::holds`] is false. @@ -729,7 +745,7 @@ impl AlwaysHappensBefore { ensure!( self.holds(), "AlwaysHappensBefore failed: found {} violating paths", - self.num_reached + self.reached.len() ); Ok(()) } @@ -738,7 +754,7 @@ impl AlwaysHappensBefore { /// or no path from them can reach the terminal or the checkpoints (the /// graphs are disjoined). pub fn is_vacuous(&self) -> bool { - self.num_checkpointed + self.num_reached == 0 + self.checkpointed.is_empty() && self.reached.is_empty() } } @@ -747,8 +763,6 @@ fn overlaps( one: impl IntoIterator, other: impl IntoIterator, ) -> bool { - use paralegal_spdg::HashSet; - let target = one.into_iter().collect::>(); other.into_iter().any(|n| target.contains(&n)) } From 81e7740e876e8d054e51e86a5663b5c2ca04eb0d Mon Sep 17 00:00:00 2001 From: Justus Adam Date: Sat, 2 Mar 2024 19:19:22 +0000 Subject: [PATCH 06/20] Track always_happens_before origins --- crates/paralegal-policy/src/context.rs | 41 +++++++++++++------------- 1 file changed, 21 insertions(+), 20 deletions(-) diff --git a/crates/paralegal-policy/src/context.rs b/crates/paralegal-policy/src/context.rs index 2f2b3fde46..a4d9e658d8 100644 --- a/crates/paralegal-policy/src/context.rs +++ b/crates/paralegal-policy/src/context.rs @@ -11,7 +11,7 @@ use paralegal_spdg::{ use anyhow::{anyhow, bail, ensure, Result}; use itertools::{Either, Itertools}; use petgraph::prelude::Bfs; -use petgraph::visit::{Control, DfsEvent, EdgeFiltered, EdgeRef, Walker}; +use petgraph::visit::{Control, DfsEvent, EdgeFiltered, EdgeRef, GraphBase, NodeIndexable, Walker}; use petgraph::Incoming; use super::flows_to::CtrlFlowsTo; @@ -483,7 +483,7 @@ impl Context { mut is_checkpoint: impl FnMut(GlobalNode) -> bool, mut is_terminal: impl FnMut(GlobalNode) -> bool, ) -> Result { - let mut reached = HashSet::new(); + let mut reached = HashMap::new(); let mut checkpointed = HashSet::new(); let start_map = starting_points @@ -494,14 +494,25 @@ impl Context { for (ctrl_id, starts) in &start_map { let spdg = &self.desc.controllers[&ctrl_id]; let g = &spdg.graph; + let mut origin_map = vec![::NodeId::end(); g.node_bound()]; + for s in starts { + origin_map[s.index()] = *s; + } petgraph::visit::depth_first_search(g, starts.iter().copied(), |event| match event { + DfsEvent::TreeEdge(from, to) => { + origin_map[to.index()] = origin_map[from.index()]; + Control::<()>::Continue + } DfsEvent::Discover(inner, _) => { let as_node = GlobalNode::from_local_node(*ctrl_id, inner); if is_checkpoint(as_node) { checkpointed.insert(as_node); Control::<()>::Prune } else if is_terminal(as_node) { - reached.insert(as_node); + reached.insert( + as_node, + GlobalNode::from_local_node(*ctrl_id, origin_map[inner.index()]), + ); Control::Prune } else { Control::Continue @@ -510,19 +521,11 @@ impl Context { _ => Control::Continue, }); } - let started_with = start_map - .into_iter() - .flat_map(|(ctrl_id, nodes)| { - nodes - .into_iter() - .map(move |node| GlobalNode::from_local_node(ctrl_id, node)) - }) - .collect(); Ok(AlwaysHappensBefore { reached: reached.into_iter().collect(), checkpointed: checkpointed.into_iter().collect(), - started_with, + started_with: start_map.values().map(Vec::len).sum(), }) } @@ -684,11 +687,11 @@ impl<'a> std::fmt::Display for DisplayDef<'a> { /// for-human-eyes-only. pub struct AlwaysHappensBefore { /// How many paths terminated at the end? - reached: Vec, + reached: Vec<(GlobalNode, GlobalNode)>, /// How many paths lead to the checkpoints? checkpointed: Vec, /// How large was the set of initial nodes this traversal started with. - started_with: Vec, + started_with: usize, } impl std::fmt::Display for AlwaysHappensBefore { @@ -702,7 +705,7 @@ impl std::fmt::Display for AlwaysHappensBefore { started with {} nodes", self.reached.len(), self.checkpointed.len(), - self.started_with.len(), + self.started_with, ) } } @@ -718,15 +721,13 @@ impl AlwaysHappensBefore { /// nodes. pub fn report(&self, ctx: Arc) { let ctx = CombinatorContext::new(*ALWAYS_HAPPENS_BEFORE_NAME, ctx); - assert_warning!(ctx, self.started_with.len() != 0, "Started with 0 nodes."); + assert_warning!(ctx, self.started_with != 0, "Started with 0 nodes."); assert_warning!(ctx, !self.is_vacuous(), "Is vacuously true."); if !self.holds() { - for &reached in &self.reached { + for &(reached, from) in &self.reached { ctx.print_node_error(reached, "Reached this terminal") .unwrap(); - } - for &start in &self.started_with { - ctx.print_node_note(start, "Started from here").unwrap(); + ctx.print_node_note(from, "Started from this node").unwrap(); } for &check in &self.checkpointed { ctx.print_node_hint(check, "This checkpoint was reached") From a40804dabdd74dfa4e6f4091adce2667a5f7ba92 Mon Sep 17 00:00:00 2001 From: Justus Adam Date: Sun, 3 Mar 2024 02:34:22 +0000 Subject: [PATCH 07/20] Conveneient location printing --- crates/paralegal-policy/src/lib.rs | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/crates/paralegal-policy/src/lib.rs b/crates/paralegal-policy/src/lib.rs index 63bebef12c..51746f41f4 100644 --- a/crates/paralegal-policy/src/lib.rs +++ b/crates/paralegal-policy/src/lib.rs @@ -191,3 +191,19 @@ impl GraphLocation { Ok(Context::new(desc)) } } + +/// A convenience macro that uses `file!`, `line!` and `column!` to return the +/// string `"file:line:column"`. This can be used to mention policy source +/// locations in policies. +/// +/// If additional arguments are procided these are `concat!`ed to the end with a +/// space in betwee the location and the rest. +#[macro_export] +macro_rules! loc { + () => { + concat!(file!(), ':', line!(), ':', column!(),) + }; + ($($t:tt)+) => { + concat!(file!(), ':', line!(), ':', column!(), ' ', $($t)+) + }; +} From 61a9a63e3b381e3d7ed87f7a11278874663ce9fd Mon Sep 17 00:00:00 2001 From: Justus Adam Date: Sun, 3 Mar 2024 12:08:23 -0500 Subject: [PATCH 08/20] Include argument spans --- crates/paralegal-flow/src/ana/mod.rs | 59 ++++++++++------------ crates/paralegal-flow/src/test_utils.rs | 1 - crates/paralegal-policy/src/context.rs | 13 +++-- crates/paralegal-policy/src/diagnostics.rs | 8 +-- crates/paralegal-policy/src/test_utils.rs | 2 +- crates/paralegal-spdg/src/dot.rs | 2 +- crates/paralegal-spdg/src/lib.rs | 14 ++--- 7 files changed, 39 insertions(+), 60 deletions(-) diff --git a/crates/paralegal-flow/src/ana/mod.rs b/crates/paralegal-flow/src/ana/mod.rs index d8a83e7a19..2409d5aca1 100644 --- a/crates/paralegal-flow/src/ana/mod.rs +++ b/crates/paralegal-flow/src/ana/mod.rs @@ -136,7 +136,7 @@ impl<'tcx> SPDGGenerator<'tcx> { fn collect_instruction_info( &self, controllers: &HashMap, - ) -> HashMap { + ) -> HashMap { let all_instructions = controllers .values() .flat_map(|v| { @@ -149,50 +149,27 @@ impl<'tcx> SPDGGenerator<'tcx> { all_instructions .into_iter() .map(|i| { - let tcx = self.tcx; let body = self.tcx.body_for_def_id(i.function).unwrap(); - let with_default_spans = |kind| { - let default_span = src_loc_for_span(tcx.def_span(i.function.to_def_id()), tcx); - InstructionInfo { - kind, - call_loc: CallSiteSpan { - loc: default_span.clone(), - expanded_loc: default_span, - }, - } - }; let info = match i.location { - RichLocation::End => with_default_spans(InstructionKind::Return), - RichLocation::Start => with_default_spans(InstructionKind::Start), + RichLocation::End => InstructionKind::Return, + RichLocation::Start => InstructionKind::Start, RichLocation::Location(loc) => { - let (kind, expanded_span) = match body.body.stmt_at(loc) { + let kind = match body.body.stmt_at(loc) { crate::Either::Right(term) => { - let kind = if let Ok((id, ..)) = term.as_fn_and_args(self.tcx) { + if let Ok((id, ..)) = term.as_fn_and_args(self.tcx) { InstructionKind::FunctionCall(FunctionCallInfo { id, is_inlined: id.is_local(), }) } else { InstructionKind::Terminator - }; - (kind, term.source_info.span) - } - crate::Either::Left(stmt) => { - (InstructionKind::Statement, stmt.source_info.span) + } } + crate::Either::Left(_) => InstructionKind::Statement, }; - let stmt_span = tcx - .sess - .source_map() - .stmt_span(expanded_span, body.body.span); - InstructionInfo { - kind, - call_loc: CallSiteSpan { - loc: src_loc_for_span(stmt_span, tcx), - expanded_loc: src_loc_for_span(expanded_span, tcx), - }, - } + + kind } }; (i, info) @@ -600,16 +577,34 @@ impl<'a, 'tcx, C: Extend> GraphConverter<'tcx, 'a, C> { use petgraph::prelude::*; let g_ref = self.dep_graph.clone(); let input = &g_ref.graph; + let tcx = self.tcx(); let mut markers: HashMap> = HashMap::new(); for (i, weight) in input.node_references() { let (kind, is_external_call_source, node_markers) = self.determine_node_kind(weight); + let at = weight.at.leaf(); + let body = &tcx.body_for_def_id(at.function).unwrap().body; + + let rustc_span = match at.location { + RichLocation::End | RichLocation::Start => { + let def = &body.local_decls[weight.place.local]; + def.source_info.span + } + RichLocation::Location(loc) => { + let expanded_span = match body.stmt_at(loc) { + crate::Either::Right(term) => term.source_info.span, + crate::Either::Left(stmt) => stmt.source_info.span, + }; + tcx.sess.source_map().stmt_span(expanded_span, body.span) + } + }; let new_idx = self.register_node( i, NodeInfo { at: weight.at, description: format!("{:?}", weight.place), kind, + span: src_loc_for_span(rustc_span, tcx), }, ); diff --git a/crates/paralegal-flow/src/test_utils.rs b/crates/paralegal-flow/src/test_utils.rs index 9c87583eb0..96c558889a 100644 --- a/crates/paralegal-flow/src/test_utils.rs +++ b/crates/paralegal-flow/src/test_utils.rs @@ -350,7 +350,6 @@ impl<'g> CtrlRef<'g> { .chain(self.ctrl.graph.node_weights().map(|info| info.at)) .filter(|m| { instruction_info[&m.leaf()] - .kind .as_function_call() .map_or(false, |i| i.id == fun.ident) }) diff --git a/crates/paralegal-policy/src/context.rs b/crates/paralegal-policy/src/context.rs index a4d9e658d8..d8d36c6256 100644 --- a/crates/paralegal-policy/src/context.rs +++ b/crates/paralegal-policy/src/context.rs @@ -3,9 +3,9 @@ use std::{collections::HashSet, io::Write, process::exit, sync::Arc}; pub use paralegal_spdg::rustc_portable::{DefId, LocalDefId}; use paralegal_spdg::traverse::{generic_flows_to, EdgeSelection}; use paralegal_spdg::{ - CallSiteSpan, CallString, DisplayNode, Endpoint, GlobalNode, HashMap, Identifier, - InstructionInfo, IntoIterGlobalNodes, Node as SPDGNode, NodeCluster, NodeInfo, - ProgramDescription, SPDGImpl, TypeId, SPDG, + CallString, DisplayNode, Endpoint, GlobalNode, HashMap, Identifier, InstructionKind, + IntoIterGlobalNodes, Node as SPDGNode, NodeCluster, NodeInfo, ProgramDescription, SPDGImpl, + SrcCodeSpan, TypeId, SPDG, }; use anyhow::{anyhow, bail, ensure, Result}; @@ -604,7 +604,7 @@ impl Context { } /// Retrieve metadata about the instruction executed by a specific node. - pub fn instruction_at_node(&self, node: GlobalNode) -> &InstructionInfo { + pub fn instruction_at_node(&self, node: GlobalNode) -> &InstructionKind { let node_info = self.node_info(node); &self.desc.instruction_info[&node_info.at.leaf()] } @@ -640,9 +640,8 @@ impl Context { NodeCluster::new(src.controller_id(), start) } - pub fn get_location(&self, node: GlobalNode) -> Option<&CallSiteSpan> { - let at = self.node_info(node).at; - Some(&self.desc().instruction_info.get(&at.leaf())?.call_loc) + pub fn get_location(&self, node: GlobalNode) -> &SrcCodeSpan { + &self.node_info(node).span } } diff --git a/crates/paralegal-policy/src/diagnostics.rs b/crates/paralegal-policy/src/diagnostics.rs index 2650341fd1..e8f7968fd8 100644 --- a/crates/paralegal-policy/src/diagnostics.rs +++ b/crates/paralegal-policy/src/diagnostics.rs @@ -254,13 +254,7 @@ pub trait Diagnostics: HasDiagnosticsBase { use std::io::BufRead; let node_kind = self.as_ctx().node_info(node).kind; - let src_loc = &self - .as_ctx() - .get_location(node) - .ok_or(anyhow::Error::msg( - "node's location was not found in mapping", - ))? - .loc; + let src_loc = &self.as_ctx().get_location(node); let max_line_len = std::cmp::max( src_loc.start_line.to_string().len(), diff --git a/crates/paralegal-policy/src/test_utils.rs b/crates/paralegal-policy/src/test_utils.rs index 9c0c76441f..62a67fb3f9 100644 --- a/crates/paralegal-policy/src/test_utils.rs +++ b/crates/paralegal-policy/src/test_utils.rs @@ -51,7 +51,7 @@ fn is_at_function_call_with_name( let weight = ctrl.graph.node_weight(node).unwrap().at; let instruction = &ctx.desc().instruction_info[&weight.leaf()]; matches!( - instruction.kind, + instruction, InstructionKind::FunctionCall(call) if ctx.desc().def_info[&call.id].name == name ) diff --git a/crates/paralegal-spdg/src/dot.rs b/crates/paralegal-spdg/src/dot.rs index e6c228f682..c3225b5057 100644 --- a/crates/paralegal-spdg/src/dot.rs +++ b/crates/paralegal-spdg/src/dot.rs @@ -116,7 +116,7 @@ impl<'a, 'd> dot::Labeller<'a, CallString, GlobalEdge> for DotPrintableProgramDe write!(s, "{}|", self.format_call_string(*n))?; - match instruction.kind { + match instruction { InstructionKind::Statement => s.push('S'), InstructionKind::FunctionCall(function) => { let info = &self.spdg.def_info[&function.id]; diff --git a/crates/paralegal-spdg/src/lib.rs b/crates/paralegal-spdg/src/lib.rs index aaab50a045..bcd26fb70c 100644 --- a/crates/paralegal-spdg/src/lib.rs +++ b/crates/paralegal-spdg/src/lib.rs @@ -209,16 +209,6 @@ pub enum InstructionKind { Return, } -/// Information about instructions -#[derive(Serialize, Deserialize, Debug)] -pub struct InstructionInfo { - /// The kind of instruction - pub kind: InstructionKind, - /// call chain from within the controller to the call site of said function (this field - /// is empty for a controller) - pub call_loc: CallSiteSpan, -} - impl InstructionKind { /// If this identifies a function call, return the information inside. pub fn as_function_call(self) -> Option { @@ -251,7 +241,7 @@ pub struct ProgramDescription { /// Metadata about the instructions that are executed at all program /// locations we know about. #[serde(with = "serde_map_via_vec")] - pub instruction_info: HashMap, + pub instruction_info: HashMap, #[cfg_attr(not(feature = "rustc"), serde(with = "serde_map_via_vec"))] #[cfg_attr(feature = "rustc", serde(with = "ser_defid_map"))] @@ -593,6 +583,8 @@ pub struct NodeInfo { pub description: String, /// Additional information of how this node is used in the source. pub kind: NodeKind, + /// Span information for this node + pub span: SrcCodeSpan, } impl Display for NodeInfo { From 48455fbfe7a7103f8525b4f1b7b6264d1f5f5bbd Mon Sep 17 00:00:00 2001 From: Justus Adam Date: Sun, 3 Mar 2024 12:19:48 -0500 Subject: [PATCH 09/20] Intern the source files --- crates/paralegal-flow/src/ana/mod.rs | 11 ++--- crates/paralegal-policy/src/diagnostics.rs | 13 +++--- crates/paralegal-spdg/src/lib.rs | 53 ++++++++++++---------- 3 files changed, 41 insertions(+), 36 deletions(-) diff --git a/crates/paralegal-flow/src/ana/mod.rs b/crates/paralegal-flow/src/ana/mod.rs index 2409d5aca1..8e2198238d 100644 --- a/crates/paralegal-flow/src/ana/mod.rs +++ b/crates/paralegal-flow/src/ana/mod.rs @@ -224,9 +224,12 @@ fn src_loc_for_span(span: Span, tcx: TyCtxt) -> SrcCodeSpan { } else { std::path::PathBuf::from(&file_path) }; - SrcCodeSpan { + let src_info = SourceFileInfo { file_path, abs_file_path, + }; + SrcCodeSpan { + source_file: src_info.intern(), start_line, start_col, end_line, @@ -779,15 +782,11 @@ fn def_info_for_item(id: DefId, tcx: TyCtxt) -> DefInfo { } })) .collect(); - let src_info = SrcCodeInfo { - func_iden: tcx.def_path_str(id), - func_header_loc: src_loc_for_span(tcx.def_span(id), tcx), - }; DefInfo { name, path, kind, - src_info, + src_info: src_loc_for_span(tcx.def_span(id), tcx), } } diff --git a/crates/paralegal-policy/src/diagnostics.rs b/crates/paralegal-policy/src/diagnostics.rs index e8f7968fd8..3ca7633f4c 100644 --- a/crates/paralegal-policy/src/diagnostics.rs +++ b/crates/paralegal-policy/src/diagnostics.rs @@ -267,16 +267,17 @@ pub trait Diagnostics: HasDiagnosticsBase { "{}{} {}:{}:{} ({node_kind})", tab, "-->".blue(), - src_loc.file_path, + src_loc.source_file.file_path, src_loc.start_line, src_loc.start_col, ); println!("{} {}", tab, "|".blue()); - let lines = std::io::BufReader::new(std::fs::File::open(&src_loc.abs_file_path)?) - .lines() - .skip(src_loc.start_line - 1) - .take(src_loc.end_line - src_loc.start_line + 1) - .enumerate(); + let lines = + std::io::BufReader::new(std::fs::File::open(&src_loc.source_file.abs_file_path)?) + .lines() + .skip(src_loc.start_line - 1) + .take(src_loc.end_line - src_loc.start_line + 1) + .enumerate(); for (i, line) in lines { let line_content: String = line?; let line_num = src_loc.start_line + i; diff --git a/crates/paralegal-spdg/src/lib.rs b/crates/paralegal-spdg/src/lib.rs index bcd26fb70c..0436eb54fa 100644 --- a/crates/paralegal-spdg/src/lib.rs +++ b/crates/paralegal-spdg/src/lib.rs @@ -126,7 +126,7 @@ pub struct DefInfo { /// Kind of object pub kind: DefKind, /// Information about the span - pub src_info: SrcCodeInfo, + pub src_info: SrcCodeSpan, } /// Similar to `DefKind` in rustc but *not the same*! @@ -144,14 +144,39 @@ pub enum DefKind { Type, } -/// Encodes a source code location -#[derive(Clone, PartialEq, Eq, Serialize, Deserialize, Debug)] -pub struct SrcCodeSpan { +/// An interned [`SourceFileInfo`] +#[derive(Clone, Copy, PartialEq, Eq, Serialize, Deserialize, Debug, Hash)] +pub struct SourceFile(Intern); + +impl std::ops::Deref for SourceFile { + type Target = SourceFileInfo; + fn deref(&self) -> &Self::Target { + &self.0 + } +} + +/// Information about a source file +#[derive(Clone, PartialEq, Eq, Serialize, Deserialize, Debug, Hash)] +pub struct SourceFileInfo { /// Printable location of the source code file - either an absolute path to library source code /// or a path relative to within the compiled crate (e.g. `src/...`) pub file_path: String, /// Absolute path to source code file pub abs_file_path: PathBuf, +} + +impl SourceFileInfo { + /// Intern the source file + pub fn intern(self) -> SourceFile { + SourceFile(Intern::new(self)) + } +} + +/// Encodes a source code location +#[derive(Clone, PartialEq, Eq, Serialize, Deserialize, Debug)] +pub struct SrcCodeSpan { + /// Which file this comes from + pub source_file: SourceFile, /// The starting line of the location within the file (note: a one-based index) pub start_line: usize, /// The column of starting line that the location starts at within the file (note: a one-based index) @@ -162,26 +187,6 @@ pub struct SrcCodeSpan { pub end_col: usize, } -/// Encodes a location of a call site -#[derive(Clone, PartialEq, Eq, Serialize, Deserialize, Debug)] -pub struct CallSiteSpan { - /// The source code location of the call site - if the call site occurs within a macro, this - /// refers to the macro's call site - pub loc: SrcCodeSpan, - /// The expanded location of the call site - if the call site occurs within a macro, this - /// refers to its location within the macro's definition - pub expanded_loc: SrcCodeSpan, -} - -/// Encodes source code information for controllers and call site nodes in the SPDG -#[derive(Clone, PartialEq, Eq, Serialize, Deserialize, Debug)] -pub struct SrcCodeInfo { - /// Identifier of the function - pub func_iden: String, - /// Location of the header of the function's definition - pub func_header_loc: SrcCodeSpan, -} - /// Metadata on a function call. #[derive(Debug, Clone, Copy, Serialize, Deserialize, Eq, Ord, PartialOrd, PartialEq)] pub struct FunctionCallInfo { From 4fee6ee356f5a91c5c4b77755b5c31a96e3b3e72 Mon Sep 17 00:00:00 2001 From: Justus Adam Date: Sun, 3 Mar 2024 12:21:42 -0500 Subject: [PATCH 10/20] A missing documentation --- crates/paralegal-policy/src/context.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/paralegal-policy/src/context.rs b/crates/paralegal-policy/src/context.rs index d8d36c6256..245d66d142 100644 --- a/crates/paralegal-policy/src/context.rs +++ b/crates/paralegal-policy/src/context.rs @@ -640,6 +640,7 @@ impl Context { NodeCluster::new(src.controller_id(), start) } + /// Get the span of a node pub fn get_location(&self, node: GlobalNode) -> &SrcCodeSpan { &self.node_info(node).span } From e14d8af6d614c97b41183c6fbb43787a93baadb9 Mon Sep 17 00:00:00 2001 From: Justus Adam Date: Sun, 3 Mar 2024 12:24:21 -0500 Subject: [PATCH 11/20] Don't print the checkpoints for no reason --- crates/paralegal-policy/src/context.rs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/crates/paralegal-policy/src/context.rs b/crates/paralegal-policy/src/context.rs index 245d66d142..10bb5bcc03 100644 --- a/crates/paralegal-policy/src/context.rs +++ b/crates/paralegal-policy/src/context.rs @@ -729,10 +729,6 @@ impl AlwaysHappensBefore { .unwrap(); ctx.print_node_note(from, "Started from this node").unwrap(); } - for &check in &self.checkpointed { - ctx.print_node_hint(check, "This checkpoint was reached") - .unwrap(); - } } } From fb31dd9f8d0f26f21050271b651f5a2d502b887e Mon Sep 17 00:00:00 2001 From: Justus Adam Date: Sun, 3 Mar 2024 14:30:54 -0500 Subject: [PATCH 12/20] Fix documentation --- crates/paralegal-policy/src/context.rs | 6 ++++-- crates/paralegal-policy/src/diagnostics.rs | 11 +++++++---- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/crates/paralegal-policy/src/context.rs b/crates/paralegal-policy/src/context.rs index 10bb5bcc03..af3b57bfdc 100644 --- a/crates/paralegal-policy/src/context.rs +++ b/crates/paralegal-policy/src/context.rs @@ -84,9 +84,11 @@ fn bfs_iter< /// /// To communicate the results of your policies with the user you can emit /// diagnostic messages. To communicate a policy failure use -/// [`error`](crate::Diagnostics::error) or the [`assert_error`] macro. To +/// [`error`](crate::Diagnostics::error) or the [`crate::assert_error`] macro. To /// communicate suspicious circumstances that are not outright cause for failure -/// use [`warning`](crate::Diagnostics::error) or [`assert_warning`]. +/// use [`warning`](crate::Diagnostics::warning) or [`assert_warning`]. For all +/// types of errors, including those with span information for a particular +/// node, see the [`crate::Diagnostics`] trait. /// /// Note that these methods just queue the diagnostics messages. To emit them /// (and potentially terminate the program if the policy does not hold) use diff --git a/crates/paralegal-policy/src/diagnostics.rs b/crates/paralegal-policy/src/diagnostics.rs index 3ca7633f4c..ad494c2187 100644 --- a/crates/paralegal-policy/src/diagnostics.rs +++ b/crates/paralegal-policy/src/diagnostics.rs @@ -229,12 +229,15 @@ pub trait Diagnostics: HasDiagnosticsBase { self.record(msg.into(), Severity::Help, vec![]) } - /// Prints a diagnostic message for a given problematic node, given the type and coloring - /// of said diagnostic and the message to be printed + /// Prints a diagnostic message and the source code that corresponds to the + /// given node. + /// + /// The severity governs the severity of the emitted message (the same as + /// e.g. [`Self::error`]) and the coloring of the span information. fn node_diagnostic( &self, node: GlobalNode, - msg: &str, + msg: impl Into, severity: Severity, ) -> anyhow::Result<()> { use std::fmt::Write; @@ -261,7 +264,7 @@ pub trait Diagnostics: HasDiagnosticsBase { src_loc.end_line.to_string().len(), ); - println!("{}: {}", coloring(diag_type), msg); + println!("{}: {}", coloring(diag_type), msg.into()); let tab: String = " ".repeat(max_line_len); println!( "{}{} {}:{}:{} ({node_kind})", From 26bb8e79dc138ffae86f7b372b488cbf7b235c5a Mon Sep 17 00:00:00 2001 From: Justus Adam Date: Sun, 3 Mar 2024 14:49:40 -0500 Subject: [PATCH 13/20] Simplifying --- Cargo.lock | 1 + Cargo.toml | 8 ++- crates/paralegal-flow/Cargo.toml | 2 +- crates/paralegal-policy/Cargo.toml | 1 + crates/paralegal-policy/src/diagnostics.rs | 69 +++++++++++----------- crates/paralegal-spdg/Cargo.toml | 2 +- props/Cargo.lock | 15 ++++- 7 files changed, 59 insertions(+), 39 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index baea76fe4f..984344d7ff 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -843,6 +843,7 @@ dependencies = [ "petgraph", "serde_json", "simple_logger", + "strum", ] [[package]] diff --git a/Cargo.toml b/Cargo.toml index 5ebb9522ff..f852567703 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,12 +1,18 @@ [workspace] members = ["crates/*"] -exclude = ["props", "crates/paralegal-flow/tests", "crates/paralegal-policy/tests", "crates/paralegal-explore"] +exclude = [ + "props", + "crates/paralegal-flow/tests", + "crates/paralegal-policy/tests", + "crates/paralegal-explore", +] resolver = "2" [workspace.dependencies] indexical = "0.3.1" serde = "1.0.188" petgraph = { version = "0.6", features = ["serde-1"] } +strum = { version = "0.25", features = ["derive"] } [profile.release] debug = true diff --git a/crates/paralegal-flow/Cargo.toml b/crates/paralegal-flow/Cargo.toml index 4249ba6d85..e5975be499 100644 --- a/crates/paralegal-flow/Cargo.toml +++ b/crates/paralegal-flow/Cargo.toml @@ -40,7 +40,7 @@ num-derive = "0.4" num-traits = "0.2" petgraph = { workspace = true } humantime = "2" -strum = { version = "0.25", features = ["derive"] } +strum = { workspace = true } #dot = "0.1" diff --git a/crates/paralegal-policy/Cargo.toml b/crates/paralegal-policy/Cargo.toml index f9e69c230e..f150c6b738 100644 --- a/crates/paralegal-policy/Cargo.toml +++ b/crates/paralegal-policy/Cargo.toml @@ -17,6 +17,7 @@ lazy_static = "1" bitvec = "1" petgraph = { workspace = true } colored = "1" +strum = { workspace = true } [dev-dependencies] paralegal-flow = { path = "../paralegal-flow", features = ["test"] } diff --git a/crates/paralegal-policy/src/diagnostics.rs b/crates/paralegal-policy/src/diagnostics.rs index ad494c2187..404fbf131b 100644 --- a/crates/paralegal-policy/src/diagnostics.rs +++ b/crates/paralegal-policy/src/diagnostics.rs @@ -127,10 +127,11 @@ macro_rules! assert_warning { } /// Severity of a recorded diagnostic message -#[derive(Debug, Clone, Copy)] +#[derive(Debug, Clone, Copy, strum::AsRefStr)] +#[strum(serialize_all = "camel_case")] pub enum Severity { /// This indicates that the policy failed. - Fail, + Error, /// This could indicate that the policy does not operate as intended. Warning, /// Additional information for a diagnostic @@ -141,7 +142,16 @@ pub enum Severity { impl Severity { fn must_abort(self) -> bool { - matches!(self, Severity::Fail) + matches!(self, Severity::Error) + } + + fn color(self) -> Color { + match self { + Severity::Error => Color::Red, + Severity::Warning => Color::Yellow, + Severity::Note => Color::Blue, + Severity::Help => Color::Green, + } } } @@ -210,7 +220,7 @@ impl HasDiagnosticsBase for Rc { pub trait Diagnostics: HasDiagnosticsBase { /// Emit a message that is severe enough that it causes the policy to fail. fn error(&self, msg: impl Into) { - self.record(msg.into(), Severity::Fail, vec![]) + self.record(msg.into(), Severity::Error, vec![]) } /// Emit a message that indicates to the user that the policy might be @@ -225,7 +235,7 @@ pub trait Diagnostics: HasDiagnosticsBase { } /// Emit a message that suggests something to the user. - fn hint(&self, msg: impl Into) { + fn help(&self, msg: impl Into) { self.record(msg.into(), Severity::Help, vec![]) } @@ -241,40 +251,30 @@ pub trait Diagnostics: HasDiagnosticsBase { severity: Severity, ) -> anyhow::Result<()> { use std::fmt::Write; - let (diag_type, coloring) = match severity { - Severity::Fail => ("error", (|s| s.red()) as fn(&str) -> ColoredString), - Severity::Warning => ("warning", (|s: &str| s.yellow()) as _), - Severity::Note => ("note", (|s: &str| s.blue()) as _), - Severity::Help => ("help", (|s: &str| s.green()) as _), - }; + let coloring = severity.color(); let mut s = String::new(); - macro_rules! println { - ($($t:tt)*) => { - writeln!(s, $($t)*)?; - }; - } use std::io::BufRead; let node_kind = self.as_ctx().node_info(node).kind; - let src_loc = &self.as_ctx().get_location(node); + let src_loc = self.as_ctx().get_location(node); let max_line_len = std::cmp::max( src_loc.start_line.to_string().len(), src_loc.end_line.to_string().len(), ); - println!("{}: {}", coloring(diag_type), msg.into()); + writeln!(s, "{}: {}", severity.as_ref().color(coloring), msg.into())?; let tab: String = " ".repeat(max_line_len); - println!( - "{}{} {}:{}:{} ({node_kind})", - tab, + writeln!( + s, + "{tab}{} {}:{}:{} ({node_kind})", "-->".blue(), src_loc.source_file.file_path, src_loc.start_line, src_loc.start_col, - ); - println!("{} {}", tab, "|".blue()); + )?; + writeln!(s, "{tab} {}", "|".blue())?; let lines = std::io::BufReader::new(std::fs::File::open(&src_loc.source_file.abs_file_path)?) .lines() @@ -294,31 +294,30 @@ pub trait Diagnostics: HasDiagnosticsBase { } else { line_length_while(&line_content, char::is_whitespace) }; - let tab_len = max_line_len - line_num.to_string().len(); - println!( - "{}{} {} {}", - " ".repeat(tab_len), + writeln!( + s, + "{: anyhow::Result<()> { - self.node_diagnostic(node, msg, Severity::Fail) + self.node_diagnostic(node, msg, Severity::Error) } /// Prints a warning message for a problematic node diff --git a/crates/paralegal-spdg/Cargo.toml b/crates/paralegal-spdg/Cargo.toml index 2e85c4a999..f200fe0680 100644 --- a/crates/paralegal-spdg/Cargo.toml +++ b/crates/paralegal-spdg/Cargo.toml @@ -16,7 +16,7 @@ log = "0.4" internment = { version = "0.7.1", features = ["serde"] } indexical = { workspace = true } itertools = "0.11.0" -strum = { version = "0.25", features = ["derive"] } +strum = { workspace = true } cfg-if = "1" #flowistry_pdg = { path = "../../../flowistry/crates/flowistry_pdg" } flowistry_pdg = { git = "https://github.com/brownsys/flowistry", rev = "d1fcc76509032dd94f5255fd03c0ad0397efe834" } diff --git a/props/Cargo.lock b/props/Cargo.lock index 2fe0d87fda..48dbb9fd20 100644 --- a/props/Cargo.lock +++ b/props/Cargo.lock @@ -153,6 +153,17 @@ dependencies = [ "os_str_bytes", ] +[[package]] +name = "colored" +version = "1.9.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5a5f741c91823341bebf717d4c71bda820630ce065443b58bd1b7451af008355" +dependencies = [ + "is-terminal", + "lazy_static", + "winapi", +] + [[package]] name = "colored" version = "2.0.4" @@ -462,6 +473,7 @@ version = "0.1.0" dependencies = [ "anyhow", "bitvec", + "colored 1.9.4", "indexical", "itertools 0.12.1", "lazy_static", @@ -470,6 +482,7 @@ dependencies = [ "petgraph", "serde_json", "simple_logger", + "strum", ] [[package]] @@ -671,7 +684,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "48047e77b528151aaf841a10a9025f9459da80ba820e425ff7eb005708a76dc7" dependencies = [ "atty", - "colored", + "colored 2.0.4", "log", "time", "winapi", From 24aaddfd86138b6781d36cd852ba542d7642ce01 Mon Sep 17 00:00:00 2001 From: Justus Adam Date: Sun, 3 Mar 2024 14:53:52 -0500 Subject: [PATCH 14/20] Wrong casing --- crates/paralegal-policy/src/diagnostics.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/paralegal-policy/src/diagnostics.rs b/crates/paralegal-policy/src/diagnostics.rs index 404fbf131b..4bddff0770 100644 --- a/crates/paralegal-policy/src/diagnostics.rs +++ b/crates/paralegal-policy/src/diagnostics.rs @@ -128,7 +128,7 @@ macro_rules! assert_warning { /// Severity of a recorded diagnostic message #[derive(Debug, Clone, Copy, strum::AsRefStr)] -#[strum(serialize_all = "camel_case")] +#[strum(serialize_all = "snake_case")] pub enum Severity { /// This indicates that the policy failed. Error, From 9a2a2a5fcad5ef48b327faf0dd9116bb1f06562f Mon Sep 17 00:00:00 2001 From: Justus Adam Date: Sun, 3 Mar 2024 16:25:32 -0500 Subject: [PATCH 15/20] Expanded builder --- crates/paralegal-policy/src/context.rs | 6 +- crates/paralegal-policy/src/diagnostics.rs | 491 ++++++++++++++++----- 2 files changed, 380 insertions(+), 117 deletions(-) diff --git a/crates/paralegal-policy/src/context.rs b/crates/paralegal-policy/src/context.rs index af3b57bfdc..59442f5a3c 100644 --- a/crates/paralegal-policy/src/context.rs +++ b/crates/paralegal-policy/src/context.rs @@ -727,9 +727,9 @@ impl AlwaysHappensBefore { assert_warning!(ctx, !self.is_vacuous(), "Is vacuously true."); if !self.holds() { for &(reached, from) in &self.reached { - ctx.print_node_error(reached, "Reached this terminal") - .unwrap(); - ctx.print_node_note(from, "Started from this node").unwrap(); + ctx.struct_node_error(reached, "Reached this terminal") + .with_node_note(from, "Started from this node") + .emit(); } } } diff --git a/crates/paralegal-policy/src/diagnostics.rs b/crates/paralegal-policy/src/diagnostics.rs index 4bddff0770..7feb0e0f4b 100644 --- a/crates/paralegal-policy/src/diagnostics.rs +++ b/crates/paralegal-policy/src/diagnostics.rs @@ -82,7 +82,7 @@ use colored::*; use std::rc::Rc; use std::{io::Write, sync::Arc}; -use paralegal_spdg::{GlobalNode, Identifier, SPDG}; +use paralegal_spdg::{GlobalNode, Identifier, SrcCodeSpan, SPDG}; use crate::{Context, ControllerId}; @@ -156,13 +156,210 @@ impl Severity { } /// Context provided to [`HasDiagnosticsBase::record`]. -pub type DiagnosticContextStack = Vec; +type DiagnosticContextStack = Vec; +/// Representation of a diagnostic message. You should not interact with this +/// type directly but use the methods on [`Diagnostics`] or +/// [`DiagnosticBuilder`] to create these. #[derive(Debug)] -struct Diagnostic { +pub struct Diagnostic { + context: DiagnosticContextStack, + main: DiagnosticPart, + children: Vec, +} + +impl Diagnostic { + fn write(&self, w: &mut impl std::fmt::Write) -> std::fmt::Result { + for ctx in self.context.iter().rev() { + write!(w, "{ctx} ")?; + } + self.main.write(w)?; + for c in &self.children { + c.write(w)?; + } + Ok(()) + } +} + +#[derive(Debug)] +struct DiagnosticPart { message: String, severity: Severity, - context: DiagnosticContextStack, + span: Option, +} + +impl DiagnosticPart { + fn write(&self, s: &mut impl std::fmt::Write) -> std::fmt::Result { + let severity = self.severity; + let coloring = severity.color(); + + use std::io::BufRead; + + writeln!(s, "{}: {}", severity.as_ref().color(coloring), self.message)?; + if let Some(src_loc) = &self.span { + let max_line_len = std::cmp::max( + src_loc.start_line.to_string().len(), + src_loc.end_line.to_string().len(), + ); + let tab: String = " ".repeat(max_line_len); + writeln!( + s, + "{tab}{} {}:{}:{}", + "-->".blue(), + src_loc.source_file.file_path, + src_loc.start_line, + src_loc.start_col, + )?; + writeln!(s, "{tab} {}", "|".blue())?; + let lines = std::io::BufReader::new( + std::fs::File::open(&src_loc.source_file.abs_file_path).unwrap(), + ) + .lines() + .skip(src_loc.start_line - 1) + .take(src_loc.end_line - src_loc.start_line + 1) + .enumerate(); + for (i, line) in lines { + let line_content: String = line.unwrap(); + let line_num = src_loc.start_line + i; + let end: usize = if line_num == src_loc.end_line { + line_length_while(&line_content[0..src_loc.end_col - 1], |_| true) + } else { + line_length_while(&line_content, |_| true) + }; + let start: usize = if line_num == src_loc.start_line { + line_length_while(&line_content[0..src_loc.start_col - 1], |_| true) + } else { + line_length_while(&line_content, char::is_whitespace) + }; + + writeln!( + s, + "{:` creates simple main diagnostics with only a message, +/// `struct_node_` creates a main diagnostic with a message and the +/// span of a graph node and `struct_span_` creates a main diagnostic +/// with a message and a custom source code span. +/// +/// The builder allows chaining additional sub diagnostics to the main +/// diagnostic. Analogous to the initializers the `with_` family of +/// functions adds simple messages, `with_node_` adds messages with +/// spans from a node and `with_span_` adds messages with custom +/// spans. +/// +/// Make sure to call [`Self::emit`] after construction, otherwise the +/// diagnostic is not shown. +#[derive(Debug)] +#[must_use = "you must call `emit`, otherwise the message is not shown"] +pub struct DiagnosticBuilder<'a, A: ?Sized> { + diagnostic: Diagnostic, + base: &'a A, +} + +impl<'a, A: ?Sized> DiagnosticBuilder<'a, A> { + fn init(message: String, severity: Severity, span: Option, base: &'a A) -> Self { + DiagnosticBuilder { + diagnostic: Diagnostic { + context: vec![], + main: DiagnosticPart { + message, + severity, + span, + }, + children: vec![], + }, + base, + } + } + + fn with_child( + mut self, + message: impl Into, + severity: Severity, + span: Option, + ) -> Self { + self.diagnostic.children.push(DiagnosticPart { + message: message.into(), + severity, + span, + }); + self + } +} + +impl<'a, A: HasDiagnosticsBase + ?Sized> DiagnosticBuilder<'a, A> { + /// Queue the diagnostic for display to the user. + pub fn emit(self) { + self.base.record(self.diagnostic) + } + + /// Append a help message to the diagnostic. + pub fn with_help(self, message: impl Into) -> Self { + self.with_child(message, Severity::Help, None) + } + + /// Append a help message with a source code span to the diagnostic. + pub fn with_span_help(self, span: SrcCodeSpan, message: impl Into) -> Self { + self.with_child(message, Severity::Help, Some(span)) + } + + /// Append a help message and the span of a graph node to the diagnostic. + pub fn with_node_help(self, node: GlobalNode, message: impl Into) -> Self { + let span = self.base.as_ctx().get_location(node).clone(); + self.with_child(message, Severity::Help, Some(span)) + } + + /// Append a warning to the diagnostic. + pub fn with_warning(self, message: impl Into) -> Self { + self.with_child(message, Severity::Warning, None) + } + + /// Append a warning and the span of a graph node to the diagnostic. + pub fn with_span_warning(self, span: SrcCodeSpan, message: impl Into) -> Self { + self.with_child(message, Severity::Warning, Some(span)) + } + + /// Append a warning with a source code span to the diagnostic. + pub fn with_node_warning(self, node: GlobalNode, message: impl Into) -> Self { + let span = self.base.as_ctx().get_location(node).clone(); + self.with_child(message, Severity::Warning, Some(span)) + } + + /// Append a note to the diagnostic. + pub fn with_note(self, message: impl Into) -> Self { + self.with_child(message, Severity::Note, None) + } + + /// Append a note with a source code span to the diagnostic. + pub fn with_span_note(self, span: SrcCodeSpan, message: impl Into) -> Self { + self.with_child(message, Severity::Note, Some(span)) + } + + /// Append a note and the span of a graph node to the diagnostic. + pub fn with_node_note(self, node: GlobalNode, message: impl Into) -> Self { + let span = self.base.as_ctx().get_location(node).clone(); + self.with_child(message, Severity::Note, Some(span)) + } } /// Low level machinery for diagnostics. @@ -176,16 +373,16 @@ pub trait HasDiagnosticsBase { /// This should be used by implementors of new wrappers, users should use /// high level functions like [`Diagnostics::error`] or /// [`Diagnostics::warning`] instead. - fn record(&self, msg: String, severity: Severity, context: DiagnosticContextStack); + fn record(&self, diagnostic: Diagnostic); /// Access to [`Context`], usually also available via [`std::ops::Deref`]. fn as_ctx(&self) -> &Context; } impl HasDiagnosticsBase for Arc { - fn record(&self, msg: String, severity: Severity, context: DiagnosticContextStack) { + fn record(&self, diagnostic: Diagnostic) { let t: &T = self.as_ref(); - t.record(msg, severity, context) + t.record(diagnostic) } fn as_ctx(&self) -> &Context { @@ -198,8 +395,8 @@ impl HasDiagnosticsBase for &'_ T { (*self).as_ctx() } - fn record(&self, msg: String, severity: Severity, context: DiagnosticContextStack) { - (*self).record(msg, severity, context) + fn record(&self, diagnostic: Diagnostic) { + (*self).record(diagnostic) } } @@ -208,8 +405,8 @@ impl HasDiagnosticsBase for Rc { (**self).as_ctx() } - fn record(&self, msg: String, severity: Severity, context: DiagnosticContextStack) { - (**self).record(msg, severity, context) + fn record(&self, diagnostic: Diagnostic) { + (**self).record(diagnostic) } } @@ -218,124 +415,189 @@ impl HasDiagnosticsBase for Rc { /// This is how any types implementing [`HasDiagnosticsBase`] should actually be /// used. pub trait Diagnostics: HasDiagnosticsBase { + /// Initialize a diagnostic builder for an error. + /// + /// This will fail the policy. + fn struct_error(&self, msg: impl Into) -> DiagnosticBuilder<'_, Self> { + DiagnosticBuilder::init(msg.into(), Severity::Error, None, self) + } + + /// Initialize a diagnostic builder for an error with a source code span. + /// + /// This will fail the policy. + fn struct_span_error( + &self, + span: SrcCodeSpan, + msg: impl Into, + ) -> DiagnosticBuilder<'_, Self> { + DiagnosticBuilder::init(msg.into(), Severity::Error, Some(span), self) + } + + /// Initialize a diagnostic builder for a warning. + /// + /// Does not fail the policy. + fn struct_warning(&self, msg: impl Into) -> DiagnosticBuilder<'_, Self> { + DiagnosticBuilder::init(msg.into(), Severity::Warning, None, self) + } + + /// Initialize a diagnostic builder for a warning with a source code span + /// + /// Does not fail the policy. + fn struct_span_warning( + &self, + span: SrcCodeSpan, + msg: impl Into, + ) -> DiagnosticBuilder<'_, Self> { + DiagnosticBuilder::init(msg.into(), Severity::Warning, Some(span), self) + } + + /// Initialize a diagnostic builder for a help message. + fn struct_help(&self, msg: impl Into) -> DiagnosticBuilder<'_, Self> { + DiagnosticBuilder::init(msg.into(), Severity::Help, None, self) + } + + /// Initialize a diagnostic builder for a help message with a source code span + fn struct_span_help( + &self, + span: SrcCodeSpan, + msg: impl Into, + ) -> DiagnosticBuilder<'_, Self> { + DiagnosticBuilder::init(msg.into(), Severity::Help, Some(span), self) + } + + /// Initialize a diagnostic builder for a note + fn struct_note(&self, msg: impl Into) -> DiagnosticBuilder<'_, Self> { + DiagnosticBuilder::init(msg.into(), Severity::Note, None, self) + } + + /// Initialize a diagnostic builder for a note with a source code span + fn struct_span_note( + &self, + span: SrcCodeSpan, + msg: impl Into, + ) -> DiagnosticBuilder<'_, Self> { + DiagnosticBuilder::init(msg.into(), Severity::Note, Some(span), self) + } + /// Emit a message that is severe enough that it causes the policy to fail. fn error(&self, msg: impl Into) { - self.record(msg.into(), Severity::Error, vec![]) + self.struct_error(msg).emit() } /// Emit a message that indicates to the user that the policy might be /// fraudulent but could be correct. fn warning(&self, msg: impl Into) { - self.record(msg.into(), Severity::Warning, vec![]) + self.struct_warning(msg).emit() } /// Emit a message that provides additional information to the user. fn note(&self, msg: impl Into) { - self.record(msg.into(), Severity::Note, vec![]) + self.struct_note(msg).emit() } /// Emit a message that suggests something to the user. fn help(&self, msg: impl Into) { - self.record(msg.into(), Severity::Help, vec![]) + self.struct_help(msg).emit() + } + + /// Emit a message that is severe enough that it causes the policy to fail + /// with a source code span. + fn span_error(&self, msg: impl Into, span: SrcCodeSpan) { + self.struct_span_error(span, msg).emit() + } + + /// Emit a message that indicates to the user that the policy might be + /// fraudulent but could be correct. Includes a source code span. + fn span_warning(&self, msg: impl Into, span: SrcCodeSpan) { + self.struct_span_warning(span, msg).emit() + } + + /// Emit a message that provides additional information to the user. + fn span_note(&self, msg: impl Into, span: SrcCodeSpan) { + self.struct_span_note(span, msg).emit() } - /// Prints a diagnostic message and the source code that corresponds to the - /// given node. + /// Emit a message that suggests something to the user. + fn span_help(&self, msg: impl Into, span: SrcCodeSpan) { + self.struct_span_help(span, msg).emit() + } + + /// Initialize a diagnostic builder for an error with the span of a graph + /// node. /// - /// The severity governs the severity of the emitted message (the same as - /// e.g. [`Self::error`]) and the coloring of the span information. - fn node_diagnostic( + /// This will fail the policy. + fn struct_node_error( &self, node: GlobalNode, msg: impl Into, - severity: Severity, - ) -> anyhow::Result<()> { - use std::fmt::Write; - let coloring = severity.color(); - - let mut s = String::new(); - use std::io::BufRead; - let node_kind = self.as_ctx().node_info(node).kind; - - let src_loc = self.as_ctx().get_location(node); + ) -> DiagnosticBuilder<'_, Self> { + struct_node_diagnostic(self, node, Severity::Error, msg) + } - let max_line_len = std::cmp::max( - src_loc.start_line.to_string().len(), - src_loc.end_line.to_string().len(), - ); + /// Initialize a diagnostic builder for an error with the span of a graph + /// node. + /// + /// This will not fail the policy. + fn struct_node_warning( + &self, + node: GlobalNode, + msg: impl Into, + ) -> DiagnosticBuilder<'_, Self> { + struct_node_diagnostic(self, node, Severity::Warning, msg) + } - writeln!(s, "{}: {}", severity.as_ref().color(coloring), msg.into())?; - let tab: String = " ".repeat(max_line_len); - writeln!( - s, - "{tab}{} {}:{}:{} ({node_kind})", - "-->".blue(), - src_loc.source_file.file_path, - src_loc.start_line, - src_loc.start_col, - )?; - writeln!(s, "{tab} {}", "|".blue())?; - let lines = - std::io::BufReader::new(std::fs::File::open(&src_loc.source_file.abs_file_path)?) - .lines() - .skip(src_loc.start_line - 1) - .take(src_loc.end_line - src_loc.start_line + 1) - .enumerate(); - for (i, line) in lines { - let line_content: String = line?; - let line_num = src_loc.start_line + i; - let end: usize = if line_num == src_loc.end_line { - line_length_while(&line_content[0..src_loc.end_col - 1], |_| true) - } else { - line_length_while(&line_content, |_| true) - }; - let start: usize = if line_num == src_loc.start_line { - line_length_while(&line_content[0..src_loc.start_col - 1], |_| true) - } else { - line_length_while(&line_content, char::is_whitespace) - }; + /// Initialize a diagnostic builder for an note with the span of a graph + /// node. + fn struct_node_note( + &self, + node: GlobalNode, + msg: impl Into, + ) -> DiagnosticBuilder<'_, Self> { + struct_node_diagnostic(self, node, Severity::Note, msg) + } - writeln!( - s, - "{:, + ) -> DiagnosticBuilder<'_, Self> { + struct_node_diagnostic(self, node, Severity::Help, msg) } - /// Prints an error message for a problematic node - fn print_node_error(&self, node: GlobalNode, msg: &str) -> anyhow::Result<()> { - self.node_diagnostic(node, msg, Severity::Error) + /// Emit an error, failing the policy, with the span of a graph node. + fn node_error(&self, node: GlobalNode, msg: impl Into) { + self.struct_node_error(node, msg).emit() } - /// Prints a warning message for a problematic node - fn print_node_warning(&self, node: GlobalNode, msg: &str) -> anyhow::Result<()> { - self.node_diagnostic(node, msg, Severity::Warning) + /// Emit an warning, that does not fail the policy, with the span of a graph + /// node. + fn node_warning(&self, node: GlobalNode, msg: impl Into) { + self.struct_node_warning(node, msg).emit() } - /// Prints a note for a problematic node - fn print_node_note(&self, node: GlobalNode, msg: &str) -> anyhow::Result<()> { - self.node_diagnostic(node, msg, Severity::Note) + /// Emit a note with the span of a graph node. + fn node_note(&self, node: GlobalNode, msg: impl Into) { + self.struct_node_note(node, msg).emit() } - /// Print a hint with a node - fn print_node_hint(&self, node: GlobalNode, msg: &str) -> anyhow::Result<()> { - self.node_diagnostic(node, msg, Severity::Help) + /// Emit a help message with the span of a graph node. + fn node_help(&self, node: GlobalNode, msg: impl Into) { + self.struct_node_note(node, msg).emit() } } +fn struct_node_diagnostic<'a, B: HasDiagnosticsBase + ?Sized>( + base: &'a B, + node: GlobalNode, + severity: Severity, + msg: impl Into, +) -> DiagnosticBuilder<'a, B> { + let span = base.as_ctx().get_location(node); + DiagnosticBuilder::init(msg.into(), severity, Some(span.clone()), base) +} + const TAB_SIZE: usize = 4; fn line_length_while(s: &str, mut cont: impl FnMut(char) -> bool) -> usize { @@ -431,9 +693,9 @@ impl PolicyContext { } impl HasDiagnosticsBase for PolicyContext { - fn record(&self, msg: String, severity: Severity, mut context: DiagnosticContextStack) { - context.push(format!("[policy: {}]", self.name)); - self.inner.record(msg, severity, context) + fn record(&self, mut diagnostic: Diagnostic) { + diagnostic.context.push(format!("[policy: {}]", self.name)); + self.inner.record(diagnostic) } fn as_ctx(&self) -> &Context { @@ -519,10 +781,10 @@ impl ControllerContext { } impl HasDiagnosticsBase for ControllerContext { - fn record(&self, msg: String, severity: Severity, mut context: DiagnosticContextStack) { + fn record(&self, mut diagnostic: Diagnostic) { let name = self.as_ctx().desc().controllers[&self.id].name; - context.push(format!("[controller: {}]", name)); - self.inner.record(msg, severity, context) + diagnostic.context.push(format!("[controller: {}]", name)); + self.inner.record(diagnostic) } fn as_ctx(&self) -> &Context { @@ -572,9 +834,9 @@ impl CombinatorContext { } impl HasDiagnosticsBase for CombinatorContext { - fn record(&self, msg: String, severity: Severity, mut context: DiagnosticContextStack) { - context.push(format!("{}", self.name)); - self.inner.record(msg, severity, context) + fn record(&self, mut diagnostic: Diagnostic) { + diagnostic.context.push(format!("{}", self.name)); + self.inner.record(diagnostic) } fn as_ctx(&self) -> &Context { @@ -635,6 +897,14 @@ impl Context { #[derive(Debug, Default)] pub(crate) struct DiagnosticsRecorder(std::sync::Mutex>); +struct DisplayDiagnostic<'a>(&'a Diagnostic); + +impl<'a> std::fmt::Display for DisplayDiagnostic<'a> { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + self.0.write(f) + } +} + impl DiagnosticsRecorder { /// Emit queued diagnostics, draining the internal queue of diagnostics. /// @@ -644,11 +914,8 @@ impl DiagnosticsRecorder { let w = &mut w; let mut can_continue = true; for diag in self.0.lock().unwrap().drain(..) { - for ctx in diag.context.iter().rev() { - write!(w, "{ctx} ")?; - } - writeln!(w, "{}", diag.message)?; - can_continue &= !diag.severity.must_abort(); + writeln!(w, "{}", DisplayDiagnostic(&diag))?; + can_continue &= !diag.main.severity.must_abort(); } Ok(can_continue) } @@ -656,12 +923,8 @@ impl DiagnosticsRecorder { impl HasDiagnosticsBase for Context { /// Record a diagnostic message. - fn record(&self, message: String, severity: Severity, context: DiagnosticContextStack) { - self.diagnostics.0.lock().unwrap().push(Diagnostic { - message, - severity, - context, - }) + fn record(&self, diagnostic: Diagnostic) { + self.diagnostics.0.lock().unwrap().push(diagnostic); } fn as_ctx(&self) -> &Context { From ed3958fe41b17a81479be0f3ebbf36459e1be6f3 Mon Sep 17 00:00:00 2001 From: Justus Adam Date: Sun, 3 Mar 2024 17:03:14 -0500 Subject: [PATCH 16/20] Use mut ref for builder --- crates/paralegal-policy/src/context.rs | 6 ++--- crates/paralegal-policy/src/diagnostics.rs | 26 +++++++++++++--------- 2 files changed, 18 insertions(+), 14 deletions(-) diff --git a/crates/paralegal-policy/src/context.rs b/crates/paralegal-policy/src/context.rs index 59442f5a3c..383e89780b 100644 --- a/crates/paralegal-policy/src/context.rs +++ b/crates/paralegal-policy/src/context.rs @@ -727,9 +727,9 @@ impl AlwaysHappensBefore { assert_warning!(ctx, !self.is_vacuous(), "Is vacuously true."); if !self.holds() { for &(reached, from) in &self.reached { - ctx.struct_node_error(reached, "Reached this terminal") - .with_node_note(from, "Started from this node") - .emit(); + let mut err = ctx.struct_node_error(reached, "Reached this terminal"); + err.with_node_note(from, "Started from this node"); + err.emit(); } } } diff --git a/crates/paralegal-policy/src/diagnostics.rs b/crates/paralegal-policy/src/diagnostics.rs index 7feb0e0f4b..7d7790e0e7 100644 --- a/crates/paralegal-policy/src/diagnostics.rs +++ b/crates/paralegal-policy/src/diagnostics.rs @@ -293,11 +293,11 @@ impl<'a, A: ?Sized> DiagnosticBuilder<'a, A> { } fn with_child( - mut self, + &mut self, message: impl Into, severity: Severity, span: Option, - ) -> Self { + ) -> &mut Self { self.diagnostic.children.push(DiagnosticPart { message: message.into(), severity, @@ -314,49 +314,53 @@ impl<'a, A: HasDiagnosticsBase + ?Sized> DiagnosticBuilder<'a, A> { } /// Append a help message to the diagnostic. - pub fn with_help(self, message: impl Into) -> Self { + pub fn with_help(&mut self, message: impl Into) -> &mut Self { self.with_child(message, Severity::Help, None) } /// Append a help message with a source code span to the diagnostic. - pub fn with_span_help(self, span: SrcCodeSpan, message: impl Into) -> Self { + pub fn with_span_help(&mut self, span: SrcCodeSpan, message: impl Into) -> &mut Self { self.with_child(message, Severity::Help, Some(span)) } /// Append a help message and the span of a graph node to the diagnostic. - pub fn with_node_help(self, node: GlobalNode, message: impl Into) -> Self { + pub fn with_node_help(&mut self, node: GlobalNode, message: impl Into) -> &mut Self { let span = self.base.as_ctx().get_location(node).clone(); self.with_child(message, Severity::Help, Some(span)) } /// Append a warning to the diagnostic. - pub fn with_warning(self, message: impl Into) -> Self { + pub fn with_warning(&mut self, message: impl Into) -> &mut Self { self.with_child(message, Severity::Warning, None) } /// Append a warning and the span of a graph node to the diagnostic. - pub fn with_span_warning(self, span: SrcCodeSpan, message: impl Into) -> Self { + pub fn with_span_warning( + &mut self, + span: SrcCodeSpan, + message: impl Into, + ) -> &mut Self { self.with_child(message, Severity::Warning, Some(span)) } /// Append a warning with a source code span to the diagnostic. - pub fn with_node_warning(self, node: GlobalNode, message: impl Into) -> Self { + pub fn with_node_warning(&mut self, node: GlobalNode, message: impl Into) -> &mut Self { let span = self.base.as_ctx().get_location(node).clone(); self.with_child(message, Severity::Warning, Some(span)) } /// Append a note to the diagnostic. - pub fn with_note(self, message: impl Into) -> Self { + pub fn with_note(&mut self, message: impl Into) -> &mut Self { self.with_child(message, Severity::Note, None) } /// Append a note with a source code span to the diagnostic. - pub fn with_span_note(self, span: SrcCodeSpan, message: impl Into) -> Self { + pub fn with_span_note(&mut self, span: SrcCodeSpan, message: impl Into) -> &mut Self { self.with_child(message, Severity::Note, Some(span)) } /// Append a note and the span of a graph node to the diagnostic. - pub fn with_node_note(self, node: GlobalNode, message: impl Into) -> Self { + pub fn with_node_note(&mut self, node: GlobalNode, message: impl Into) -> &mut Self { let span = self.base.as_ctx().get_location(node).clone(); self.with_child(message, Severity::Note, Some(span)) } From c14d88f999a86c1f6b2f0e70564599d717733fc3 Mon Sep 17 00:00:00 2001 From: Justus Adam Date: Sun, 3 Mar 2024 17:05:51 -0500 Subject: [PATCH 17/20] Clippy --- crates/paralegal-policy/src/diagnostics.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/paralegal-policy/src/diagnostics.rs b/crates/paralegal-policy/src/diagnostics.rs index 7d7790e0e7..22eeb21354 100644 --- a/crates/paralegal-policy/src/diagnostics.rs +++ b/crates/paralegal-policy/src/diagnostics.rs @@ -592,12 +592,12 @@ pub trait Diagnostics: HasDiagnosticsBase { } } -fn struct_node_diagnostic<'a, B: HasDiagnosticsBase + ?Sized>( - base: &'a B, +fn struct_node_diagnostic( + base: &B, node: GlobalNode, severity: Severity, msg: impl Into, -) -> DiagnosticBuilder<'a, B> { +) -> DiagnosticBuilder<'_, B> { let span = base.as_ctx().get_location(node); DiagnosticBuilder::init(msg.into(), severity, Some(span.clone()), base) } From d18a73d293268c136c64e54d3a1aacad1b39aa95 Mon Sep 17 00:00:00 2001 From: Justus Adam Date: Sun, 3 Mar 2024 17:20:01 -0500 Subject: [PATCH 18/20] Make "always happens before" "must use" --- crates/paralegal-policy/src/context.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/paralegal-policy/src/context.rs b/crates/paralegal-policy/src/context.rs index 383e89780b..ce2a46cc41 100644 --- a/crates/paralegal-policy/src/context.rs +++ b/crates/paralegal-policy/src/context.rs @@ -687,6 +687,7 @@ impl<'a> std::fmt::Display for DisplayDef<'a> { /// [`Self::is_vacuous`]. Otherwise the information in this struct and its /// printed representations should be considered unstable and /// for-human-eyes-only. +#[must_use = "call `report` or similar evaluations function to ensure the property is checked"] pub struct AlwaysHappensBefore { /// How many paths terminated at the end? reached: Vec<(GlobalNode, GlobalNode)>, From a84430f82079d6c30752fc0dc9433becf2784cce Mon Sep 17 00:00:00 2001 From: Justus Adam Date: Mon, 4 Mar 2024 12:05:48 -0500 Subject: [PATCH 19/20] Subspans --- crates/paralegal-flow/src/ana/mod.rs | 63 ++++--- crates/paralegal-flow/src/test_utils.rs | 1 + crates/paralegal-policy/src/context.rs | 8 +- crates/paralegal-policy/src/diagnostics.rs | 191 +++++++++++++++------ crates/paralegal-policy/src/test_utils.rs | 2 +- crates/paralegal-spdg/src/dot.rs | 2 +- crates/paralegal-spdg/src/lib.rs | 48 ++++-- 7 files changed, 214 insertions(+), 101 deletions(-) diff --git a/crates/paralegal-flow/src/ana/mod.rs b/crates/paralegal-flow/src/ana/mod.rs index 8e2198238d..e8cde14c52 100644 --- a/crates/paralegal-flow/src/ana/mod.rs +++ b/crates/paralegal-flow/src/ana/mod.rs @@ -27,7 +27,7 @@ use flowistry::pdg::{ }; use itertools::Itertools; use petgraph::visit::{GraphBase, IntoNodeReferences, NodeIndexable, NodeRef}; -use rustc_span::{FileNameDisplayPreference, Span}; +use rustc_span::{FileNameDisplayPreference, Span as RustSpan}; mod inline_judge; @@ -136,7 +136,7 @@ impl<'tcx> SPDGGenerator<'tcx> { fn collect_instruction_info( &self, controllers: &HashMap, - ) -> HashMap { + ) -> HashMap { let all_instructions = controllers .values() .flat_map(|v| { @@ -149,13 +149,13 @@ impl<'tcx> SPDGGenerator<'tcx> { all_instructions .into_iter() .map(|i| { - let body = self.tcx.body_for_def_id(i.function).unwrap(); + let body = &self.tcx.body_for_def_id(i.function).unwrap().body; - let info = match i.location { + let kind = match i.location { RichLocation::End => InstructionKind::Return, RichLocation::Start => InstructionKind::Start, RichLocation::Location(loc) => { - let kind = match body.body.stmt_at(loc) { + let kind = match body.stmt_at(loc) { crate::Either::Right(term) => { if let Ok((id, ..)) = term.as_fn_and_args(self.tcx) { InstructionKind::FunctionCall(FunctionCallInfo { @@ -172,7 +172,26 @@ impl<'tcx> SPDGGenerator<'tcx> { kind } }; - (i, info) + let rust_span = match i.location { + RichLocation::Location(loc) => { + let expanded_span = match body.stmt_at(loc) { + crate::Either::Right(term) => term.source_info.span, + crate::Either::Left(stmt) => stmt.source_info.span, + }; + self.tcx + .sess + .source_map() + .stmt_span(expanded_span, body.span) + } + RichLocation::Start | RichLocation::End => self.tcx.def_span(i.function), + }; + ( + i, + InstructionInfo { + kind, + span: src_loc_for_span(rust_span, self.tcx), + }, + ) }) .collect() } @@ -209,7 +228,7 @@ impl<'tcx> SPDGGenerator<'tcx> { } } -fn src_loc_for_span(span: Span, tcx: TyCtxt) -> SrcCodeSpan { +fn src_loc_for_span(span: RustSpan, tcx: TyCtxt) -> Span { let (source_file, start_line, start_col, end_line, end_col) = tcx.sess.source_map().span_to_location_info(span); let file_path = source_file @@ -228,12 +247,16 @@ fn src_loc_for_span(span: Span, tcx: TyCtxt) -> SrcCodeSpan { file_path, abs_file_path, }; - SrcCodeSpan { + Span { source_file: src_info.intern(), - start_line, - start_col, - end_line, - end_col, + start: SpanCoord { + line: start_line as u32, + col: start_col as u32, + }, + end: SpanCoord { + line: end_line as u32, + col: end_col as u32, + }, } } @@ -588,26 +611,14 @@ impl<'a, 'tcx, C: Extend> GraphConverter<'tcx, 'a, C> { let at = weight.at.leaf(); let body = &tcx.body_for_def_id(at.function).unwrap().body; - let rustc_span = match at.location { - RichLocation::End | RichLocation::Start => { - let def = &body.local_decls[weight.place.local]; - def.source_info.span - } - RichLocation::Location(loc) => { - let expanded_span = match body.stmt_at(loc) { - crate::Either::Right(term) => term.source_info.span, - crate::Either::Left(stmt) => stmt.source_info.span, - }; - tcx.sess.source_map().stmt_span(expanded_span, body.span) - } - }; + let node_span = body.local_decls[weight.place.local].source_info.span; let new_idx = self.register_node( i, NodeInfo { at: weight.at, description: format!("{:?}", weight.place), kind, - span: src_loc_for_span(rustc_span, tcx), + span: src_loc_for_span(node_span, tcx), }, ); diff --git a/crates/paralegal-flow/src/test_utils.rs b/crates/paralegal-flow/src/test_utils.rs index 96c558889a..9c87583eb0 100644 --- a/crates/paralegal-flow/src/test_utils.rs +++ b/crates/paralegal-flow/src/test_utils.rs @@ -350,6 +350,7 @@ impl<'g> CtrlRef<'g> { .chain(self.ctrl.graph.node_weights().map(|info| info.at)) .filter(|m| { instruction_info[&m.leaf()] + .kind .as_function_call() .map_or(false, |i| i.id == fun.ident) }) diff --git a/crates/paralegal-policy/src/context.rs b/crates/paralegal-policy/src/context.rs index ce2a46cc41..bb366a5eaf 100644 --- a/crates/paralegal-policy/src/context.rs +++ b/crates/paralegal-policy/src/context.rs @@ -3,9 +3,9 @@ use std::{collections::HashSet, io::Write, process::exit, sync::Arc}; pub use paralegal_spdg::rustc_portable::{DefId, LocalDefId}; use paralegal_spdg::traverse::{generic_flows_to, EdgeSelection}; use paralegal_spdg::{ - CallString, DisplayNode, Endpoint, GlobalNode, HashMap, Identifier, InstructionKind, + CallString, DisplayNode, Endpoint, GlobalNode, HashMap, Identifier, InstructionInfo, IntoIterGlobalNodes, Node as SPDGNode, NodeCluster, NodeInfo, ProgramDescription, SPDGImpl, - SrcCodeSpan, TypeId, SPDG, + Span, TypeId, SPDG, }; use anyhow::{anyhow, bail, ensure, Result}; @@ -606,7 +606,7 @@ impl Context { } /// Retrieve metadata about the instruction executed by a specific node. - pub fn instruction_at_node(&self, node: GlobalNode) -> &InstructionKind { + pub fn instruction_at_node(&self, node: GlobalNode) -> &InstructionInfo { let node_info = self.node_info(node); &self.desc.instruction_info[&node_info.at.leaf()] } @@ -643,7 +643,7 @@ impl Context { } /// Get the span of a node - pub fn get_location(&self, node: GlobalNode) -> &SrcCodeSpan { + pub fn get_location(&self, node: GlobalNode) -> &Span { &self.node_info(node).span } } diff --git a/crates/paralegal-policy/src/diagnostics.rs b/crates/paralegal-policy/src/diagnostics.rs index 22eeb21354..5e61081c12 100644 --- a/crates/paralegal-policy/src/diagnostics.rs +++ b/crates/paralegal-policy/src/diagnostics.rs @@ -82,7 +82,7 @@ use colored::*; use std::rc::Rc; use std::{io::Write, sync::Arc}; -use paralegal_spdg::{GlobalNode, Identifier, SrcCodeSpan, SPDG}; +use paralegal_spdg::{GlobalNode, Identifier, Span, SpanCoord, SPDG}; use crate::{Context, ControllerId}; @@ -185,7 +185,42 @@ impl Diagnostic { struct DiagnosticPart { message: String, severity: Severity, - span: Option, + span: Option, +} + +#[derive(Clone, Debug)] +struct SubSpan { + start: SpanCoord, + end: SpanCoord, +} + +#[derive(Clone, Debug)] +/// A span with only a portion highlighted. +pub struct HighlightedSpan { + span: Span, + highlight: Option, +} + +impl HighlightedSpan { + /// Create a new span with a highlighted section + pub fn new(span: Span, start: SpanCoord, end: SpanCoord) -> Self { + assert!(start >= span.start); + assert!(end <= span.end); + assert!(start <= end); + HighlightedSpan { + span, + highlight: Some(SubSpan { start, end }), + } + } +} + +impl From for HighlightedSpan { + fn from(value: Span) -> Self { + Self { + span: value, + highlight: None, + } + } } impl DiagnosticPart { @@ -197,40 +232,43 @@ impl DiagnosticPart { writeln!(s, "{}: {}", severity.as_ref().color(coloring), self.message)?; if let Some(src_loc) = &self.span { - let max_line_len = std::cmp::max( - src_loc.start_line.to_string().len(), - src_loc.end_line.to_string().len(), - ); + let start_line = src_loc.span.start.line as usize; + let start_col = src_loc.span.start.col as usize; + let end_line = src_loc.span.end.line as usize; + let end_col = src_loc.span.end.col as usize; + let (hl_start_line, hl_start_col, hl_end_line, hl_end_col) = + if let Some(hl) = &src_loc.highlight { + ( + hl.start.line as usize, + hl.start.col as usize, + hl.end.line as usize, + hl.end.col as usize, + ) + } else { + (start_line, start_col, end_line, end_col) + }; + let max_line_len = + std::cmp::max(start_line.to_string().len(), end_line.to_string().len()); let tab: String = " ".repeat(max_line_len); writeln!( s, "{tab}{} {}:{}:{}", "-->".blue(), - src_loc.source_file.file_path, - src_loc.start_line, - src_loc.start_col, + src_loc.span.source_file.file_path, + start_line, + start_col, )?; writeln!(s, "{tab} {}", "|".blue())?; let lines = std::io::BufReader::new( - std::fs::File::open(&src_loc.source_file.abs_file_path).unwrap(), + std::fs::File::open(&src_loc.span.source_file.abs_file_path).unwrap(), ) .lines() - .skip(src_loc.start_line - 1) - .take(src_loc.end_line - src_loc.start_line + 1) + .skip(start_line - 1) + .take(end_line - start_line + 1) .enumerate(); for (i, line) in lines { let line_content: String = line.unwrap(); - let line_num = src_loc.start_line + i; - let end: usize = if line_num == src_loc.end_line { - line_length_while(&line_content[0..src_loc.end_col - 1], |_| true) - } else { - line_length_while(&line_content, |_| true) - }; - let start: usize = if line_num == src_loc.start_line { - line_length_while(&line_content[0..src_loc.start_col - 1], |_| true) - } else { - line_length_while(&line_content, char::is_whitespace) - }; + let line_num = start_line + i; writeln!( s, @@ -239,13 +277,25 @@ impl DiagnosticPart { "|".blue(), line_content.replace('\t', &" ".repeat(TAB_SIZE)) )?; - writeln!( - s, - "{tab} {} {}{}", - "|".blue(), - " ".repeat(start), - "^".repeat(end - start).color(coloring) - )?; + if line_num >= hl_start_line && line_num <= hl_end_line { + let end: usize = if line_num == hl_end_line { + line_length_while(&line_content[0..hl_end_col - 1], |_| true) + } else { + line_length_while(&line_content, |_| true) + }; + let start: usize = if line_num == hl_start_line { + line_length_while(&line_content[0..hl_start_col - 1], |_| true) + } else { + line_length_while(&line_content, char::is_whitespace) + }; + writeln!( + s, + "{tab} {} {}{}", + "|".blue(), + " ".repeat(start), + "^".repeat(end - start).color(coloring) + )?; + } } writeln!(s, "{tab} {}", "|".blue())?; } @@ -277,14 +327,19 @@ pub struct DiagnosticBuilder<'a, A: ?Sized> { } impl<'a, A: ?Sized> DiagnosticBuilder<'a, A> { - fn init(message: String, severity: Severity, span: Option, base: &'a A) -> Self { + fn init( + message: String, + severity: Severity, + span: Option>, + base: &'a A, + ) -> Self { DiagnosticBuilder { diagnostic: Diagnostic { context: vec![], main: DiagnosticPart { message, severity, - span, + span: span.map(Into::into), }, children: vec![], }, @@ -296,12 +351,12 @@ impl<'a, A: ?Sized> DiagnosticBuilder<'a, A> { &mut self, message: impl Into, severity: Severity, - span: Option, + span: Option>, ) -> &mut Self { self.diagnostic.children.push(DiagnosticPart { message: message.into(), severity, - span, + span: span.map(Into::into), }); self } @@ -315,11 +370,11 @@ impl<'a, A: HasDiagnosticsBase + ?Sized> DiagnosticBuilder<'a, A> { /// Append a help message to the diagnostic. pub fn with_help(&mut self, message: impl Into) -> &mut Self { - self.with_child(message, Severity::Help, None) + self.with_child(message, Severity::Help, Option::::None) } /// Append a help message with a source code span to the diagnostic. - pub fn with_span_help(&mut self, span: SrcCodeSpan, message: impl Into) -> &mut Self { + pub fn with_span_help(&mut self, span: Span, message: impl Into) -> &mut Self { self.with_child(message, Severity::Help, Some(span)) } @@ -331,15 +386,11 @@ impl<'a, A: HasDiagnosticsBase + ?Sized> DiagnosticBuilder<'a, A> { /// Append a warning to the diagnostic. pub fn with_warning(&mut self, message: impl Into) -> &mut Self { - self.with_child(message, Severity::Warning, None) + self.with_child(message, Severity::Warning, Option::::None) } /// Append a warning and the span of a graph node to the diagnostic. - pub fn with_span_warning( - &mut self, - span: SrcCodeSpan, - message: impl Into, - ) -> &mut Self { + pub fn with_span_warning(&mut self, span: Span, message: impl Into) -> &mut Self { self.with_child(message, Severity::Warning, Some(span)) } @@ -351,11 +402,11 @@ impl<'a, A: HasDiagnosticsBase + ?Sized> DiagnosticBuilder<'a, A> { /// Append a note to the diagnostic. pub fn with_note(&mut self, message: impl Into) -> &mut Self { - self.with_child(message, Severity::Note, None) + self.with_child(message, Severity::Note, Option::::None) } /// Append a note with a source code span to the diagnostic. - pub fn with_span_note(&mut self, span: SrcCodeSpan, message: impl Into) -> &mut Self { + pub fn with_span_note(&mut self, span: Span, message: impl Into) -> &mut Self { self.with_child(message, Severity::Note, Some(span)) } @@ -423,7 +474,12 @@ pub trait Diagnostics: HasDiagnosticsBase { /// /// This will fail the policy. fn struct_error(&self, msg: impl Into) -> DiagnosticBuilder<'_, Self> { - DiagnosticBuilder::init(msg.into(), Severity::Error, None, self) + DiagnosticBuilder::init( + msg.into(), + Severity::Error, + Option::::None, + self, + ) } /// Initialize a diagnostic builder for an error with a source code span. @@ -431,7 +487,7 @@ pub trait Diagnostics: HasDiagnosticsBase { /// This will fail the policy. fn struct_span_error( &self, - span: SrcCodeSpan, + span: impl Into, msg: impl Into, ) -> DiagnosticBuilder<'_, Self> { DiagnosticBuilder::init(msg.into(), Severity::Error, Some(span), self) @@ -441,7 +497,12 @@ pub trait Diagnostics: HasDiagnosticsBase { /// /// Does not fail the policy. fn struct_warning(&self, msg: impl Into) -> DiagnosticBuilder<'_, Self> { - DiagnosticBuilder::init(msg.into(), Severity::Warning, None, self) + DiagnosticBuilder::init( + msg.into(), + Severity::Warning, + Option::::None, + self, + ) } /// Initialize a diagnostic builder for a warning with a source code span @@ -449,7 +510,7 @@ pub trait Diagnostics: HasDiagnosticsBase { /// Does not fail the policy. fn struct_span_warning( &self, - span: SrcCodeSpan, + span: impl Into, msg: impl Into, ) -> DiagnosticBuilder<'_, Self> { DiagnosticBuilder::init(msg.into(), Severity::Warning, Some(span), self) @@ -457,13 +518,18 @@ pub trait Diagnostics: HasDiagnosticsBase { /// Initialize a diagnostic builder for a help message. fn struct_help(&self, msg: impl Into) -> DiagnosticBuilder<'_, Self> { - DiagnosticBuilder::init(msg.into(), Severity::Help, None, self) + DiagnosticBuilder::init( + msg.into(), + Severity::Help, + Option::::None, + self, + ) } /// Initialize a diagnostic builder for a help message with a source code span fn struct_span_help( &self, - span: SrcCodeSpan, + span: impl Into, msg: impl Into, ) -> DiagnosticBuilder<'_, Self> { DiagnosticBuilder::init(msg.into(), Severity::Help, Some(span), self) @@ -471,13 +537,18 @@ pub trait Diagnostics: HasDiagnosticsBase { /// Initialize a diagnostic builder for a note fn struct_note(&self, msg: impl Into) -> DiagnosticBuilder<'_, Self> { - DiagnosticBuilder::init(msg.into(), Severity::Note, None, self) + DiagnosticBuilder::init( + msg.into(), + Severity::Note, + Option::::None, + self, + ) } /// Initialize a diagnostic builder for a note with a source code span fn struct_span_note( &self, - span: SrcCodeSpan, + span: impl Into, msg: impl Into, ) -> DiagnosticBuilder<'_, Self> { DiagnosticBuilder::init(msg.into(), Severity::Note, Some(span), self) @@ -506,23 +577,23 @@ pub trait Diagnostics: HasDiagnosticsBase { /// Emit a message that is severe enough that it causes the policy to fail /// with a source code span. - fn span_error(&self, msg: impl Into, span: SrcCodeSpan) { + fn span_error(&self, msg: impl Into, span: Span) { self.struct_span_error(span, msg).emit() } /// Emit a message that indicates to the user that the policy might be /// fraudulent but could be correct. Includes a source code span. - fn span_warning(&self, msg: impl Into, span: SrcCodeSpan) { + fn span_warning(&self, msg: impl Into, span: Span) { self.struct_span_warning(span, msg).emit() } /// Emit a message that provides additional information to the user. - fn span_note(&self, msg: impl Into, span: SrcCodeSpan) { + fn span_note(&self, msg: impl Into, span: Span) { self.struct_span_note(span, msg).emit() } /// Emit a message that suggests something to the user. - fn span_help(&self, msg: impl Into, span: SrcCodeSpan) { + fn span_help(&self, msg: impl Into, span: Span) { self.struct_span_help(span, msg).emit() } @@ -598,7 +669,13 @@ fn struct_node_diagnostic( severity: Severity, msg: impl Into, ) -> DiagnosticBuilder<'_, B> { - let span = base.as_ctx().get_location(node); + let node_span = base.as_ctx().get_location(node); + let stmt_span = &base.as_ctx().instruction_at_node(node).span; + let span = if stmt_span.contains(node_span) { + HighlightedSpan::new(stmt_span.clone(), node_span.start, node_span.end) + } else { + stmt_span.clone().into() + }; DiagnosticBuilder::init(msg.into(), severity, Some(span.clone()), base) } diff --git a/crates/paralegal-policy/src/test_utils.rs b/crates/paralegal-policy/src/test_utils.rs index 62a67fb3f9..9c0c76441f 100644 --- a/crates/paralegal-policy/src/test_utils.rs +++ b/crates/paralegal-policy/src/test_utils.rs @@ -51,7 +51,7 @@ fn is_at_function_call_with_name( let weight = ctrl.graph.node_weight(node).unwrap().at; let instruction = &ctx.desc().instruction_info[&weight.leaf()]; matches!( - instruction, + instruction.kind, InstructionKind::FunctionCall(call) if ctx.desc().def_info[&call.id].name == name ) diff --git a/crates/paralegal-spdg/src/dot.rs b/crates/paralegal-spdg/src/dot.rs index c3225b5057..e6c228f682 100644 --- a/crates/paralegal-spdg/src/dot.rs +++ b/crates/paralegal-spdg/src/dot.rs @@ -116,7 +116,7 @@ impl<'a, 'd> dot::Labeller<'a, CallString, GlobalEdge> for DotPrintableProgramDe write!(s, "{}|", self.format_call_string(*n))?; - match instruction { + match instruction.kind { InstructionKind::Statement => s.push('S'), InstructionKind::FunctionCall(function) => { let info = &self.spdg.def_info[&function.id]; diff --git a/crates/paralegal-spdg/src/lib.rs b/crates/paralegal-spdg/src/lib.rs index 0436eb54fa..5ba464d017 100644 --- a/crates/paralegal-spdg/src/lib.rs +++ b/crates/paralegal-spdg/src/lib.rs @@ -126,7 +126,7 @@ pub struct DefInfo { /// Kind of object pub kind: DefKind, /// Information about the span - pub src_info: SrcCodeSpan, + pub src_info: Span, } /// Similar to `DefKind` in rustc but *not the same*! @@ -172,19 +172,34 @@ impl SourceFileInfo { } } +/// A "point" within a source file. Used to compose and compare spans. +/// +/// NOTE: The ordering of this type must be such that if point "a" is earlier in +/// the file than "b", then "a" < "b". +#[derive(Clone, Copy, PartialEq, Eq, Serialize, Deserialize, Debug, PartialOrd, Ord)] +pub struct SpanCoord { + /// Line in the source file + pub line: u32, + /// Column of the line + pub col: u32, +} + /// Encodes a source code location #[derive(Clone, PartialEq, Eq, Serialize, Deserialize, Debug)] -pub struct SrcCodeSpan { +pub struct Span { /// Which file this comes from pub source_file: SourceFile, - /// The starting line of the location within the file (note: a one-based index) - pub start_line: usize, - /// The column of starting line that the location starts at within the file (note: a one-based index) - pub start_col: usize, - /// The ending line of the location within the file (note: a one-based index) - pub end_line: usize, - /// The column of ending line that the location ends at within the file (note: a one-based index) - pub end_col: usize, + /// Starting coordinates of the span + pub start: SpanCoord, + /// Ending coordinates of the span, + pub end: SpanCoord, +} + +impl Span { + /// Is `other` completely contained within `self` + pub fn contains(&self, other: &Self) -> bool { + self.source_file == other.source_file && self.start <= other.start && self.end >= other.end + } } /// Metadata on a function call. @@ -224,6 +239,15 @@ impl InstructionKind { } } +/// Information about an instruction represented in the PDG +#[derive(Clone, Debug, Serialize, Deserialize)] +pub struct InstructionInfo { + /// Classification of the instruction + pub kind: InstructionKind, + /// The source code span + pub span: Span, +} + /// information about each encountered type. pub type TypeInfoMap = HashMap; @@ -246,7 +270,7 @@ pub struct ProgramDescription { /// Metadata about the instructions that are executed at all program /// locations we know about. #[serde(with = "serde_map_via_vec")] - pub instruction_info: HashMap, + pub instruction_info: HashMap, #[cfg_attr(not(feature = "rustc"), serde(with = "serde_map_via_vec"))] #[cfg_attr(feature = "rustc", serde(with = "ser_defid_map"))] @@ -589,7 +613,7 @@ pub struct NodeInfo { /// Additional information of how this node is used in the source. pub kind: NodeKind, /// Span information for this node - pub span: SrcCodeSpan, + pub span: Span, } impl Display for NodeInfo { From 48345ca1ec9a06b50b7b48f1c5c929a6018cf67b Mon Sep 17 00:00:00 2001 From: Justus Adam Date: Mon, 4 Mar 2024 12:17:47 -0500 Subject: [PATCH 20/20] Subspans for subdiagnostics --- crates/paralegal-policy/src/diagnostics.rs | 43 ++++++++++++++-------- 1 file changed, 28 insertions(+), 15 deletions(-) diff --git a/crates/paralegal-policy/src/diagnostics.rs b/crates/paralegal-policy/src/diagnostics.rs index 5e61081c12..35aba31b92 100644 --- a/crates/paralegal-policy/src/diagnostics.rs +++ b/crates/paralegal-policy/src/diagnostics.rs @@ -380,8 +380,7 @@ impl<'a, A: HasDiagnosticsBase + ?Sized> DiagnosticBuilder<'a, A> { /// Append a help message and the span of a graph node to the diagnostic. pub fn with_node_help(&mut self, node: GlobalNode, message: impl Into) -> &mut Self { - let span = self.base.as_ctx().get_location(node).clone(); - self.with_child(message, Severity::Help, Some(span)) + self.with_node(Severity::Help, node, message.into()) } /// Append a warning to the diagnostic. @@ -396,8 +395,7 @@ impl<'a, A: HasDiagnosticsBase + ?Sized> DiagnosticBuilder<'a, A> { /// Append a warning with a source code span to the diagnostic. pub fn with_node_warning(&mut self, node: GlobalNode, message: impl Into) -> &mut Self { - let span = self.base.as_ctx().get_location(node).clone(); - self.with_child(message, Severity::Warning, Some(span)) + self.with_node(Severity::Warning, node, message.into()) } /// Append a note to the diagnostic. @@ -407,13 +405,20 @@ impl<'a, A: HasDiagnosticsBase + ?Sized> DiagnosticBuilder<'a, A> { /// Append a note with a source code span to the diagnostic. pub fn with_span_note(&mut self, span: Span, message: impl Into) -> &mut Self { - self.with_child(message, Severity::Note, Some(span)) + self.with_child(message.into(), Severity::Note, Some(span)) } /// Append a note and the span of a graph node to the diagnostic. pub fn with_node_note(&mut self, node: GlobalNode, message: impl Into) -> &mut Self { - let span = self.base.as_ctx().get_location(node).clone(); - self.with_child(message, Severity::Note, Some(span)) + self.with_node(Severity::Note, node, message.into()) + } + + fn with_node(&mut self, severity: Severity, node: GlobalNode, message: String) -> &mut Self { + self.with_child( + message, + severity, + Some(highlighted_node_span(self.base.as_ctx(), node)), + ) } } @@ -663,20 +668,28 @@ pub trait Diagnostics: HasDiagnosticsBase { } } +fn highlighted_node_span(ctx: &Context, node: GlobalNode) -> HighlightedSpan { + let node_span = ctx.get_location(node); + let stmt_span = &ctx.instruction_at_node(node).span; + if stmt_span.contains(node_span) { + HighlightedSpan::new(stmt_span.clone(), node_span.start, node_span.end) + } else { + stmt_span.clone().into() + } +} + fn struct_node_diagnostic( base: &B, node: GlobalNode, severity: Severity, msg: impl Into, ) -> DiagnosticBuilder<'_, B> { - let node_span = base.as_ctx().get_location(node); - let stmt_span = &base.as_ctx().instruction_at_node(node).span; - let span = if stmt_span.contains(node_span) { - HighlightedSpan::new(stmt_span.clone(), node_span.start, node_span.end) - } else { - stmt_span.clone().into() - }; - DiagnosticBuilder::init(msg.into(), severity, Some(span.clone()), base) + DiagnosticBuilder::init( + msg.into(), + severity, + Some(highlighted_node_span(base.as_ctx(), node)), + base, + ) } const TAB_SIZE: usize = 4;