Auto merge of rust-lang#135272 - BoxyUwU:generic_arg_infer_reliability_2, r=compiler-errors

Forbid usage of `hir` `Infer` const/ty variants in ambiguous contexts

The feature `generic_arg_infer` allows providing `_` as an argument to const generics in order to infer them. This introduces a syntactic ambiguity as to whether generic arguments are type or const arguments. In order to get around this we introduced a fourth `GenericArg` variant, `Infer` used to represent `_` as an argument to generic parameters when we don't know if its a type or a const argument.

This made hir visitors that care about `TyKind::Infer` or `ConstArgKind::Infer` very error prone as checking for `TyKind::Infer`s in  `visit_ty` would find *some* type infer arguments but not *all* of them as they would sometimes be lowered to `GenericArg::Infer` instead.

Additionally the `visit_infer` method would previously only visit `GenericArg::Infer` not *all* infers (e.g. `TyKind::Infer`), this made it very easy to override `visit_infer` and expect it to visit all infers when in reality it would only visit *some* infers.

---

This PR aims to fix those issues by making the `TyKind` and `ConstArgKind` types generic over whether the infer types/consts are represented by `Ty/ConstArgKind::Infer` or out of line (e.g. by a `GenericArg::Infer` or accessible by overiding `visit_infer`). We then make HIR Visitors convert all const args and types to the versions where infer vars are stored out of line and call `visit_infer` in cases where a `Ty`/`Const` would previously have had a `Ty/ConstArgKind::Infer` variant:

API Summary
```rust
enum AmbigArg {}

enum Ty/ConstArgKind<Unambig = ()> {
   ...
   Infer(Unambig),
}

impl Ty/ConstArg {
  fn try_as_ambig_ty/ct(self) -> Option<Ty/ConstArg<AmbigArg>>;
}
impl Ty/ConstArg<AmbigArg> {
  fn as_unambig_ty/ct(self) -> Ty/ConstArg;
}

enum InferKind {
  Ty(Ty),
  Const(ConstArg),
  Ambig(InferArg),
}

trait Visitor {
  ...
  fn visit_ty/const_arg(&mut self, Ty/ConstArg<AmbigArg>) -> Self::Result;
  fn visit_infer(&mut self, id: HirId, sp: Span, kind: InferKind) -> Self::Result;
}

// blanket impl'd, not meant to be overriden
trait VisitorExt {
  fn visit_ty/const_arg_unambig(&mut self, Ty/ConstArg) -> Self::Result;
}

fn walk_unambig_ty/const_arg(&mut V, Ty/ConstArg) -> Self::Result;
fn walk_ty/const_arg(&mut V, Ty/ConstArg<AmbigArg>) -> Self::Result;
```

The end result is that `visit_infer` visits *all* infer args and is also the *only* way to visit an infer arg, `visit_ty` and `visit_const_arg` can now no longer encounter a `Ty/ConstArgKind::Infer`. Representing this in the type system means that it is now very difficult to mess things up, either accessing `TyKind::Infer` "just works" and you won't miss *some* type infers- or it doesn't work and you have to look at `visit_infer` or some `GenericArg::Infer` which forces you to think about the full complexity involved.

Unfortunately there is no lint right now about explicitly matching on uninhabited variants, I can't find the context for why this is the case 🤷‍♀️

I'm not convinced the framing of un/ambig ty/consts is necessarily the right one but I'm not sure what would be better. I somewhat like calling them full/partial types based on the fact that `Ty<Partial>`/`Ty<Full>` directly specifies how many of the type kinds are actually represented compared to `Ty<Ambig>` which which leaves that to the reader to figure out based on the logical consequences of it the type being in an ambiguous position.

---

tool changes have been modified in their own commits for easier reviewing by anyone getting cc'd from subtree changes. I also attempted to split out "bug fixes arising from the refactoring" into their own commit so they arent lumped in with a big general refactor commit

Fixes rust-lang#112110

Author: bors

compiler/rustc_ast/src/ast.rs

@@ -28,6 +28,7 @@ use rustc_data_structures::packed::Pu128;
 use rustc_data_structures::stable_hasher::{HashStable, StableHasher};
 use rustc_data_structures::stack::ensure_sufficient_stack;
 use rustc_data_structures::sync::Lrc;
+use rustc_data_structures::tagged_ptr::Tag;
 use rustc_macros::{Decodable, Encodable, HashStable_Generic};
 pub use rustc_span::AttrId;
 use rustc_span::source_map::{Spanned, respan};
@@ -287,6 +288,7 @@ impl ParenthesizedArgs {
     }
 }
 
+use crate::AstDeref;
 pub use crate::node_id::{CRATE_NODE_ID, DUMMY_NODE_ID, NodeId};
 
 /// Modifiers on a trait bound like `~const`, `?` and `!`.
@@ -2165,6 +2167,14 @@ impl Ty {
         }
         final_ty
     }
+
+    pub fn is_maybe_parenthesised_infer(&self) -> bool {
+        match &self.kind {
+            TyKind::Infer => true,
+            TyKind::Paren(inner) => inner.ast_deref().is_maybe_parenthesised_infer(),
+            _ => false,
+        }
+    }
 }
 
 #[derive(Clone, Encodable, Decodable, Debug)]
@@ -2269,10 +2279,32 @@ impl TyKind {
 
 /// Syntax used to declare a trait object.
 #[derive(Clone, Copy, PartialEq, Encodable, Decodable, Debug, HashStable_Generic)]
+#[repr(u8)]
 pub enum TraitObjectSyntax {
-    Dyn,
-    DynStar,
-    None,
+    // SAFETY: When adding new variants make sure to update the `Tag` impl.
+    Dyn = 0,
+    DynStar = 1,
+    None = 2,
+}
+
+/// SAFETY: `TraitObjectSyntax` only has 3 data-less variants which means
+/// it can be represented with a `u2`. We use `repr(u8)` to guarantee the
+/// discriminants of the variants are no greater than `3`.
+unsafe impl Tag for TraitObjectSyntax {
+    const BITS: u32 = 2;
+
+    fn into_usize(self) -> usize {
+        self as u8 as usize
+    }
+
+    unsafe fn from_usize(tag: usize) -> Self {
+        match tag {
+            0 => TraitObjectSyntax::Dyn,
+            1 => TraitObjectSyntax::DynStar,
+            2 => TraitObjectSyntax::None,
+            _ => unreachable!(),
+        }
+    }
 }
 
 #[derive(Clone, Encodable, Decodable, Debug)]

compiler/rustc_ast_lowering/src/index.rs

@@ -1,3 +1,4 @@
+use intravisit::InferKind;
 use rustc_data_structures::sorted_map::SortedMap;
 use rustc_hir as hir;
 use rustc_hir::def_id::{LocalDefId, LocalDefIdMap};
@@ -265,14 +266,6 @@ impl<'a, 'hir> Visitor<'hir> for NodeCollector<'a, 'hir> {
         });
     }
 
-    fn visit_const_arg(&mut self, const_arg: &'hir ConstArg<'hir>) {
-        self.insert(const_arg.span(), const_arg.hir_id, Node::ConstArg(const_arg));
-
-        self.with_parent(const_arg.hir_id, |this| {
-            intravisit::walk_const_arg(this, const_arg);
-        });
-    }
-
     fn visit_expr(&mut self, expr: &'hir Expr<'hir>) {
         self.insert(expr.span, expr.hir_id, Node::Expr(expr));
 
@@ -302,22 +295,41 @@ impl<'a, 'hir> Visitor<'hir> for NodeCollector<'a, 'hir> {
         intravisit::walk_path_segment(self, path_segment);
     }
 
-    fn visit_ty(&mut self, ty: &'hir Ty<'hir>) {
-        self.insert(ty.span, ty.hir_id, Node::Ty(ty));
+    fn visit_ty(&mut self, ty: &'hir Ty<'hir, AmbigArg>) {
+        self.insert(ty.span, ty.hir_id, Node::Ty(ty.as_unambig_ty()));
 
         self.with_parent(ty.hir_id, |this| {
             intravisit::walk_ty(this, ty);
         });
     }
 
-    fn visit_infer(&mut self, inf: &'hir InferArg) {
-        self.insert(inf.span, inf.hir_id, Node::Infer(inf));
+    fn visit_const_arg(&mut self, const_arg: &'hir ConstArg<'hir, AmbigArg>) {
+        self.insert(
+            const_arg.as_unambig_ct().span(),
+            const_arg.hir_id,
+            Node::ConstArg(const_arg.as_unambig_ct()),
+        );
 
-        self.with_parent(inf.hir_id, |this| {
-            intravisit::walk_inf(this, inf);
+        self.with_parent(const_arg.hir_id, |this| {
+            intravisit::walk_ambig_const_arg(this, const_arg);
         });
     }
 
+    fn visit_infer(
+        &mut self,
+        inf_id: HirId,
+        inf_span: Span,
+        kind: InferKind<'hir>,
+    ) -> Self::Result {
+        match kind {
+            InferKind::Ty(ty) => self.insert(inf_span, inf_id, Node::Ty(ty)),
+            InferKind::Const(ct) => self.insert(inf_span, inf_id, Node::ConstArg(ct)),
+            InferKind::Ambig(inf) => self.insert(inf_span, inf_id, Node::Infer(inf)),
+        }
+
+        self.visit_id(inf_id);
+    }
+
     fn visit_trait_ref(&mut self, tr: &'hir TraitRef<'hir>) {
         self.insert(tr.path.span, tr.hir_ref_id, Node::TraitRef(tr));
 

compiler/rustc_ast_lowering/src/lib.rs

@@ -48,6 +48,7 @@ use rustc_data_structures::fingerprint::Fingerprint;
 use rustc_data_structures::sorted_map::SortedMap;
 use rustc_data_structures::stable_hasher::{HashStable, StableHasher};
 use rustc_data_structures::sync::Lrc;
+use rustc_data_structures::tagged_ptr::TaggedRef;
 use rustc_errors::{DiagArgFromDisplay, DiagCtxtHandle, StashKey};
 use rustc_hir::def::{DefKind, LifetimeRes, Namespace, PartialRes, PerNS, Res};
 use rustc_hir::def_id::{CRATE_DEF_ID, LOCAL_CRATE, LocalDefId};
@@ -1083,17 +1084,22 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
         match arg {
             ast::GenericArg::Lifetime(lt) => GenericArg::Lifetime(self.lower_lifetime(lt)),
             ast::GenericArg::Type(ty) => {
+                // We cannot just match on `TyKind::Infer` as `(_)` is represented as
+                // `TyKind::Paren(TyKind::Infer)` and should also be lowered to `GenericArg::Infer`
+                if ty.is_maybe_parenthesised_infer() {
+                    return GenericArg::Infer(hir::InferArg {
+                        hir_id: self.lower_node_id(ty.id),
+                        span: self.lower_span(ty.span),
+                    });
+                }
+
                 match &ty.kind {
-                    TyKind::Infer if self.tcx.features().generic_arg_infer() => {
-                        return GenericArg::Infer(hir::InferArg {
-                            hir_id: self.lower_node_id(ty.id),
-                            span: self.lower_span(ty.span),
-                        });
-                    }
                     // We parse const arguments as path types as we cannot distinguish them during
                     // parsing. We try to resolve that ambiguity by attempting resolution in both the
                     // type and value namespaces. If we resolved the path in the value namespace, we
                     // transform it into a generic const argument.
+                    //
+                    // FIXME: Should we be handling `(PATH_TO_CONST)`?
                     TyKind::Path(None, path) => {
                         if let Some(res) = self
                             .resolver
@@ -1110,15 +1116,17 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
 
                                 let ct =
                                     self.lower_const_path_to_const_arg(path, res, ty.id, ty.span);
-                                return GenericArg::Const(ct);
+                                return GenericArg::Const(ct.try_as_ambig_ct().unwrap());
                             }
                         }
                     }
                     _ => {}
                 }
-                GenericArg::Type(self.lower_ty(ty, itctx))
+                GenericArg::Type(self.lower_ty(ty, itctx).try_as_ambig_ty().unwrap())
+            }
+            ast::GenericArg::Const(ct) => {
+                GenericArg::Const(self.lower_anon_const_to_const_arg(ct).try_as_ambig_ct().unwrap())
             }
-            ast::GenericArg::Const(ct) => GenericArg::Const(self.lower_anon_const_to_const_arg(ct)),
         }
     }
 
@@ -1158,7 +1166,10 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
                 let lifetime_bound = this.elided_dyn_bound(t.span);
                 (bounds, lifetime_bound)
             });
-            let kind = hir::TyKind::TraitObject(bounds, lifetime_bound, TraitObjectSyntax::None);
+            let kind = hir::TyKind::TraitObject(
+                bounds,
+                TaggedRef::new(lifetime_bound, TraitObjectSyntax::None),
+            );
             return hir::Ty { kind, span: self.lower_span(t.span), hir_id: self.next_id() };
         }
 
@@ -1185,7 +1196,7 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
 
     fn lower_ty_direct(&mut self, t: &Ty, itctx: ImplTraitContext) -> hir::Ty<'hir> {
         let kind = match &t.kind {
-            TyKind::Infer => hir::TyKind::Infer,
+            TyKind::Infer => hir::TyKind::Infer(()),
             TyKind::Err(guar) => hir::TyKind::Err(*guar),
             TyKind::Slice(ty) => hir::TyKind::Slice(self.lower_ty(ty, itctx)),
             TyKind::Ptr(mt) => hir::TyKind::Ptr(self.lower_mt(mt, itctx)),
@@ -1309,7 +1320,7 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
                         lifetime_bound.unwrap_or_else(|| this.elided_dyn_bound(t.span));
                     (bounds, lifetime_bound)
                 });
-                hir::TyKind::TraitObject(bounds, lifetime_bound, *kind)
+                hir::TyKind::TraitObject(bounds, TaggedRef::new(lifetime_bound, *kind))
             }
             TyKind::ImplTrait(def_node_id, bounds) => {
                 let span = t.span;
@@ -2041,7 +2052,7 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
                     )
                     .stash(c.value.span, StashKey::UnderscoreForArrayLengths);
                 }
-                let ct_kind = hir::ConstArgKind::Infer(self.lower_span(c.value.span));
+                let ct_kind = hir::ConstArgKind::Infer(self.lower_span(c.value.span), ());
                 self.arena.alloc(hir::ConstArg { hir_id: self.lower_node_id(c.id), kind: ct_kind })
             }
             _ => self.lower_anon_const_to_const_arg(c),
@@ -2365,8 +2376,7 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
                         hir_id = self.next_id();
                         hir::TyKind::TraitObject(
                             arena_vec![self; principal],
-                            self.elided_dyn_bound(span),
-                            TraitObjectSyntax::None,
+                            TaggedRef::new(self.elided_dyn_bound(span), TraitObjectSyntax::None),
                         )
                     }
                     _ => hir::TyKind::Path(hir::QPath::Resolved(None, path)),

compiler/rustc_ast_lowering/src/path.rs

@@ -525,7 +525,9 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
             }
             FnRetTy::Default(_) => self.arena.alloc(self.ty_tup(*span, &[])),
         };
-        let args = smallvec![GenericArg::Type(self.arena.alloc(self.ty_tup(*inputs_span, inputs)))];
+        let args = smallvec![GenericArg::Type(
+            self.arena.alloc(self.ty_tup(*inputs_span, inputs)).try_as_ambig_ty().unwrap()
+        )];
 
         // If we have a bound like `async Fn() -> T`, make sure that we mark the
         // `Output = T` associated type bound with the right feature gates.

compiler/rustc_borrowck/src/diagnostics/region_errors.rs

@@ -8,7 +8,7 @@ use rustc_hir::QPath::Resolved;
 use rustc_hir::WherePredicateKind::BoundPredicate;
 use rustc_hir::def::Res::Def;
 use rustc_hir::def_id::DefId;
-use rustc_hir::intravisit::Visitor;
+use rustc_hir::intravisit::VisitorExt;
 use rustc_hir::{PolyTraitRef, TyKind, WhereBoundPredicate};
 use rustc_infer::infer::{NllRegionVariableOrigin, RelateParamBound};
 use rustc_middle::bug;
@@ -887,7 +887,7 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, 'infcx, 'tcx> {
                 if alias_ty.span.desugaring_kind().is_some() {
                     // Skip `async` desugaring `impl Future`.
                 }
-                if let TyKind::TraitObject(_, lt, _) = alias_ty.kind {
+                if let TyKind::TraitObject(_, lt) = alias_ty.kind {
                     if lt.ident.name == kw::Empty {
                         spans_suggs.push((lt.ident.span.shrink_to_hi(), " + 'a".to_string()));
                     } else {
@@ -987,7 +987,7 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, 'infcx, 'tcx> {
         for found_did in found_dids {
             let mut traits = vec![];
             let mut hir_v = HirTraitObjectVisitor(&mut traits, *found_did);
-            hir_v.visit_ty(self_ty);
+            hir_v.visit_ty_unambig(self_ty);
             debug!("trait spans found: {:?}", traits);
             for span in &traits {
                 let mut multi_span: MultiSpan = vec![*span].into();

compiler/rustc_borrowck/src/diagnostics/region_name.rs

@@ -432,7 +432,7 @@ impl<'tcx> MirBorrowckCtxt<'_, '_, 'tcx> {
             // must highlight the variable.
             // NOTE(eddyb) this is handled in/by the sole caller
             // (`give_name_if_anonymous_region_appears_in_arguments`).
-            hir::TyKind::Infer => None,
+            hir::TyKind::Infer(()) => None,
 
             _ => Some(argument_hir_ty),
         }
@@ -615,7 +615,7 @@ impl<'tcx> MirBorrowckCtxt<'_, '_, 'tcx> {
                 }
 
                 (GenericArgKind::Type(ty), hir::GenericArg::Type(hir_ty)) => {
-                    search_stack.push((ty, hir_ty));
+                    search_stack.push((ty, hir_ty.as_unambig_ty()));
                 }
 
                 (GenericArgKind::Const(_ct), hir::GenericArg::Const(_hir_ct)) => {