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

Add lint could_be_unsized #8526

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

Jarcho
Copy link
Contributor

@Jarcho Jarcho commented Mar 11, 2022

fixes: #1368
fixes: #2308

Worklist of things still to do:

  • Finish checking types for constraints. e.g. dyn Iterator<Item = T> would require T be Sized
  • Actually resolve dependencies. e.g. T: Foo<U> need to check the constraint from Foo to determine whether T and U need to be Sized
  • Check impl bodies for types. e.g. struct Foo<T>(..) should consider impl<T> Foo<T> {..} to determine it T could be ?Sized (will be done in a future PR)
  • Check impl bodes for traits. e.g. impl<T> Foo<T> for Bar<T> {..} could be T: ?Sized

changelog: Add lint could_be_unsized

@rust-highfive
Copy link

r? @giraffate

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Mar 11, 2022
@Jarcho Jarcho force-pushed the could_be_unsized branch 2 times, most recently from cc6107b to 6949e70 Compare March 11, 2022 23:55
@Jarcho Jarcho force-pushed the could_be_unsized branch 8 times, most recently from 5e3277b to 9b016d9 Compare March 26, 2022 02:21
@Jarcho
Copy link
Contributor Author

Jarcho commented Mar 26, 2022

This should be mostly done now. Just need to take another pass through the code and clean things up. Linting types can wait for another PR, but everything else is linted.

There is a false positive when adding constraints to trait associated types. e.g.

trait Assoc {
    type Type: ?Sized;
}
fn foo<T: Assoc<Type = T>>(x: &T) -> &T::Assoc where T::Assoc: Sized { ... }

This will suggest add ?Sized to T even though that won't compile. I don't think it's a blocker. Unsized associated types are unusual to start with.

I'd like to keep it opt-in right now due to the possibility of ICE's. It runs successfully on all the crates in lintcheck, but I'd like better testing before enabling it by default.

@Jarcho Jarcho force-pushed the could_be_unsized branch from 5e8c596 to f0877c7 Compare March 28, 2022 04:06
@Jarcho Jarcho changed the title WIP Add lint could_be_unsized Add lint could_be_unsized Mar 28, 2022
@Jarcho
Copy link
Contributor Author

Jarcho commented Mar 28, 2022

I think it's ready now. It can remain in the nursery for a bit.

@bors
Copy link
Contributor

bors commented Mar 30, 2022

☔ The latest upstream changes (presumably #8576) made this pull request unmergeable. Please resolve the merge conflicts.

@Jarcho Jarcho force-pushed the could_be_unsized branch 2 times, most recently from 8f737f3 to 362b205 Compare February 25, 2023 04:55
@Jarcho
Copy link
Contributor Author

Jarcho commented Feb 27, 2023

@rust-lang/clippy ping to whoever wants to take this one.

@bors
Copy link
Contributor

bors commented Mar 7, 2023

☔ The latest upstream changes (presumably #10415) made this pull request unmergeable. Please resolve the merge conflicts.

@Jarcho
Copy link
Contributor Author

Jarcho commented Feb 14, 2024

r? clippy

@rustbot rustbot assigned Manishearth and unassigned giraffate Feb 14, 2024
@Manishearth
Copy link
Member

Overall structure looks good but I don't have the time for a thorough review.

r? @blyxyas you might find this one fun

@rustbot
Copy link
Collaborator

rustbot commented Feb 15, 2024

Could not assign reviewer from: blyxyas.
User(s) blyxyas are either the PR author, already assigned, or on vacation, and there are no other candidates.
Use r? to specify someone else to assign.

@blyxyas
Copy link
Member

blyxyas commented Feb 15, 2024

I'll take a look

r? blyxyas

@rustbot rustbot assigned blyxyas and unassigned Manishearth Feb 15, 2024
@bors
Copy link
Contributor

bors commented Feb 24, 2024

☔ The latest upstream changes (presumably #12259) made this pull request unmergeable. Please resolve the merge conflicts.

@xFrednet
Copy link
Member

Hey @blyxyas, this is a ping from triage. Would you mind taking a look at this PR if you have the time? Otherwise, you can reassign it, by commenting r? clippy.

@blyxyas
Copy link
Member

blyxyas commented Mar 21, 2024

Yeah, I had the intention of looking at it because it has been been rolled like 3 times. But with how many reviews I get assigned, + the performance stuff, I simply son't see any time to review this.

r? clippy

@rustbot rustbot assigned dswij and unassigned blyxyas Mar 21, 2024
@xFrednet
Copy link
Member

Hey, this is a ping from triage. @dswij can you give this PR a review? It's totally fine if you don't have the time right now, you can reassign the PR to a random team member using r? clippy.

@rustbot ready

@xFrednet
Copy link
Member

Guess I can take this one on.

@Jarcho would you mind rebasing? Then we can also see a better lintcheck sample size :D

@xFrednet
Copy link
Member

r? xFrednet

@rustbot rustbot assigned xFrednet and unassigned dswij Jul 23, 2024
@xFrednet
Copy link
Member

This is still on my todo list, but I didn't have the time to take a look at it in the last month. The next week will also be full, but then I'll hopefully have time to review this :)

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
9 participants