Style guide: Don't use cross-crate internal APIs #7550
Conversation
Manishearth
left a comment
Manishearth
left a comment
There was a problem hiding this comment.
We use ~ deps so that we can do this safely. There are still some issues, but I somewhat like where we have landed on that.
I think there are a couple issues that can crop up, and we should address those by policy, but I don't think we should discourage such internal APIs, especially not in favor of new public APIs. I want us to move towards being more careful about adding new public APIs.
Here are the issues I perceive around these right now, and style guide suggestions that should help there.
Unintentional breakages
I think this is a problem for all public APIs.
A very good thing for us to do would be to encourage proper documentation and testing for cross-crate internal APIs, to the same level of scrutiny we have for public APIs, with the caveat that the scrutiny is around making things clearer, not about API design itself.
Reducing the need for change
When it comes to crate-internal APIs we typically do not care too much about their precise API except for developer ergonomics reasons. We do not try to find the "best" option in API design space.
We should potentially spend some time doing so for cross-crate internal APIs, to reduce the likelihood that they will change. I think this would be a positive addition to the style guide.
icu_provider and icu_locale_core
ICU4X components have ~ deps on each other. By and large, the upgrade hazards are minimized there for Cargo users.
My firsthand experience doing upgrades in a vendoring situation is that for the components you kind of want to group them together anyway. Having to do a piecemeal update of components doesn't really come up, and if it does it's not a big deal to update 2-3 components together.
On the other hand, the "scaffolding" icu_provider and icu_locale_core crates are depended upon by everyone and it's usually important to update them together. I pushed for us to decouple this, and we did: we do not use ~ deps for these crates.
When adding hidden APIs to these crates I think we should be more careful. I think this is a good situation to think hard about whether you would ever change an internal API, and consider designing it for public usage (even if it starts out as crate-internal and is graduated separately).
Unintentional usage by clients
Our north star on "all APIs that are unstable should require users to type unstable" from #6659 (comment) is probably good to apply here. Such APIs should as much as possible be named unstable_internal or something similar.
This also seems good as a style guide policy.
| All public APIs, even those marked `#[doc(hidden)]`, add a cost to maintenance and integration: | ||
|
|
||
| - Action at a distance: you might make a change to an internal API in one crate and all the tests pass only to discover that a client in another crate was assuming something else about that function's behavior. | ||
| - Piecewise integration: when clients are in the process of updating crates, they may update one crate at a time, resulting in an ephemeral build breakage. Since we care about easing clients' integration processes, we should generally avoid causing this type of issue. |
There was a problem hiding this comment.
As the person who has mostly done these updates: I think our versioning policy around icu_provider and icu_locale_core already handles 99% of the problems here.
|
|
||
| All public APIs, even those marked `#[doc(hidden)]`, add a cost to maintenance and integration: | ||
|
|
||
| - Action at a distance: you might make a change to an internal API in one crate and all the tests pass only to discover that a client in another crate was assuming something else about that function's behavior. |
There was a problem hiding this comment.
thought: IMO most of this is mitigated by proper documentation. We should do more of that, and have a decently high documentation requirement on these internal APIs.
"you might change an API and break assumptions" is true of public APIs as well.
| - Piecewise integration: when clients are in the process of updating crates, they may update one crate at a time, resulting in an ephemeral build breakage. Since we care about easing clients' integration processes, we should generally avoid causing this type of issue. | ||
| - Use outside ICU4X: if an API is exported, it is subject to being used by clients, whether intentionally or unintentionally. | ||
|
|
||
| If you are tempted to add an internal cross-crate API, take a step back, identify the fundamental need, and consider introducing a properly documented and tested public API. |
There was a problem hiding this comment.
issue: I'm worried about pressure towards early-stabilizing public APIs because we need them for something else. I consider that to be a major tradeoff, enough to usually make me prefer internal APIs.
Even if we cfg(feature = unstable) these APIs, they will need to be paired with an internal hidden API so that ICU4X crates can use them.
2 participants
We have been operating with this assumption for some time. I think we should write it down.
It is "suggested", not "required", giving leeway to authors and reviewers.