feat(lints): add unused_workspace_exclude lint#17143
Conversation
|
r? @epage rustbot has assigned @epage. Use Why was this reviewer chosen?The reviewer was selected based on:
|
There was a problem hiding this comment.
@raushan728 I really appreciate your work and directly went with an implementation. You've contributed quite a lot useful fixes and improvements recently!
I believe you may already know but just remind you again that the issue is not marked as S-accepted and hasn't yet got sufficient design discussions.
There was a problem hiding this comment.
Thanks for the kind words. I understand the issue isn't accepted yet. #17089 (comment)
I'm assuming this wouldn't qualify as a hard coded diagnostic and would need to be implemented as a lint.
Also, warning is a Solution. Lack of it is not Problem. Be sure to reframe the Problem statement in terms of end user impact.
Based on that, it seemed clear that a lint (not a hard-coded diagnostic) was the right path. There wasn’t any pushback on the issue, so I went ahead with an implementation that tracks which exclude patterns actually matched a crate during resolution only truly unused ones warn
There was a problem hiding this comment.
Not exactly a pushback, though I have one new comment #17089 (comment). I don't really have time thinking on this harder unfortunately
There was a problem hiding this comment.
Thanks! I've seen #6745 the nested-path exclude behavior is definitely wonky but, this lint is completely independent though. It only warns when an exclude pattern matched zero packages during resolution, so it catches typos and stale config without touching the buggy nested-path logic. That bug exists with or without the lint.
If you'd prefer to land fixes for #6745 first, we can convert this PR to draft until then. Just let me know.
There was a problem hiding this comment.
Based on that, it seemed clear that a lint (not a hard-coded diagnostic) was the right path. There wasn’t any pushback on the issue, so I went ahead with an implementation that tracks which exclude patterns actually matched a crate during resolution only truly unused ones warn
To add to what was said, no pushback does not mean everything has been figured out. In particular, this was still in the Triage state which means we haven't really thought at all about this. There is one lint where we are unsure if it is worth the review and maintainance cost. We could also have a different way to reframe a lint to take it in a different direction.
One particular aspect of our label system is to align contributions with our availability, including people pressuring us (intententionally or not) to commit time we do not have by posting PRs. What we most need is thinking, not code, and that is what we lack to move a lot of these issues forward. Note that Weihang did not have time to think more on this but that led to more questions. This is why we encourage contributing through working on accepted issues and enabling decisions on those that are not.
|
☔ The latest upstream changes (possibly #17169) made this pull request unmergeable. Please resolve the merge conflicts. |
Closes #17089.
unused_workspace_excludelint that warns when a pattern in[workspace.exclude]matches no crate.Workspace).