- 
                Notifications
    You must be signed in to change notification settings 
- Fork 13
feat: ReplaceTypes: recursively replace on types too #2442
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
| Codecov ReportAll modified and coverable lines are covered by tests ✅ 
 Additional details and impacted files@@                   Coverage Diff                   @@
##           acl/replace_recursive    #2442    +/-   ##
=======================================================
  Coverage                  82.59%   82.59%            
=======================================================
  Files                        247      246     -1     
  Lines                      45988    45879   -109     
  Branches                   41622    41615     -7     
=======================================================
- Hits                       37982    37893    -89     
+ Misses                      5987     5967    -20     
  Partials                    2019     2019            
 Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
 | 
|  | ||
| /// Options for how the replacement for an op is processed. May be specified by | ||
| /// [ReplaceTypes::replace_op_with] and [ReplaceTypes::replace_parametrized_op_with]. | ||
| // TODO also replacement consts. | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At this point I am groaning and wishing that maybe I hadn't gone this way.....but, it does continue the theme of, make the framework do the work - I am thinking to put in a breaking change next that changes ReplacementOptions::default() to turn on recursion, hence, normal users won't have to turn this on and it'll just happen.
Actually, I can do that by deprecating the non-options variant, and adding a new variant with a different ReplacementOptions....even in the same PR....
Follow-up to #2435 - as the test demonstrated, if we recursively replace within ops, we need to do types as well (if the op replacements change their input/output types), due to e.g. Input/Output.