Skip to content

compiler: suggest .collect() when String is expected and `Iterato…#156121

Open
thiago-fealves wants to merge 2 commits intorust-lang:mainfrom
thiago-fealves:suggest-collect-string
Open

compiler: suggest .collect() when String is expected and `Iterato…#156121
thiago-fealves wants to merge 2 commits intorust-lang:mainfrom
thiago-fealves:suggest-collect-string

Conversation

@thiago-fealves
Copy link
Copy Markdown

@thiago-fealves thiago-fealves commented May 3, 2026

…r` is found

This commit adds a diagnostic suggestion to help users who forget to call .collect() when they have an iterator and the function or variable expects a String.

The logic checks if the expected type is std::string::String and if the found type implements the Iterator trait, if so the compiler provides a suggestion to add .collect()

Includes also a UI test to verify if the suggestion appears correctly

@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 May 3, 2026
@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented May 3, 2026

r? @JohnTitor

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

Why was this reviewer chosen?

The reviewer was selected based on:

  • Owners of files modified in this PR: compiler
  • compiler expanded to 73 candidates
  • Random selection from 22 candidates

@thiago-fealves thiago-fealves force-pushed the suggest-collect-string branch from 5513a24 to 41aa2be Compare May 3, 2026 23:25
teor2345
teor2345 reviewed May 3, 2026
View reviewed changes
Comment thread tests/ui/suggestions/suggest-collect.rs
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@teor2345
Copy link
Copy Markdown
Contributor

teor2345 commented May 4, 2026

The job aarch64-gnu-llvm-21-1 failed!

Looks like you need to run ./x test --bless tests/ui to update the expected test outputs for existing tests with the new suggestion.

Most of the failures are:

help: consider using .collect() to convert the Iterator into a ()

Is this a useful suggestion?
We might want to skip the unit type if other suggestions are more useful, or put it lower down in the suggestion priority list.

@thiago-fealves
Copy link
Copy Markdown
Author

thiago-fealves commented May 4, 2026

The job aarch64-gnu-llvm-21-1 failed!

Looks like you need to run ./x test --bless tests/ui to update the expected test outputs for existing tests with the new suggestion.

Most of the failures are:

help: consider using .collect() to convert the Iterator into a ()

Is this a useful suggestion? We might want to skip the unit type if other suggestions are more useful, or put it lower down in the suggestion priority list.

I didn't anticipate this could happen, i was alway thinking about more concrete types like String and int, honestly i think in this case the suggestion is not being useful for the user anymore, i can't think of a situation where you would collect into () at least. What do you suggest in this case?

I'm gonna implement more tests for non-String objects in the meantime

@teor2345
Copy link
Copy Markdown
Contributor

teor2345 commented May 4, 2026

I didn't anticipate this could happen, i was alway thinking about more concrete types like String and int, honestly i think in this case the suggestion is not being useful for the user anymore, i can't think of a situation where you would collect into () at least. What do you suggest in this case?

The docs say this is intended for cases like Result<(), E>:
https://doc.rust-lang.org/std/iter/trait.FromIterator.html#impl-FromIterator%3C()%3E-for-()

So skipping the suggestion when the expected type is just () seems like a good idea.

@thiago-fealves
Copy link
Copy Markdown
Author

thiago-fealves commented May 4, 2026

I didn't anticipate this could happen, i was alway thinking about more concrete types like String and int, honestly i think in this case the suggestion is not being useful for the user anymore, i can't think of a situation where you would collect into () at least. What do you suggest in this case?

The docs say this is intended for cases like Result<(), E>: https://doc.rust-lang.org/std/iter/trait.FromIterator.html#impl-FromIterator%3C()%3E-for-()

So skipping the suggestion when the expected type is just () seems like a good idea.

I agree, gonna finish implementing that and adding the UI tests.

@thiago-fealves
compiler: suggest .collect() when String is expected and `Iterato…
a4bd5f5 
…r` is found

This commit adds a diagnostic suggestion to help users who forget to
call `.collect()` when they have an iterator and the function or
variable expects a `String`.

The logic checks if the expected type is `std::string::String` and if
the found type implements the `Iterator` trait, if so the compiler
provides a suggestion to add `.collect()`

Includes also a UI test to verify if the suggestion appears correctly
@thiago-fealves thiago-fealves force-pushed the suggest-collect-string branch from 41aa2be to a4bd5f5 Compare May 4, 2026 13:55
@thiago-fealves
Copy link
Copy Markdown
Author

thiago-fealves commented May 4, 2026

Just added a check for the unit type, let's see if the tests pass now

@thiago-fealves
Copy link
Copy Markdown
Author

thiago-fealves commented May 4, 2026

Looks like tests succeeded!

qaijuang
qaijuang reviewed May 4, 2026
View reviewed changes
Comment thread tests/ui/suggestions/suggest-collect.rs
qaijuang
qaijuang reviewed May 4, 2026
View reviewed changes
Comment thread compiler/rustc_hir_typeck/src/fn_ctxt/suggestions.rs Outdated
teor2345
teor2345 reviewed May 4, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@teor2345 teor2345 left a comment •
edited by rustbot
Loading

Choose a reason for hiding this comment

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

This looks good to me, but I'm not a typeck expert.

Feel free to resolve my earlier feedback, I think GitHub is having issues and dropping my attempts to do it.

View changes since this review

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

Reviewers

2 more reviewers

@teor2345 teor2345 teor2345 left review comments

@qaijuang qaijuang qaijuang left review comments

Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

@JohnTitor JohnTitor

Labels

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.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@thiago-fealves @rustbot @rust-log-analyzer @teor2345 @qaijuang @JohnTitor