-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Don't allow the #[pointee]
attribute where it doesn't belong
#128721
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @pnkfelix (or someone else) some time within the next two weeks. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
@rustbot label F-derive_smart_pointer |
This still allows |
This comment has been minimized.
This comment has been minimized.
db05a4d
to
027b6db
Compare
This comment has been minimized.
This comment has been minimized.
027b6db
to
4043d90
Compare
This PR also adds a check that |
That change could still make sense. |
☔ The latest upstream changes (presumably #129202) made this pull request unmergeable. Please resolve the merge conflicts. |
4043d90
to
fcdb5f0
Compare
I've shrunk the PR down to just checking if the attribute is applied to a generic type parameter. |
☔ The latest upstream changes (presumably #129721) made this pull request unmergeable. Please resolve the merge conflicts. |
fcdb5f0
to
a26d609
Compare
|
||
fn visit_generic_param(&mut self, param: &'a rustc_ast::GenericParam) -> Self::Result { | ||
match param.kind { | ||
GenericParamKind::Type { .. } => 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.
Hmm.
Technically I think you still need to ensure you recur into the default: &Option<P<Ty>>
that's held within that enum payload above.
I think the below is the kind of case you need to worry about that motivates recurring here:
struct SomeStruct<'a, T = [u32; const { #[pointee] struct UhOhYetAnotherTypeDefn<U>; 10 }]> { ... }
(Yes its a corner 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.
You can model the recursion you need to do by looking at walk_generic_param
, and just leaving out the visit_attribute
part:
rust/compiler/rustc_ast/src/visit.rs
Lines 749 to 767 in c39f318
pub fn walk_generic_param<'a, V: Visitor<'a>>( | |
visitor: &mut V, | |
param: &'a GenericParam, | |
) -> V::Result { | |
let GenericParam { id: _, ident, attrs, bounds, is_placeholder: _, kind, colon_span: _ } = | |
param; | |
walk_list!(visitor, visit_attribute, attrs); | |
try_visit!(visitor.visit_ident(*ident)); | |
walk_list!(visitor, visit_param_bound, bounds, BoundKind::Bound); | |
match kind { | |
GenericParamKind::Lifetime => (), | |
GenericParamKind::Type { default } => visit_opt!(visitor, visit_ty, default), | |
GenericParamKind::Const { ty, default, kw_span: _ } => { | |
try_visit!(visitor.visit_ty(ty)); | |
visit_opt!(visitor, visit_anon_const, default); | |
} | |
} | |
V::Result::output() | |
} |
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've fixed the edge case. As a comment in the new code mentions we end up rejecting valid code like:
#![feature(derive_smart_pointer)]
use std::marker::SmartPointer;
#[derive(SmartPointer)]
#[repr(transparent)]
struct Weird<
'a,
T: ?Sized,
const V: u32 = {
#[derive(SmartPointer)]
#[repr(transparent)]
struct UhOh<'b, #[pointee] X: ?Sized>(&'b X);
10
},
> {
ptr: &'a T,
}
Handling this properly isn't possible in the generic case since we can't know if the inner derive is actually a different trait with the same name. Given how unlikely the whole scenario is I've decided to go ahead with the changes anyway.
@rustbot author |
a26d609
to
cc9d8b5
Compare
This comment has been minimized.
This comment has been minimized.
cc9d8b5
to
aa4f16a
Compare
@rustbot review |
@bors r+ rollup |
…kingjubilee Rollup of 7 pull requests Successful merges: - rust-lang#128721 (Don't allow the `#[pointee]` attribute where it doesn't belong) - rust-lang#130479 (skip in-tree compiler build for llvm-bitcode-linker if ci-rustc is on) - rust-lang#130899 (Couple of changes to make it easier to compile rustc for wasm) - rust-lang#131225 (`rustc_borrowck` memory management tweaks) - rust-lang#131351 (Remove valgrind test suite and support from compiletest, bootstrap and opt-dist) - rust-lang#131359 (Fix used_underscore_binding in rustc_serialize) - rust-lang#131367 (Mark Boxy as on vacation) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#128721 - Brezak:pointee-in-strange-places, r=pnkfelix Don't allow the `#[pointee]` attribute where it doesn't belong Error if the `#[pointee]` attribute is applied to anything but generic type parameters. Closes rust-lang#128485 Related to rust-lang#123430
Error if the
#[pointee]
attribute is applied to anything but generic type parameters.Closes #128485
Related to #123430