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

Make fn work like lambda and remove lambda #1228

Merged
merged 3 commits into from
Feb 22, 2017
Merged

Conversation

Kodiologist
Copy link
Member

Closes #910.

@tuturto
Copy link
Contributor

tuturto commented Feb 19, 2017

First thing to come to mind is that lambdas can't be decorated:

=> (defn double [func] (fn [x] (* (func x) 2)))
=> ((with-decorator double (fn [x] x)) 5)
  File "<input>", line 1, column 2

  ((with-decorator double (fn [x] x)) 5)
   ^--------------------------------^
HyTypeError: b'Decorated a non-function'

But without the changes, example works:

(defn double [func] (fn [x] (* (func x) 2)))

=> ((with-decorator double (fn [x] x)) 5)
10

=> ((with-decorator double (lambda [x] x)) 5)
  File "<input>", line 1, column 2

  ((with-decorator double (lambda [x] x)) 5)
   ^------------------------------------^
HyTypeError: b'Decorated a non-function'

In my opinion, this is big enough drawback not to do this change.

@Kodiologist
Copy link
Member Author

That could be implemented, but why would you write (with-decorator double (fn [x] x)) when you could just write (double (fn [x] x))? What's the point?

@tuturto
Copy link
Contributor

tuturto commented Feb 19, 2017

Point of the code was to show a simple testcase which will fail if this change is pulled in. Actual content of the example decorator isn't important. Currently user can be confident that result of fn can be decorated, regardless of how simple or complex it actually is. With the change, they can't anymore.

@Kodiologist
Copy link
Member Author

Kodiologist commented Feb 19, 2017

My argument isn't specific to your test case. It applies no matter how complex the decorator or the decorated (fn ...) is: (with-decorator d (fn ...)) can always be written as (d (fn ...)), so with-decorator is wholly redundant when using fn. The reason decorators exist is so you can use them with def (defn in Hy) and defclass, not with expressions. Indeed, the fact that (with-decorator d (fn ...)) worked was untested and undocumented.

This feature is of dubious value by itself, but it's necessary to allow `defn` to create a lambda instead of a `def`.
That is, allow it to generate a `lambda` instead of a `def` statement if the function body is just an expression.

I've removed two uses of with_decorator in hy.compiler because they'd require adding another case to HyASTCompiler.compile_decorate_expression and they have no ultimate effect, anyway.

In a few tests, I've added a meaningless statement in `fn` bodies to force generation of a `def`.

I've removed `test_fn_compiler_empty_function` rather than rewrite it because it seems like a pain to maintain and not very useful.
@tuturto
Copy link
Contributor

tuturto commented Feb 19, 2017

Right, now I follow. Shouldn't test things before second cup of coffee in morning.

Indeed, following works as expected:

=> (defn double [func] (fn [x] (* (func x) 2)))
=> (with-decorator double (defn test [x] x))
=> (test 2)
4

Copy link
Contributor

@tuturto tuturto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't find any funny edge cases with quick testing, so this in general looks good to me.

Copy link
Contributor

@refi64 refi64 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be worth it to mention why the setv change is needed in a bit more detail somewhere? At a glance, it just seems really confusing.

@Kodiologist
Copy link
Member Author

Like the commit message says, "it's necessary to allow defn to create a lambda instead of a def." For example, if you do (with-decorator d (defn f [x] (+ x 1))), the (defn ...) gets compiled to something like f = lambda x: x + 1, so with-decorator needs to be able to handle ast.Assign, which in Hy usually appears from a (setv ...) form.

@Kodiologist
Copy link
Member Author

So technically, supporting (setv ...) forms isn't necessary; it's just an immediate consequence of supporting ast.Assign.

@Kodiologist
Copy link
Member Author

Or, putting it another way, the (setv ...) produced by defn used to always lead to ast.FunctionDef downstream. Now it might be an ast.Assign.

@refi64 refi64 merged commit e4a7b31 into hylang:master Feb 22, 2017
@Kodiologist
Copy link
Member Author

And that's commit 2,000. This calls for celebration!

@Kodiologist Kodiologist deleted the unlambda branch February 24, 2017 20:48
Kodiologist added a commit to Kodiologist/hy that referenced this pull request Mar 26, 2017
The bug was a regression that I introduced in hylang#1228.

I've created a new special form named `fn*` that works like the old `fn` (that is, it always creates a FunctionDef). Since this is intended only for internal use, like `with*`, I haven't documented it.
Kodiologist added a commit to Kodiologist/hy that referenced this pull request Mar 26, 2017
The bug was a regression that I introduced in hylang#1228.

I've created a new special form named `fn*` that works like the old `fn` (that is, it always creates a `FunctionDef`). Since this is intended only for internal use, like `with*`, I haven't documented it.
tuturto pushed a commit that referenced this pull request Apr 13, 2017
The bug was a regression that I introduced in #1228.

I've created a new special form named `fn*` that works like the old `fn` (that is, it always creates a `FunctionDef`). Since this is intended only for internal use, like `with*`, I haven't documented it.
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