Fix suggestion span for `?Sized` when param type has default #120915

OdenShirataki · 2024-02-11T06:05:59Z

Fixes #120878

Diagnostic suggests adding : ?Sized in an incorrect place if a type parameter default is present

rustbot · 2024-02-11T06:06:06Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @fmease (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

@rustbot author: the review is finished, PR author should check the comments and take action accordingly
@rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

fmease

Please address my suggestions, then we are good to go unless you'd also like to fix the other bug I just found.

fmease · 2024-02-11T20:00:21Z

tests/ui/traits/issue-120878.rs

Thanks for adding a test! Could you move it into tests/ui/trait-bounds/ though? I think that'd be a better fit. I would also rename to something more descriptive like suggest-maybe-sized-bound.rs and add the issue number as a comment // issue: 120878.

Note that there already exists trait-bounds/unsized-bound.rs but that one's more about removing ?Sized bounds rather than adding them, so I wouldn't extend it.

I noticed that the following code also leads to an incorrect suggestion although that very likely hits a different code path:

trait Trait { type P<X>; } impl Trait for () { type P<X> = [u8]; }

trait Trait { type P: ?Sized<X>; } ++++++++

If you have time & interest in fixing this, too, while you're at it, go for it (and just amend the existing test file)!

Thank you for the review and for suggesting interesting assignments.

I think I can fix it, so I'll give it a try.

when param type has default and type in trait is generic.

fmease · 2024-02-13T23:55:17Z

compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs

                                (last.span().shrink_to_hi(), " +")
                            } else {
-                                (ident.span.shrink_to_hi(), ":")
+                                (generics.span.shrink_to_hi(), ":")


Right, and if there are no generic params, this is still correct since the span will be a zero-sized span located directly after the ident 👍

fmease · 2024-02-13T23:55:57Z

Great, thanks a lot!
@bors r+ rollup

bors · 2024-02-13T23:56:00Z

📌 Commit 4ac90e2 has been approved by fmease

It is now in the queue for this repository.

Rollup of 13 pull requests Successful merges: - rust-lang#116387 (Additional doc links and explanation of `Wake`.) - rust-lang#118738 (Netbsd10 update) - rust-lang#118890 (Clarify the lifetimes of allocations returned by the `Allocator` trait) - rust-lang#120498 (Uplift `TypeVisitableExt` into `rustc_type_ir`) - rust-lang#120530 (Be less confident when `dyn` suggestion is not checked for object safety) - rust-lang#120915 (Fix suggestion span for `?Sized` when param type has default) - rust-lang#121015 (Optimize `delayed_bug` handling.) - rust-lang#121024 (implement `Default` for `AsciiChar`) - rust-lang#121039 (Correctly compute adjustment casts in GVN) - rust-lang#121045 (Fix two UI tests with incorrect directive / invalid revision) - rust-lang#121049 (Do not point at `#[allow(_)]` as the reason for compat lint triggering) - rust-lang#121071 (Use fewer delayed bugs.) - rust-lang#121073 (Fix typos in `OneLock` doc) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#120915 - OdenShirataki:master, r=fmease Fix suggestion span for `?Sized` when param type has default Fixes rust-lang#120878 Diagnostic suggests adding `: ?Sized` in an incorrect place if a type parameter default is present r? `@fmease`

rustbot assigned fmease Feb 11, 2024

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 Feb 11, 2024