Tracking Issue for large_assignments lint

[oli-obk]

This is a tracking issue for the implementation of the MCP rust-lang/compiler-team#420
The feature gate for the issue is #![feature(large_assignments)].

About tracking issues

Tracking issues are used to record the overall progress of implementation.
They are also used as hubs connecting to other relevant issues, e.g., bugs or open design questions.
A tracking issue is however not meant for large scale discussion, questions, or bug reports about a feature.
Instead, open a dedicated issue for the specific matter and add the relevant feature gate label.

Steps

• Implement the RFC (Implement a lint that highlights all moves larger than a configured limit #83519)
• make sure we inform the user properly about the fact that they can change the limit E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue.
  • this just needs an additional note on the diagnostic informing the user about the current limit and how to change it.
• reduce the noisiness (it's reported multiple times on different parts of an expression, but should only trigger on the innermost/first one) E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue.
  • this can be done by adding the current span to a Vec on self in https://github.com/oli-obk/rust/blob/85b1c67910dbeede8aabb16bc0c1c09a2fefe8ab/compiler/rustc_mir/src/monomorphize/collector.rs#L784
  • and then, right before https://github.com/oli-obk/rust/blob/85b1c67910dbeede8aabb16bc0c1c09a2fefe8ab/compiler/rustc_mir/src/monomorphize/collector.rs#L760 checking if the current span overlaps with any span in the vec.
• stop linting in box/arc initialization E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate.
• make span point to specific arg when moving large args into functions (large_assignments: Lint on specific large args passed to functions #116520)
• Differentiate between Operand::Move and Operand::Copy, see rustc_monomorphize: Introduce check_fn_args_move_size() #116310 (comment) and https://rust-lang.zulipchat.com/#narrow/stream/182449-t-compiler.2Fhelp/topic/arg.20Spans.20for.20TerminatorKind.3A.3ACall.3F/near/393865180
  • large_assignments: Allow moves into functions #120773
• Make sure diagnostics point to correct span when -Zinline-mir is involed. See large_assignments: Unactionable diagnostics with -Zinline-mir #121672
• fix lint duplication with multiple generic instantiations
• Suggest borrow instead of #[allow(...)] if possible
• enable the lint by default with a 4k threshold (Set default move_size_limit to 4kB for large_assignments lint on nightly #115684)
• disable the lint in tests (by default, users can still turn it on) E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate.
• move lint logic out of compiler/rustc_monomorphize/src/collector.rs (we should do this after we enable the lint by default since doing this has a high risk of regressing performance)
• Adjust documentation (see instructions on rustc-dev-guide)
• Stabilization PR (see instructions on rustc-dev-guide)

Unresolved Questions

• Should this threshold be configurable via the command line?
• Should we split lint into multiple size limits lints?

Implementation history

• https://github.com/rust-lang/rust/pull/83519

[oli-obk]

We don't really have a precedent for heuristic lints. The closest equivalent are the other raisable limits. I don't think we can use crater to find good limits, as most large types will be occuring in binary crates or tests on edge cases.

So... I think we should pick an arbitrary limit (e.g. 4kb) and only enable it by default if cfg(test) is not set and just wait for reports to come in. It's easily circumvented (just change the limit)

So: things to do before enabling:

• we should make sure we inform the user properly about the fact that they can change the limit
• we should disable the lint in tests (by default, users can still turn it on)
• should look into reducing the noisiness (it's reported multiple times on different parts of an expression, but should only trigger on the innermost/first one)

[joshtriplett]

Sounds reasonable to me. And yes, I think the most obvious thresholds would be something like 1k or 4k.

[nikomatsakis]

One thing I considered: multiple lints for different size thresholds? Probably not so nice.

[scottmcm]

Multiple lints could be nice for having a couple of different thresholds, with correspondingly-different default lint levels.

As a strawman, thinking only about x64,

• huge_assignments, deny-by-default, for (64 KiB, ∞)
• large_assignments, warn-by-default, for (2 KiB, 64 KiB]
• medium_assignments, allow-by-default, for (64 B, 2 KiB]

But absolutely my biggest uncertainty is how to specify the threshold, and how to scope it.

Perhaps the defaults should be something that the target definition gets to pick? I assume msp430-none-elf wants something very different from x86_64_v3-unknown-linux-gnu.

[tmiasko]

My general experience from testing the lint with limit starting from around a few kilobytes, is that it is very likely to lint about copies of large structs. This is actionable, because one can wrap the struct in a box or an arc, but leads to a situation where the initialization of a box or an arc will still be linted about, which is unfortunately generally unactionable currently

[joshtriplett]

I think, for a first pass, we should just have a single lint, and set the threshold high enough that people won't typically encounter it (e.g. 1k). If we find that people want the threshold lower, and want to have different behavior at different thresholds, we could talk about having multiple lints with different thresholds. But in practice I'd expect people to lower or raise the threshold, but not need multiple thresholds.

[nikomatsakis]

My general experience from testing the lint with limit starting from around a few kilobytes, is that it is very likely to lint about copies of large structs. This is actionable, because one can wrap the struct in a box or an arc, but leads to a situation where the initialization of a box or an arc will still be linted about, which is unfortunately generally unactionable currently

This is interesting. We may want to have certain "exemptions" from the lint.

[RalfJung]

Regarding the implementation, I am slightly concerned that this lint has logic that is entangled with compiler/rustc_monomorphize/src/collector.rs. That file does important and subtle things; is there any way we can avoid mixing that up with the lint logic?

[Enselic]

I managed to make some progress on this. However, other things have arisen that I would like to focus my attention on, so I will now unassign myself to unblock others from taking over the implementation of this feature. I want to thank my mentor oli-obk who was of great help during my work on this 🙏

The next natural step for this feature would be to fix #121672.
See the top issue description for a somewhat updated list of things to do after that.

@rustbot release-assignment

[oli-obk]

Thanks for the work you've done!