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.

Sign up for GitHub

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

unhandled kwargs throw errors #422

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

unhandled kwargs throw errors #422

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 4 additions & 5 deletions src/expr.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ function _string_to_Expr(k, args)
# """\n a\n b""" ==> "a\nb"
return only(args2)
else
# This only happens when k == K"string" or when an error has occurred.
# This only happens when k == K"string" or when an error has occurred.
return Expr(:string, args2...)
end
end
Expand All @@ -159,7 +159,7 @@ function _fixup_Expr_children!(head, loc, args)
arg = args[i]
was_parens = @isexpr(arg, :parens)
arg = _strip_parens(arg)
if @isexpr(arg, :(=)) && eq_to_kw_in_call && i > 1
if @isexpr(arg, :(=)) && eq_to_kw_in_call && i > 1
arg = Expr(:kw, arg.args...)
elseif k != K"parens" && @isexpr(arg, :., 1) && arg.args[1] isa Tuple
h, a = arg.args[1]::Tuple{SyntaxHead,Any}
Expand Down Expand Up @@ -489,13 +489,13 @@ struct _BuildExprStackEntry
end

function build_tree(::Type{Expr}, stream::ParseStream;
filename=nothing, first_line=1, kws...)
filename=nothing, first_line=1)
source = SourceFile(stream, filename=filename, first_line=first_line)
txtbuf = unsafe_textbuf(stream)
args = Any[]
childranges = UnitRange{Int}[]
childheads = SyntaxHead[]
entry = build_tree(_BuildExprStackEntry, stream; kws...) do head, srcrange, nodechildren
entry = build_tree(_BuildExprStackEntry, stream) do head, srcrange, nodechildren
if is_trivia(head) && !is_error(head)
return nothing
end
Expand Down Expand Up @@ -534,4 +534,3 @@ function Base.Expr(node::SyntaxNode)
loc = source_location(LineNumberNode, node.source, first(range(node)))
only(_fixup_Expr_children!(SyntaxHead(K"None",EMPTY_FLAGS), loc, Any[ex]))
end

6 changes: 3 additions & 3 deletions src/green_tree.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,11 +87,11 @@ function Base.show(io::IO, ::MIME"text/plain", node::GreenNode, str::AbstractStr
_show_green_node(io, node, "", 1, str, show_trivia)
end

function build_tree(::Type{GreenNode}, stream::ParseStream; kws...)
build_tree(GreenNode{SyntaxHead}, stream; kws...) do h, srcrange, cs
# we don't need the `filename` and `first_line` kwargs, but some trees do, so we allow them to be passed
function build_tree(::Type{GreenNode}, stream::ParseStream; filename=nothing, first_line=nothing)
Copy link
Member

Choose a reason for hiding this comment

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

It's confusing to know what to do with this part: do we want all build_tree(::Type{TreeType}, ...) methods to need to know about each other for all TreeType? This seems impossible (eg, external packages) which would imply these should just be an error rather than ignored.

But on the other hand making this an error means build_tree() with keywords can't be used generically.

It seems there's two consistent things to do:

This PR takes an awkward middle path between those two options. Which might be ok but seems inconsistent?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I guess if we want a more decoupled approach, we could copy Makie and add a validate_kwargs function which is called on the tree type and the passed-in kwargs (and throws or returns nothing), and is called in parseall or _parse or something like that. Then custom trees which wish to support additional kwargs would need to define a validate_kwargs method to allow them to pass through.

Copy link
Member

@c42f c42f Aug 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

That sounds promising! So we define validate_parse_args(::Type{GreenNode}; kws...) etc and users with custom trees can overload it?

build_tree(GreenNode{SyntaxHead}, stream) do h, srcrange, cs
span = length(srcrange)
isnothing(cs) ? GreenNode(h, span, ()) :
GreenNode(h, span, collect(GreenNode{SyntaxHead}, cs))
end
end

5 changes: 2 additions & 3 deletions src/parse_stream.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -994,7 +994,7 @@ end
# API for extracting results from ParseStream

"""
build_tree(make_node::Function, ::Type{StackEntry}, stream::ParseStream; kws...)
build_tree(make_node::Function, ::Type{StackEntry}, stream::ParseStream)

Construct a tree from a ParseStream using depth-first traversal. `make_node`
must have the signature
Expand All @@ -1010,8 +1010,7 @@ wrap them in a single node.

The tree here is constructed depth-first in postorder.
"""
function build_tree(make_node::Function, ::Type{NodeType}, stream::ParseStream;
kws...) where NodeType
function build_tree(make_node::Function, ::Type{NodeType}, stream::ParseStream) where NodeType
stack = Vector{NamedTuple{(:first_token,:node),Tuple{Int,NodeType}}}()

tokens = stream.tokens
Expand Down
3 changes: 3 additions & 0 deletions test/parser_api.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,9 @@
Expr(:toplevel, LineNumberNode(2), :a, LineNumberNode(3), :b)
@test parseall(Expr, "a\nb\n#==#") ==
Expr(:toplevel, LineNumberNode(1), :a, LineNumberNode(2), :b)

# unknown kwargs error (#416)
@test_throws MethodError parseall(Expr, "a\nb"; ignore_error=true)
end

@testset "IO input" begin
Expand Down
Loading