Preserve attributes for imports_granularity=Item
#5031
Conversation
|
r? @karyon @ytmimi - in case either of you have bandwidth/interest in taking a look. With the other variants we typically have to skip any imports that have attributes because they are more of the merge/combine variety whereas the |
src/imports.rs
Outdated
| @@ -608,7 +608,7 @@ impl UseTree { | |||
| span: self.span, | |||
| list_item: None, | |||
| visibility: self.visibility.clone(), | |||
| attrs: None, | |||
| attrs: self.attrs.clone(), | |||
There was a problem hiding this comment.
I can't come up with a specific use case at the moment, but have some concerns about this having unintended knock on effects (even if not now, perhaps later on).
Would you mind updating this just a bit so that this is only done conditionally with the Item granularity variant? Should be feasible to do so by updating the function signature to take a param (boolean, ImportGranularity, etc.) or attaching the granularity variant to the UseTree, etc. and feel free to leave an inline comment on this line (or wherever the attrs are conditionally derived) explaining that we only want to do this for the Item variant
narpfel
force-pushed
the
preserve-attrs-in-imports-granularity-item
branch
from
35a4edc to
ac910f4
Compare
|
I’ve made the requested changes and rebased onto I don’t really like the name |
|
Yeah it sounds a bit odd doesn't it. I'd still have preferred that we actually passed the config value of However, after refreshing my memory I bit I do so that this is only on the call path when the granularity is set to item, so I don't want to block getting this bug fixed for too long. I'd be alright with moving ahead without the extra enum/arg (similar to what you had originally) provided we add a couple TODO comments (on the |
|
Also for my own sanity, I wouldn't hate seeing test cases that included combinations of this variant of the granularity option with other relevant imports-related options (though don't feel obligated to do so) |
narpfel
force-pushed
the
preserve-attrs-in-imports-granularity-item
branch
from
ac910f4 to
90022d6
Compare
|
I haven’t had time to look at adding more tests yet. |
| pub(crate) fn regroup_use_trees( | ||
| use_trees: Vec<UseTree>, | ||
| import_granularity: ImportGranularity, | ||
| ) -> Vec<UseTree> { | ||
| let merge_by = match import_granularity { | ||
| ImportGranularity::Item => return flatten_use_trees(use_trees, ImportGranularity::Item), | ||
| ImportGranularity::Preserve => return use_trees, | ||
| ImportGranularity::Crate => SharedPrefix::Crate, | ||
| ImportGranularity::Module => SharedPrefix::Module, | ||
| ImportGranularity::One => SharedPrefix::One, | ||
| }; | ||
|
|
There was a problem hiding this comment.
Apologies as I feel like we're getting into nit territory but I don't want to make this change, at least not with the function name including regroup. We have other options and flows on these same constructs where group has specific denotations and using group here is confusing/implies behavior not actually happening.
I'd be fine with this if we can come up with a more apt name (not sure if something like normalize_use_trees or even something more generic like handle_use_trees would work?)
|
As this PR seems to have gone stale, I've updated it and opened a new one here: #5314 Happy if you'd rather I provided patches to this existing PR instead. |
|
Closing in favor of #5314 |
Fixes #5030