Skip to content

Conversation

Qelxiros
Copy link
Contributor

@Qelxiros Qelxiros commented Jul 24, 2025

Tracking issue: #144419

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jul 24, 2025
@rust-log-analyzer

This comment has been minimized.

@Qelxiros Qelxiros marked this pull request as ready for review July 25, 2025 00:12
@rustbot
Copy link
Collaborator

rustbot commented Jul 25, 2025

r? @thomcc

rustbot has assigned @thomcc.
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. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 25, 2025
@Qelxiros
Copy link
Contributor Author

As far as I can tell, the ICE here is because of the function signature of Box::try_map. The return type is equivalent to ChangeOutputType<...> from the ACP, but that definition is in core, so I implemented it manually here. However, when I add the same type alias to alloc, the ICE persists. Is there a workaround here that remains in the spirit of the ACP? Is this a known compiler bug?

Copy link
Member

@joboet joboet left a comment

Choose a reason for hiding this comment

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

I have no clue about the ICE – perhaps try filing an issue?

View changes since this review

@Qelxiros Qelxiros force-pushed the smart_pointer_try_map branch from d5d5fea to 929d326 Compare September 2, 2025 20:53
@rustbot

This comment has been minimized.

@Qelxiros Qelxiros force-pushed the smart_pointer_try_map branch from 929d326 to 6a69f39 Compare September 2, 2025 20:57
@rustbot

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

if size_of::<T>() == size_of::<U>() && align_of::<T>() == align_of::<U>() {
let ptr = &raw const *this;
unsafe {
let new = f(ptr.read());
Copy link
Member

Choose a reason for hiding this comment

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

If f panics, this will lead to a double-free.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I think this iteration works. I'm still wrapping my head around working with closures that can panic, so thanks for bearing with me.

@rust-log-analyzer

This comment has been minimized.


impl<T> Drop for Guard<T> {
fn drop(&mut self) {
let _box = unsafe { Box::from_raw(self.0) };
Copy link
Member

Choose a reason for hiding this comment

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

This will still drop the Box's contents and hence result in a double-free.

Comment on lines 421 to 428
let guard = Guard(Box::into_raw(this));
unsafe {
let new = f(guard.0.read());
let ptr = guard.0;
mem::forget(guard);
ptr.cast::<U>().write(new);
Box::from_raw(ptr.cast::<U>())
}
Copy link
Member

Choose a reason for hiding this comment

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

I'd recommend converting the Box<T> to a Box<MaybeUninit<U>> before calling f. This ensures that the allocation is freed, but won't drop the old value on panic. After reading from that, you can use Box::write to write the final value back.

Suggested change
let guard = Guard(Box::into_raw(this));
unsafe {
let new = f(guard.0.read());
let ptr = guard.0;
mem::forget(guard);
ptr.cast::<U>().write(new);
Box::from_raw(ptr.cast::<U>())
}
let (allocation, value) = unsafe {
let ptr = Box::into_raw(this);
let value = ptr.read();
let allocation = Box::from_raw(ptr.cast::<MaybeUninit<U>());
(allocation, value)
};
Box::write(allocation, f(value))

@Qelxiros Qelxiros force-pushed the smart_pointer_try_map branch from 64b65c1 to bfc61d6 Compare September 4, 2025 00:21
@rust-log-analyzer

This comment has been minimized.

@Qelxiros
Copy link
Contributor Author

Qelxiros commented Sep 4, 2025

I'm not sure how to move forward given the ICE (issue #146174).

@rustbot label +S-blocked

@rustbot rustbot added the S-blocked Status: Blocked on something else such as an RFC or other implementation work. label Sep 4, 2025
tgross35 added a commit to tgross35/rust that referenced this pull request Sep 5, 2025
…iler-errors

fix ICE when suggesting `::new`

fixes rust-lang#146174

This code suggests to write `Foo::new(...)` when the user writes `Foo(...)` or `Foo { ... }` and the constructor is private, where `new` is some associated function that returns `Self`.

When checking that the return type of `new` is `Self`, we need to instantiate the parameters of `new` with infer vars, so we don't end up with a type like `Box<$param(0)>` in a context that doesn't have any parameters. But then we can't use `normalize_erasing_late_bound_regions` anymore because that goes though a query that can't deal with infer vars.

Since this is diagnostic-only code that is supposed to check for exactly `-> Self`, I think it's fine to just skip normalizing here, especially since The Correct Way<sup>TM</sup> would involve a probe and make this code even more complicated.

Also, the code here does almost the same thing, and these suggestions can probably be unified in the future: https://github.com/rust-lang/rust/blob/4ca8078d37c53ee4ff8fb32b4453b915116f25b8/compiler/rustc_hir_typeck/src/method/suggest.rs#L2123-L2129

r? `@compiler-errors`
cc `@Qelxiros` -- this should unblock rust-lang#144420
tgross35 added a commit to tgross35/rust that referenced this pull request Sep 5, 2025
…iler-errors

fix ICE when suggesting `::new`

fixes rust-lang#146174

This code suggests to write `Foo::new(...)` when the user writes `Foo(...)` or `Foo { ... }` and the constructor is private, where `new` is some associated function that returns `Self`.

When checking that the return type of `new` is `Self`, we need to instantiate the parameters of `new` with infer vars, so we don't end up with a type like `Box<$param(0)>` in a context that doesn't have any parameters. But then we can't use `normalize_erasing_late_bound_regions` anymore because that goes though a query that can't deal with infer vars.

Since this is diagnostic-only code that is supposed to check for exactly `-> Self`, I think it's fine to just skip normalizing here, especially since The Correct Way<sup>TM</sup> would involve a probe and make this code even more complicated.

Also, the code here does almost the same thing, and these suggestions can probably be unified in the future: https://github.com/rust-lang/rust/blob/4ca8078d37c53ee4ff8fb32b4453b915116f25b8/compiler/rustc_hir_typeck/src/method/suggest.rs#L2123-L2129

r? ```@compiler-errors```
cc ```@Qelxiros``` -- this should unblock rust-lang#144420
tgross35 added a commit to tgross35/rust that referenced this pull request Sep 5, 2025
…iler-errors

fix ICE when suggesting `::new`

fixes rust-lang#146174

This code suggests to write `Foo::new(...)` when the user writes `Foo(...)` or `Foo { ... }` and the constructor is private, where `new` is some associated function that returns `Self`.

When checking that the return type of `new` is `Self`, we need to instantiate the parameters of `new` with infer vars, so we don't end up with a type like `Box<$param(0)>` in a context that doesn't have any parameters. But then we can't use `normalize_erasing_late_bound_regions` anymore because that goes though a query that can't deal with infer vars.

Since this is diagnostic-only code that is supposed to check for exactly `-> Self`, I think it's fine to just skip normalizing here, especially since The Correct Way<sup>TM</sup> would involve a probe and make this code even more complicated.

Also, the code here does almost the same thing, and these suggestions can probably be unified in the future: https://github.com/rust-lang/rust/blob/4ca8078d37c53ee4ff8fb32b4453b915116f25b8/compiler/rustc_hir_typeck/src/method/suggest.rs#L2123-L2129

r? ````@compiler-errors````
cc ````@Qelxiros```` -- this should unblock rust-lang#144420
rust-timer added a commit that referenced this pull request Sep 5, 2025
Rollup merge of #146217 - lukas-code:suggest-new-ice, r=compiler-errors

fix ICE when suggesting `::new`

fixes #146174

This code suggests to write `Foo::new(...)` when the user writes `Foo(...)` or `Foo { ... }` and the constructor is private, where `new` is some associated function that returns `Self`.

When checking that the return type of `new` is `Self`, we need to instantiate the parameters of `new` with infer vars, so we don't end up with a type like `Box<$param(0)>` in a context that doesn't have any parameters. But then we can't use `normalize_erasing_late_bound_regions` anymore because that goes though a query that can't deal with infer vars.

Since this is diagnostic-only code that is supposed to check for exactly `-> Self`, I think it's fine to just skip normalizing here, especially since The Correct Way<sup>TM</sup> would involve a probe and make this code even more complicated.

Also, the code here does almost the same thing, and these suggestions can probably be unified in the future: https://github.com/rust-lang/rust/blob/4ca8078d37c53ee4ff8fb32b4453b915116f25b8/compiler/rustc_hir_typeck/src/method/suggest.rs#L2123-L2129

r? ````@compiler-errors````
cc ````@Qelxiros```` -- this should unblock #144420
@Qelxiros Qelxiros force-pushed the smart_pointer_try_map branch from bfc61d6 to 0c65df4 Compare September 7, 2025 20:04
@rustbot
Copy link
Collaborator

rustbot commented Sep 7, 2025

This PR was rebased onto a different master 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.

@rust-log-analyzer

This comment has been minimized.

@Qelxiros Qelxiros force-pushed the smart_pointer_try_map branch from 0c65df4 to 59acdbc Compare September 8, 2025 12:44
@rust-log-analyzer

This comment has been minimized.

@Qelxiros Qelxiros force-pushed the smart_pointer_try_map branch from 59acdbc to 2ee0f1d Compare September 8, 2025 14:54
@Qelxiros Qelxiros force-pushed the smart_pointer_try_map branch from 2ee0f1d to dd0846e Compare September 8, 2025 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-blocked Status: Blocked on something else such as an RFC or other implementation work. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants