Introduce #[diagnostic::on_move(message)] by rperier · Pull Request #150935 · rust-lang/rust

rperier · 2026-01-10T18:09:26Z

This is a first proposal. I have deliberately kept it simpler than diagnostic::on_unimplemented.

Few questions/remarks:

Do I need to move the OnMoveDirective logic into a dedicated module perhaps ? let's say into compiler/rustc_borrowck/src/diagnostics/on_move.rs
No problems to depend on crates like rustc_ast from the borrowck ?
Notes are not supported yet. While message and label are very static , in the sense that they are emitted in the same way from the same place in the borrowck, it is not the case for the notes. It would make the code more complex. But, I can add support for notes if it does make sense.

Suggestions are welcomed !

rustbot · 2026-01-10T18:09:30Z

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

Some changes occurred in compiler/rustc_passes/src/check_attr.rs

cc @jdonszelmann

rustbot · 2026-01-10T18:09:32Z

r? @JonathanBrouwer

rustbot has assigned @JonathanBrouwer.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

rperier · 2026-01-10T18:11:08Z

r? @estebank

compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs

mejrs · 2026-01-13T18:28:02Z

I'm currently working on migrating the existing diagnostic attribute infrastructure, can you do this PR after that? It's going to be quite a mess to resolve that conflict.

Is this meant to be only used by the standard library or also by users? If only by std, I think you should forego the attribute and instead check whether T implements UseCloned and recommend a clone (or .use if on nightly).

rperier · 2026-01-14T07:05:01Z

Yeah I can completly wait until you finished your work and do my PR after that, It was my intention to be honest . Resolving conflicts or rebasing is not an issue for me.

rustbot · 2026-01-21T19:15:46Z

Some changes occurred in compiler/rustc_attr_parsing

cc @jdonszelmann, @JonathanBrouwer

Changes to the size of AST and/or HIR nodes.

cc @nnethercote

Some changes occurred in compiler/rustc_hir/src/attrs

cc @jdonszelmann, @JonathanBrouwer

rperier · 2026-01-21T19:16:23Z

I have reworked my code and moved everything to rustc_attr_parsing and rustc_hir/src/attrs/data_structures.rs. Few questions/remarks remain:

Perhaps, It would make sense to allow this diagnostic attribute to be used only for nightly, for now, what do you think ?
I can wait until some refactoring for attributes are merged upstream, and wait for my PR being merged after that, if it helps, no problems for me

tests/ui/stats/input-stats.stderr

rperier · 2026-01-22T19:23:55Z

Rebased onto main, I have also fixed the size of AttributeKind::OnMove

mejrs · 2026-01-22T23:12:30Z

tests/ui/diagnostic_namespace/on_move/report_warning_on_malformed_options_without_delimiters.rs

+#[diagnostic::on_move(
+    message = Foo,
+    //~^ERROR expected a literal (`1u8`, `1.0f32`, `"string"`, etc.) here, found expression
+    label = "Bar",
+)]
+#[diagnostic::on_move(
+    message = "Foo"
+    label = "Bar",
+    //~^ERROR expected `,`, found `label`
+)]


This can't be an error; this must be a lint. But you will need #151516 to do so.

Hi. Since this morning, these "cases" are no longer handled at all. The parser does not report anything and I get an empty MetaItemListParser (there are no sub_parsers) in onMoveParser::convert , see #t-compiler/help > #diagnostic::on_move(message) (PR #150935) @ 💬 for the full description. Is it expected ? I mean I cannot emit the lint myself, I have an empty ArgParser context, and the parser reports nothing.

I think, I got it. This is due to https://github.com/rust-lang/rust/blob/main/compiler/rustc_attr_parsing/src/parser.rs#L132 . I can of course emit a lint myself when the MetaItemListParser is empty, it's a shame to no longer have the context of the error, we had a more fine grained context with a span before. The span now will be the full diagnostic attribute.

It is possible to replace the entire span https://github.com/rust-lang/rust/blob/main/compiler/rustc_attr_parsing/src/parser.rs#L135 by e.span (or build a span from e) ? so we would have a span on the attr in this case instead of the whole diagnostic attribute.

mejrs · 2026-01-22T23:52:27Z

compiler/rustc_hir/src/attrs/data_structures.rs

+#[derive(Clone, Debug, Encodable, Decodable, HashStable_Generic, PrintAttribute)]
+pub struct OnMoveAttrArg {
+    pub symbol: Symbol,
+    pub format_ranges: ThinVec<(usize, usize)>,
+}


I plan on moving a fair bit of the format and string stuff that on_unimplemented uses into rustc_hir; you should be able to reuse some of that in this PR :)

Sure, I will do

rperier · 2026-01-28T14:25:48Z

Rebased onto main. I have covered changes related to #151516 .
I assume that changes are still underway in rustc_attr_parsing , but I would be nice to discuss about generic meta item list errors:

Do a generic lint will be emitted by the parser itself in this case ? Instead of the parser of each diagnostic attribute has to do it by itself.. it would be better for code refactoring.
In case this is not the case, it might be helpful to being able to retrieve the span of the error instead of the span of the whole diagnostic attribute (better granularity) . See the previous discussion.

If I can help for this (by proposing a patch for a generic lint, for example), feel free to ask.

JonathanBrouwer · 2026-02-18T09:43:55Z

I'll wait until #151558 is merged and then re-review, sorry for all the delays :)
@rustbot +S-blocked

rperier · 2026-02-18T10:01:51Z

I'll wait until #151558 is merged and then re-review, sorry for all the delays :) @rustbot +S-blocked

no worries ;)

mejrs · 2026-02-24T22:25:17Z

#151558 has merged :)

JonathanBrouwer · 2026-02-25T07:09:18Z

@rustbot -S-blocked
Could you rebase? :)

rperier · 2026-02-25T08:31:37Z

@rustbot -S-blocked Could you rebase? :)

Yes, This is what I am currently doing :) . I am also realigning on stuffs like format attr handling, like it is done on ::on_unimplemented. But I can do this in severals steps, first let's rebasing and then I can rework some stuffs...

rperier · 2026-02-25T09:16:48Z

Rebased onto main:

I have moved on_move.rs into rustc_attr_parsing/src/attributes/diagnostic
I use the new find_attr!()

Remaining tasks:

Use the new format attr handling, like it is done in ::on_unimplemented
Re-analyse lints/errors to see if I can re-use existing ones (instead of introduce new ones), or can I share lints/error with ::on_unimplemented (I don't know yet)

EDIT: I have to use parse_directive_items(...)

JonathanBrouwer · 2026-02-27T09:30:33Z

I assume this is still work in progress right? Make sure to mark it as waiting-on-review when it's ready, then I'll take a look

rperier · 2026-02-27T10:06:28Z

Hi, yes, it is still work in progress. I think it should be ready this weekend.

JonathanBrouwer · 2026-02-27T10:12:10Z

No hurry :) just wanted to verify

This might be helpful for smart pointers to explains why they aren't Copy and what to do instead or just to let the user know that .clone() is very cheap and can be called without a performance penalty.

rustbot · 2026-03-02T18:09:49Z

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.

rperier · 2026-03-02T18:14:55Z

I think that's it for now, two remaining tasks:

I have to better use FormatArgs
Do I add "notes=" to diagnostic::on_move(...) as it is parsed and part of the Directive ? I could emit the notes seperately when these are present. It could be helpful to show an url for example or redirect the user to an api documentation.

@rustbot ready

mejrs

Thanks, I think this looks good generally. One important thing missing though is a feature gate; the consensus on Zulip was that this should be unstable for now.

Do I add "notes=" to diagnostic::on_move(...) as it is parsed and part of the Directive ?

Sure, why not? I'd prefer if this attribute was consistent with the others.

Please also add more tests:

for when the item is in another crate as the error message generally is different (there's no recommendation to implement Clone, for example).
for when we hit an explicit Copy bound, like fn take_copy<T: Copy>(t: T){}; then this attribute should do nothing. (or should it?)

View changes since this review

mejrs · 2026-03-02T18:22:57Z

compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs

+                    this: "This".to_string(),
+                    trait_sugared: "Sugared".to_string(),
+                    item_context: "Context",


You can set these to "" or anything else, they only do something for rustc_on_unimplemented anyway. (note to self: maybe this could be nicer)

Yeah, perhaps a "default" constructor or something, might help in such a case... no ?

mejrs · 2026-03-02T18:29:05Z

compiler/rustc_attr_parsing/src/attributes/diagnostic/on_move.rs

+impl<S: Stage> SingleAttributeParser<S> for OnMoveParser {
+    const PATH: &[Symbol] = &[sym::diagnostic, sym::on_move];
+    const ATTRIBUTE_ORDER: AttributeOrder = AttributeOrder::KeepInnermost;
+    const ON_DUPLICATE: OnDuplicate<S> = OnDuplicate::Warn;


The other diagnostic attributes allow multiple uses of the attribute and merge it, for consistency this one should too.

mejrs · 2026-03-02T18:47:17Z

compiler/rustc_passes/src/check_attr.rs

+        if let Some(directive) = directive {
+            directive.visit_params(&mut |argument_name, span| {
+                self.tcx.emit_node_span_lint(
+                    MALFORMED_DIAGNOSTIC_FORMAT_LITERALS,
+                    hir_id,
+                    span,
+                    errors::OnMoveMalformedFormatLiterals { name: argument_name },
+                )
+            });
+        }


IMO something like this should be allowed

#[diagnostic::on_move(message = "please do not the {X}")] pub struct MyStruct<X>{ _x: X, }

mejrs · 2026-03-02T18:51:50Z

compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs

+                && let Some(directive) = find_attr!(self.infcx.tcx, item_def.did(), OnMove { directive, .. }  => directive)
+                && let Some(directive) = directive


Suggested change

&& let Some(directive) = find_attr!(self.infcx.tcx, item_def.did(), OnMove { directive, .. } => directive)

&& let Some(directive) = directive

&& let Some(Some(directive)) = find_attr!(self.infcx.tcx, item_def.did(), OnMove { directive, .. } => directive)

Or

Suggested change

&& let Some(directive) = find_attr!(self.infcx.tcx, item_def.did(), OnMove { directive, .. } => directive)

&& let Some(directive) = directive

&& let Some(directive) = find_attr!(self.infcx.tcx, item_def.did(), OnMove { directive, .. } => directive).flatten()

mejrs · 2026-03-02T19:31:43Z

compiler/rustc_attr_parsing/src/attributes/diagnostic/on_move.rs

+
+    fn convert(cx: &mut AcceptContext<'_, '_, S>, args: &ArgParser) -> Option<AttributeKind> {
+        let Some(list) = args.list() else {
+            cx.expected_list(cx.attr_span, args);


Doesn't this raise an error?

mejrs · 2026-03-02T19:32:16Z

compiler/rustc_attr_parsing/src/attributes/diagnostic/on_move.rs

+            return None;
+        };
+
+        Some(AttributeKind::OnMove { span: cx.attr_span, directive: Some(Box::new(directive)) })


All these return None should really return OnMove {span, None}

rustbot added A-attributes Area: Attributes (`#[…]`, `#![…]`) S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 10, 2026

rustbot assigned JonathanBrouwer Jan 10, 2026

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jan 10, 2026

rustbot assigned estebank and unassigned JonathanBrouwer Jan 10, 2026

fmease reviewed Jan 10, 2026

View reviewed changes

compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs Outdated Show resolved Hide resolved

JonathanBrouwer mentioned this pull request Jan 13, 2026

Port diagnostic::do_not_recommend to new attr parsing #151056

Merged

rperier force-pushed the provide_diagnostic_on_move_for_smart_pointers branch from 9a0162d to 8071a7d Compare January 21, 2026 19:15