-
Notifications
You must be signed in to change notification settings - Fork 71
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
Rename bound variables #1295
base: main
Are you sure you want to change the base?
Rename bound variables #1295
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.
Looks good overall - just needs to address the issue with handling direct-node-application
.
@@ -70,14 +73,34 @@ a subtree of `node`." | |||
:type (node-type node) | |||
:bindings let-bindings | |||
:subexpr new-lisp-node))))) | |||
(action (:before node-direct-application node) | |||
(action (:before node-direct-application node subs) | |||
(when (find (node-direct-application-rator node) subs :key #'ast-substitution-from) |
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 believe the issue is localized to the node-direct-application
pass. The safest way to handle this would be to add an additional argument rename-bound-variables
so that you can be sure that you can track variable renaming separately from the other substitutions. A solution that seems to work, at least well enough to pass the tests, is to replace this when
with the following
(if (and rename-bound-variables
(node-variable-p (ast-substitution-to res)))
(make-node-direct-application
:type (node-type node)
:rator-type (node-direct-application-rator-type node)
:rator (node-variable-value (ast-substitution-to res))
:rands (node-direct-application-rands node))
(util:coalton-bug
"Failure to apply ast substitution on variable ~A to node-direct-application"
(node-direct-application-rator node)))
This seems to be the only issue. Once it is addressed, this PR is ready to merge in my opinion. @amorphedstar let me know if you would want me to work on this, although I would definitely appreciate your input either way.
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.
Yeah, that looks about right. I think the thing that concerned me is that the kind of rewriting used for inlining here is different from the most general AST substitution which will replace node-variable
with arbitrary AST nodes.
For the current use case in inlining, it just rewrites variable names in the function body, and now it also recursively renames variables bound in let
blocks, but this is just about the names, and really doesn't do anything with AST nodes.
So I don't know if it makes sense to use the same
(defstruct ast-substitution
(from (util:required 'from) :type parser:identifier :read-only t)
(to (util:required 'to) :type node :read-only t))
for both cases.
Maybe it would be better to separate this to a more specialized variable renaming function, with a more specialized substitution structure like this:
(defstruct identifier-renaming
(from (util:required 'from) :type parser:identifier :read-only t)
(to (util:required 'to) :type parser:identifier :read-only t))
Do you think that sounds like a better structure, or is it nicer to have the similar functionality together in one function as it currently is?
One other thing I think would be good to know: @stylewarning mentioned that there are some subtleties for correctness in inlining.
The function arguments obviously need to be renamed in the body, but we found that we also needed to rename type variables so that they aren't unified over different calls, and now bound variables also need to be renamed.
I wonder if there is a good reference listing exactly what is necessary, so we can have more confidence that we aren't missing something else still.
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 think the structure identifier-renaming
is the most correct thing to do, even if the code will be a bit more verbose.
aeb43c9
to
bb8e0f7
Compare
This fixes #1293
The codegen assumes that each variable only appears with one meaning, and in particular can never be bound in a context where it is already bound. When recursively unrolling functions with
node-let
, this was no longer true. This PR adds bound-variable renaming as an option as a part ofapply-ast-substitution
.