From 3443fb5612d1697e6c2642697d42d239654cc038 Mon Sep 17 00:00:00 2001 From: gavinleroy Date: Thu, 15 Aug 2024 16:53:04 -0400 Subject: [PATCH] Filter ambiguities when an error exists, shorten expression snippet for method calls --- crates/argus/src/analysis/entry.rs | 10 +- crates/argus/src/analysis/hir.rs | 17 ++-- crates/argus/src/analysis/transform.rs | 37 ++++---- crates/argus/src/tls.rs | 10 +- ide/packages/common/src/BodyInfo.ts | 91 +++++++++++-------- ide/packages/common/src/func.ts | 20 +++- ide/packages/panoptes/src/Expr.tsx | 6 +- ide/packages/panoptes/src/File.tsx | 5 +- ide/packages/panoptes/src/Obligation.tsx | 8 +- .../panoptes/src/TreeView/Wrappers.css | 2 +- 10 files changed, 121 insertions(+), 85 deletions(-) diff --git a/crates/argus/src/analysis/entry.rs b/crates/argus/src/analysis/entry.rs index 7a410aa..e3f65b8 100644 --- a/crates/argus/src/analysis/entry.rs +++ b/crates/argus/src/analysis/entry.rs @@ -66,9 +66,9 @@ pub fn process_obligation<'tcx>( // the predicate implies the previous. This is necessary because we // can't (currently) distinguish between a subsequent solving attempt // of a previous obligation. - if result.is_yes() || result.is_no() { - tls::drain_implied_ambiguities(infcx, obl); - } + // if result.is_yes() || result.is_no() { + // tls::drain_implied_ambiguities(infcx, obl); + // } if !INCLUDE_SUCCESSES.copied().unwrap_or(false) && result.is_yes() { log::debug!("Skipping successful obligation {obl:?}"); @@ -219,13 +219,13 @@ pub(in crate::analysis) fn build_obligations_in_body<'tcx>( // so as a first heuristic, if the body isn't tainted by errors, we'll just remove // all non-successful obligations. if typeck_results.tainted_by_errors.is_none() { - log::info!( + log::debug!( "Removing failures: Body not-tainted {:?}", typeck_results.hir_owner ); obligations.retain(|prov| prov.it.result.is_yes()); } else { - log::info!("Body tainted! {:?}", typeck_results.hir_owner); + log::debug!("Body tainted! {:?}", typeck_results.hir_owner); } let ctx = ErrorAssemblyCtx { diff --git a/crates/argus/src/analysis/hir.rs b/crates/argus/src/analysis/hir.rs index 9a519a7..3a9423e 100644 --- a/crates/argus/src/analysis/hir.rs +++ b/crates/argus/src/analysis/hir.rs @@ -48,6 +48,8 @@ fn bin_expressions( }; binner.visit_body(ctx.tcx.hir().body(ctx.body_id)); + + // Add remaining miscelanous unbinned obligations let mut bins = binner.bins; for (hir_id, obligations) in map { bins.push(Bin { @@ -65,14 +67,13 @@ pub enum BinKind { CallableExpr, CallArg, Call, - // MethodCall, - // MethodReceiver, Misc, } pub struct Bin { pub hir_id: HirId, - // TODO: use IndexVec for obligations. + // TODO: use IndexVec for obligations instead of the-- + // // usize indexes into the obligation vec pub obligations: Vec, pub kind: BinKind, @@ -158,21 +159,21 @@ pub fn find_most_enclosing_node( .map(|t| t.0) } -// NOTE: this probably needs to be expanded to account for all nodes, not just expressions. +/// Visitor for finding a `HirId` given a span. +/// +/// Code taken from rustc_trait_selection::traits::error_reporting and modified +/// to find items that enclose the span, not just match it exactly. struct FindNodeBySpan { pub span: Span, pub result: Option<(HirId, Span)>, } -// Code taken from rustc_trait_selection::traits::error_reporting, -// modified to find items that enclose the span, not just match it -// exactly. -// TODO: this should work on all nodes, not just expressions. impl FindNodeBySpan { pub fn new(span: Span) -> Self { Self { span, result: None } } + /// Is span `s` a closer match than the current best? fn is_better_match(&self, s: Span) -> bool { s.overlaps(self.span) && match self.result { diff --git a/crates/argus/src/analysis/transform.rs b/crates/argus/src/analysis/transform.rs index b85d60d..9cc6bf2 100644 --- a/crates/argus/src/analysis/transform.rs +++ b/crates/argus/src/analysis/transform.rs @@ -8,7 +8,7 @@ use argus_ext::{ use index_vec::IndexVec; use indexmap::IndexSet; use rustc_data_structures::fx::{FxHashMap as HashMap, FxIndexMap}; -use rustc_hir::{BodyId, HirId}; +use rustc_hir::{self as hir, intravisit::Map, BodyId, HirId}; use rustc_infer::{infer::InferCtxt, traits::PredicateObligation}; use rustc_middle::ty::{TyCtxt, TypeckResults}; use rustc_span::Span; @@ -182,6 +182,17 @@ impl<'a, 'tcx: 'a> ObligationsBuilder<'a, 'tcx> { span.sanitized_snippet(source_map) } + fn hir_id_to_span(&self, hir_id: HirId) -> Span { + let hir = self.tcx.hir(); + match hir.hir_node(hir_id) { + hir::Node::Expr(hir::Expr { + kind: hir::ExprKind::MethodCall(_, _, _, span), + .. + }) => self.to_local(*span), + _ => self.to_local(hir.span_with_body(hir_id)), + } + } + fn sort_bins(&mut self, bins: Vec) { use ExprKind as EK; @@ -193,7 +204,7 @@ impl<'a, 'tcx: 'a> ObligationsBuilder<'a, 'tcx> { mut obligations, kind, } = bin; - let span = self.to_local(hir.span_with_body(hir_id)); + let span = self.hir_id_to_span(hir_id); let snippet = self.local_snip(span); let Ok(range) = CharRange::from_span(span, source_map) else { log::error!( @@ -286,9 +297,7 @@ impl<'a, 'tcx: 'a> ObligationsBuilder<'a, 'tcx> { let uoidx = prov.full_data?; let full_data = self.full_data.get(uoidx); tree_search::tree_contains_in_branchless( - // something &full_data.infcx, - // something &full_data.obligation, needle, ) @@ -297,15 +306,7 @@ impl<'a, 'tcx: 'a> ObligationsBuilder<'a, 'tcx> { } fn relate_trait_bounds(&mut self) { - for (&span, predicates) in self.reported_trait_errors { - let span = self.to_local(span); - let range = CharRange::from_span(span, self.tcx.sess.source_map()) - .expect("failed to get range for reported trait error"); - - log::debug!( - "Relating trait bounds:\nrange {range:?}\nspan: {span:?}\n{predicates:#?}" - ); - + for (error_span, predicates) in self.reported_trait_errors { // Search for the obligation hash in our set of computed obligations. let predicates = predicates .iter() @@ -334,18 +335,20 @@ impl<'a, 'tcx: 'a> ObligationsBuilder<'a, 'tcx> { self.trait_errors.push(TraitError { idx: expr_id, - range, + range: self.exprs[expr_id].range.clone(), hashes, }); continue; } - log::error!("failed to find root expression for {span:?} {predicates:?}"); + log::error!( + "failed to find root expression for {error_span:?} {predicates:?}" + ); // A predicate did not match exactly, now we're scrambling // to find an expression by span, and pick an obligation. let Some(err_hir_id) = - hier_hir::find_most_enclosing_node(self.tcx, self.body_id, span) + hier_hir::find_most_enclosing_node(self.tcx, self.body_id, *error_span) else { log::error!("reported error doesn't have an associated span ..."); continue; @@ -377,7 +380,7 @@ impl<'a, 'tcx: 'a> ObligationsBuilder<'a, 'tcx> { // Mark the found Expr as containing an error. self.trait_errors.push(TraitError { idx: *expr_id, - range, + range: self.exprs[*expr_id].range.clone(), hashes: vec![], }); } diff --git a/crates/argus/src/tls.rs b/crates/argus/src/tls.rs index 6a340a6..cfb6b39 100644 --- a/crates/argus/src/tls.rs +++ b/crates/argus/src/tls.rs @@ -29,8 +29,6 @@ const DRAIN_WINDOW: usize = 100; // NOTE: we use thread local storage to accumulate obligations // accross call to the obligation inspector in `typeck_inspect`. // DO NOT set this directly, make sure to use the function `push_obligaion`. -// -// TODO: documentation thread_local! { static BODY_DEF_PATH: RefCell> = RefCell::default(); @@ -49,15 +47,17 @@ pub fn store_obligation(obl: Provenance) { }); } -// TODO: using `infcx` for error implication panics, but using +// FIXME: using `infcx` for error implication panics, but using // stored contexts doesn't. Investigate why, as this certainly // isn't a "solution." +// +// NOTE this causes an internal compiler error that *should not happen*, it's setting +// tainted by errors to be true when it shouldn't. Disabling for now. +#[allow(unused)] pub fn drain_implied_ambiguities<'tcx>( _infcx: &InferCtxt<'tcx>, obligation: &PredicateObligation<'tcx>, ) { - // FIXME: this causes an internal compiler error that *should not happen*, it's setting - // tainted by errors to be true when it shouldn't. return; OBLIGATIONS.with(|obls| { diff --git a/ide/packages/common/src/BodyInfo.ts b/ide/packages/common/src/BodyInfo.ts index 72ddbb6..1d7cad1 100644 --- a/ide/packages/common/src/BodyInfo.ts +++ b/ide/packages/common/src/BodyInfo.ts @@ -5,18 +5,29 @@ import type { Expr, ExprIdx, Obligation, - ObligationHash, ObligationIdx, ObligationsInBody } from "./bindings"; -import { isHiddenObl } from "./func"; +import { isVisibleObligation } from "./func"; class BodyInfo { + private existsImportantFailure; + constructor( private readonly oib: ObligationsInBody, - readonly idx: number, public readonly showHidden: boolean - ) {} + ) { + // An important failure is a *necessary* and *failing* obligation. We say that + // there exists an important failure if any of the expressions has an obligation + // that meets this criteria. + this.existsImportantFailure = false; + this.existsImportantFailure = _.some(this.exprs(), eidx => + _.some(this.obligations(eidx), oidx => { + const o = this.obligation(oidx)!; + return o.result === "no" && o.necessity === "Yes"; + }) + ); + } get hash(): BodyHash { return this.oib.hash; @@ -46,44 +57,21 @@ class BodyInfo { return this.oib.tys; } - notHidden(hash: ObligationIdx): boolean { - const o = this.getObligation(hash); - if (o === undefined) { - return false; - } - return this.showHidden || isHiddenObl(o); - } - exprs(): ExprIdx[] { - return _.map(this.oib.exprs, (_, idx) => idx); - } - - hasVisibleExprs() { - return _.some(this.exprs(), idx => this.hasVisibleObligations(idx)); - } - - hasVisibleObligations(idx: ExprIdx) { - return _.some(this.oib.exprs[idx].obligations, i => this.notHidden(i)); + return _.range(0, this.oib.exprs.length); } - byHash(hash: ObligationHash): Obligation | undefined { - return this.oib.obligations.find(o => o.hash === hash); + expr(idx: ExprIdx): Expr | undefined { + return this.oib.exprs[idx]; } - getObligation(idx: ObligationIdx): Obligation { - return this.oib.obligations[idx]; + private visibleObligations(idx: ExprIdx): ObligationIdx[] { + return _.filter(this.oib.exprs[idx].obligations, i => this.isVisible(i)); } - getExpr(idx: ExprIdx): Expr { - return this.oib.exprs[idx]; - } - - visibleObligations(idx: ExprIdx): ObligationIdx[] { - const filtered = _.filter(this.oib.exprs[idx].obligations, i => - this.notHidden(i) - ); - const sorted = _.sortBy(filtered, i => { - switch (this.getObligation(i).result) { + obligations(idx: ExprIdx): ObligationIdx[] { + return _.sortBy(this.visibleObligations(idx), i => { + switch (this.obligation(i)!.result) { case "no": return 0; case "yes": @@ -92,7 +80,38 @@ class BodyInfo { return 1; } }); - return sorted; + } + + obligation(idx: ObligationIdx): Obligation | undefined { + return this.oib.obligations[idx]; + } + + // Does this body have any expressions that have visible obligations? + hasVisibleExprs() { + return _.some(this.exprs(), idx => this.hasVisibleObligations(idx)); + } + + // Does the given expression have any visible obligations? + hasVisibleObligations(idx: ExprIdx) { + return this.visibleObligations(idx).length > 0; + } + + // Is the given obligation visible? + isVisible(idx: ObligationIdx) { + const o = this.obligation(idx); + if (o === undefined) return false; + + return ( + this.showHidden || + isVisibleObligation( + o, + // HACK: If there is a failing obligation, we filter ambiguities. This is + // a short workaround for a backend incompleteness. We can't filter obligations + // that get resolved in a second round of trait solving, this leaves Argus with + // more "failures" than rustc shows. + this.existsImportantFailure + ) + ); } } diff --git a/ide/packages/common/src/func.ts b/ide/packages/common/src/func.ts index ae81a5e..98ab8c7 100644 --- a/ide/packages/common/src/func.ts +++ b/ide/packages/common/src/func.ts @@ -5,8 +5,10 @@ import type { BoundTyKind, BoundVariableKind, CharRange, + EvaluationResult, GenericArg, ObligationHash, + ObligationNecessity, Predicate, Region, Ty, @@ -65,11 +67,19 @@ export function makeHighlightPosters( return [addHighlight, removeHighlight]; } -export function isHiddenObl(o: { necessity: string; result: string }) { - return ( - o.necessity === "Yes" || (o.necessity === "OnError" && o.result === "no") - ); -} +export const isVisibleObligation = ( + o: { necessity: ObligationNecessity; result: EvaluationResult }, + filterAmbiguities = false +) => + // Short-circuit ambiguities if we're filtering them + !( + (o.result === "maybe-ambiguity" || o.result === "maybe-overflow") && + filterAmbiguities + ) && + // If the obligation is listed as necessary, it's visible + (o.necessity === "Yes" || + // If the obligation is listed as necessary on error, and it failed, it's visible + (o.necessity === "OnError" && o.result === "no")); export function searchObject(obj: any, target: any) { for (let key in obj) { diff --git a/ide/packages/panoptes/src/Expr.tsx b/ide/packages/panoptes/src/Expr.tsx index 95099d2..850f4e2 100644 --- a/ide/packages/panoptes/src/Expr.tsx +++ b/ide/packages/panoptes/src/Expr.tsx @@ -16,7 +16,9 @@ import { HighlightTargetStore } from "./signals"; const Expr = observer(({ idx }: { idx: ExprIdx }) => { const bodyInfo = useContext(BodyInfoContext)!; const file = useContext(FileContext)!; - const expr = bodyInfo.getExpr(idx); + const expr = bodyInfo.expr(idx); + if (expr === undefined) return null; + const messageSystem = useContext(AppContext.MessageSystemContext)!; const [addHighlight, removeHighlight] = makeHighlightPosters( messageSystem, @@ -28,7 +30,7 @@ const Expr = observer(({ idx }: { idx: ExprIdx }) => { return null; } - const visibleObligations = bodyInfo.visibleObligations(idx); + const visibleObligations = bodyInfo.obligations(idx); if (visibleObligations.length === 0) { return null; } diff --git a/ide/packages/panoptes/src/File.tsx b/ide/packages/panoptes/src/File.tsx index 85fb21e..c7b5b5a 100644 --- a/ide/packages/panoptes/src/File.tsx +++ b/ide/packages/panoptes/src/File.tsx @@ -74,10 +74,7 @@ export interface FileProps { const File = ({ file, osibs }: FileProps) => { const showHidden = useContext(AppContext.ShowHiddenObligationsContext); - const bodyInfos = _.map( - osibs, - (osib, idx) => new BodyInfo(osib, idx, showHidden) - ); + const bodyInfos = _.map(osibs, osib => new BodyInfo(osib, showHidden)); const noBodiesFound = ( diff --git a/ide/packages/panoptes/src/Obligation.tsx b/ide/packages/panoptes/src/Obligation.tsx index 9a03c7f..a15f7da 100644 --- a/ide/packages/panoptes/src/Obligation.tsx +++ b/ide/packages/panoptes/src/Obligation.tsx @@ -28,13 +28,17 @@ import { HighlightTargetStore } from "./signals"; export const ObligationFromIdx = ({ idx }: { idx: ObligationIdx }) => { const bodyInfo = useContext(BodyInfoContext)!; - const o = bodyInfo.getObligation(idx); + const o = bodyInfo.obligation(idx); + if (o === undefined) return null; + return ; }; export const ObligationResultFromIdx = ({ idx }: { idx: ObligationIdx }) => { const bodyInfo = useContext(BodyInfoContext)!; - const o = bodyInfo.getObligation(idx); + const o = bodyInfo.obligation(idx); + if (o === undefined) return null; + return ; }; diff --git a/ide/packages/panoptes/src/TreeView/Wrappers.css b/ide/packages/panoptes/src/TreeView/Wrappers.css index 2a73aa6..bffb6f7 100644 --- a/ide/packages/panoptes/src/TreeView/Wrappers.css +++ b/ide/packages/panoptes/src/TreeView/Wrappers.css @@ -37,7 +37,7 @@ icons to overflow the bounding box of the proof node labels TODO: why is this default necessary for the codicons, and how - come it breaks our bounding box here. CSS is a mystery. + come it breaks our bounding box here? CSS is a mystery. */ display: inline; }