make more diagnostic structs pub(crate) - #160744
Conversation
It was never used in that crate, so rustc_parse is the next obvious place to go. It's also used by rustc_hir_typeck, but sharing diagnostics between crates makes it easy for such things to become dead, so duplicate it.
rustbot
added
S-waiting-on-review
|
r? @khyperia rustbot has assigned @khyperia. Use Why was this reviewer chosen?The reviewer was selected based on:
|
This comment has been minimized.
This comment has been minimized.
d292479 to
4077d5c
Compare
| "parentheses are required to parse this as an expression", | ||
| applicability = "machine-applicable" | ||
| )] | ||
| pub struct ExprParenthesesNeeded { |
There was a problem hiding this comment.
This PR duplicates this struct to make it pub(crate), I think having less code is better even if that means having that code be public.
rustc_session is a weird place for this diagnostic to be tho... Is there a better place?
There was a problem hiding this comment.
This PR duplicates this struct to make it pub(crate), I think having less code is better even if that means having that code be public.
The downside of having diagnostic structs pub is that it's easy for these things to become orphaned or for crates to depend on other crates for just a diagnostic. So usually I prefer duplicating over having things public See also #155364
I think the risk of that for this one is particularly low, up to you if you would rather have less duplication.
Is there a better place?
rustc_parse would be the natural place, that's the first time it's used.
There was a problem hiding this comment.
I think I'd rather risk this specific diagnostic becoming orphaned, rather than having it be duplicated. This is personal taste tho and I can see arguments for either option.
We can consider moving the sole copy to rustc_parse and adding a dependency from rustc_hir_typeck to rustc_parse. The indirect dependency already exists (rustc_hir_typeck -> rustc_lint -> rustc_attr_parsing -> rustc_parse). Tho that does risk exactly what you mentioned, where rustc_hir_typeck depends only on rustc_parse for the diagnostic, so meh
There was a problem hiding this comment.
Right, that's why I duplicated it :)
5 participants
No description provided.