RFC: Extend manifest dependencies with used#3920
RFC: Extend manifest dependencies with used#3920epage wants to merge 10 commits intorust-lang:masterfrom
used#3920Conversation
text/3920-used-deps.md
Outdated
|
|
||
| However, not all dependencies exist for using their `extern` in the current package, including | ||
| - activating a feature on a transitive dependency | ||
| - pinning the version of a transitive dependency in `Cargo.toml` (however, normally this would be done via `Cargo.lock`) |
There was a problem hiding this comment.
Maybe we could create a [peer-dependencies] section for that just like npm? This would also improve compile time by avoiding a need to block compilation of the current crate on said dependency. And cargo could get the option to warn if a peer-dependency is not used by any other crate (aka pinning a transitive dependency has no effect).
There was a problem hiding this comment.
Maybe we could create a [peer-dependencies] section for that just like npm?
There is an ecosystem cost to adding new dependencies tables. We'd have to weigh the cost against the benefits.
This would also improve compile time by avoiding a need to block compilation of the current crate on said dependency.
For the two use cases we have, compilation will already be blocked on them. Do we know of any use cases where this would be a concern?
There was a problem hiding this comment.
Reading up on peerDependencies, they sounds like target."cfg(false)".dependencies which helps with version requirements but not with feature activation.
|
This looks great, and would enable us to warn about unused dependencies by default. |
text/3920-used-deps.md
Outdated
| ## Motivation | ||
| [motivation]: #motivation | ||
|
|
||
| Rusts can report to Cargo |
There was a problem hiding this comment.
I think you meant Rustc here
| To resolve this, `user` can: | ||
| ```toml | ||
| [package] | ||
| name = "user" | ||
|
|
||
| [dependencies] | ||
| abstraction = "1.0" | ||
| mechanism = { version = "1.0", features = ["semantic-addition"], used.reason = "feature activation" } | ||
| ``` |
There was a problem hiding this comment.
One problematic scenario I see with this is that if later I remove abstraction without removing mechanism we no longer get the benefit of unused lint. In this case its fairly obvious, but for larger Cargo.toml files it might be easier to miss.
Though I think this is a solvable problem by extending the current design.
I am imagining a used.transitive which would could conditionally disable the unused lint if this dependency is also imported transitively. Now removing the abstraction dependency in the example would also lint that mechanism is unused.
Though I think this could be left as a future possibility :)
| ### `used` | ||
|
|
||
| Pros: | ||
| - Close to the dependency |
There was a problem hiding this comment.
Is that really such a benefit? When searching for how to modify the behavior of a lint, checking how is a lint being configured or why is a given dependency not being marked as unused, I wouldn't really think of going to the dependency table. Rather I would go look at the lint configuration. Similarly to when I configure e.g. disallowed functions (disallowed_methods) for Clippy, that's also not specified on the dependency.
The way I interpret the [dependencies] table is "how to fetch and build dependencies used by my crate", possibly extended by "how to expose my dependencies to external crates", once we get public/private dependencies.
But configuring lints seems like a separate concern, that is also much more of an implementation detail of the given crate, and it IMO makes more sense for it to live in lint config, rather than here.
There was a problem hiding this comment.
I think there is a benefit. CPU caches are driven by the assumption of locality, that the same they will access memory that is close together over a close together period of time. I think locality of code is also an important consideration for us as humans to read and understand code and data. The perfect abstraction means you don't need to look at a function implementation, only its call site. Perfect abstractions don't exist but they are on a gradient. You can get away with a lower quality abstraction when the code is closer together while further away, you need a higher quality abstraction because it is harder to process and reason about because of the lack of locality.
I think the analogy with disallowed_methods doesn't work because you are applying a setting that affects a package globally while we are trying to control individual uses. We specifically call out, if we went with lint config, setting it in workspace.lints would be bad and that we likely would need a way to control which lints table the package exists in that we are referring to, to avoid being overly broad with this lint control.
That said, this isn't the only consideration here. We are weighing multiple benefits and downsides.
Extend Cargo's dependency specifications so users can mark that a dependency is used, independent of what unused externs says.
Important
When responding to RFCs, try to use inline review comments (it is possible to leave an inline review comment for the entire file at the top) instead of direct comments for normal comments and keep normal comments for procedural matters like starting FCPs.
This keeps the discussion more organized.
Rendered