Skip to content

[missing_const_for_fn]: fix FP when arg ty is impl trait alias ty #13045

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 8, 2024
Merged

[missing_const_for_fn]: fix FP when arg ty is impl trait alias ty #13045

merged 1 commit into from
Jul 8, 2024

Conversation

J-ZhengLi
Copy link
Member

closes: #13009

changelog: [missing_const_for_fn]: fix FP when arg ty is impl trait alias ty

@rustbot
Copy link
Collaborator

rustbot commented Jul 5, 2024

r? @blyxyas

rustbot has assigned @blyxyas.
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 the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jul 5, 2024
blyxyas
blyxyas reviewed Jul 7, 2024
View reviewed changes
clippy_lints/src/missing_const_for_fn.rs Outdated
inputs.iter().any(|input| {
// NB: Other alias ty kind might missing default.
// For example, an associate type alias declared as `type T: Fn();`
// would cause ICE when `type_of` is called with it.
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what this comment means, I've tested if changing test's Foo to that piece of code caused an ICE (that would be quite the blocker) and it doesn't cause one.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, what I meant was:

When matching the input.kind() on the next line, if the alias ty is something else, i.e. AliasTyKind::Projection would cause ICE in that scenario. You can verify it by changing the ty::AliasTyKind::Weak to _ as well.

(It seems that it only happens with AliasTyKind::Projection atm, but I wasn't too sure, so I put the comment there trying to give a head's up for others)

I'll make some adjustments on this, along with the test case~

Copy link
Member Author

@J-ZhengLi J-ZhengLi Jul 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

wait, as I was adding a test to trigger ICE, I accidently found a case that triggers issue #13013 , might as well try to fix that as well, hope you wouldn't mind

edit: seems hard to fix, nvm, sorry lol 😂

@rustbot ready

blyxyas
blyxyas reviewed Jul 7, 2024
View reviewed changes
Copy link
Member

@blyxyas blyxyas left a comment

Choose a reason for hiding this comment

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

Just a question, and this should be ready! Thanks for the contribution.

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Jul 8, 2024
@J-ZhengLi J-ZhengLi force-pushed the missing_const_for_fn_FP branch from 70c8010 to 1bc298a Compare July 8, 2024 09:42
@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 from the author. (Use `@rustbot ready` to update this status) labels Jul 8, 2024
@J-ZhengLi J-ZhengLi force-pushed the missing_const_for_fn_FP branch from 1bc298a to 08992d0 Compare July 8, 2024 10:10
blyxyas
blyxyas approved these changes Jul 8, 2024
View reviewed changes
Copy link
Member

@blyxyas blyxyas left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! ❤️

@blyxyas
Copy link
Member

blyxyas commented Jul 8, 2024

@bors r+

@bors
Copy link
Contributor

bors commented Jul 8, 2024

📌 Commit 08992d0 has been approved by blyxyas

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Jul 8, 2024

⌛ Testing commit 08992d0 with merge 2ad8cdc...

@bors
Copy link
Contributor

bors commented Jul 8, 2024

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: blyxyas
Pushing 2ad8cdc to master...

@bors bors merged commit 2ad8cdc into rust-lang:master Jul 8, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@blyxyas blyxyas blyxyas approved these changes

Assignees

@blyxyas blyxyas

Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

missing_const_for_fn does not work with TAIT
4 participants
@J-ZhengLi @rustbot @blyxyas @bors