Skip to content
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

Update OR= to support unbound variables #158

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

shawwn
Copy link
Member

@shawwn shawwn commented Feb 6, 2019

@akkartik
Copy link
Member

akkartik commented Feb 6, 2019

Could you update the title (and maybe description) with precisely what functionality is being added here? I know you link to the discussion, but I'm not confident I followed it entirely. Is it just supporting unbound variables? If so, it's interesting that the new implementation is nothing like the old one.

If I know what functionality is being added, I can go back and add more tests once this PR is merged.

@shawwn shawwn changed the title Update OR= Update OR= to support unbound variables Feb 6, 2019
@rocketnia
Copy link
Member

Hmm, I'm not confident with that approach -- I think setforms oughta be in control of performing ssexpansion -- but my first stab at it would have issues of its own:

 (mac or= (place expr)
   (let (binds val setter) (setforms place)
     `(atwiths ,binds
-       (or ,val (,setter ,expr)))))
+       (or (errsafe ,val) (,setter ,expr)))))

In official Arc, maybe that would be fine, but in Anarki, it's possible for variable names to perform complex behaviors when they're evaluated, so they might throw exceptions for reasons other than being unbound.

I feel like a proper approach to this would involve refactoring how setforms works, which I don't think is worth it for this. What you have is probably good.

@akkartik
Copy link
Member

akkartik commented Feb 6, 2019

👍 to the title change.

@shawwn
Copy link
Member Author

shawwn commented Feb 6, 2019

Yeah, I think it's just unbound variable support.

As for the implementation, I lifted it from laarc. I didn't know arc already had an or=. But I know this one works. :)

I attempted to make anarki reloadable in this commit: 7ed7d12

@rocketnia
Copy link
Member

Oh, I just realized another thing about this that isn't quite right: The design of setforms makes it so in operations like (or= ((...) (...)) 42), the (...) expressions only get evaluated once. This change causes or= to evaluate those expressions more than once.

@rocketnia
Copy link
Member

Hmm, I'm sorry I seem to be rattling off a lot of different comments in a disorganized way today, but another thing comes to mind:

Your implementation uses (or (if (bound ',slot) ,slot) ...), but (bound ',slot) checks whether something is bound in the global scope, not the local scope. Honestly, it's hard for Arc macros to check for bindings in the local scope until we give them access to env, so this weaves into a lot of the other topics we've been talking about.

@shawwn
Copy link
Member Author

shawwn commented Feb 6, 2019

This change causes or= to evaluate those expressions more than once.

Interesting. Could you give an example?

@shawwn
Copy link
Member Author

shawwn commented Feb 6, 2019

(bound ',slot) checks whether something is bound in the global scope, not the local scope

Good point! This should get resolved with the env changes from #151

@rocketnia
Copy link
Member

This change causes or= to evaluate those expressions more than once.

Interesting. Could you give an example?

I haven't tested this yet, but try this:

(= foo (table))
(or= ((do (prn "hello") foo) (do (prn "world") 'bar)) 'baz)

I think the current or= will only print "hello" and "world" once, but I think yours will print them twice.

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