Skip to content

fix: Improve hygiene of trait impl macros#839

Merged
RobertJacobsonCDC merged 1 commit intomainfrom
RobertJacobsonCDC_fully_qualify_trait_methods
Apr 7, 2026
Merged

fix: Improve hygiene of trait impl macros#839
RobertJacobsonCDC merged 1 commit intomainfrom
RobertJacobsonCDC_fully_qualify_trait_methods

Conversation

@RobertJacobsonCDC
Copy link
Copy Markdown
Collaborator

This PR improves the hygiene of macros that call trait methods on concrete types. Since we are touching this macro code anyway, we also normalize a particular syntax we use in macro definitions (described below), which is merely cosmetic and has no functional effect.

Use fully qualified trait method invocation in macro-generated trait impls when name collisions are possible

Suppose we have a macro that implements a trait for a concrete type identified by a macro parameter like $identifier, and that same macro also generates code that calls methods from that trait on that type. If those generated calls use ordinary syntax like $identifier::id() or $identifier::is_derived(), they can collide with inherent methods already defined on the concrete type. For example, if the macro implements Property<Person> for Age, and Age already has its own inherent fn id() -> usize, then generated code that says Age::id() may call the inherent method instead of the trait method Property<Person>::id(). And if Age::id() has a different signature from Property<Person>::id(), you get a compiler error.

We fix this by changing such calls to fully qualified trait method syntax, so the generated code explicitly names both the trait and the receiver type. Instead of generating $identifier::id() or $identifier::is_derived(), we generate calls like <$identifier as $crate::entity::property::Property<$entity>>::id() and <$identifier as $crate::entity::property::Property<$entity>>::is_derived(). This removes the ambiguity by telling Rust exactly which trait method to call, even if the concrete type also defines an inherent method with the same name.

This is motivated by the real example of trying to implement Property<Person> for FIPSCode, because the concrete type has a method FIPSCode::id that collides with Property::id.

Use consistent syntax for Self in fully qualified trait method invocation

This PR also normalizes fully qualified trait method calls so that, whenever the receiver is the current impl type, we write Self instead of repeating the concrete type name. For example, in src/macros/property_impl.rs we use <Self as $crate::entity::property::Property<$entity>>::id() rather than <$property as $crate::entity::property::Property<$entity>>::id().

The justification for this is:

  • Consistency.
  • Better with generics. Self avoids having to restate type parameters, lifetimes, and wrappers in the qualified path. That makes the macro simpler and less fragile.
  • Better for refactors. If the concrete type spelling changes, Self keeps the call correct without needing to update repeated type syntax.
  • Communicates intent more clearly. <Self as Trait>::... reads as "call the trait method from this impl."

This isn't a very important change, but it is also pretty low risk. Syntactic housekeeping since we are touching the code anyway.

…o-based trait impls where name collisions with methods on the concrete type are possible
@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 4, 2026

Benchmark Results

Hyperfine

Command Mean [ms] Min [ms] Max [ms] Relative
large_sir::baseline 2.8 ± 0.0 2.7 3.0 1.00
large_sir::entities 12.4 ± 0.5 12.0 14.5 4.45 ± 0.20

Criterion

Regressions (slower)
Group Bench Param Change CI Lower CI Upper
counts single_property_indexed_entities 13.380% 12.021% 14.390%
sample_entity sample_entity_single_property_indexed 100000 10.516% 9.620% 11.323%
counts multi_property_unindexed_entities 5.928% 3.828% 8.044%
sample_entity sample_entity_single_property_unindexed 10000 5.674% 1.921% 10.290%
counts index_after_adding_entities 4.397% 3.993% 4.750%
sample_entity sample_entity_single_property_indexed 1000 3.509% 2.134% 4.800%
algorithm_benches algorithm_sampling_multiple_l_reservoir 3.317% 2.959% 3.652%
algorithm_benches algorithm_sampling_multiple_known_length 2.868% 2.665% 3.078%
sample_entity sample_entity_multi_property_indexed 10000 2.620% 1.371% 3.790%
indexing with_query_results_single_indexed_property_entities 1.877% 1.330% 2.393%
sampling sampling_single_known_length_entities 1.631% 1.305% 1.958%
indexing query_people_count_single_indexed_property_entities 1.534% 1.297% 1.759%
counts reindex_after_adding_more_entities 1.304% 1.065% 1.538%
indexing query_people_count_multiple_individually_indexed_properties_enti 1.233% 1.061% 1.386%
Improvements (faster)
Group Bench Param Change CI Lower CI Upper
sample_entity sample_entity_single_property_unindexed 1000 -23.509% -23.852% -23.240%
indexing query_people_single_indexed_property_entities -19.706% -20.024% -19.266%
indexing query_people_count_indexed_multi-property_entities -19.535% -19.803% -19.115%
indexing with_query_results_indexed_multi-property_entities -13.616% -14.382% -12.999%
large_dataset bench_query_population_derived_property_entities -8.140% -9.174% -7.147%
large_dataset bench_match_entity -5.652% -7.043% -4.708%
sample_entity sample_entity_multi_property_indexed 1000 -5.185% -7.378% -3.346%
sampling sampling_single_l_reservoir_entities -5.085% -5.963% -4.221%
large_dataset bench_query_population_multi_indexed_entities -4.641% -5.061% -4.298%
indexing query_people_indexed_multi-property_entities -4.190% -4.425% -3.945%
sampling sampling_multiple_l_reservoir_entities -2.572% -2.722% -2.442%
sampling sampling_single_unindexed_concrete_plus_derived_entities -1.995% -2.236% -1.748%
Unchanged / inconclusive (CI crosses 0%)
Group Bench Param Change CI Lower CI Upper
large_dataset bench_query_population_property_entities -1.776% -3.117% -0.518%
sample_entity sample_entity_single_property_indexed 10000 -1.394% -2.181% -0.691%
sample_entity sample_entity_multi_property_indexed 100000 1.368% 0.341% 2.383%
examples example-basic-infection -1.204% -1.694% -0.745%
sampling sampling_multiple_known_length_entities -1.183% -1.489% -0.859%
sample_entity sample_entity_whole_population 100000 0.739% 0.377% 1.198%
algorithm_benches algorithm_sampling_single_l_reservoir 0.737% 0.547% 0.863%
examples example-births-deaths 0.554% 0.276% 0.845%
indexing with_query_results_multiple_individually_indexed_properties_enti -0.525% -0.973% -0.066%
sampling count_and_sampling_single_known_length_entities -0.461% -0.775% -0.058%
large_dataset bench_filter_unindexed_entity -0.444% -3.334% 2.767%
indexing query_people_multiple_individually_indexed_properties_entities -0.437% -0.688% -0.198%
sample_entity sample_entity_single_property_unindexed 100000 0.395% 0.084% 0.706%
sample_entity sample_entity_whole_population 10000 0.335% -0.250% 0.919%
large_dataset bench_filter_indexed_entity 0.330% -7.247% 8.113%
large_dataset bench_query_population_multi_unindexed_entities -0.317% -0.624% -0.054%
sampling count_and_sampling_single_unindexed_concrete_plus_derived_entiti -0.166% -0.238% -0.090%
counts multi_property_indexed_entities 0.155% -0.073% 0.383%
sampling sampling_multiple_unindexed_entities -0.137% -0.186% -0.094%
algorithm_benches algorithm_sampling_single_rand_reservoir -0.137% -0.306% -0.016%
counts single_property_unindexed_entities -0.128% -0.888% 0.586%
large_dataset bench_query_population_indexed_property_entities -0.109% -0.317% 0.119%
sampling sampling_single_unindexed_entities -0.099% -0.182% -0.018%
counts concrete_plus_derived_unindexed_entities -0.076% -0.350% 0.181%
algorithm_benches algorithm_sampling_single_known_length -0.057% -0.507% 0.283%
sample_entity sample_entity_whole_population 1000 -0.005% -0.367% 0.253%

github-actions bot added a commit that referenced this pull request Apr 4, 2026
Copy link
Copy Markdown
Collaborator

@k88hudson-cfa k88hudson-cfa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, it does make me wonder if this happens elsewhere in the codebase?

@RobertJacobsonCDC RobertJacobsonCDC merged commit 597b397 into main Apr 7, 2026
22 checks passed
@RobertJacobsonCDC RobertJacobsonCDC deleted the RobertJacobsonCDC_fully_qualify_trait_methods branch April 7, 2026 12:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants