Skip to content

add support for empty nd-array syntax#4

Merged
c42f merged 3 commits intoJuliaLang:mainfrom
simeonschaub:sds/empty_ncat
Feb 7, 2022
Merged

add support for empty nd-array syntax#4
c42f merged 3 commits intoJuliaLang:mainfrom
simeonschaub:sds/empty_ncat

Conversation

@simeonschaub
Copy link
Copy Markdown
Member

What's still missing here is the correct compat check, since this is
only supported in Julia 1.8. I wasn't sure how to check for this in
check_ncat_compat.

What's still missing here is the correct compat check, since this is
only supported in Julia 1.8. I wasn't sure how to check for this in
`check_ncat_compat`.
Copy link
Copy Markdown
Member

@c42f c42f left a comment

Choose a reason for hiding this comment

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

Awesome, this is the first PR to this repo!

For now, this will need the tests like [;;] ==> (ncat 2) added in test/parser.jl (actually this should be (ncat-2) because the 2 is stored in the flags, not as a child node in the tree. Search for similar tests to figure out where best to add them.

For interactively developing the parser and tests, I'd suggest using the itest_parse function which will check several things and do some pretty printing:

julia> include("test/test_utils.jl")
julia> itest_parse(JuliaSyntax.parse_atom, "[;;]")

# Code:
[;;]

# Green tree:
     1:4      │[error]                   ✘
     1:4      │  [ncat]
     1:1      │    [                        "["
     2:2      │    ;                        ";"
     3:3      │    ;                        ";"
     4:4      │    ]                        "]"
Error: unexpected closing token:
[;;]
Error: multidimensional array syntax not supported in Julia version 1.6 < 1.7:
[;;]

# SyntaxNode:
(error (ncat-2))

# Julia Expr:
:($(Expr(:error, :([;;]))))

# AST dump
Expr
  head: Symbol error
  args: Array{Any}((1,))
    1: Expr
      head: Symbol ncat
      args: Array{Any}((1,))
        1: Int64 2

# flisp Julia Expr:
:($(Expr(:error, "unexpected \";\"")))

Also, optional... but perhaps consider moving this logic to the top of parse_array, because that's the function which handles the rest of ncat syntax? On second though, I don't know if this is useful. Either way is fine and what you have is more direct.

I wasn't sure how to check for this in check_ncat_compat.

This should already work as-is, because you emit K"ncat" and check_ncat_compat just checks for that.

@c42f
Copy link
Copy Markdown
Member

c42f commented Feb 7, 2022

This should already work as-is, because you emit K"ncat" and check_ncat_compat just checks for that.

Oh, I see the problem... it's not the same version check as other ncat stuff which is in 1.7.

We could consider adding the version check immediately where you detect the K";"? The only issue with that is it will cause two related and somewhat redundant errors on Julia <= 1.6, one for ncat syntax and one for this particular empty array syntax. But perhaps we can live with that.

@simeonschaub
Copy link
Copy Markdown
Member Author

We could consider adding the version check immediately where you detect the K";"? The only issue with that is it will cause two related and somewhat redundant errors on Julia <= 1.6, one for ncat syntax and one for this particular empty array syntax. But perhaps we can live with that.

Ok, I can do that. My initial thought was that we could maybe also just check if the ncat expression is otherwise empty in check_ncat_compat, but I wasn't sure how to do that.

@c42f
Copy link
Copy Markdown
Member

c42f commented Feb 7, 2022

just check if the ncat expression is otherwise empty in check_ncat_compat, but I wasn't sure how to do that

We'd have to add more sophisticated ways of looking back on the output to make that easy. That's possible, but I do like to avoid it because I think it leads to confusing code. I'd even like to remove peek_behind() completely but I'm not sure it's possible...

@c42f c42f merged commit e2fe70d into JuliaLang:main Feb 7, 2022
Comment thread src/parser.jl
Comment on lines +2730 to +2731
# [;;] ==> (ncat 2)
# [;; \n ] ==> (ncat 2)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'll fix these separately to keep them in sync with the actual tests (yeah, I need to improve the tooling so we don't need to repeat ourselves 😅 )

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Oops! Yeah, I think I was just thinking of the s-expression syntax instead of JuliaSyntax' one.

@c42f
Copy link
Copy Markdown
Member

c42f commented Feb 8, 2022

This looks excellent, thanks :-D

@simeonschaub simeonschaub deleted the sds/empty_ncat branch February 8, 2022 00:07
c42f pushed a commit to JuliaLang/julia that referenced this pull request Oct 17, 2025
topolarity pushed a commit to JuliaLang/julia that referenced this pull request Nov 14, 2025
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.

2 participants