Skip to content

report call site of inlined scopes for large assignment lints #139551

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Apr 10, 2025

Conversation

jogru0
Copy link
Contributor

@jogru0 jogru0 commented Apr 8, 2025

Addressed issue: #121672
Tracking issue: #83518

r? @oli-obk

I tried to follow your comment about what to do here. However, I'm totally unfamiliar with the code so far (this is my first contribution touching compiler code), so I apologize in advance if I did something stupid 😅

In particular, I'm not sure I use the correct source scope to look for inline data, as there is a whole IndexVec of them. My changes definitely did something, as can be seen by the added ui test. However, the result is not as anticipated in the issue:

LL |     let cell = std::cell::UnsafeCell::new(data);
   |                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ value moved from here

instead of

LL |     let cell = std::cell::UnsafeCell::new(data);
   |                                           ^^^^ value moved from here

raising my suspicion that maybe I got the wrong source scope.

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 8, 2025
.body
.source_scopes
.get(source_info.scope)
.and_then(|source_scope_data| source_scope_data.inlined)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, you could try just returning early if inlined is Some. I think then you should get the improved span that you were looking for. The call site span points to the entire function call indeed, and will result in exactly what you showed. But I'm not sure the more detailed span of the argument still exists

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I do this early return, I don't get any lint at all:

failures:

---- [ui] tests/ui/lint/large_assignments/inline_mir.rs stdout ----

error: ui test did not emit an error
note: by default, ui tests are expected not to compile
status: exit status: 0

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm our docs are very underwhelming on this. Maybe use the source_scope_data.span if inlined is set?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the ui test example, span points to

UnsafeCell { value }

and source_scope_data.span points to

pub const fn new(value: T) -> UnsafeCell<T> {
    UnsafeCell { value }
}

so both seem to be wrong to me.

By the way, both currently don't show up in the actual stderr output, contrary to how it was reported in the issue originally. They both look like this:

error: moving 9999 bytes
  --> $SRC_DIR/core/src/cell.rs:LL:COL
  ::: $SRC_DIR/core/src/cell.rs:LL:COL
   |
   = note: value moved from here
   |
   = note: The current maximum size is 1000, but it can be customized with the move_size_limit attribute: `#![move_size_limit = "..."]`
note: the lint level is defined here
  --> $DIR/inline_mir.rs:2:9
   |
LL | #![deny(large_assignments)]
   |         ^^^^^^^^^^^^^^^^^

error: aborting due to 1 previous error

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm... I guess we lose the argument span entirely in favor of spans from the inlined item. That may be something we may want to adjust, but not in this PR. I'd say we land it with the caller span

@oli-obk
Copy link
Contributor

oli-obk commented Apr 9, 2025

Thanks for the analyses of the alternatives!

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Apr 9, 2025

📌 Commit 6d71fc1 has been approved by oli-obk

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 9, 2025
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 9, 2025
…iaskrgr

Rollup of 10 pull requests

Successful merges:

 - rust-lang#138470 (Test interaction between RFC 2229 migration and use closures)
 - rust-lang#138628 (Add more ergonomic clone tests)
 - rust-lang#139164 (std: improve documentation for get_mut() methods regarding forgotten guards)
 - rust-lang#139488 (Add missing regression GUI test)
 - rust-lang#139489 (compiletest: Add directive `dont-require-annotations`)
 - rust-lang#139513 (Report higher-ranked trait error when higher-ranked projection goal fails in new solver)
 - rust-lang#139521 (triagebot: roll compiler reviewers for rustc/unstable book)
 - rust-lang#139532 (Update `u8`-to-and-from-`i8` suggestions.)
 - rust-lang#139551 (report call site of inlined scopes for large assignment lints)
 - rust-lang#139575 (Remove redundant words)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit e962e52 into rust-lang:master Apr 10, 2025
6 checks passed
@rustbot rustbot added this to the 1.88.0 milestone Apr 10, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Apr 10, 2025
Rollup merge of rust-lang#139551 - jogru0:121672, r=oli-obk

report call site of inlined scopes for large assignment lints

Addressed issue: rust-lang#121672
Tracking issue: rust-lang#83518

r? `@oli-obk`

I tried to follow your comment about what to do [here](rust-lang#121672 (comment)). However, I'm totally unfamiliar with the code so far (this is my first contribution touching compiler code), so I apologize in advance if I did something stupid 😅

In particular, I'm not sure I use the _correct_ source scope to look for inline data, as there is a whole `IndexVec` of them. My changes definitely did something, as can be seen by the added ui test. However, the result is not as anticipated in the issue:
```
LL |     let cell = std::cell::UnsafeCell::new(data);
   |                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ value moved from here
```
instead of
```
LL |     let cell = std::cell::UnsafeCell::new(data);
   |                                           ^^^^ value moved from here
```
raising my suspicion that maybe I got the wrong source scope.
github-actions bot pushed a commit to model-checking/verify-rust-std that referenced this pull request Apr 19, 2025
…iaskrgr

Rollup of 10 pull requests

Successful merges:

 - rust-lang#138470 (Test interaction between RFC 2229 migration and use closures)
 - rust-lang#138628 (Add more ergonomic clone tests)
 - rust-lang#139164 (std: improve documentation for get_mut() methods regarding forgotten guards)
 - rust-lang#139488 (Add missing regression GUI test)
 - rust-lang#139489 (compiletest: Add directive `dont-require-annotations`)
 - rust-lang#139513 (Report higher-ranked trait error when higher-ranked projection goal fails in new solver)
 - rust-lang#139521 (triagebot: roll compiler reviewers for rustc/unstable book)
 - rust-lang#139532 (Update `u8`-to-and-from-`i8` suggestions.)
 - rust-lang#139551 (report call site of inlined scopes for large assignment lints)
 - rust-lang#139575 (Remove redundant words)

r? `@ghost`
`@rustbot` modify labels: rollup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants