Conversation
|
Some changes occurred in compiler/rustc_attr_ir cc @jdonszelmann, @JonathanBrouwer Some changes occurred in compiler/rustc_attr_parsing cc @jdonszelmann, @JonathanBrouwer Some changes occurred in compiler/rustc_passes/src/check_attr.rs |
|
Thanks for the pull request, and welcome! The Rust Project has assigned @mejrs (or someone else) to review your changes, you should hear from them (or someone else) within the next two weeks. Please see the contribution instructions and our LLM policy for more information. |
There was a problem hiding this comment.
To improve readability, I decided to have the attribute attach to the ADT rather than to a particular parameter
On the other hand, this choice complicates the syntax (in a different way) and, as you say, introduces some ambiguity. It also means we have to check whether the given parameter really is a generic parameter of the type. I strongly prefer to have it on the generic parameter instead.
|
Reminder, once the PR becomes ready for a review, use |
| } | ||
| } | ||
|
|
||
| fn check_rustc_assert_variance( |
There was a problem hiding this comment.
In PR #161351 I moved a #[rustc_dump*] impl out of check_attr because I believe this module is meant to only contain complex target & validity checks for attributes, not however their "business logic".
I guess rustc_hir_analysis would be a good fit here, too?
There was a problem hiding this comment.
I see no harm in it being in check_attr, stuff like "is #[may_dangle] on a Drop impl" is also in check_attr. Happy to cede to wherever you think this fits best though.
There was a problem hiding this comment.
Ah, I just thought y'all wanted to yeet check_attr.rs at some point & have parsing & validity checks in rustc_attr_parsing in which case it would be better not to add onto it but I don't know if that's still the plan / even feasible.
There was a problem hiding this comment.
There's a decent amount of "need to run queries etc for this check" code in check_attr, so getting rid of check_attr entirely isn't going to happen sadly.
|
@rustbot ready Following mejrs' feedback, I changed the attribute to be attached to the generic parameters instead of the ADT. This simplified a lot of the implementation (AttributeKind::RustcAssertVariance no longer has to be boxed due to size concerns for instance). I updated the tests to match, added support + testing for bivariance asserts, added a test for const generics, and fixed the diagnostic message. I left the variance check in check_attrs.rs for now, as it is arguably just a complex test of valid targets, but will happily move it if needed. |
There was a problem hiding this comment.
I think it's a little confusing to try to tell what's wrong by just looking at the diagnostics. Because the attribute is long, many structs will get formatted like
struct Foo6<
#[rustc_assert_variance(contravariant)]
//~^ NOTE required by this annotation
'a,
}so the error messages will look like this:
error: `Foo6` is covariant in `'a`; expected variance: contravariant
--> $DIR/rustc_assert_variance.rs:44:5
|
LL | 'a,
| ^^
|
and just by glancing at this it's hard to tell what's happening. And someone, somewhere, is going to have to look at this when it's in a CI error log or lazily pasted into an issue or whatever.
Can you use the struct definition span for the primary span and the parameter span for a label?
Then the error message should look something like this:
error: `Foo6` is expected to be contravariant in `'a` but it is not
--> $DIR/rustc_assert_variance.rs:44:5
/ struct Foo6<
| #[rustc_assert_variance(contravariant)]
| 'a,
| } ^^ `Foo6` is covariant in `'a`
|-^(possibly with the note pointing into that as well)
|
Other than the error message spans it looks good. Please apply it to a bunch of std types next. I'm thinking of the "phantom family", the "cell" types (Cell/UnsafeCell/etc) and the lock and guard types? Then can you figure out who actually asked for this (I'm not sure, someone on the libs team?) and ask them to take a look and whether they're happy with it? |
…stcAssertVariance for diagnostic purposes.
…the size of `AttributeKind` - Add ui test for #[rustc_assert_variance]
…rameters instead of their parent ADTs - Add support for bivariance to rustc_assert_variance - Update rustc_assert_variance ui test accordingly and add case for const generic params - Minor diagnostic message fix
9cc4d54 to
78f106f
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
@Mark-Simulacrum I think you were the one who initially suggested something like this in the discussion on #159838 (comment). Does this seem like a good solution to assert variance properties of public APIs in std, or do you think something is still missing here? |
This comment has been minimized.
This comment has been minimized.
|
Please also update the pr description, its outdated. |
…r types, cell types, sync lock and guard types, Arc, Rc, Unique, Box, and NonNull
78f106f to
08c336c
Compare
Done. I also fixed a ui test failure that was happening because of the definition of MutexGuard spilling onto multiple lines with the attribute applied. |
cc @WaffleLapkin @clarfonthey as well |
| Covariant, | ||
| Invariant, | ||
| Contravariant, | ||
| Bivariant, |
There was a problem hiding this comment.
I know it technically doesn't matter for the implementation, but I do kinda wish these were:
| Covariant, | |
| Invariant, | |
| Contravariant, | |
| Bivariant, | |
| Invariant = 0b00, | |
| Covariant = 0b01, | |
| Contravariant = 0b10, | |
| Bivariant = 0b11, |
so that they internally represent the variance a bit better, even if we don't rely on that.
There was a problem hiding this comment.
I like this idea, but it seems more relevant for rustc_type_ir::Variance since that's what's actually used by the compiler to represent variance.
There was a problem hiding this comment.
(also worth asking whether these two types should be different at all)
There was a problem hiding this comment.
I would rather not that rustc_attr_ir depends on rustc_type_ir, or the other way around.
It could live earlier in the crate graph, in rustc_structures, but moving it would also mean moving a lot of Variance related methods out of rustc_type_ir. So merging these two comes with downsides.
| const ALLOWED_TARGETS: AllowedTargets<'_> = AllowedTargets::AllowList(&[ | ||
| Allow(Target::TypeParam), | ||
| Allow(Target::LifetimeParam), | ||
| Allow(Target::ConstParam), |
There was a problem hiding this comment.
Can const parameters be anything other than invariant? 😨
There was a problem hiding this comment.
Not to my knowledge. It's more there for syntactic completeness so that the attribute can attach to any generic parameter.
|
Side note worth mentioning: there currently isn't a test here for something that has an actually bivariant lifetime, and that probably should be added. |
I'm struggling to come up with an example where this is possible for an ADT (as opposed to a function). You can't do something like a struct that only stores a I'm not super familiar with the intricacies of variance semantics though, so if you know how this could be done I'd love to add a case like that for sure. |
|
So, was looking at this myself, and here's a great example of a bivariant lifetime: struct Test<'a>;🤦🏻 Yes, I had forgotten about this case, but it's a pretty easy one to cover. |
|
I already gave a positive example for bivariant parameters here #163578 (comment) but I guess y'all couldn't find it because it's marked as resolved. As I said, bivariant parameters are legal if they're constrained. |
That case is in the tests, we were discussing bivariant lifetime parameters specifically.
The issue with this is that when I try to compile this with the attribute, the 'lifetime not used' error aborts compilation before the attribute is checked, so there's no error from the attribute to test: |
|
You could just allow the lint and then it should work fine. |
|
The very same thing applies: You just need to constrain the bivariant generic parameter in question: struct X</*bi*/ 'a, /*co*/ T>(T) where T: Iterator<Item = &'a ()>; |
I believe unused generic parameters are a hard error in the compiler, not a lint.
🤦♀️ this is so obvious in retrospect. Test case added, thank you for the help. |
Parameters yes, lifetime parameters no, unfortunately. |
Really? I can't find what the lint to disable would even be, and the Rust Reference seems to say otherwise:
|
This comment has been minimized.
This comment has been minimized.
Right, I actually completely misremembered this; you can have unconstrained lifetime parameters in impls, but not structs. |
…/attributes/rustc_assert_variance.rs
6ced8bb to
7cf50bf
Compare
View all comments
Add a new attribute
#[rustc_assert_variance]to ensure the variance of a generic parameter of an algebraic data type at compile time.The attribute attaches directly to generic parameters of an ADT declaration, and will cause a compile error if the variance of the type does not match what is stated. For example:
Trying to compile this struct definition gives this error:
The goal is for this to help prevent regression in the variance of standard library types leading to accidental breaking changes in the API. This PR adds this attribute to the definitions of many std wrapper types to this end.
Closes #163513
r? mejrs