-
Notifications
You must be signed in to change notification settings - Fork 214
Add ReplaceInvocationsInModule #3061
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
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.
LGTM with comments addressed
return ParseTypecheckResult{.import_data = std::move(import_data), .tm = std::move(tm)}; | ||
} | ||
|
||
TEST(ReplaceInvocationsTest, NonParametricSimpleReplacement) { |
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.
I would suggest parameterizing these tests to run on both typecheck versions so they don't become tech debt for the deletion of TIv1. The contract for what TI needs to include in TypeInfo is not always clear, so any new use outside of IR converter could encounter subtle differences.
Also as a side note, once we delete TIv1 we can probably expose better data from TIv2 for this use case (e.g. TIv2 keeps parametric envs in ExprOrType format natively).
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.
There are actually a couple of tests in here that fail with TIv2 and so are failing internally at Google:
[ PASSED ] 23 tests.
[ FAILED ] 2 tests, listed below:
[ FAILED ] ReplaceInvocationsTest.ParametricEnumUseImportExplicitReplacement
[ FAILED ] ReplaceInvocationsTest.ParametricToEnvNonEnumTypeAnnotationErrors
Could you either parameterize it or do a quick hack where you change kDefaultTypecheckVersion in parse_and_typecheck.h and run it that way to debug? Let me know if you need help investigating.
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.
I see. Let me have a look when I got some time.
This PR introduces
ReplaceInvocationsInModule
. It operates onTypecheckedModule
since type information is required to perform the transformation.ReplaceInvocationsInModule
rewrites all invocations that matches a given callee function and parameter list to different callees and parameter lists. This is particularly useful for tools I am building for transforming a DSLX file into a fully monomorphized form.The result is a new
TypecheckedModule
.