Skip to content

[WIP] Fix compiler regression from 1.64.0 relating to &(dyn Trait + '_) params #104272

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed

[WIP] Fix compiler regression from 1.64.0 relating to &(dyn Trait + '_) params #104272

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8834cfc
Use tracing::instrument in more places in rustc_resolve::late.
BGR360 Nov 11, 2022
8636dd4
First attempt at fixing compiler regression.
BGR360 Nov 11, 2022
fb3c318
Introduce LifetimeRes::ImplicitObjectLifetimeDefault to deal with `+ …
BGR360 Nov 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
111 changes: 85 additions & 26 deletions compiler/rustc_resolve/src/late.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -584,6 +584,12 @@ struct LateResolutionVisitor<'a, 'b, 'ast> {
/// If it is `true`, then it will be updated when entering a nested function or trait body.
in_func_body: bool,

/// State used to know whether we are currently resolving params for a
/// function or closure.
///
/// Notably, rustc uses this to allow `dyn Trait + '_` in certain cases.
in_func_params: bool,

/// Count the number of places a lifetime is used.
lifetime_uses: FxHashMap<LocalDefId, LifetimeUseSet>,
}
Expand Down Expand Up @@ -633,6 +639,7 @@ impl<'a: 'ast, 'ast> Visitor<'ast> for LateResolutionVisitor<'a, '_, 'ast> {
self.resolve_local(local);
self.diagnostic_metadata.current_let_binding = original;
}
#[instrument(level = "trace", skip_all, fields(?ty.kind))]
fn visit_ty(&mut self, ty: &'ast Ty) {
let prev = self.diagnostic_metadata.current_trait_object;
let prev_ty = self.diagnostic_metadata.current_type_path;
Expand Down Expand Up @@ -922,6 +929,16 @@ impl<'a: 'ast, 'ast> Visitor<'ast> for LateResolutionVisitor<'a, '_, 'ast> {
fn visit_lifetime(&mut self, lifetime: &'ast Lifetime, use_ctxt: visit::LifetimeCtxt) {
self.resolve_lifetime(lifetime, use_ctxt)
}
fn visit_param_bound(&mut self, bound: &'ast GenericBound, bound_kind: BoundKind) {
match (bound_kind, bound) {
(BoundKind::TraitObject, GenericBound::Outlives(lifetime @ Lifetime { ident, .. }))
if ident.name == kw::UnderscoreLifetime =>
{
self.resolve_anonymous_lifetime_in_trait_object_bounds(lifetime);
}
_ => visit::walk_param_bound(self, bound),
}
}

fn visit_generics(&mut self, generics: &'ast Generics) {
self.visit_generic_params(
Expand All @@ -945,8 +962,8 @@ impl<'a: 'ast, 'ast> Visitor<'ast> for LateResolutionVisitor<'a, '_, 'ast> {
}
}

#[instrument(level = "debug", skip(self))]
fn visit_generic_arg(&mut self, arg: &'ast GenericArg) {
debug!("visit_generic_arg({:?})", arg);
let prev = replace(&mut self.diagnostic_metadata.currently_processing_generics, true);
match arg {
GenericArg::Type(ref ty) => {
Expand Down Expand Up @@ -1072,8 +1089,8 @@ impl<'a: 'ast, 'ast> Visitor<'ast> for LateResolutionVisitor<'a, '_, 'ast> {
}
}

#[instrument(level = "debug", skip(self))]
fn visit_where_predicate(&mut self, p: &'ast WherePredicate) {
debug!("visit_where_predicate {:?}", p);
let previous_value =
replace(&mut self.diagnostic_metadata.current_where_predicate, Some(p));
self.with_lifetime_rib(LifetimeRibKind::AnonymousReportError, |this| {
Expand Down Expand Up @@ -1172,6 +1189,7 @@ impl<'a: 'ast, 'b, 'ast> LateResolutionVisitor<'a, 'b, 'ast> {
diagnostic_metadata: Box::new(DiagnosticMetadata::default()),
// errors at module scope should always be reported
in_func_body: false,
in_func_params: false,
lifetime_uses: Default::default(),
}
}
Expand Down Expand Up @@ -1787,6 +1805,32 @@ impl<'a: 'ast, 'b, 'ast> LateResolutionVisitor<'a, 'b, 'ast> {
}
}

/// For backwards compatibility, we need to accept the following:
///
/// ```
/// trait AsStr {
/// fn as_str(&self) -> &str;
/// }
///
/// fn foo(a: &(dyn AsStr + '_)) -> &str { a.as_str() }
/// ```
///
/// When we're resolving function parameters, we usually want each elided or
/// anonymous lifetime to result in a new elision candidate, but not in this case.
#[instrument(level = "trace", skip(self))]
fn resolve_anonymous_lifetime_in_trait_object_bounds(&mut self, lifetime: &Lifetime) {
debug_assert_eq!(lifetime.ident.name, kw::UnderscoreLifetime);
debug_assert!(self.diagnostic_metadata.current_trait_object.is_some());

if self.in_func_params {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is likely to be incorrect in some corner cases. Could you:

  • iterate on self.lifetime_ribs like resolve_anonymous_lifetime does,
  • check LifetimeRibKind::Fresh?

Is the behaviour restricted to function parameters, or anywhere we have a Fresh rib?
What should impl &(dyn Trait + '_) { /**/ } mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

impl &(dyn Trait + '_) { ... } isn't allowed:

error[E0390]: cannot define inherent `impl` for primitive types
  --> /Users/ben/Documents/Rust/rust-lang/rust/src/test/ui/lifetimes/issue-103762-1.rs:42:6
   |
LL | impl &(dyn Trait + '_) {
   |      ^^^^^^^^^^^^^^^^^
   |
   = help: consider using an extension trait instead
   = note: you could also try moving the reference to uses of `dyn Trait` (such as `self`) within the implementation

I can believe that this way of doing it is brittle. Can you try to think of any more edge cases? I'm fresh out of ideas at the moment.

I'll try out your strategy of checking for the presence of the rib. Though I think you probably meant LifetimeRibKind::AnonymousCreateParameter rather than LifetimeRibKind::Fresh?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

impl Trait for &(dyn Trait2 + '_) {} is allowed.

If you want corner cases:

fn foo(_: fn(&(dyn Trait + '_)) -> &(dyn Trait + '_)) {}
fn bar(_: [u8; { { fn blah() -> &(dyn Trait + '_) {} }; 5 }]) {}

I'll try out your strategy of checking for the presence of the rib. Though I think you probably meant LifetimeRibKind::AnonymousCreateParameter rather than LifetimeRibKind::Fresh?

Exactly.

self.with_lifetime_rib(LifetimeRibKind::Elided(LifetimeRes::Infer), |this| {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The resolution is not Infer. In HIR, we should get an ImplicitObjectLifetimeDefault, but I did not create such variant in LifetimeRes. Could you create an extra ImplicitObjectLifetimeDefault variant in LifetimeRes and adapt lowering to translate it to LifetimeName::ImplicitObjectLifetimeDefault?

Infer means that type-checking will try to find the correct lifetime through type-inference. This is typically what appears in function bodies. ImplicitObjectLifetimeDefault means that we will inspect the surrounding type to apply the object lifetime rules: &'a (dyn Trait + '_) implicitly becomes &'a (dyn Trait + 'a) etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the pointer!

This does indeed get us past any errors in resolve and astconv, and all the tests I wrote compile as I had intended.

However, it seems ImplicitObjectLifetimeDefault results in the wrong lifetime for Box<dyn Trait + '_>, as this failed test shows:

// check-pass

struct Ctxt {
    v: usize,
}

trait GetCtxt {
    // Here the `&` is bound in the method definition:
    fn get_ctxt(&self) -> &Ctxt;
}

struct HasCtxt<'a> {
    c: &'a Ctxt,
}

impl<'a> GetCtxt for HasCtxt<'a> {
    // Ok: Have implied bound of WF(&'b HasCtxt<'a>)
    // so know 'a: 'b
    // so know &'a Ctxt <: &'b Ctxt
    fn get_ctxt<'b>(&'b self) -> &'a Ctxt {
        self.c
    }
}

fn get_v(gc: Box<dyn GetCtxt + '_>) -> usize {
    gc.get_ctxt().v
}

fn main() {
    let ctxt = Ctxt { v: 22 };
    let hc = HasCtxt { c: &ctxt };
    assert_eq!(get_v(Box::new(hc) as Box<dyn GetCtxt>), 22);
}
---- [ui] src/test/ui/regions/regions-trait-1.rs stdout ----

error: test compilation failed although it shouldn't!
status: exit status: 1
command: "/Users/ben/Documents/Rust/rust-lang/rust/build/aarch64-apple-darwin/stage1/bin/rustc" "/Users/ben/Documents/Rust/rust-lang/rust/src/test/ui/regions/regions-trait-1.rs" "-Zthreads=1" "--target=aarch64-apple-darwin" "--error-format" "json" "--json" "future-incompat" "-Ccodegen-units=1" "-Zui-testing" "-Zdeduplicate-diagnostics=no" "-Cstrip=debuginfo" "--emit" "metadata" "-C" "prefer-dynamic" "--out-dir" "/Users/ben/Documents/Rust/rust-lang/rust/build/aarch64-apple-darwin/test/ui/regions/regions-trait-1" "-A" "unused" "-Crpath" "-Cdebuginfo=0" "-Lnative=/Users/ben/Documents/Rust/rust-lang/rust/build/aarch64-apple-darwin/native/rust-test-helpers" "-L" "/Users/ben/Documents/Rust/rust-lang/rust/build/aarch64-apple-darwin/test/ui/regions/regions-trait-1/auxiliary"
stdout: none
--- stderr -------------------------------
error[E0597]: `ctxt` does not live long enough
  --> /Users/ben/Documents/Rust/rust-lang/rust/src/test/ui/regions/regions-trait-1.rs:31:27
   |
LL |     let hc = HasCtxt { c: &ctxt };
   |                           ^^^^^ borrowed value does not live long enough
LL |     assert_eq!(get_v(Box::new(hc) as Box<dyn GetCtxt>), 22);
   |                      ------------ cast requires that `ctxt` is borrowed for `'static`
LL | }
   | - `ctxt` dropped here while still borrowed

error: aborting due to previous error

For more information about this error, try `rustc --explain E0597`.
------------------------------------------

So, I'm not sure where to go from there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^^ This is the more concerning bit. Please advise when you can.

this.resolve_anonymous_lifetime(lifetime, false)
});
} else {
self.resolve_anonymous_lifetime(lifetime, false);
}
}

#[instrument(level = "debug", skip(self))]
fn record_lifetime_res(
&mut self,
Expand Down Expand Up @@ -1833,6 +1877,19 @@ impl<'a: 'ast, 'b, 'ast> LateResolutionVisitor<'a, 'b, 'ast> {
let elision_lifetime = self.resolve_fn_params(has_self, inputs);
debug!(?elision_lifetime);

// We might be about to resolve the return type of some
// `fn() -> T`-typed parameter, like:
//
// ```
// fn foo(f: fn(&str) -> Box<dyn Trait + '_>) { ... }
// ```
//
// In that case, we don't want to act like we're resolving function
// parameters.
//
// FIXME(bgr360): test multiple levels of this
let outer_in_params = replace(&mut self.in_func_params, false);

let outer_failures = take(&mut self.diagnostic_metadata.current_elision_failures);
let output_rib = if let Ok(res) = elision_lifetime.as_ref() {
LifetimeRibKind::Elided(*res)
Expand All @@ -1846,16 +1903,20 @@ impl<'a: 'ast, 'b, 'ast> LateResolutionVisitor<'a, 'b, 'ast> {
let Err(failure_info) = elision_lifetime else { bug!() };
self.report_missing_lifetime_specifiers(elision_failures, Some(failure_info));
}

self.in_func_params = outer_in_params;
}

/// Resolve inside function parameters and parameter types.
/// Returns the lifetime for elision in fn return type,
/// or diagnostic information in case of elision failure.
#[instrument(level = "debug", skip(self, inputs))]
fn resolve_fn_params(
&mut self,
has_self: bool,
inputs: impl Iterator<Item = (Option<&'ast Pat>, &'ast Ty)>,
) -> Result<LifetimeRes, (Vec<MissingLifetime>, Vec<ElisionFnParameter>)> {
#[derive(Debug)]
enum Elision {
/// We have not found any candidate.
None,
Expand All @@ -1867,7 +1928,8 @@ impl<'a: 'ast, 'b, 'ast> LateResolutionVisitor<'a, 'b, 'ast> {
Err,
}

// Save elision state to reinstate it later.
// Save state to reinstate it later.
let outer_in_params = replace(&mut self.in_func_params, true);
let outer_candidates = self.lifetime_elision_candidates.take();

// Result of elision.
Expand All @@ -1890,9 +1952,11 @@ impl<'a: 'ast, 'b, 'ast> LateResolutionVisitor<'a, 'b, 'ast> {
self.lifetime_elision_candidates = Some(Default::default());
self.visit_ty(ty);
let local_candidates = self.lifetime_elision_candidates.take();
trace!(?local_candidates);

if let Some(candidates) = local_candidates {
let distinct: FxHashSet<_> = candidates.iter().map(|(res, _)| *res).collect();
trace!(?distinct);
let lifetime_count = distinct.len();
if lifetime_count != 0 {
parameter_info.push(ElisionFnParameter {
Expand Down Expand Up @@ -1947,12 +2011,14 @@ impl<'a: 'ast, 'b, 'ast> LateResolutionVisitor<'a, 'b, 'ast> {
elision_lifetime = Elision::None;
}
}
trace!(?elision_lifetime);
debug!("(resolving function / closure) recorded parameter");
}

// Reinstate elision state.
// Reinstate state.
debug_assert_matches!(self.lifetime_elision_candidates, None);
self.lifetime_elision_candidates = outer_candidates;
self.in_func_params = outer_in_params;

if let Elision::Param(res) | Elision::Self_(res) = elision_lifetime {
return Ok(res);
Expand Down Expand Up @@ -2085,8 +2151,8 @@ impl<'a: 'ast, 'b, 'ast> LateResolutionVisitor<'a, 'b, 'ast> {
true
}

#[instrument(level = "debug", skip_all)]
fn resolve_adt(&mut self, item: &'ast Item, generics: &'ast Generics) {
debug!("resolve_adt");
self.with_current_self_item(item, |this| {
this.with_generic_param_rib(
&generics.params,
Expand Down Expand Up @@ -2156,10 +2222,8 @@ impl<'a: 'ast, 'b, 'ast> LateResolutionVisitor<'a, 'b, 'ast> {
}
}

#[instrument(level = "debug", skip_all, fields(name = %item.ident.name, kind = ?item.kind))]
fn resolve_item(&mut self, item: &'ast Item) {
let name = item.ident.name;
debug!("(resolving item) resolving {} ({:?})", name, item.kind);

match item.kind {
ItemKind::TyAlias(box TyAlias { ref generics, .. }) => {
self.with_generic_param_rib(
Expand Down Expand Up @@ -2293,6 +2357,7 @@ impl<'a: 'ast, 'b, 'ast> LateResolutionVisitor<'a, 'b, 'ast> {
}
}

#[instrument(level = "debug", skip(self, f))]
fn with_generic_param_rib<'c, F>(
&'c mut self,
params: &'c [GenericParam],
Expand All @@ -2302,7 +2367,6 @@ impl<'a: 'ast, 'b, 'ast> LateResolutionVisitor<'a, 'b, 'ast> {
) where
F: FnOnce(&mut Self),
{
debug!("with_generic_param_rib");
let LifetimeRibKind::Generics { binder, span: generics_span, kind: generics_kind, .. }
= lifetime_kind else { panic!() };

Expand Down Expand Up @@ -2337,7 +2401,7 @@ impl<'a: 'ast, 'b, 'ast> LateResolutionVisitor<'a, 'b, 'ast> {

for param in params {
let ident = param.ident.normalize_to_macros_2_0();
debug!("with_generic_param_rib: {}", param.id);
debug!(%param.id);

if let GenericParamKind::Lifetime = param.kind
&& let Some(&original) = seen_lifetimes.get(&ident)
Expand Down Expand Up @@ -2601,6 +2665,7 @@ impl<'a: 'ast, 'b, 'ast> LateResolutionVisitor<'a, 'b, 'ast> {
self.with_self_rib_ns(TypeNS, self_res, f)
}

#[instrument(level = "debug", skip_all)]
fn resolve_implementation(
&mut self,
generics: &'ast Generics,
Expand All @@ -2609,7 +2674,6 @@ impl<'a: 'ast, 'b, 'ast> LateResolutionVisitor<'a, 'b, 'ast> {
item_id: NodeId,
impl_items: &'ast [P<AssocItem>],
) {
debug!("resolve_implementation");
// If applicable, create a rib for the type parameters.
self.with_generic_param_rib(
&generics.params,
Expand Down Expand Up @@ -2680,6 +2744,7 @@ impl<'a: 'ast, 'b, 'ast> LateResolutionVisitor<'a, 'b, 'ast> {
);
}

#[instrument(level = "debug", skip_all)]
fn resolve_impl_item(
&mut self,
item: &'ast AssocItem,
Expand All @@ -2688,7 +2753,7 @@ impl<'a: 'ast, 'b, 'ast> LateResolutionVisitor<'a, 'b, 'ast> {
use crate::ResolutionError::*;
match &item.kind {
AssocItemKind::Const(_, ty, default) => {
debug!("resolve_implementation AssocItemKind::Const");
debug!("AssocItemKind::Const");
// If this is a trait impl, ensure the const
// exists in trait
self.check_trait_item(
Expand Down Expand Up @@ -2719,7 +2784,7 @@ impl<'a: 'ast, 'b, 'ast> LateResolutionVisitor<'a, 'b, 'ast> {
}
}
AssocItemKind::Fn(box Fn { generics, .. }) => {
debug!("resolve_implementation AssocItemKind::Fn");
debug!("AssocItemKind::Fn");
// We also need a new scope for the impl item type parameters.
self.with_generic_param_rib(
&generics.params,
Expand Down Expand Up @@ -2747,7 +2812,7 @@ impl<'a: 'ast, 'b, 'ast> LateResolutionVisitor<'a, 'b, 'ast> {
);
}
AssocItemKind::Type(box TyAlias { generics, .. }) => {
debug!("resolve_implementation AssocItemKind::Type");
debug!("AssocItemKind::Type");
// We also need a new scope for the impl item type parameters.
self.with_generic_param_rib(
&generics.params,
Expand Down Expand Up @@ -2884,8 +2949,8 @@ impl<'a: 'ast, 'b, 'ast> LateResolutionVisitor<'a, 'b, 'ast> {
}
}

#[instrument(level = "debug", skip(self))]
fn resolve_local(&mut self, local: &'ast Local) {
debug!("resolving local ({:?})", local);
// Resolve the type.
walk_list!(self, visit_ty, &local.ty);

Expand Down Expand Up @@ -3310,17 +3375,14 @@ impl<'a: 'ast, 'b, 'ast> LateResolutionVisitor<'a, 'b, 'ast> {
);
}

#[instrument(level = "debug", skip(self, source), ret)]
fn smart_resolve_path_fragment(
&mut self,
qself: Option<&QSelf>,
path: &[Segment],
source: PathSource<'ast>,
finalize: Finalize,
) -> PartialRes {
debug!(
"smart_resolve_path_fragment(qself={:?}, path={:?}, finalize={:?})",
qself, path, finalize,
);
let ns = source.namespace();

let Finalize { node_id, path_span, .. } = finalize;
Expand Down Expand Up @@ -3575,18 +3637,14 @@ impl<'a: 'ast, 'b, 'ast> LateResolutionVisitor<'a, 'b, 'ast> {
}

/// Handles paths that may refer to associated items.
#[instrument(level = "debug", skip(self), ret)]
fn resolve_qpath(
&mut self,
qself: Option<&QSelf>,
path: &[Segment],
ns: Namespace,
finalize: Finalize,
) -> Result<Option<PartialRes>, Spanned<ResolutionError<'a>>> {
debug!(
"resolve_qpath(qself={:?}, path={:?}, ns={:?}, finalize={:?})",
qself, path, ns, finalize,
);

if let Some(qself) = qself {
if qself.position == 0 {
// This is a case like `<T>::B`, where there is no
Expand Down Expand Up @@ -3685,6 +3743,7 @@ impl<'a: 'ast, 'b, 'ast> LateResolutionVisitor<'a, 'b, 'ast> {
Ok(Some(result))
}

#[instrument(level = "trace", skip(self, f))]
fn with_resolved_label(&mut self, label: Option<Label>, id: NodeId, f: impl FnOnce(&mut Self)) {
if let Some(label) = label {
if label.ident.as_str().as_bytes()[1] != b'_' {
Expand Down Expand Up @@ -3760,8 +3819,8 @@ impl<'a: 'ast, 'b, 'ast> LateResolutionVisitor<'a, 'b, 'ast> {
debug!("(resolving block) leaving block");
}

#[instrument(level = "debug", skip(self))]
fn resolve_anon_const(&mut self, constant: &'ast AnonConst, is_repeat: IsRepeatExpr) {
debug!("resolve_anon_const {:?} is_repeat: {:?}", constant, is_repeat);
self.with_constant_rib(
is_repeat,
if constant.value.is_potential_trivial_const_param() {
Expand All @@ -3774,8 +3833,8 @@ impl<'a: 'ast, 'b, 'ast> LateResolutionVisitor<'a, 'b, 'ast> {
);
}

#[instrument(level = "debug", skip(self))]
fn resolve_inline_const(&mut self, constant: &'ast AnonConst) {
debug!("resolve_anon_const {constant:?}");
self.with_constant_rib(IsRepeatExpr::No, ConstantHasGenerics::Yes, None, |this| {
visit::walk_anon_const(this, constant)
});
Expand Down
2 changes: 2 additions & 0 deletions compiler/rustc_resolve/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,13 +161,15 @@ enum ImplTraitContext {
Universal(LocalDefId),
}

#[derive(Debug)]
struct BindingError {
name: Symbol,
origin: BTreeSet<Span>,
target: BTreeSet<Span>,
could_be_path: bool,
}

#[derive(Debug)]
enum ResolutionError<'a> {
/// Error E0401: can't use type or const parameters from outer function.
GenericParamsFromOuterFunction(Res, HasGenericParams),
Expand Down
Loading