-
Notifications
You must be signed in to change notification settings - Fork 173
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
Local variables can't share names with commands/macros/functions in the dynamic context. #659
Comments
Possibly related to #597 |
Is this fixed now because of #695 being merged? |
It is not, because of the ordering of lookups still not being quite correct. The offending code block is this: Lines 44 to 64 in b63b940
The lookup order matters, and currently its behavior is to first look for builtins. This could be changed, but it breaks other things, and we agreed in Gitter to postpone fixing this. I’m not sure what exactly broke anymore, sadly. |
OK, it's not a giant problem but I'll keep the issue open then. |
As @hellerve stated, this ultimately comes down to our lookup orders. Ideally, preferring local environments before the Dynamic environment wouldn't cause any problems and could fix this issue. I'll give it a shot. |
I have a fix for this...but oddly it seems to make |
In particular, it makes the performance of walk-replace prohibitve and breaks the behavior of postwalk. Prewalk is strangely unaffected as are all other functions and test thus far—so the solution doesn’t account for an edge |
This commit adds a new lookup preference to the evaluator, LookupLocal, and uses it to lookup bindings in the scope of let forms. This fixes an issue whereby our original bias toward dynamic bindings would prevent users from shadowing dynamic bindings with local bindings of the same name. Before the following code returned `command c`: ``` (defdynamic test-val (let [c (car (list 1 2 3))] c)) ``` It now returns `1`. I also fixed a small issue with top-level (as in, without a corresponding function environment) let forms (they'd cause a crash for lack of an environment parent). Fixes carp-lang#659
…s. (#1214) * fix: don't shadow local bindings with dynamics This commit adds a new lookup preference to the evaluator, LookupLocal, and uses it to lookup bindings in the scope of let forms. This fixes an issue whereby our original bias toward dynamic bindings would prevent users from shadowing dynamic bindings with local bindings of the same name. Before the following code returned `command c`: ``` (defdynamic test-val (let [c (car (list 1 2 3))] c)) ``` It now returns `1`. I also fixed a small issue with top-level (as in, without a corresponding function environment) let forms (they'd cause a crash for lack of an environment parent). Fixes #659 * refactor: only prefer local lookups for shadows The prior commit introduced a local lookup preference into the evaluator in order allow for shadowing of global names in local scopes (like let bindings). However, this introduced prohibitive performance costs, especially for dynamic functions. To mitigate this, we only perform local-biased lookups for a set of known-shadows. Since we know the names of local bindings at form evaluation time, we can restrict our local lookup bias to these paths only. This greatly reduces the performance penalties initially incurred by the change. I've also refactored some of the lookup code for clarity. * fix: support recursive let bindings Previously, the bodies of anonymous functions bound to a let name did not have access to their names, making recursion such as: ``` (let [f (fn [x] (if (= x 1) x (f (dec x))))] (f 10)) ``` impossible. We now equip evaluation of let bindings with an additional recursion environment making this possible. The example above will now resolve to `1`. Fixes #1133
Is this still the case with #1214? |
When trying to use a name for a local variable that has the same name a command, macro, or dynamic function in the dynamic context, Carp doesn't overshadow that global variable or provide warnings. Changing the variable name to something else fixes the problem.
For example, you cannot use
c
as a variable name, as it is a function.Issue #555 suggests we wrap the result in
(quote ...)
while using the REPL, which results in:If we use a variable name that is not a function, everything works fine:
This is the case for code run in the REPL and while compiling a file. If you place the following code into a file, then load it and look at
test-val
's value, you will see that the same thing occurs.You can try this with
car
,e
,nthcdr
,nthcar
, etc. and a similar result occurs.The text was updated successfully, but these errors were encountered: