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

Allow macrocall as function sig #55040

Merged
merged 1 commit into from
Feb 7, 2025
Merged

Allow macrocall as function sig #55040

merged 1 commit into from
Feb 7, 2025

Conversation

Keno
Copy link
Member

@Keno Keno commented Jul 5, 2024

@KristofferC requested that function @main(args) end should work. This is currently a parse error. This PR makes it work as expected by allowing macrocall as a valid signature in function (needs to exapand to a call expr). Note that this is only the flisp changes. If this PR is accepted, an equivalent change would need to be made in JuliaSyntax.

@JeffBezanson JeffBezanson added the parser Language parsing and surface syntax label Jul 5, 2024
@JeffBezanson
Copy link
Member

I think this makes sense. A macro call is a very similar form to a call so it fits smoothly, and you might as well be able to generate a function signature with a macro.

decl-sig)))
(let ((ex (parse-unary-prefix s)))
(if (and (pair? ex) (eq? (car ex) 'macrocall))
ex
Copy link
Member

Choose a reason for hiding this comment

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

This is the only tricky bit --- do we allow function @mac(x, y)::T (or with where) or does the macro generate the entire signature? Probably the latter (as you have it) but it made me stop and think.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we can allow both

Copy link
Member

@topolarity topolarity Jul 9, 2024

Choose a reason for hiding this comment

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

We already have that @mac(x, y)::T is allowed in call position and the ::T is not passed to the macro (unfortunately)

I guess we can allow you to return more syntax than the macro receives, but it seems a bit odd since it means you can end up generating redundant and/or conflicting type assertions.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think a redundant expansion, you can always do the same directly without going through the parser. Needs to be some kind of lowering error or be given semantics. But that's independent of whether it's a legal parse or not.

Copy link
Member

@topolarity topolarity Jul 9, 2024

Choose a reason for hiding this comment

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

I just mean that the macro can't handle the case that it wants to insert a type assertion when one is already present - the obvious way to do that would have been to pass that syntax to the macro but I don't think we can do that

Unless I'm missing something?

Keno added a commit to JuliaLang/JuliaSyntax.jl that referenced this pull request Jul 8, 2024
@Keno Keno force-pushed the kf/macrocallsig branch from 2e552a0 to dabbc1d Compare July 8, 2024 18:03
@Keno
Copy link
Member Author

Keno commented Jul 8, 2024

Seems like people like the change, so I've addressed @JeffBezanson 's review comment and put up the same change in JuliaSyntax.jl

@Keno Keno changed the title RFC: Allow macrocall as function sig Allow macrocall as function sig Jul 8, 2024
@ararslan ararslan added the needs news A NEWS entry is required for this change label Jul 9, 2024
@c42f
Copy link
Member

c42f commented Jul 17, 2024

Wait what? Surely this is breaking - this syntax already means something different in the existing parser:

julia> dump(Meta.parse("function @main(args) end"))
Expr
  head: Symbol function
  args: Array{Any}((1,))
    1: Expr
      head: Symbol macrocall
      args: Array{Any}((3,))
        1: Symbol @main
        2: LineNumberNode
          line: Int64 1
          file: Symbol none
        3: Symbol args

@c42f
Copy link
Member

c42f commented Jul 17, 2024

Oh, I see - function @main(args) end does work in JuliaSyntax? But not in the flisp parser. Hmm!

@Keno
Copy link
Member Author

Keno commented Jul 18, 2024

Doesn't work for me in JuliaParser (without my PR):

julia> function @callmemacro(a::Int)
           return 1
       end
ERROR: ParseError:
# Error @ REPL[5]:1:10
function @callmemacro(a::Int)
#        └──────────────────┘ ── Invalid signature in function definition
Stacktrace:
 [1] top-level scope
   @ REPL:1

I guess it accidentally worked for no method body?

@c42f
Copy link
Member

c42f commented Jul 19, 2024

I guess it accidentally worked for no method body?

That's ... surprising and weird! But yes, correct.

@Keno
Copy link
Member Author

Keno commented Jul 25, 2024

So here's a bit of an annoyance. What does

:(function @f()() end)

parse to?
Currently it's (@f())() but with my implementation, it changes to @f(); (). Doesn't seem great. More generally, how do we decide whether or not we parse a function sig tuple? I guess the straightforward answer is "if there is one (without newline), we parse it"?

@Keno
Copy link
Member Author

Keno commented Jul 25, 2024

Actually, we already have that exact logic for the function f end case, so I can just tweak this.

Keno added a commit to JuliaLang/JuliaSyntax.jl that referenced this pull request Jul 26, 2024
@Keno Keno force-pushed the kf/macrocallsig branch from dabbc1d to 0029ab6 Compare July 26, 2024 00:47
@c42f
Copy link
Member

c42f commented Jul 26, 2024

More generally, how do we decide whether or not we parse a function sig tuple?

Oh yeaah. This was a whole big pain to get right in JuliaSyntax! Glad you could reuse the existing implementation!

(Conversely the flisp parser doesn't produce a sensible parse tree in various cases related to anon functions with function keyword etc. And the problems are fixed in lowering instead 😬 .)

@Keno
Copy link
Member Author

Keno commented Jul 28, 2024

This should be good to go from my perspective on JuliaSyntax is merged and bumped.

@Keno Keno force-pushed the kf/macrocallsig branch from 0029ab6 to f97a278 Compare July 31, 2024 21:09
Keno added a commit to JuliaLang/JuliaSyntax.jl that referenced this pull request Aug 2, 2024
* Allow macrocall in function def syntax

Goes with JuliaLang/julia#55040

* Update src/parser.jl

Co-authored-by: Claire Foster <[email protected]>

* Test to cover function declaration with `var""` syntax

* Make `function (@f(x)) body end` an ambiguity error

This case is ambiguous as it might be either one of the following;
require the user to explicitly disambiguate between them

```
function (@f(x),)
    body
end

function @f(x)
    body
end
```

For the same reasons, `function ($f) body end` is also ambiguous.

Also fix parsing of `function (f(x),) end` to correctly emit a tuple.

---------

Co-authored-by: Claire Foster <[email protected]>
@Keno
Copy link
Member Author

Keno commented Dec 11, 2024

This became effective upon merging #56110, so I'll rebase and merge this to restore consistency between flisp and JuliaSyntax.

@Keno
Copy link
Member Author

Keno commented Dec 11, 2024

This became effective upon merging #56110, so I'll rebase and merge this to restore consistency between flisp and JuliaSyntax.

Actually, I'm wrong about that. It's not on the release branch. That said, I still think this is worth getting in for 1.12. @c42f preferences on doing and 0.5 release branch from JuliaSyntax master vs backporting a bunch of the fixes to the 0.4 branch?

@Keno Keno added this to the 1.12 milestone Dec 11, 2024
@Keno Keno added the backport 1.12 Change should be backported to release-1.12 label Feb 4, 2025
@KristofferC KristofferC mentioned this pull request Feb 4, 2025
32 tasks
@Keno Keno removed the needs news A NEWS entry is required for this change label Feb 6, 2025
`@KristofferC` requested that `function @main(args) end` should work.
This is currently a parse error. This PR makes it work as expected
by allowing macrocall as a valid signature in function (needs
to exapand to a call expr). Note that this is only the flisp changes.
The same changes were previously applied in JuliaSyntax.
@Keno Keno merged commit b65f004 into master Feb 7, 2025
4 of 7 checks passed
@Keno Keno deleted the kf/macrocallsig branch February 7, 2025 05:08
KristofferC pushed a commit that referenced this pull request Feb 11, 2025
@KristofferC requested that `function @main(args) end` should work. This
is currently a parse error. This PR makes it work as expected by
allowing macrocall as a valid signature in function (needs to exapand to
a call expr). Note that this is only the flisp changes. If this PR is
accepted, an equivalent change would need to be made in JuliaSyntax.

(cherry picked from commit b65f004)
KristofferC added a commit that referenced this pull request Feb 13, 2025
Backported PRs:
- [x] #57142 <!-- Add reference to time_ns in time -->
- [x] #57241 <!-- Handle `waitpid` race condition when `SIGCHLD` is set
to `SIG_IGN` -->
- [x] #57249 <!-- restore non-freebsd-unix fix for profiling -->
- [x] #57211 <!-- Ensure read/readavailable for BufferStream are
threadsafe -->
- [x] #57262 <!-- edit NEWS for v1.12 -->
- [x] #57226 <!-- cfunction: reimplement, as originally planned, for
reliable performance -->
- [x] #57253 <!-- bpart: Fully switch to partitioned semantics -->
- [x] #57273 <!-- fix "Right arrow autocompletes at line end"
implementation -->
- [x] #57280 <!-- dep: Update JuliaSyntax -->
- [x] #57229 <!-- staticdata: Close data race after backedge insertion
-->
- [x] #57298 <!-- Updating binding version to fix MMTk CI -->
- [x] #57248 <!-- improve concurrency safety for `Compiler.finish!` -->
- [x] #57312 <!-- Profile.print: de-focus sleeping frames as gray -->
- [x] #57289 <!-- Make `OncePerX` subtype `Function` -->
- [x] #57310 <!-- Make ptls allocations at least 128 byte aligned -->
- [x] #57311 <!-- Add a warning for auto-import of types -->
- [x] #57338 <!-- fix typo in Float32 random number generation -->
- [x] #57293 <!-- Fix getfield_tfunc when order or boundscheck is Vararg
-->
- [x] #57349 <!-- docs: fix-up world-age handling for META access -->
- [x] #57344 <!-- Add missing type asserts when taking the queue out of
the task struct -->
- [x] #57348 <!-- 🤖 [master] Bump the SparseArrays stdlib from 212981b
to 72c7cac -->
- [x] #55040 <!-- Allow macrocall as function sig -->
- [x] #57299 <!-- Add missing latestworld after parameterized type alias
-->
@KristofferC KristofferC removed the backport 1.12 Change should be backported to release-1.12 label Feb 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
parser Language parsing and surface syntax
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants