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.

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

In "specify type" suggestion, skip type params that are already known #135965

Merged
merged 2 commits into from
Feb 12, 2025

Conversation

estebank
Copy link
Contributor

When we suggest specifying a type for an expression or pattern, like in a let binding, we previously would print the entire type as the type system knew it. We now look at the params that have no inference variables, so they are fully known to the type system which means that they don't need to be specified.

This helps in suggestions for types that are really long, because we can usually skip most of the type params and make the annotation as short as possible:

error[E0282]: type annotations needed for `Result<_, ((..., ..., ..., ...), ..., ..., ...)>`
  --> $DIR/really-long-type-in-let-binding-without-sufficient-type-info.rs:7:9
   |
LL |     let y = Err(x);
   |         ^   ------ type must be known at this point
   |
help: consider giving `y` an explicit type, where the type for type parameter `T` is specified
   |
LL |     let y: Result<T, _> = Err(x);
   |          ++++++++++++++

Fix #135919.

@rustbot
Copy link
Collaborator

rustbot commented Jan 24, 2025

r? @petrochenkov

rustbot has assigned @petrochenkov.
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. labels Jan 24, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jan 24, 2025

Some changes occurred in need_type_info.rs

cc @lcnr

@petrochenkov
Copy link
Contributor

r? compiler

Copy link
Contributor

@lcnr lcnr left a comment

Choose a reason for hiding this comment

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

U've got an infcx here, use that instead of an inference var with index 0 as that one may actually be resolved at this point, breaking any (theoretical) later calls to resolve_vars_if_possible.

I also don't fully udnerstand the point of skip and why it's necessary. Please add a comment and more descriptive name for it

@lcnr
Copy link
Contributor

lcnr commented Jan 28, 2025

r? @lcnr

@estebank
Copy link
Contributor Author

@lcnr added explanations in the comments. Hopefully it's easier to follow now. The only thing I'd like to change is the name of the Folder, but can't think of a good name for it now that it does two things. It should be something that conveys "we take a Ty<'_> and make it suitable for presentation in a suggestion", and can't come up with something short to mean that :)

@rust-log-analyzer

This comment has been minimized.

Comment on lines 192 to 201
let generics = self.cx().generics_of(def.did());
if generics.own_params.iter().any(|param| param.default_value(self.cx()).is_some())
{
// We have a type that has default types, like the allocator in Vec. We decided
// to show `Vec` itself, because it hasn't yet been replaced by an `_` `Infer`,
// but we want to ensure that the type parameter with default types does *not*
// get replaced with `_` because then we'd end up with `Vec<_, _>`, instead of
// `Vec<_>`.
self.do_not_hide_nested_type = true;
ty.super_fold_with(self)
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 there's a way to fold each param individually at this stage, then we could make it so we only avoid the type params like Vec<_, Alloc> and nothing else and we can likely get rid of the bool flag on self (if we can also make it so that consts don't get type erased too).

Copy link
Contributor

Choose a reason for hiding this comment

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

if i understand u correctly, u can zip the generics with the ty::Adt(_, args) for that 🤔 you can then fold each arg manually and use Ty::new_adt instead of relying on super_fold_with

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me know if this is the right approach for building that param list.

// type parameter with default types does *not* get replaced with
// `_` because then we'd end up with `Vec<_, _>`, instead of
// `Vec<_>`.
all_infer = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

why set all_infer to false here? if you've got Foo<T = u32>(T) and then encounter Foo<u32> as the inferred type, you can just replace Foo with _ here.

Given that all_infer only matters for whether we replace the whole adt with infer, I'd expect that we only replace the whole adt if it does not contain any infer vars, so you could make that check an early return and then not track all_infer at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. It was a mistake from faulty boolean logic caused by decaffeination :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to change the order of the check because cont params are considered to be defaulted and infered, and that could replace things like struct S<const N: i32>; with _ incorrectly if I checked for defaults first. This seems to be intentional https://github.com/rust-lang/rust/blob/3594a19f2ae792cf99a1a78a5454e52cfd7affde/tests/ui/const-generics/defaults/doesnt_infer.rs, but doesn't sound entirely correct to me at first glance.

Copy link
Contributor

Choose a reason for hiding this comment

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

This matches the behavior for type parameters:

struct Foo<T = u32>(T);

impl<T> Foo<T> {
    fn foo() -> Self {
        loop {}
    }
}

fn main() {
    let foo = Foo::<u32>::foo();
    let foo = Foo::foo(); // error
}

We only use generic arg defaults when specifying the non-defaulted generic args (modulo buggy behavior imo #136488)

Please use ty.has_infer() to decide whether the whole type should be replaced with infer before walking the adt args.

Also, you could look at what we do in the actual printing logic to decide whether to print default args:

struct Foo<T = u32, U = T>(T, U);

impl<T, U> Foo<T, U> {
    fn foo() -> Self {
        loop {}
    }
}

fn main() {
    let b = Foo::<Vec<_>>::foo(); // does not print `U`
}

When we suggest specifying a type for an expression or pattern, like in a `let` binding, we previously would print the entire type as the type system knew it. We now look at the params that have *no* inference variables, so they are fully known to the type system which means that they don't need to be specified.

This helps in suggestions for types that are really long, because we can usually skip most of the type params and make the annotation as short as possible:

```
error[E0282]: type annotations needed for `Result<_, ((..., ..., ..., ...), ..., ..., ...)>`
  --> $DIR/really-long-type-in-let-binding-without-sufficient-type-info.rs:7:9
   |
LL |     let y = Err(x);
   |         ^   ------ type must be known at this point
   |
help: consider giving `y` an explicit type, where the type for type parameter `T` is specified
   |
LL |     let y: Result<T, _> = Err(x);
   |          ++++++++++++++
```
@lcnr
Copy link
Contributor

lcnr commented Feb 11, 2025

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Feb 11, 2025

📌 Commit 576db13 has been approved by lcnr

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 Feb 11, 2025
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 11, 2025
In "specify type" suggestion, skip type params that are already known

When we suggest specifying a type for an expression or pattern, like in a `let` binding, we previously would print the entire type as the type system knew it. We now look at the params that have *no* inference variables, so they are fully known to the type system which means that they don't need to be specified.

This helps in suggestions for types that are really long, because we can usually skip most of the type params and make the annotation as short as possible:

```
error[E0282]: type annotations needed for `Result<_, ((..., ..., ..., ...), ..., ..., ...)>`
  --> $DIR/really-long-type-in-let-binding-without-sufficient-type-info.rs:7:9
   |
LL |     let y = Err(x);
   |         ^   ------ type must be known at this point
   |
help: consider giving `y` an explicit type, where the type for type parameter `T` is specified
   |
LL |     let y: Result<T, _> = Err(x);
   |          ++++++++++++++
```

Fix rust-lang#135919.
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 12, 2025
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#135549 (Document some safety constraints and use more safe wrappers)
 - rust-lang#135965 (In "specify type" suggestion, skip type params that are already known)
 - rust-lang#136193 (Implement pattern type ffi checks)
 - rust-lang#136646 (Add a TyPat in the AST to reuse the generic arg lowering logic)
 - rust-lang#136699 (std: replace the `FromInner` implementation for addresses with private conversion functions)
 - rust-lang#136874 (Change the issue number for `likely_unlikely` and `cold_path`)
 - rust-lang#136884 (Lower fn items as ZST valtrees and delay a bug)
 - rust-lang#136885 (i686-linux-android: increase CPU baseline to Pentium 4 (without an actual change)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 12, 2025
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#135549 (Document some safety constraints and use more safe wrappers)
 - rust-lang#135965 (In "specify type" suggestion, skip type params that are already known)
 - rust-lang#136193 (Implement pattern type ffi checks)
 - rust-lang#136646 (Add a TyPat in the AST to reuse the generic arg lowering logic)
 - rust-lang#136874 (Change the issue number for `likely_unlikely` and `cold_path`)
 - rust-lang#136884 (Lower fn items as ZST valtrees and delay a bug)
 - rust-lang#136885 (i686-linux-android: increase CPU baseline to Pentium 4 (without an actual change)
 - rust-lang#136891 (Check sig for errors before checking for unconstrained anonymous lifetime)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 5ebacd1 into rust-lang:master Feb 12, 2025
6 checks passed
@rustbot rustbot added this to the 1.86.0 milestone Feb 12, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 12, 2025
Rollup merge of rust-lang#135965 - estebank:shorten-ty-sugg, r=lcnr

In "specify type" suggestion, skip type params that are already known

When we suggest specifying a type for an expression or pattern, like in a `let` binding, we previously would print the entire type as the type system knew it. We now look at the params that have *no* inference variables, so they are fully known to the type system which means that they don't need to be specified.

This helps in suggestions for types that are really long, because we can usually skip most of the type params and make the annotation as short as possible:

```
error[E0282]: type annotations needed for `Result<_, ((..., ..., ..., ...), ..., ..., ...)>`
  --> $DIR/really-long-type-in-let-binding-without-sufficient-type-info.rs:7:9
   |
LL |     let y = Err(x);
   |         ^   ------ type must be known at this point
   |
help: consider giving `y` an explicit type, where the type for type parameter `T` is specified
   |
LL |     let y: Result<T, _> = Err(x);
   |          ++++++++++++++
```

Fix rust-lang#135919.
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.

Long type name emitted if the type name is part of a suggestion
7 participants