Skip to content

Commit

Permalink
Split elided_lifetime_in_paths into tied and untied
Browse files Browse the repository at this point in the history
Types that contain a reference can be confusing when lifetime elision
occurs:

```rust
// Confusing
fn foo(_: &u8) -> Bar { todo!() }

// Less confusing
fn foo(_: &u8) -> Bar<'_> { todo!() }
```

However, the previous lint did not distinguish when these types were
not "tying" lifetimes across the function inputs / outputs:

```rust
// Maybe a little confusing
fn foo(_: Bar) {}

// More explicit but noisier with less obvious value
fn foo(_: Bar<'_>) {}
```

We now report different lints for each case, hopefully paving the way
to marking the first case (when lifetimes are tied together) as
warn-by-default.
  • Loading branch information
shepmaster committed Feb 13, 2024
1 parent f914d68 commit f6d8513
Show file tree
Hide file tree
Showing 11 changed files with 579 additions and 29 deletions.
9 changes: 8 additions & 1 deletion compiler/rustc_lint/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -303,7 +303,8 @@ fn register_builtins(store: &mut LintStore) {
BARE_TRAIT_OBJECTS,
UNUSED_EXTERN_CRATES,
ELLIPSIS_INCLUSIVE_RANGE_PATTERNS,
ELIDED_LIFETIMES_IN_PATHS,
ELIDED_LIFETIMES_IN_PATHS_TIED,
ELIDED_LIFETIMES_IN_PATHS_UNTIED,
EXPLICIT_OUTLIVES_REQUIREMENTS,
// FIXME(#52665, #47816) not always applicable and not all
// macros are ready for this yet.
Expand All @@ -313,6 +314,12 @@ fn register_builtins(store: &mut LintStore) {
// MACRO_USE_EXTERN_CRATE
);

add_lint_group!(
"elided_lifetimes_in_paths",
ELIDED_LIFETIMES_IN_PATHS_TIED,
ELIDED_LIFETIMES_IN_PATHS_UNTIED,
);

// Register renamed and removed lints.
store.register_renamed("single_use_lifetime", "single_use_lifetimes");
// store.register_renamed("elided_lifetime_in_path", "elided_lifetimes_in_paths");
Expand Down
54 changes: 48 additions & 6 deletions compiler/rustc_lint_defs/src/builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,8 @@ declare_lint_pass! {
DEPRECATED_WHERE_CLAUSE_LOCATION,
DUPLICATE_MACRO_ATTRIBUTES,
ELIDED_LIFETIMES_IN_ASSOCIATED_CONSTANT,
ELIDED_LIFETIMES_IN_PATHS,
ELIDED_LIFETIMES_IN_PATHS_TIED,
ELIDED_LIFETIMES_IN_PATHS_UNTIED,
EXPORTED_PRIVATE_DEPENDENCIES,
FFI_UNWIND_CALLS,
FORBIDDEN_LINT_GROUPS,
Expand Down Expand Up @@ -1701,19 +1702,21 @@ declare_lint! {
}

declare_lint! {
/// The `elided_lifetimes_in_paths` lint detects the use of hidden
/// lifetime parameters.
/// The `elided_lifetimes_in_paths_tied` lint detects the use of
/// hidden lifetime parameters when those lifetime parameters tie
/// an input lifetime parameter to an output lifetime parameter.
///
/// ### Example
///
/// ```rust,compile_fail
/// #![deny(elided_lifetimes_in_paths)]
/// #![deny(elided_lifetimes_in_paths_tied)]
/// #![deny(warnings)]
/// struct Foo<'a> {
/// x: &'a u32
/// }
///
/// fn foo(x: &Foo) {
/// fn foo(x: Foo) -> &u32 {
/// &x.0
/// }
/// ```
///
Expand All @@ -1730,12 +1733,51 @@ declare_lint! {
/// may require a significant transition for old code.
///
/// [placeholder lifetime]: https://doc.rust-lang.org/reference/lifetime-elision.html#lifetime-elision-in-functions
pub ELIDED_LIFETIMES_IN_PATHS,
pub ELIDED_LIFETIMES_IN_PATHS_TIED,
Allow,
"hidden lifetime parameters in types are deprecated",
crate_level_only
}

declare_lint! {
/// The `elided_lifetimes_in_paths_untied` lint detects the use of
/// hidden lifetime parameters when those lifetime parameters do
/// not tie an input lifetime parameter to an output lifetime
/// parameter.
///
/// ### Example
///
/// ```rust,compile_fail
/// #![deny(elided_lifetimes_in_paths_untied)]
/// #![deny(warnings)]
/// struct Foo<'a> {
/// x: &'a u32
/// }
///
/// fn foo(x: Foo) -> u32 {
/// x.0
/// }
/// ```
///
/// {{produces}}
///
/// ### Explanation
///
/// Elided lifetime parameters can make it difficult to see at a glance
/// that borrowing is occurring. This lint ensures that lifetime
/// parameters are always explicitly stated, even if it is the `'_`
/// [placeholder lifetime].
///
/// This lint is "allow" by default because it has some known issues, and
/// may require a significant transition for old code.
///
/// [placeholder lifetime]: https://doc.rust-lang.org/reference/lifetime-elision.html#lifetime-elision-in-functions
pub ELIDED_LIFETIMES_IN_PATHS_UNTIED,
Allow,
"hidden lifetime parameters in types make it hard to tell when borrowing is happening",
crate_level_only
}

declare_lint! {
/// The `bare_trait_objects` lint suggests using `dyn Trait` for trait
/// objects.
Expand Down
119 changes: 99 additions & 20 deletions compiler/rustc_resolve/src/late.rs
Original file line number Diff line number Diff line change
Expand Up @@ -646,7 +646,24 @@ struct DiagnosticMetadata<'ast> {
}

#[derive(Debug)]
struct ResolvedNestedElisionTarget {
enum ResolvedElisionTarget {
/// Elision in `&u8` -> `&'_ u8`
TopLevel(NodeId),
/// Elision in `Foo` -> `Foo<'_>`
Nested(NestedResolvedElisionTarget),
}

impl ResolvedElisionTarget {
fn node_id(&self) -> NodeId {
match *self {
Self::TopLevel(n) => n,
Self::Nested(NestedResolvedElisionTarget { segment_id, .. }) => segment_id,
}
}
}

#[derive(Debug)]
struct NestedResolvedElisionTarget {
segment_id: NodeId,
elided_lifetime_span: Span,
diagnostic: lint::BuiltinLintDiagnostics,
Expand Down Expand Up @@ -694,7 +711,7 @@ struct LateResolutionVisitor<'a, 'b, 'ast, 'tcx> {
lifetime_uses: FxHashMap<LocalDefId, LifetimeUseSet>,

/// Track which types participated in lifetime elision
resolved_lifetime_elisions: Vec<ResolvedNestedElisionTarget>,
resolved_lifetime_elisions: Vec<ResolvedElisionTarget>,
}

/// Walks the whole crate in DFS order, visiting each item, resolving names as it goes.
Expand Down Expand Up @@ -1745,6 +1762,8 @@ impl<'a: 'ast, 'b, 'ast, 'tcx> LateResolutionVisitor<'a, 'b, 'ast, 'tcx> {
LifetimeElisionCandidate::Ignore,
);
self.resolve_anonymous_lifetime(&lt, true);

self.resolved_lifetime_elisions.push(ResolvedElisionTarget::TopLevel(anchor_id));
}

#[instrument(level = "debug", skip(self))]
Expand Down Expand Up @@ -1957,16 +1976,18 @@ impl<'a: 'ast, 'b, 'ast, 'tcx> LateResolutionVisitor<'a, 'b, 'ast, 'tcx> {
}

if should_lint {
self.resolved_lifetime_elisions.push(ResolvedNestedElisionTarget {
segment_id,
elided_lifetime_span,
diagnostic: lint::BuiltinLintDiagnostics::ElidedLifetimesInPaths(
expected_lifetimes,
path_span,
!segment.has_generic_args,
self.resolved_lifetime_elisions.push(ResolvedElisionTarget::Nested(
NestedResolvedElisionTarget {
segment_id,
elided_lifetime_span,
),
});
diagnostic: lint::BuiltinLintDiagnostics::ElidedLifetimesInPaths(
expected_lifetimes,
path_span,
!segment.has_generic_args,
elided_lifetime_span,
),
},
));
}
}
}
Expand Down Expand Up @@ -2028,22 +2049,80 @@ impl<'a: 'ast, 'b, 'ast, 'tcx> LateResolutionVisitor<'a, 'b, 'ast, 'tcx> {

let elision_failures =
replace(&mut self.diagnostic_metadata.current_elision_failures, outer_failures);
if !elision_failures.is_empty() {
let Err(failure_info) = elision_lifetime else { bug!() };
self.report_missing_lifetime_specifiers(elision_failures, Some(failure_info));
}

let elision_lifetime = match (elision_failures.is_empty(), elision_lifetime) {
(true, Ok(lifetime)) => Some(lifetime),

(true, Err(_)) => None,

(false, Ok(_)) => bug!(),

(false, Err(failure_info)) => {
self.report_missing_lifetime_specifiers(elision_failures, Some(failure_info));
None
}
};

// We've recorded all elisions that occurred in the params and
// outputs, categorized by top-level or nested.
//
// Our primary lint case is when an output lifetime is tied to
// an input lifetime. In that case, we want to warn about any
// parameters that had a nested elision.
//
// The secondary case is for nested elisions that are not part
// of the tied lifetime relationship.

let output_resolved_lifetime_elisions =
replace(&mut self.resolved_lifetime_elisions, outer_resolved_lifetime_elisions);

let resolved_lifetime_elisions =
param_resolved_lifetime_elisions.into_iter().chain(output_resolved_lifetime_elisions);
match (output_resolved_lifetime_elisions.is_empty(), elision_lifetime) {
(true, _) | (_, None) => {
// Treat all parameters as untied
self.report_elided_lifetimes_in_paths(
param_resolved_lifetime_elisions,
lint::builtin::ELIDED_LIFETIMES_IN_PATHS_UNTIED,
);
}
(false, Some(elision_lifetime)) => {
let (primary, secondary): (Vec<_>, Vec<_>) =
param_resolved_lifetime_elisions.into_iter().partition(|re| {
// Recover the `NodeId` of an elided lifetime
let lvl1 = &self.r.lifetimes_res_map[&re.node_id()];
let lvl2 = match lvl1 {
LifetimeRes::ElidedAnchor { start, .. } => {
&self.r.lifetimes_res_map[&start]
}
o => o,
};

lvl2 == &elision_lifetime
});

self.report_elided_lifetimes_in_paths(
primary.into_iter().chain(output_resolved_lifetime_elisions),
lint::builtin::ELIDED_LIFETIMES_IN_PATHS_TIED,
);
self.report_elided_lifetimes_in_paths(
secondary,
lint::builtin::ELIDED_LIFETIMES_IN_PATHS_UNTIED,
);
}
}
}

fn report_elided_lifetimes_in_paths(
&mut self,
resolved_elisions: impl IntoIterator<Item = ResolvedElisionTarget>,
lint: &'static lint::Lint,
) {
for re in resolved_elisions {
let ResolvedElisionTarget::Nested(d) = re else { continue };

for re in resolved_lifetime_elisions {
let ResolvedNestedElisionTarget { segment_id, elided_lifetime_span, diagnostic } = re;
let NestedResolvedElisionTarget { segment_id, elided_lifetime_span, diagnostic } = d;

self.r.lint_buffer.buffer_lint_with_diagnostic(
lint::builtin::ELIDED_LIFETIMES_IN_PATHS,
lint,
segment_id,
elided_lifetime_span,
"hidden lifetime parameters in types are deprecated",
Expand Down
1 change: 1 addition & 0 deletions src/tools/lint-docs/src/groups.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ static GROUP_DESCRIPTIONS: &[(&str, &str)] = &[
("let-underscore", "Lints that detect wildcard let bindings that are likely to be invalid"),
("rustdoc", "Rustdoc-specific lints"),
("rust-2018-idioms", "Lints to nudge you toward idiomatic features of Rust 2018"),
("elided-lifetimes-in-paths", "Lints that detect the use of hidden lifetime parameters"),
("nonstandard-style", "Violation of standard naming conventions"),
("future-incompatible", "Lints that detect code that has future-compatibility problems"),
("rust-2018-compatibility", "Lints used to transition code from the 2015 edition to 2018"),
Expand Down
Loading

0 comments on commit f6d8513

Please sign in to comment.