-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
derive(SmartPointer): assume pointee from the single generic and better error messages #129467
derive(SmartPointer): assume pointee from the single generic and better error messages #129467
Conversation
@rustbot label F-derive_smart_pointer |
if p.attrs().iter().any(|attr| attr.has_name(sym::pointee)) { | ||
if pointee_param.is_some() { | ||
multiple_pointee_diag.push(cx.dcx().struct_span_err( | ||
p.span(), | ||
"`SmartPointer` can only admit one type as pointee", | ||
)); | ||
} else { | ||
pointee_param = Some(idx); | ||
match pointee_param { | ||
PointeeChoice::Assumed(_) | ||
| PointeeChoice::Ambiguous | ||
| PointeeChoice::None => { | ||
pointee_param = PointeeChoice::Exactly(idx, p.span()) | ||
} | ||
PointeeChoice::Exactly(_, another) => { | ||
pointee_param = PointeeChoice::MultiplePointeeChoice(another, p.span()) | ||
} | ||
PointeeChoice::MultiplePointeeChoice(_, _) => {} | ||
} | ||
} else { | ||
match pointee_param { | ||
PointeeChoice::None => pointee_param = PointeeChoice::Assumed(idx), | ||
PointeeChoice::Assumed(_) | PointeeChoice::Ambiguous => { | ||
pointee_param = PointeeChoice::Ambiguous | ||
} | ||
PointeeChoice::Exactly(_, _) | ||
| PointeeChoice::MultiplePointeeChoice(_, _) => {} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we can simplify these map operations with side effects by iterating multiple times. For example: (untested)
let self_params = generics
.params
.iter()
.enumerate()
.map(|(idx, p)| match p.kind {
GenericParamKind::Lifetime => GenericArg::Lifetime(cx.lifetime(p.span(), p.ident)),
GenericParamKind::Type { .. } => GenericArg::Type(cx.ty_ident(p.span(), p.ident)),
GenericParamKind::Const { .. } => GenericArg::Const(cx.const_ident(p.span(), p.ident)),
})
.collect::<Vec<_>>();
let all_type_params = generics
.params
.iter()
.enumerate()
.filter_map(|(idx, p)| match p.kind {
GenericParamKind::Type { .. } => Some((idx, p.span())),
_ => None,
})
.collect::<Vec<(usize, Span)>>();
let pointee_params = generics
.params
.iter()
.enumerate()
.filter_map(|(idx, p)| match p.kind {
GenericParamKind::Type { .. } if p.attrs().iter().any(|attr| attr.has_name(sym::pointee)) => Some((idx, p.span())),
_ => None,
})
.collect::<Vec<(usize, Span)>>();
let pointee_param = match (pointee_params.as_slice(), all_type_params.as_slice()) {
([(idx, _span)], _) => idx,
([], [(idx, _span)]) => idx,
([], []) => {
cx.dcx().struct_span_err(span, "`SmartPointer` requires a generic parameter").emit();
return;
}
([], _) => {
let all_type_spans = all_type_params.into_iter().map(|(idx,span)| span).collect();
cx.dcx().struct_span_err(all_type_spans, "you must use `#[pointee]` to specify which parameter is the `SmartPointer` pointee").emit();
return;
}
(_, _) => {
let pointee_spans = pointee_params.into_iter().map(|(idx,span)| span).collect();
cx.dcx().struct_span_err(pointee_spans, "you can only use `#[pointee]` once").emit();
return;
}
};
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Applied with slight adaptation
So now we one pass through the generics to get ourserlves the self_params
. One more pass through the iterator is used to collect only the type parameters.
- If we find zero type parameters, err with diagnosis
- If we find exactly one type parameter, use it as
#[pointee]
- Otherwise, filter through type parameters on whether there is
#[pointee]
designation and err when more than one such parameters are found
bb43160
to
ea3a13b
Compare
(None, _) => { | ||
cx.dcx().struct_span_err( | ||
span, | ||
"Exactly one generic parameters when there are at least two generic type parameters \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
diagnostics should not be capitalized
also this message could be tweaked:
exactly one generic type parameter must be marked as
#[pointee]
to deriveSmartPointer
traits
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, there is no test for this message?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops. Added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forgot to add the hunk with the updated message as per your suggestion. It is checked in now.
} | ||
let pointee_param_idx = if type_params.len() == 1 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you special case the == 0
case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Applied. Moved the check above into here.
cx.dcx() | ||
.struct_span_err( | ||
vec![one, another], | ||
"`SmartPointer` can only admit one type as pointee", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you adjust this message?
only one type parameter can be marked as
#[pointee]
when derivingSmartPointer
traits
sounds more natural that way
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Applied
@rustbot author |
ea3a13b
to
447ca81
Compare
@rustbot ready
|
…er error messages
447ca81
to
3914835
Compare
@bors r+ rollup |
…ax-pointee, r=compiler-errors derive(SmartPointer): assume pointee from the single generic and better error messages Fix rust-lang#129465 Actually RFC says that `#[pointee]` can be inferred when there is no ambiguity, or there is only one generic type parameter so to say. cc `@Darksonn` r? `@compiler-errors`
…iaskrgr Rollup of 11 pull requests Successful merges: - rust-lang#128166 (Improved `checked_isqrt` and `isqrt` methods) - rust-lang#129170 (Add an ability to convert between `Span` and `visit::Location`) - rust-lang#129366 (linker: Synchronize native library search in rustc and linker) - rust-lang#129467 (derive(SmartPointer): assume pointee from the single generic and better error messages) - rust-lang#129494 (format code in tests/ui/threads-sendsync) - rust-lang#129527 (Don't use `TyKind` in a lint) - rust-lang#129617 (Update books) - rust-lang#129673 (Add fmt::Debug to sync::Weak<T, A>) - rust-lang#129683 (copysign with sign being a NaN can have non-portable results) - rust-lang#129689 (Move `'tcx` lifetime off of impl and onto methods for `CrateMetadataRef`) - rust-lang#129695 (Fix path to run clippy on rustdoc) r? `@ghost` `@rustbot` modify labels: rollup
…ax-pointee, r=compiler-errors derive(SmartPointer): assume pointee from the single generic and better error messages Fix rust-lang#129465 Actually RFC says that `#[pointee]` can be inferred when there is no ambiguity, or there is only one generic type parameter so to say. cc ``@Darksonn`` r? ``@compiler-errors``
…kingjubilee Rollup of 14 pull requests Successful merges: - rust-lang#128192 (rustc_target: Add various aarch64 features) - rust-lang#129170 (Add an ability to convert between `Span` and `visit::Location`) - rust-lang#129343 (Emit specific message for time<=0.3.35) - rust-lang#129378 (Clean up cfg-gating of ProcessPrng extern) - rust-lang#129401 (Partially stabilize `feature(new_uninit)`) - rust-lang#129467 (derive(SmartPointer): assume pointee from the single generic and better error messages) - rust-lang#129494 (format code in tests/ui/threads-sendsync) - rust-lang#129617 (Update books) - rust-lang#129673 (Add fmt::Debug to sync::Weak<T, A>) - rust-lang#129683 (copysign with sign being a NaN can have non-portable results) - rust-lang#129689 (Move `'tcx` lifetime off of impl and onto methods for `CrateMetadataRef`) - rust-lang#129695 (Fix path to run clippy on rustdoc) - rust-lang#129712 (Correct trusty targets to be tier 3) - rust-lang#129715 (Update `compiler_builtins` to `0.1.123`) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#129467 - dingxiangfei2009:smart-pointer-relax-pointee, r=compiler-errors derive(SmartPointer): assume pointee from the single generic and better error messages Fix rust-lang#129465 Actually RFC says that `#[pointee]` can be inferred when there is no ambiguity, or there is only one generic type parameter so to say. cc ```@Darksonn``` r? ```@compiler-errors```
let mut pointees = type_params | ||
.iter() | ||
.filter_map(|&(idx, span, is_pointee)| is_pointee.then_some((idx, span))) | ||
.fuse(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this fuse
here? type_params
is a Vec
so there is no way that this iterator could return None
and then later return Some
.
Fix #129465
Actually RFC says that
#[pointee]
can be inferred when there is no ambiguity, or there is only one generic type parameter so to say.cc @Darksonn
r? @compiler-errors