fix function isdef

[omlins]

fixes #172

[omlins]

This way of checking if an expression is a long function definition is taken from here:
here

However, it seems to me that the or statement has no effect and the following would be equivalent:

islongdef(ex)  = @capture(ex, function fcall_ body_ end)

[fingolfin]

I also wonder about the |... odd...

Independently of that, maybe also change https://github.com/FluxML/MacroTools.jl/blob/master/src/utils.jl#L298-L300 to use islongdef?

[cstjean]

Good catch, indeed, 69f5811 shows that I'm the one who changed a more complex | expression to this (fcall_ | fcall_). I'm going to guess that I did that purely because this is complex code and I wanted to play it safe.

Long story short: please try it without the |, and if all tests pass, we'll keep it like that.

[omlins]

@cstjean, can you review and merge this?

[cstjean]

It's on my todo list, but it may take a few more weeks, with covid... Sorry

[cstjean]

Could you please add negative examples to the test suite for isdef, to show that this PR is more correct? Particularly interested in function foo end (should that be a function definition? I think not), but others would be nice too.

[cstjean]

should that be a function definition?

We should probably check the docs. I'd call it function declaration, but if the docs say otherwise...

[matthias314]

The proposed function

islongdef(ex)  = @capture(ex, function (fcall_ | fcall_) body_ end)

seems to work (although I don't fully understand why). Replacing (fcall_ | fcall_) by fcall_ would lead to a syntax error both in Julia 1.9 and 1.10.0-beta1.

However, I think there is a problem with isshortdef: I've tried definitions of the form

ex1 = :( function f(x,y) x+y end )
ex2 = :( function (x,y) x+y end )
ex3 = :( f(x,y) = x+y )
ex4 = :( (x,y) -> x+y )

plus variants with argument type annotations, return type annotations and where clauses. They are all accepted by splitdef. Moreover, longdef preserves ex1 and ex2 and converts ex3 and ex4 to ex1 and ex2, respectively. shortdef preserves ex3 and ex4 and converts ex1 and ex2 to ex3 and ex4, respectively. islongdef is true for ex1 and ex2, but isshortdef is true only for ex3. I think it would make sense for isshortdef to return true also for ex4.

[cstjean]

Right - those would all be good test cases for whoever takes up the baton on this PR.

[matthias314]

I could give it a try. Do you insist on using @capture? For example, I think one could simply say

islongdef(ex) = isexpr(ex, :function)

Of course, this doesn't check whether ex is well-formed, but the version with @capture doesn't do this completely, either.

[cstjean]

As long as the tests have good coverage, I don't think we need to worry about malformed expressions. isexpr is fine.