Skip to content

add regression tests for the fn sig ice - #160746

Open
im-lunex wants to merge 3 commits into
rust-lang:mainfrom
im-lunex:test_for_160628
Open

add regression tests for the fn sig ice#160746
im-lunex wants to merge 3 commits into
rust-lang:mainfrom
im-lunex:test_for_160628

Conversation

@im-lunex

@im-lunex im-lunex commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

follow-up to #160628 which fixed the suggest_add_reference_to_arg fn_sig ice

adds the regression tests @teor2345 asked for in review

  • closure call with a missing-field struct literal (iced without the guard)
  • method call whose where-clause fails (covers the method-call branch)

the impl def_id case cant be constructed, those obligations get dropped during probing and turn into e0599 instead

@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 Aug 8, 2026
@rustbot

rustbot commented Aug 8, 2026

Copy link
Copy Markdown
Collaborator

r? @camelid

rustbot has assigned @camelid.
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 75 candidates
  • Random selection from 18 candidates

@teor2345

teor2345 commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Cc @JohnTitor @Kivooeo who worked on who have context on #160628

@Kivooeo

Kivooeo commented Aug 9, 2026

Copy link
Copy Markdown
Member

feel free to review i will r= you

@teor2345 teor2345 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks, these are good tests and improve the coverage of PR #160628.

the impl def_id case cant be constructed, those obligations get dropped during probing and turn into e0599 instead

I try to avoid modifying code we can't test, or code for things that can never happen. But I think this part of PR #160628 is still a good idea, because it removes an ICE that could become reachable after future code changes.

Sometimes, I add tests that are "as close as we can get" to reaching the changed code. But I think we've done a lot here already, so it's up to you if you want to add something like that in another PR. I don't mind either way.

View changes since this review

@Kivooeo Kivooeo Aug 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

also add a links to issue in both tests

View changes since the review

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

@im-lunex

Copy link
Copy Markdown
Contributor Author

Thanks, these are good tests and improve the coverage of PR #160628.

the impl def_id case cant be constructed, those obligations get dropped during probing and turn into e0599 instead

I try to avoid modifying code we can't test, or code for things that can never happen. But I think this part of PR #160628 is still a good idea, because it removes an ICE that could become reachable after future code changes.

Sometimes, I add tests that are "as close as we can get" to reaching the changed code. But I think we've done a lot here already, so it's up to you if you want to add something like that in another PR. I don't mind either way.

View changes since this review

thanks letting me choose and yeah i would like to keep this much for now.. and sure im open if you say i should add more.

@rust-log-analyzer

This comment has been minimized.

@teor2345 teor2345 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks, this looks good to me!

View changes since this review

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

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

Development

Successfully merging this pull request may close these issues.

6 participants