Skip to content

Commit

Permalink
Fix diagnostics covering trailing text
Browse files Browse the repository at this point in the history
Diagnostics may sometimes cover trailing text which wasn't consumed. For example,
`parseatom(")")` doesn't consume the errant trailing ')', but the
diagnostic refers to this character. In this case, `SourceFile` needs to
cover the location of the diagnostic in addition to any text which is
consumed.

As part of this, mark `sourcetext(::ParseStream)` as deprecated, in
favour of constructing a `SourceFile` to wrap things up more neatly.

Also fix a bug in `highlight()` for empty `SourceFile`s.
  • Loading branch information
c42f committed Jun 25, 2023
1 parent 8731bab commit 6e4e7e2
Show file tree
Hide file tree
Showing 5 changed files with 35 additions and 17 deletions.
27 changes: 15 additions & 12 deletions src/parse_stream.jl
Original file line number Diff line number Diff line change
Expand Up @@ -331,7 +331,7 @@ function Base.show(io::IO, mime::MIME"text/plain", stream::ParseStream)
end

function show_diagnostics(io::IO, stream::ParseStream)
show_diagnostics(io, stream.diagnostics, sourcetext(stream))
show_diagnostics(io, stream.diagnostics, SourceFile(stream))
end

# We manage a pool of stream positions as parser working space
Expand Down Expand Up @@ -1078,17 +1078,8 @@ function build_tree(make_node::Function, ::Type{NodeType}, stream::ParseStream;
end
end

"""
sourcetext(stream::ParseStream; steal_textbuf=true)
Return the source text being parsed by this `ParseStream` as a UTF-8 encoded
string.
If `steal_textbuf==true`, this is permitted to steal the content of the
stream's text buffer. Note that this leaves the `ParseStream` in an invalid
state for further parsing.
"""
function sourcetext(stream::ParseStream; steal_textbuf=false)
Base.depwarn("Use of `sourcetext(::ParseStream)` is deprecated. Use `SourceFile(stream)` instead", :sourcetext)
root = stream.text_root
# The following kinda works but makes the return type of this method type
# unstable. (Also codeunit(root) == UInt8 doesn't imply UTF-8 encoding?)
Expand All @@ -1109,7 +1100,19 @@ function sourcetext(stream::ParseStream; steal_textbuf=false)
end

function SourceFile(stream::ParseStream; kws...)
return SourceFile(sourcetext(stream); first_index=first_byte(stream), kws...)
fbyte = first_byte(stream)
lbyte = maximum((last_byte(d) for d in stream.diagnostics);
init=last_byte(stream))
# See also sourcetext()
srcroot = stream.text_root
str = if srcroot isa String
SubString(srcroot, fbyte, thisind(srcroot, lbyte))
elseif srcroot isa SubString{String}
SubString(srcroot, fbyte, thisind(srcroot, lbyte))
else
SubString(String(stream.textbuf[fbyte:lbyte]))
end
return SourceFile(str; first_index=first_byte(stream), kws...)
end

"""
Expand Down
3 changes: 2 additions & 1 deletion src/parser.jl
Original file line number Diff line number Diff line change
Expand Up @@ -3561,7 +3561,8 @@ function parse_atom(ps::ParseState, check_identifiers=true)
msg = leading_kind == K"EndMarker" ?
"premature end of input" :
"unexpected `$(untokenize(leading_kind))`"
bump_invisible(ps, K"error", error=msg)
emit_diagnostic(ps, error=msg)
bump_invisible(ps, K"error")
else
bump(ps, error="invalid syntax atom")
end
Expand Down
4 changes: 3 additions & 1 deletion src/source_files.jl
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,9 @@ function highlight(io::IO, source::SourceFile, range::UnitRange;
print(io, source[x:p-1])
_printstyled(io, hitext; bgcolor=color)
print(io, source[q+1:d])
source[thisind(source, d)] == '\n' || print(io, "\n")
if d >= firstindex(source) && source[thisind(source, d)] != '\n'
print(io, "\n")
end
_print_marker_line(io, source[a:p-1], hitext, true, true, marker_line_color, note, notecolor)
else
# x --------------
Expand Down
15 changes: 12 additions & 3 deletions test/diagnostics.jl
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
function diagnostic(str; only_first=false, allow_multiple=false)
function diagnostic(str; only_first=false, allow_multiple=false, rule=:all)
stream = ParseStream(str)
parse!(stream)
parse!(stream, rule=rule)
if allow_multiple
stream.diagnostics
else
Expand Down Expand Up @@ -52,7 +52,11 @@ end
Diagnostic(6, 5, :error, "invalid macro name")

@test diagnostic("a, , b") ==
Diagnostic(4, 3, :error, "unexpected `,`")
Diagnostic(4, 4, :error, "unexpected `,`")
@test diagnostic(")", allow_multiple=true) == [
Diagnostic(1, 1, :error, "unexpected `)`")
Diagnostic(1, 1, :error, "extra tokens after end of expression")
]

@test diagnostic("if\nfalse\nend") ==
Diagnostic(3, 3, :error, "missing condition in `if`")
Expand Down Expand Up @@ -99,6 +103,11 @@ end

@test diagnostic("\"\$(x,y)\"") ==
Diagnostic(3, 7, :error, "invalid interpolation syntax")

@test diagnostic("", rule=:statement) ==
Diagnostic(1, 0, :error, "premature end of input")
@test diagnostic("", rule=:atom) ==
Diagnostic(1, 0, :error, "premature end of input")
end

@testset "parser warnings" begin
Expand Down
3 changes: 3 additions & 0 deletions test/source_files.jl
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,9 @@ end
@test sprint(highlight, SourceFile("a\nα"), 1:4) == "\na\nα\n"
@test sprint(highlight, SourceFile("a\nb\nα"), 3:3) == "a\nb\n\nα"

# empty files
@test sprint(highlight, SourceFile(""), 1:0) == ""

# Multi-line ranges
@test sprint(highlight, src, 1:7) == """
┌───
Expand Down

0 comments on commit 6e4e7e2

Please sign in to comment.