warn on align on fields to avoid breaking changes #143868
Conversation
|
|
rustbot
added
A-attributes
This comment has been minimized.
This comment has been minimized.
jdonszelmann
force-pushed
the
fix-align-on-fields
branch
from
e38322a to
a5ab682
Compare
| Target::Field => { | ||
| self.tcx.emit_node_span_lint( | ||
| UNUSED_ATTRIBUTES, | ||
| hir_id, | ||
| attr_span, | ||
| AlignOnFields { span }, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Question: do we already have test coverage for this case?
There was a problem hiding this comment.
We have about 20 of these cases and to the best of my knowledge no test coverage for any :ferris clueless:
There was a problem hiding this comment.
Don't really have time to atm, am on holiday. Just knew how to fix this issue well
There was a problem hiding this comment.
Yes, we may wish to land this and maybe open an issue about getting test coverage.
There was a problem hiding this comment.
feel free to assign me or @JonathanBrouwer (who I think was interested in this sort of thing)
|
I'm landing this to fix this right now on nightly, unsure if this is the right fix for beta. @bors r+ rollup |
bors
added
S-waiting-on-bors
…r=workingjubilee warn on align on fields to avoid breaking changes r? `@workingjubilee`
Rollup of 16 pull requests Successful merges: - #142885 (core: Add `BorrowedCursor::with_unfilled_buf`) - #143217 (Port #[link_ordinal] to the new attribute parsing infrastructure) - #143355 (wrapping shift: remove first bitmask and table) - #143448 (remote-test-client: Exit code `128 + <signal-number>` instead of `3`) - #143592 (UWP: link ntdll functions using raw-dylib) - #143681 (bootstrap/miri: avoid rebuilds for test builds) - #143710 (Updates to random number generation APIs) - #143724 (Tidy cleanup) - #143820 (Fixed a core crate compilation failure when enabling the `optimize_for_size` feature on some targets) - #143850 (Compiletest: Simplify {Html,Json}DocCk directive handling) - #143855 (Port `#[omit_gdb_pretty_printer_section]` to the new attribute parsing) - #143868 (warn on align on fields to avoid breaking changes) - #143875 (update issue number for `const_trait_impl`) - #143881 (Use zero for initialized Once state) - #143887 (Run bootstrap tests sooner in the `x test` pipeline) - #143893 (Don't require `eh_personality` lang item on targets that have a personality) Failed merges: - #143878 (Port `#[pointee]` to the new attribute parsing infrastructure) - #143891 (Port `#[coverage]` to the new attribute system) r? `@ghost` `@rustbot` modify labels: rollup
…r=workingjubilee warn on align on fields to avoid breaking changes r? ``@workingjubilee``
Rollup of 17 pull requests Successful merges: - #142885 (core: Add `BorrowedCursor::with_unfilled_buf`) - #143217 (Port #[link_ordinal] to the new attribute parsing infrastructure) - #143355 (wrapping shift: remove first bitmask and table) - #143448 (remote-test-client: Exit code `128 + <signal-number>` instead of `3`) - #143681 (bootstrap/miri: avoid rebuilds for test builds) - #143710 (Updates to random number generation APIs) - #143724 (Tidy cleanup) - #143738 (Move several float tests to floats/mod.rs) - #143820 (Fixed a core crate compilation failure when enabling the `optimize_for_size` feature on some targets) - #143850 (Compiletest: Simplify {Html,Json}DocCk directive handling) - #143855 (Port `#[omit_gdb_pretty_printer_section]` to the new attribute parsing) - #143868 (warn on align on fields to avoid breaking changes) - #143875 (update issue number for `const_trait_impl`) - #143881 (Use zero for initialized Once state) - #143887 (Run bootstrap tests sooner in the `x test` pipeline) - #143893 (Don't require `eh_personality` lang item on targets that have a personality) - #143901 (Region constraint nits) Failed merges: - #143878 (Port `#[pointee]` to the new attribute parsing infrastructure) - #143891 (Port `#[coverage]` to the new attribute system) r? `@ghost` `@rustbot` modify labels: rollup
| Target::Field => { | ||
| self.tcx.emit_node_span_lint( | ||
| UNUSED_ATTRIBUTES, | ||
| hir_id, | ||
| attr_span, | ||
| AlignOnFields { span }, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Question: wait hold on, what does this PR intend to do? If the intention was to not error on #[align] in various positions where we previously did not do, then this PR doesn't do that, because #142507 is a breaking change even if #[align] is an unstable built-in attribute, because unstable built-in attributes still participate in name resolution, and thus will still cause the ambiguity. This PR additionally produces another warning (i.e. unused_attributes) but does not prevent the name resolution ambiguity error from happening, cf. #134963.
Counter-example (I tried this test which fails on master, and also the commit from this PR, ignoring the irrelevant unstable fn_align feature gate warnings):
// tests/ui/whatever_subdir/test.rs
//@ proc-macro: my_derive.rs
//@ edition: 2024
use my_derive::MyDerive;
#[derive(MyDerive)]
#[align]
//~^ ERROR `align` is ambiguous
pub struct Foo;
fn main() {}// tests/ui/whatever_subdir/auxiliary/my_derive.rs
//@ edition: 2024
extern crate proc_macro;
use proc_macro::{Span, TokenStream};
#[proc_macro_derive(
MyDerive,
attributes(
align
)
)]
pub fn derive_custom(_item: TokenStream) -> TokenStream {
TokenStream::new()
}I think if we wanted to prevent that ambiguity error from happening entirely, we'd need to pick one of the following options (or more) or some alternative solution:
- Have to fix how built-in attributes are resolved versus user attributes
- Revert use
#[align]attribute forfn_align#142507 - Rename
#[align]to something less common.
There was a problem hiding this comment.
(That is, I think this PR itself is correct, but doesn't address #143834.)
r? @workingjubilee