Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix #124478 - offset_of! returns a temporary #124484

Merged
merged 2 commits into from Apr 29, 2024
Merged

Fix #124478 - offset_of! returns a temporary #124484

merged 2 commits into from Apr 29, 2024

Conversation

GKFX
Copy link
Contributor

@GKFX GKFX commented Apr 28, 2024

This was due to the must_use() call. Adding HIR's OffsetOf to the must_use checking within the compiler avoids this issue while maintaining the lint output.

Fixes #124478. @tgross35

@GKFX
Fix rust-lang#124478 - offset_of! returns a temporary
ca79086 
This was due to the must_use() call. Adding HIR's OffsetOf to the must_use
checking within the compiler avoids this issue.
@rustbot
Copy link
Collaborator

rustbot commented Apr 28, 2024

r? @jieyouxu

rustbot has assigned @jieyouxu.
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

@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. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Apr 28, 2024
jieyouxu
jieyouxu approved these changes Apr 28, 2024
View reviewed changes
Copy link
Contributor

@jieyouxu jieyouxu left a comment •
edited

Choose a reason for hiding this comment

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

This looks reasonable to me from the compiler side, but I'll roll a libs reviewer just to make sure this also makes sense from the libs side.

@jieyouxu
Copy link
Contributor

Rolling a libs reviewer, if the changes look reasonable from the libs side, feel free to r=me.

r? libs

@rustbot rustbot assigned jhpratt and unassigned jieyouxu Apr 28, 2024
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@Nilstrieb
Copy link
Member

Nilstrieb commented Apr 28, 2024 •
edited

I am the libs reviewer and this makes sense, but I'm also a compiler reviewer and think it would make even more sense to add a test for the sample in the issue :3

(also imo this is the kind of change that does not need libs input, the builtin stuff is a compiler thing even if it's the library directory)

@jieyouxu
Copy link
Contributor

I am the libs reviewer and this makes sense, but I'm also a compiler reviewer and think it would make even more sense to add a test for the sample in the issue :3

Technically I think the behavior is exercised in tests/ui/offset-of/offset-of-self.rs, but it's probably better to add a standalone regression(?) test for the reference-to-temporary issue.

@jieyouxu
Copy link
Contributor

r? jieyouxu (assigning myself back since this doesn't need libs input)

@rustbot rustbot assigned jieyouxu and unassigned jhpratt Apr 28, 2024
@Nilstrieb
Copy link
Member

Technically I think the behavior is exercised in tests/ui/offset-of/offset-of-self.rs

the relevant behavior of it not creating a temporary is definitely not exercised or the test wouldn't pass :3

@tgross35 tgross35 mentioned this pull request Apr 28, 2024
Closed
@jieyouxu
Copy link
Contributor

Thanks!

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Apr 29, 2024

📌 Commit 2348eb2 has been approved by jieyouxu

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 29, 2024
@jieyouxu jieyouxu mentioned this pull request Apr 29, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 29, 2024
@bors
Auto merge of rust-lang#124527 - jieyouxu:rollup-eslzncy, r=jieyouxu
a8a1d3a 
Rollup of 7 pull requests

Successful merges:

 - rust-lang#124269 (Pretty-print parenthesis around binary in postfix match)
 - rust-lang#124415 (Use probes more aggressively in new solver)
 - rust-lang#124475 (Remove direct dependencies on lazy_static, once_cell and byteorder)
 - rust-lang#124484 (Fix rust-lang#124478 - offset_of! returns a temporary)
 - rust-lang#124504 (Mark unions non-const-propagatable in `KnownPanicsLint` without calling layout)
 - rust-lang#124508 (coverage: Avoid hard-coded values when visiting logical ops)
 - rust-lang#124522 ([Refactor] Rename `Lint` and `LintGroup`'s `is_loaded` to `is_externally_loaded` )

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 0580588 into rust-lang:master Apr 29, 2024
10 checks passed
@rustbot rustbot added this to the 1.80.0 milestone Apr 29, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Apr 29, 2024
@rust-timer
Unrolled build for rust-lang#124484
07e8ef2 
Rollup merge of rust-lang#124484 - GKFX:offset_of_must_use, r=jieyouxu

Fix rust-lang#124478 - offset_of! returns a temporary

This was due to the must_use() call. Adding HIR's `OffsetOf` to the must_use checking within the compiler avoids this issue while maintaining the lint output.

Fixes rust-lang#124478. `@tgross35`
@GKFX GKFX deleted the offset_of_must_use branch April 29, 2024 20:37
@RalfJung
Copy link
Member

RalfJung commented May 3, 2024

FWIW I think this could also have been fixed by marking must_use as #[rustc_promotable]. But this works as well. :)

@tgross35 tgross35 mentioned this pull request May 3, 2024
Open
@tgross35
Copy link
Contributor

tgross35 commented May 3, 2024

FWIW I think this could also have been fixed by marking must_use as #[rustc_promotable]. But this works as well. :)

Sounds easy :) I mentioned it in #124493

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jieyouxu jieyouxu jieyouxu approved these changes

Assignees

@jieyouxu jieyouxu

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. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.80.0
Development

Successfully merging this pull request may close these issues.

offset_of returns a temporary
9 participants
@GKFX @rustbot @jieyouxu @rust-log-analyzer @Nilstrieb @bors @RalfJung @tgross35 @jhpratt