Skip to content

smart pointer (try_)map#144420

Merged
bors merged 1 commit intorust-lang:masterfrom
Qelxiros:smart_pointer_try_map
Oct 31, 2025
Merged

smart pointer (try_)map#144420
bors merged 1 commit intorust-lang:masterfrom
Qelxiros:smart_pointer_try_map

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.

@rust-log-analyzer

This comment has been minimized.

@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
@rust-log-analyzer

This comment has been minimized.

@joboet
Copy link
Member

joboet commented Oct 7, 2025

I'm not sure if the transmute is worth the readability from using Box::take, or if transmute is the only way to go from Box<MaybeUninit<T>> to Box<MaybeUninit<U>>. Let me know if I'm missing something obvious.

transmute is fine. You could also use Box::from_raw(Box::into_raw(..) as ..), if you prefer.

@Qelxiros Qelxiros force-pushed the smart_pointer_try_map branch from dbbd04d to 60cdf88 Compare October 7, 2025 13:59
@rustbot
Copy link
Collaborator

rustbot commented Oct 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.

@Qelxiros
Copy link
Contributor Author

Qelxiros commented Oct 15, 2025

@rustbot ready
edit: ignore me, I got my labels confused and thought I'd forgotten to do this.

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.

Great!
r=me with the nit applied

View changes since this review

Copy link
Member

Choose a reason for hiding this comment

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

Please move this import to the top of the file.

Copy link
Member

Choose a reason for hiding this comment

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

Ditto.

@joboet
Copy link
Member

joboet commented Oct 31, 2025

@bors delegate+

@bors
Copy link
Collaborator

bors commented Oct 31, 2025

✌️ @Qelxiros, you can now approve this pull request!

If @joboet told you to "r=me" after making some further change, please make that change, then do @bors r=@joboet

@Qelxiros Qelxiros force-pushed the smart_pointer_try_map branch from 60cdf88 to 9c5be67 Compare October 31, 2025 14:14
@Qelxiros
Copy link
Contributor Author

@bors r=@joboet

@bors
Copy link
Collaborator

bors commented Oct 31, 2025

📌 Commit 9c5be67 has been approved by joboet

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 Oct 31, 2025
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 31, 2025
bors added a commit that referenced this pull request Oct 31, 2025
Rollup of 7 pull requests

Successful merges:

 - #139310 (add first HelenOS compilation targets)
 - #144420 (smart pointer (try_)map)
 - #145974 (Stabilize -Zno-jump-tables into -Cjump-tables=bool)
 - #147161 (implement VecDeque extend_from_within and prepend_from_within)
 - #147780 (Implement VecDeque::extract_if)
 - #148319 (docs: Fix argument names for `carrying_mul_add`)
 - #148322 (Enable file locking support in illumos)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors
Copy link
Collaborator

bors commented Oct 31, 2025

⌛ Testing commit 9c5be67 with merge 82ae0ee...

@bors
Copy link
Collaborator

bors commented Oct 31, 2025

☀️ Test successful - checks-actions
Approved by: joboet
Pushing 82ae0ee to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 31, 2025
@bors bors merged commit 82ae0ee into rust-lang:master Oct 31, 2025
12 checks passed
@rustbot rustbot added this to the 1.93.0 milestone Oct 31, 2025
@github-actions
Copy link
Contributor

What is this? This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.

Comparing 51f5892 (parent) -> 82ae0ee (this PR)

Test differences

Show 994 test diffs

994 doctest diffs were found. These are ignored, as they are noisy.

Test dashboard

Run

cargo run --manifest-path src/ci/citool/Cargo.toml -- \
    test-dashboard 82ae0ee6487e93bd6c05167ccb2ef3485fdbc890 --output-dir test-dashboard

And then open test-dashboard/index.html in your browser to see an overview of all executed tests.

Job duration changes

  1. dist-x86_64-apple: 7286.5s -> 8916.6s (+22.4%)
  2. aarch64-apple: 6823.9s -> 8325.5s (+22.0%)
  3. i686-gnu-nopt-1: 7847.9s -> 6802.5s (-13.3%)
  4. test-various: 7105.6s -> 6160.7s (-13.3%)
  5. x86_64-gnu-tools: 3474.8s -> 3068.9s (-11.7%)
  6. x86_64-gnu-aux: 7047.9s -> 6274.4s (-11.0%)
  7. x86_64-mingw-1: 9720.2s -> 10475.4s (+7.8%)
  8. x86_64-gnu-nopt: 7244.4s -> 6719.1s (-7.3%)
  9. dist-aarch64-linux: 6635.8s -> 6166.4s (-7.1%)
  10. dist-ohos-x86_64: 4258.9s -> 3959.2s (-7.0%)
How to interpret the job duration changes?

Job durations can vary a lot, based on the actual runner instance
that executed the job, system noise, invalidated caches, etc. The table above is provided
mostly for t-infra members, for simpler debugging of potential CI slow-downs.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (82ae0ee): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results (primary 0.7%, secondary -1.9%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)
0.7% [0.7%, 0.7%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-1.9% [-1.9%, -1.9%] 1
All ❌✅ (primary) 0.7% [0.7%, 0.7%] 1

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

Results (secondary 0.0%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
0.0% [0.0%, 0.0%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Bootstrap: 474.501s -> 475.716s (0.26%)
Artifact size: 390.85 MiB -> 390.89 MiB (0.01%)

makai410 pushed a commit to makai410/rust that referenced this pull request Nov 8, 2025
makai410 pushed a commit to makai410/rust that referenced this pull request Nov 10, 2025
github-actions bot pushed a commit to model-checking/verify-rust-std that referenced this pull request Nov 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.

7 participants

Comments