Filter ambiguities when an error exists, shorten expression snippet f…

…or method calls

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 {

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<usize>,
   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 {

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<Bin>) {
     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![],
       });
     }

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<Option<serde_json::Value>> = RefCell::default();
 
@@ -49,15 +47,17 @@ pub fn store_obligation(obl: Provenance<Obligation>) {
   });
 }
 
-// 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| {

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
+      )
+    );
   }
 }
 

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) {

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;
   }