Skip to content
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.

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

Detect when user is trying to create a lending Iterator and give a custom explanation #125407

Merged
merged 2 commits into from
Jun 5, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
8 changes: 8 additions & 0 deletions compiler/rustc_resolve/messages.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,10 @@ resolve_added_macro_use =
resolve_ancestor_only =
visibilities can only be restricted to ancestor modules

resolve_anonymous_livetime_non_gat_report_error =
in the trait associated type is declared without lifetime parameters, so using a borrowed type for them requires that lifetime to come from the implemented type
.label = this lifetime must come from the implemented type

resolve_arguments_macro_use_not_allowed = arguments to `macro_use` are not allowed here

resolve_associated_const_with_similar_name_exists =
Expand Down Expand Up @@ -234,6 +238,10 @@ resolve_items_in_traits_are_not_importable =
resolve_label_with_similar_name_reachable =
a label with a similar name is reachable

resolve_lending_iterator_report_error =
associated type `Iterator::Item` is declared without lifetime parameters, so using a borrowed type for them requires that lifetime to come from the implemented type
.note = you can't create an `Iterator` that borrows each `Item` from itself, but you can instead create a new type that borrows your existing type and implement `Iterator` for that new type.

resolve_lifetime_param_in_enum_discriminant =
lifetime parameters may not be used in enum discriminant values

Expand Down
17 changes: 17 additions & 0 deletions compiler/rustc_resolve/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -882,6 +882,23 @@ pub(crate) struct ElidedAnonymousLivetimeReportError {
pub(crate) suggestion: Option<ElidedAnonymousLivetimeReportErrorSuggestion>,
}

#[derive(Diagnostic)]
#[diag(resolve_lending_iterator_report_error)]
pub(crate) struct LendingIteratorReportError {
#[primary_span]
pub(crate) lifetime: Span,
#[note]
pub(crate) ty: Span,
}

#[derive(Diagnostic)]
#[diag(resolve_anonymous_livetime_non_gat_report_error)]
pub(crate) struct AnonymousLivetimeNonGatReportError {
#[primary_span]
#[label]
pub(crate) lifetime: Span,
}

#[derive(Subdiagnostic)]
#[multipart_suggestion(
resolve_elided_anonymous_lifetime_report_error_suggestion,
Expand Down
52 changes: 47 additions & 5 deletions compiler/rustc_resolve/src/late.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
use crate::{errors, path_names_to_string, rustdoc, BindingError, Finalize, LexicalScopeBinding};
use crate::{BindingKey, Used};
use crate::{Module, ModuleOrUniformRoot, NameBinding, ParentScope, PathResult};
use crate::{ResolutionError, Resolver, Segment, UseError};
use crate::{ResolutionError, Resolver, Segment, TyCtxt, UseError};

use rustc_ast::ptr::P;
use rustc_ast::visit::{visit_opt, walk_list, AssocCtxt, BoundKind, FnCtxt, FnKind, Visitor};
Expand Down Expand Up @@ -629,6 +629,9 @@ struct DiagMetadata<'ast> {
in_assignment: Option<&'ast Expr>,
is_assign_rhs: bool,

/// If we are setting an associated type in trait impl, is it a non-GAT type?
in_non_gat_assoc_type: Option<bool>,

/// Used to detect possible `.` -> `..` typo when calling methods.
in_range: Option<(&'ast Expr, &'ast Expr)>,

Expand Down Expand Up @@ -1703,10 +1706,35 @@ impl<'a: 'ast, 'b, 'ast, 'tcx> LateResolutionVisitor<'a, 'b, 'ast, 'tcx> {
break;
}
}
self.r.dcx().emit_err(errors::ElidedAnonymousLivetimeReportError {
span: lifetime.ident.span,
suggestion,
});

// are we trying to use an anonymous lifetime
// on a non GAT associated trait type?
if !self.in_func_body
&& let Some((module, _)) = &self.current_trait_ref
&& let Some(ty) = &self.diag_metadata.current_self_type
&& Some(true) == self.diag_metadata.in_non_gat_assoc_type
&& let crate::ModuleKind::Def(DefKind::Trait, trait_id, _) = module.kind
{
if def_id_matches_path(
self.r.tcx,
trait_id,
&["core", "iter", "traits", "iterator", "Iterator"],
Copy link
Member

Choose a reason for hiding this comment

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

Can this be extended to all non-generic associated types?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Which part specifically?

Copy link
Member

Choose a reason for hiding this comment

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

I think Jack is saying that this PR has been tailored to just Iterator (and Iterator::Item), and is then posing the thought exercise of whether the logic here (and the associated diagnostic output) could be generalized, so that it would cover all associated types lacking a parameter (i.e. "non-generic").

I.e., instead of specializing this check to trait Iterator { type Item; ... }, can it be extended to trait <AnyTraitNameHere> { type AnyAssociatedTypeHere; ... // but not GATs } ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Possibly, but I think this would be a separate diagnostic. I've seen people trying to implement lending iterators, I even tried to implement it myself years ago before I understood how it works :) So note about iterators is the main part and I want to keep it.

I can try to make the check more generic and add an error message about using non-GAT associated type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rebased, added a better error message for general case of non-GAT associated type, left the error for iterators as is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And I'm open to any suggestions on wording/formatting/etc.

) {
self.r.dcx().emit_err(errors::LendingIteratorReportError {
lifetime: lifetime.ident.span,
ty: ty.span(),
});
} else {
self.r.dcx().emit_err(errors::AnonymousLivetimeNonGatReportError {
lifetime: lifetime.ident.span,
});
}
} else {
self.r.dcx().emit_err(errors::ElidedAnonymousLivetimeReportError {
span: lifetime.ident.span,
suggestion,
});
}
} else {
self.r.dcx().emit_err(errors::ExplicitAnonymousLivetimeReportError {
span: lifetime.ident.span,
Expand Down Expand Up @@ -3058,6 +3086,7 @@ impl<'a: 'ast, 'b, 'ast, 'tcx> LateResolutionVisitor<'a, 'b, 'ast, 'tcx> {
);
}
AssocItemKind::Type(box TyAlias { generics, .. }) => {
self.diag_metadata.in_non_gat_assoc_type = Some(generics.params.is_empty());
debug!("resolve_implementation AssocItemKind::Type");
// We also need a new scope for the impl item type parameters.
self.with_generic_param_rib(
Expand Down Expand Up @@ -3086,6 +3115,7 @@ impl<'a: 'ast, 'b, 'ast, 'tcx> LateResolutionVisitor<'a, 'b, 'ast, 'tcx> {
});
},
);
self.diag_metadata.in_non_gat_assoc_type = None;
}
AssocItemKind::Delegation(box delegation) => {
debug!("resolve_implementation AssocItemKind::Delegation");
Expand Down Expand Up @@ -4824,3 +4854,15 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
}
}
}

/// Check if definition matches a path
fn def_id_matches_path(tcx: TyCtxt<'_>, mut def_id: DefId, expected_path: &[&str]) -> bool {
let mut path = expected_path.iter().rev();
while let (Some(parent), Some(next_step)) = (tcx.opt_parent(def_id), path.next()) {
if !tcx.opt_item_name(def_id).map_or(false, |n| n.as_str() == *next_step) {
return false;
}
def_id = parent;
}
return true;
}
2 changes: 1 addition & 1 deletion tests/ui/impl-header-lifetime-elision/assoc-type.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ trait MyTrait {

impl MyTrait for &i32 {
type Output = &i32;
//~^ ERROR `&` without an explicit lifetime name cannot be used here
//~^ ERROR 11:19: 11:20: in the trait associated type is declared without lifetime parameters, so using a borrowed type for them requires that lifetime to come from the implemented type
}

impl MyTrait for &u32 {
Expand Down
4 changes: 2 additions & 2 deletions tests/ui/impl-header-lifetime-elision/assoc-type.stderr
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
error[E0637]: `&` without an explicit lifetime name cannot be used here
error: in the trait associated type is declared without lifetime parameters, so using a borrowed type for them requires that lifetime to come from the implemented type
--> $DIR/assoc-type.rs:11:19
|
LL | type Output = &i32;
| ^ explicit lifetime name needed here
| ^ this lifetime must come from the implemented type

error[E0637]: `'_` cannot be used here
--> $DIR/assoc-type.rs:16:20
Expand Down
35 changes: 35 additions & 0 deletions tests/ui/lifetimes/no_lending_iterators.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
struct Data(String);

impl Iterator for Data {
type Item = &str;
//~^ ERROR 4:17: 4:18: associated type `Iterator::Item` is declared without lifetime parameters, so using a borrowed type for them requires that lifetime to come from the implemented type

fn next(&mut self) -> Option<Self::Item> {
Some(&self.0)
}
}

trait Bar {
type Item;
fn poke(&mut self, item: Self::Item);
}

impl Bar for usize {
type Item = &usize;
//~^ ERROR 18:17: 18:18: in the trait associated type is declared without lifetime parameters, so using a borrowed type for them requires that lifetime to come from the implemented type

fn poke(&mut self, item: Self::Item) {
self += *item;
}
}

impl Bar for isize {
type Item<'a> = &'a isize;
//~^ ERROR 27:14: 27:18: lifetime parameters or bounds on type `Item` do not match the trait declaration [E0195]

fn poke(&mut self, item: Self::Item) {
self += *item;
}
}

fn main() {}
30 changes: 30 additions & 0 deletions tests/ui/lifetimes/no_lending_iterators.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
error: associated type `Iterator::Item` is declared without lifetime parameters, so using a borrowed type for them requires that lifetime to come from the implemented type
--> $DIR/no_lending_iterators.rs:4:17
|
LL | type Item = &str;
| ^
|
note: you can't create an `Iterator` that borrows each `Item` from itself, but you can instead create a new type that borrows your existing type and implement `Iterator` for that new type.
--> $DIR/no_lending_iterators.rs:3:19
|
LL | impl Iterator for Data {
| ^^^^

error: in the trait associated type is declared without lifetime parameters, so using a borrowed type for them requires that lifetime to come from the implemented type
--> $DIR/no_lending_iterators.rs:18:17
|
LL | type Item = &usize;
| ^ this lifetime must come from the implemented type

error[E0195]: lifetime parameters or bounds on type `Item` do not match the trait declaration
--> $DIR/no_lending_iterators.rs:27:14
|
LL | type Item;
| - lifetimes in impl do not match this type in trait
...
LL | type Item<'a> = &'a isize;
| ^^^^ lifetimes do not match type in trait

error: aborting due to 3 previous errors

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