Initial work towards dynamically resolving enum variants. - #926
Conversation
| let base_iter = enum_item.variants.iter(); | ||
|
|
||
| base_iter.copied().enumerate().map(move |(i, var_id)| { | ||
| let var = index.get(&var_id).expect("Variant id should be correct."); |
There was a problem hiding this comment.
rustdoc JSON occasionally has bugs that result in dangling IDs, and some of those issues are still open. We shouldn't hit this panic, but we might.
Slight preference for using filter_map() and omitting invalid items from the index instead of exploding, just so we don't cause ourselves extra support burden if such a bug affects our users.
There was a problem hiding this comment.
I have made the change, but I'm not convinced that it's a good idea. If dangling ids do exist, then it's likely that they will cause a false positive in enum_variant_missing. Then, instead of being able to point to the backtrace and easily identify the cause as dangling ids in rustdoc, there will be no clear signal that determines the cause.
|
Broadly looks in decent shape, just needs some more polishing to improve maintainability: using |
|
This is ready for review. I'm not happy with the code duplication in |
|
After some more analysis, this isn't producing the performance results that I had initially hoped for. I would like to see whether there are other ways of speeding up the same queries that are more impactful. EDIT: After a little more research, the only really discontinuous query is |
…h. (#927) **Purpose of Change** This is something that is done a lot and is basically copy-pasted between call sites. Especially in lookup code, such as in #926, the match can harm readability. **Describe the Change** Introduce a new function `crate_at_origin`. Since it's `pub(crate)`, it won't affect the API, but it can make internal code easier to read. I have changed some of the uses to demonstrate the changes. **Describe Alternatives** Perhaps `package_index_at_origin` may be a better name. --------- Co-authored-by: Predrag Gruevski <2348618+obi1kenobi@users.noreply.github.com>
…h. (#927) **Purpose of Change** This is something that is done a lot and is basically copy-pasted between call sites. Especially in lookup code, such as in #926, the match can harm readability. **Describe the Change** Introduce a new function `crate_at_origin`. Since it's `pub(crate)`, it won't affect the API, but it can make internal code easier to read. I have changed some of the uses to demonstrate the changes. **Describe Alternatives** Perhaps `package_index_at_origin` may be a better name. --------- Co-authored-by: Predrag Gruevski <2348618+obi1kenobi@users.noreply.github.com>
…h. (#927) **Purpose of Change** This is something that is done a lot and is basically copy-pasted between call sites. Especially in lookup code, such as in #926, the match can harm readability. **Describe the Change** Introduce a new function `crate_at_origin`. Since it's `pub(crate)`, it won't affect the API, but it can make internal code easier to read. I have changed some of the uses to demonstrate the changes. **Describe Alternatives** Perhaps `package_index_at_origin` may be a better name. --------- Co-authored-by: Predrag Gruevski <2348618+obi1kenobi@users.noreply.github.com>
…h. (#927) **Purpose of Change** This is something that is done a lot and is basically copy-pasted between call sites. Especially in lookup code, such as in #926, the match can harm readability. **Describe the Change** Introduce a new function `crate_at_origin`. Since it's `pub(crate)`, it won't affect the API, but it can make internal code easier to read. I have changed some of the uses to demonstrate the changes. **Describe Alternatives** Perhaps `package_index_at_origin` may be a better name. --------- Co-authored-by: Predrag Gruevski <2348618+obi1kenobi@users.noreply.github.com>
…h. (#927) **Purpose of Change** This is something that is done a lot and is basically copy-pasted between call sites. Especially in lookup code, such as in #926, the match can harm readability. **Describe the Change** Introduce a new function `crate_at_origin`. Since it's `pub(crate)`, it won't affect the API, but it can make internal code easier to read. I have changed some of the uses to demonstrate the changes. **Describe Alternatives** Perhaps `package_index_at_origin` may be a better name. --------- Co-authored-by: Predrag Gruevski <2348618+obi1kenobi@users.noreply.github.com>
…h. (#927) (#929) **Purpose of Change** This is something that is done a lot and is basically copy-pasted between call sites. Especially in lookup code, such as in #926, the match can harm readability. **Describe the Change** Introduce a new function `crate_at_origin`. Since it's `pub(crate)`, it won't affect the API, but it can make internal code easier to read. I have changed some of the uses to demonstrate the changes. **Describe Alternatives** Perhaps `package_index_at_origin` may be a better name. --------- Co-authored-by: Predrag Gruevski <2348618+obi1kenobi@users.noreply.github.com> Co-authored-by: CLIDragon <84266961+CLIDragon@users.noreply.github.com>
…h. (#927) (#930) **Purpose of Change** This is something that is done a lot and is basically copy-pasted between call sites. Especially in lookup code, such as in #926, the match can harm readability. **Describe the Change** Introduce a new function `crate_at_origin`. Since it's `pub(crate)`, it won't affect the API, but it can make internal code easier to read. I have changed some of the uses to demonstrate the changes. **Describe Alternatives** Perhaps `package_index_at_origin` may be a better name. --------- Co-authored-by: Predrag Gruevski <2348618+obi1kenobi@users.noreply.github.com> Co-authored-by: CLIDragon <84266961+CLIDragon@users.noreply.github.com>
…h. (#927) (#931) **Purpose of Change** This is something that is done a lot and is basically copy-pasted between call sites. Especially in lookup code, such as in #926, the match can harm readability. **Describe the Change** Introduce a new function `crate_at_origin`. Since it's `pub(crate)`, it won't affect the API, but it can make internal code easier to read. I have changed some of the uses to demonstrate the changes. **Describe Alternatives** Perhaps `package_index_at_origin` may be a better name. --------- Co-authored-by: Predrag Gruevski <2348618+obi1kenobi@users.noreply.github.com> Co-authored-by: CLIDragon <84266961+CLIDragon@users.noreply.github.com>
|
After some review, resolving variants does appear to have the intended effect - it's simply smaller in magnitude than I was hoping for. |
6cc49e9 to
9888ade
Compare
|
Apologies for the churn. While the variant resolution itself appears to be correct, I'm currently running into an issue where tracing reports that this should be reducing |
|
No worries at all. If you feel a second pair of eyes on the trace might be helpful, I'm happy to take a look too! |
|
Running @obi1kenobi Could you confirm that, when running on 4 threads in release mode on |
|
Yes, I'm seeing a 10x speedup on that lint with this branch patched in than without it. |
|
Awesome, this should (finally) be ready to merge then. |
|
Aside from the So while I believe the implementation is correct, it's concerning that it might not be (or might break in the future) and we'd never know. Please consider adding test cases that hit both code paths, and produce a non-empty set of results. To make the {
Crate {
item {
... on Enum {
variant {
name @tag @output
}
variant {
other: name @filter(op: "=", value: ["%name"]) @output
}
}
}
}
}This is a bit of a silly query, but it should still be optimized and not be O(n^2). You could add a new test crate for the optimizations specifically, or extend one of the existing test crates that deals with enum variants — your call. |
|
Heads up: since Rust 1.89 was released ~yesterday, I'm hoping to release a new cargo-semver-checks very soon, ideally this weekend time permitting. It's not a big deal if we can't get this PR in in time, and I don't want you to re-plan your entire weekend over it. There will be more releases! I just wanted to mention it so you aren't surprised. |
9888ade to
97d7ee2
Compare
Comparing enum variants by name is time consuming because each variant needs to be compared name by name to find the correct variant. Adding in a cache reduces this from a quadratic search into an O(1) lookup.
97d7ee2 to
fe13742
Compare
|
I've added tests and removed the excess |
**Purpose of Change** Resolving enum variants costs a lot of time in already heavy queries. In particular, the following lints will run faster after this change. ``` enum_no_repr_variant_discriminant_changed enum_unit_variant_changed_kind enum_variant_marked_non_exhaustive enum_variant_missing partial_ord_enum_variants_reordered ``` **Describe the Change** Implement a custom resolver for enum variants based on a new index. **Additional Context** If all of the above lints act only on public enums then index creation can be made faster by using `pub_item_kind_index`. --------- Co-authored-by: Predrag Gruevski <2348618+obi1kenobi@users.noreply.github.com>
**Purpose of Change** Resolving enum variants costs a lot of time in already heavy queries. In particular, the following lints will run faster after this change. ``` enum_no_repr_variant_discriminant_changed enum_unit_variant_changed_kind enum_variant_marked_non_exhaustive enum_variant_missing partial_ord_enum_variants_reordered ``` **Describe the Change** Implement a custom resolver for enum variants based on a new index. **Additional Context** If all of the above lints act only on public enums then index creation can be made faster by using `pub_item_kind_index`. --------- Co-authored-by: Predrag Gruevski <2348618+obi1kenobi@users.noreply.github.com> Co-authored-by: CLIDragon <84266961+CLIDragon@users.noreply.github.com>
**Purpose of Change** Resolving enum variants costs a lot of time in already heavy queries. In particular, the following lints will run faster after this change. ``` enum_no_repr_variant_discriminant_changed enum_unit_variant_changed_kind enum_variant_marked_non_exhaustive enum_variant_missing partial_ord_enum_variants_reordered ``` **Describe the Change** Implement a custom resolver for enum variants based on a new index. **Additional Context** If all of the above lints act only on public enums then index creation can be made faster by using `pub_item_kind_index`. --------- Co-authored-by: Predrag Gruevski <2348618+obi1kenobi@users.noreply.github.com> Co-authored-by: CLIDragon <84266961+CLIDragon@users.noreply.github.com>
**Purpose of Change** Resolving enum variants costs a lot of time in already heavy queries. In particular, the following lints will run faster after this change. ``` enum_no_repr_variant_discriminant_changed enum_unit_variant_changed_kind enum_variant_marked_non_exhaustive enum_variant_missing partial_ord_enum_variants_reordered ``` **Describe the Change** Implement a custom resolver for enum variants based on a new index. **Additional Context** If all of the above lints act only on public enums then index creation can be made faster by using `pub_item_kind_index`. --------- Co-authored-by: Predrag Gruevski <2348618+obi1kenobi@users.noreply.github.com> Co-authored-by: CLIDragon <84266961+CLIDragon@users.noreply.github.com>
… (#937) **Purpose of Change** Resolving enum variants costs a lot of time in already heavy queries. In particular, the following lints will run faster after this change. ``` enum_no_repr_variant_discriminant_changed enum_unit_variant_changed_kind enum_variant_marked_non_exhaustive enum_variant_missing partial_ord_enum_variants_reordered ``` **Describe the Change** Implement a custom resolver for enum variants based on a new index. **Additional Context** If all of the above lints act only on public enums then index creation can be made faster by using `pub_item_kind_index`. --------- Co-authored-by: Predrag Gruevski <2348618+obi1kenobi@users.noreply.github.com> Co-authored-by: CLIDragon <84266961+CLIDragon@users.noreply.github.com> Co-authored-by: CLIDragon <84266961+CLIDragon@users.noreply.github.com>
… (#938) **Purpose of Change** Resolving enum variants costs a lot of time in already heavy queries. In particular, the following lints will run faster after this change. ``` enum_no_repr_variant_discriminant_changed enum_unit_variant_changed_kind enum_variant_marked_non_exhaustive enum_variant_missing partial_ord_enum_variants_reordered ``` **Describe the Change** Implement a custom resolver for enum variants based on a new index. **Additional Context** If all of the above lints act only on public enums then index creation can be made faster by using `pub_item_kind_index`. --------- Co-authored-by: Predrag Gruevski <2348618+obi1kenobi@users.noreply.github.com> Co-authored-by: CLIDragon <84266961+CLIDragon@users.noreply.github.com> Co-authored-by: CLIDragon <84266961+CLIDragon@users.noreply.github.com>
Purpose of Change
Resolving enum variants costs a lot of time in already heavy queries. In particular, the following lints will run faster after this change.
Describe the Change
Implement a custom resolver for enum variants based on a new index.
Additional Context
If all of the above lints act only on public enums then index creation can be made faster by using
pub_item_kind_index.