Add min_adt_const_params gate#153592
Conversation
|
|
| /// Allows using the `unadjusted` ABI; perma-unstable. | ||
| (internal, abi_unadjusted, "1.16.0", None), | ||
| /// Restrict adt_const_params fields to share same privacy. | ||
| (internal, adt_const_params_restricted_privacy, "CURRENT_RUSTC_VERSION", None), |
There was a problem hiding this comment.
I am really not sure on whether this should be internal or unstable (I guess should share issue number with adt_const_params, so it would be unstable then)
There was a problem hiding this comment.
this can be unstable. the difference there is generally about whether we expect users to actually use this feature. internal is for when users arent supposed to use it, but unstable is for stuff we expect/hope/want to stabilize eventually.
There was a problem hiding this comment.
thanks this looks good! can you change the feature to be min_adt_const_params and have it not depend on adt_const_params. i expect this will likely require updating any features().adt_const_params() feature checks across the compiler to also check for min_adt_const_params()
sorry for sending you down a different path initially
04c868a to
2871ec1
Compare
| (unstable, marker_trait_attr, "1.30.0", Some(29864)), | ||
| /// Enable mgca `type const` syntax before expansion. | ||
| (incomplete, mgca_type_const_syntax, "1.95.0", Some(132980)), | ||
| /// Restrict adt_const_params fields to share same privacy. |
There was a problem hiding this comment.
not sure about what should I write here as it will be a standalone feature
There was a problem hiding this comment.
I would do similar wording to what's on the adt_const_params entry, so maybe:
Allows additional const parameter types, such as
[u8; 10]or user defined types. User defined types must not have fields more private than the type itself.
| /// Enable mgca `type const` syntax before expansion. | ||
| (incomplete, mgca_type_const_syntax, "1.95.0", Some(132980)), | ||
| /// Restrict adt_const_params fields to share same privacy. | ||
| (unstable, min_adt_const_params, "CURRENT_RUSTC_VERSION", Some(95174)), |
There was a problem hiding this comment.
| (unstable, min_adt_const_params, "CURRENT_RUSTC_VERSION", Some(95174)), | |
| (unstable, min_adt_const_params, "CURRENT_RUSTC_VERSION", Some(154042)), |
I've made a new tracking issue for this since we'll probably want a separate list of unresolved questions and whatnot 🤔
| let struct_vis = tcx.visibility(adt.did()); | ||
| for variant in adt.variants() { | ||
| for field in &variant.fields { | ||
| if field.vis != struct_vis { |
There was a problem hiding this comment.
Can you add the following as a test:
#![feature(min_adt_const_params)]
use core::marker::ConstParamTy;
#[derive(ConstParamTy, Eq, PartialEq)]
struct Foo {
pub field: u32,
}I think this will fail under your PR due to using equality instead of checking fields are "at least as visible as the struct". I'm not really sure what the desired behaviour is but it'll need to figured out as part of the RFC and having a test which demonstrates the choice is useful.
There was a problem hiding this comment.
yes it will fail I think I tested this earlier, idk why I don't have this in test though. Will add to existing one
I see its better as the distinct test as it will show existing test doesn't require adt_const_params
|
implementation looks good to me, thanks for working on this :) |
adt_const_params_restricted_privacy gatemin_adt_const_params gate
| return Ok(()); | ||
| } | ||
|
|
||
| if tcx.features().min_adt_const_params() { |
There was a problem hiding this comment.
I guess this should actually be if tcx.features().min_adt_const_params() && !tcx.features().adt_const_params() so that once we stabilize min_adt_const_params you can avoid the restriction by enabling full adt const params
Can you add a test that the following compiles:
#![feature(min_adt_const_params, adt_const_params)]
use core::marker::ConstParamTy;
#[derive(ConstParamTy, Eq, PartialEq)]
enum Foo {
pub field: u32,
}fcb960a to
2c27a37
Compare
| return Ok(()); | ||
| } | ||
|
|
||
| if tcx.features().min_adt_const_params() && !tcx.features().adt_const_params() { |
There was a problem hiding this comment.
lol yeah I guess I didn't think deeply enough about the fact that this means you can implement ConstParamTy for arbitrary stuff if you don't enable any feature gate x)
| if tcx.features().min_adt_const_params() && !tcx.features().adt_const_params() { | |
| if !tcx.features().adt_const_params() { |
I'll r+ once this is done
There was a problem hiding this comment.
Make sense, although even if they would implement ConstParamTy_ for their arbitrary struct, they couldn't use it anyway without min_adt_const_params or adt_const_params :> I am playing with tests a bit to include we ensurew this also without any gate
There was a problem hiding this comment.
in theory you could have an unstable crate without min_adt_const_params and then a downstream crate which does enable it and can observe the broken impl and use it 🤔
There was a problem hiding this comment.
but if they would implement it with adt_const_params, they still can use this unrestricted impl with min_adt_const_params on another crate 🧐
There was a problem hiding this comment.
aux:
#![allow(incomplete_features)]
#![feature(adt_const_params, const_param_ty_trait)]
use std::marker::ConstParamTy_;
#[derive(PartialEq, Eq)]
pub struct Fumo {
cirno: i32,
pub(crate) reimu: i32
}
impl ConstParamTy_ for Fumo {}test:
//@aux-build:cross-crate-const_param_ty-impl-leak.rs
#![allow(incomplete_features)]
#![feature(min_adt_const_params)]
extern crate cross_crate_const_param_ty_impl_leak;
use cross_crate_const_param_ty_impl_leak::Fumo;
fn fumo_fn<const N: Fumo>() {}
fn main() {}this passes for example
There was a problem hiding this comment.
hmm yeah thats kinda weird lol. though it doesn't feel as bad to me because atleast someone enabled the feature gate for supporting private fields in const generics
There was a problem hiding this comment.
eh I would add this test but it seems aux isn't supposed to fail -_-
2c27a37 to
4222812
Compare
This comment was marked as outdated.
This comment was marked as outdated.
|
It happens 😁 |
11d5d14 to
72fbd1c
Compare
|
@bors r+ thx |
…d_privacy-gate, r=BoxyUwU Add `min_adt_const_params` gate Add `min_adt_const_params` feature gate to disallow `ConstParamTy_` impl for types which fields visibility mismatches with struct itself r? BoxyUwU
Rollup of 7 pull requests Successful merges: - #153286 (various fixes for scalable vectors) - #153592 (Add `min_adt_const_params` gate) - #154675 (Improve shadowed private field diagnostics) - #154653 (Remove rustc_on_unimplemented's append_const_msg) - #154743 (Remove an unused `StableHash` impl.) - #154752 (Add comment to borrow-checker) - #154764 (Add tests for three ICEs that have already been fixed)
Rollup of 7 pull requests Successful merges: - #153286 (various fixes for scalable vectors) - #153592 (Add `min_adt_const_params` gate) - #154675 (Improve shadowed private field diagnostics) - #154653 (Remove rustc_on_unimplemented's append_const_msg) - #154743 (Remove an unused `StableHash` impl.) - #154752 (Add comment to borrow-checker) - #154764 (Add tests for three ICEs that have already been fixed)
…d_privacy-gate, r=BoxyUwU Add `min_adt_const_params` gate Add `min_adt_const_params` feature gate to disallow `ConstParamTy_` impl for types which fields visibility mismatches with struct itself r? BoxyUwU
Rollup of 10 pull requests Successful merges: - #154376 (Remove more BuiltinLintDiag variants - part 4) - #127534 (feat(core): impl Step for NonZero<u*>) - #153286 (various fixes for scalable vectors) - #153592 (Add `min_adt_const_params` gate) - #154675 (Improve shadowed private field diagnostics) - #154703 (Fix trailing comma in lifetime suggestion for empty angle brackets) - #154653 (Remove rustc_on_unimplemented's append_const_msg) - #154743 (Remove an unused `StableHash` impl.) - #154752 (Add comment to borrow-checker) - #154764 (Add tests for three ICEs that have already been fixed)
Rollup of 7 pull requests Successful merges: - #153286 (various fixes for scalable vectors) - #153592 (Add `min_adt_const_params` gate) - #154675 (Improve shadowed private field diagnostics) - #154653 (Remove rustc_on_unimplemented's append_const_msg) - #154743 (Remove an unused `StableHash` impl.) - #154752 (Add comment to borrow-checker) - #154764 (Add tests for three ICEs that have already been fixed)
Rollup merge of #153592 - zedddie:adt_const_params_restricted_privacy-gate, r=BoxyUwU Add `min_adt_const_params` gate Add `min_adt_const_params` feature gate to disallow `ConstParamTy_` impl for types which fields visibility mismatches with struct itself r? BoxyUwU
View all comments
Add
min_adt_const_paramsfeature gate to disallowConstParamTy_impl for types which fields visibility mismatches with struct itselfr? BoxyUwU