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

impl_trait_in_params now supports impls and traits #11550

Merged
merged 2 commits into from Oct 8, 2023

Conversation

blyxyas
Copy link
Member

@blyxyas blyxyas commented Sep 21, 2023

Before this PR, the lint impl_trait_in_params. This PR gives the lint support for functions in impls and traits. (Also, some pretty heavy refactor)

fixes #11548
changelog:[impl_trait_in_params] now supports impl blocks and functions in traits

@rustbot
Copy link
Collaborator

rustbot commented Sep 21, 2023

r? @dswij

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Sep 21, 2023
@taiki-e
Copy link
Member

taiki-e commented Sep 22, 2023

Thanks!

I recommend adding a test for trait implementations -- I believe this lint should not warn for trait implementations because traits may be defined in the upstream crate, and the user who implements them may be unable to change them.

Comment on lines 38 to 45
// Trying with imported traits
impl ToString for S {
fn to_string(&self) -> String {
String::from("a")
}
}
Copy link
Member

@taiki-e taiki-e Sep 22, 2023

Choose a reason for hiding this comment

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

This lint warns for impl trait in parameters, so I think we also need to test for a case where parameters include impl trait.

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 a brain melt. I really didn't notice that 🐱

@blyxyas blyxyas force-pushed the fix-impl_trait_in_params-for_assocfn branch from f5860aa to 8eccfc2 Compare September 22, 2023 17:35
tests/ui/impl_trait_in_params.rs Outdated Show resolved Hide resolved
Comment on lines +28 to +36
struct S;
impl S {
pub fn h(_: impl Trait) {} //~ ERROR: `impl Trait` used as a function parameter
Copy link
Member

Choose a reason for hiding this comment

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

This lint is for public APIs, so I think it would be preferable to warn only of cases where both the type and the associated function are pub.

@blyxyas
Copy link
Member Author

blyxyas commented Oct 6, 2023

I just noticed the failing CI, I'll get to work on it right now 🐈

Copy link
Member

@dswij dswij 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 for this!

@dswij
Copy link
Member

dswij commented Oct 8, 2023

@blyxyas can you squash some commits? And then you can r=me

@@ -0,0 +1,41 @@
error: trait `Trait` is more private than the item `Public::t`
Copy link
Member

Choose a reason for hiding this comment

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

This error seems unrelated to what we want to test here. I guess you have to make Trait trait public.

@blyxyas blyxyas force-pushed the fix-impl_trait_in_params-for_assocfn branch 2 times, most recently from 2c15981 to 689c46f Compare October 8, 2023 21:49
@blyxyas blyxyas force-pushed the fix-impl_trait_in_params-for_assocfn branch from 689c46f to 7755737 Compare October 8, 2023 21:49
@blyxyas
Copy link
Member Author

blyxyas commented Oct 8, 2023

Wow I really fucked up the Git history for a couple minutes (like, adding 21 commits, 2K lines changed that aren't mine).
But don't worry, the good ol' reliable "entering panic mode" saved the day again

@blyxyas
Copy link
Member Author

blyxyas commented Oct 8, 2023

@bors r=dswij

@bors
Copy link
Collaborator

bors commented Oct 8, 2023

📌 Commit 7755737 has been approved by dswij

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Oct 8, 2023

⌛ Testing commit 7755737 with merge bde0482...

@bors
Copy link
Collaborator

bors commented Oct 8, 2023

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: dswij
Pushing bde0482 to master...

@bors bors merged commit bde0482 into rust-lang:master Oct 8, 2023
5 checks passed
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

impl_trait_in_params should warn associated functions
5 participants