From d6195054fb087f5deb1614a0642e704e5986ddfe Mon Sep 17 00:00:00 2001 From: Morten Piibeleht Date: Tue, 9 Jul 2019 11:13:07 +1200 Subject: [PATCH 1/6] Handle invalid syntax in doctests --- src/DocTests.jl | 5 ++-- src/Documenter.jl | 5 ++-- src/Utilities/Utilities.jl | 8 ++++-- test/doctests/doctestapi.jl | 57 +++++++++++++++++++++++++++++++++++++ test/doctests/fix/broken.md | 4 +++ test/doctests/fix/fixed.md | 4 +++ 6 files changed, 77 insertions(+), 6 deletions(-) diff --git a/src/DocTests.jl b/src/DocTests.jl index 845e2e4d57..43a7979904 100644 --- a/src/DocTests.jl +++ b/src/DocTests.jl @@ -200,7 +200,7 @@ end function eval_repl(block, sandbox, meta::Dict, doc::Documents.Document, page) for (input, output) in repl_splitter(block.code) result = Result(block, input, output, meta[:CurrentFile]) - for (ex, str) in Utilities.parseblock(input, doc, page; keywords = false) + for (ex, str) in Utilities.parseblock(input, doc, page; keywords = false, raise=false) # Input containing a semi-colon gets suppressed in the final output. result.hide = REPL.ends_with_semicolon(str) (value, success, backtrace, text) = Utilities.withoutput() do @@ -211,6 +211,7 @@ function eval_repl(block, sandbox, meta::Dict, doc::Documents.Document, page) print(result.stdout, text) if !success result.bt = backtrace + break # don't evaluate further if there is an error (e.g. a parse error) end end checkresult(sandbox, result, meta, doc) @@ -227,7 +228,7 @@ function eval_script(block, sandbox, meta::Dict, doc::Documents.Document, page) input = rstrip(input, '\n') output = lstrip(output, '\n') result = Result(block, input, output, meta[:CurrentFile]) - for (ex, str) in Utilities.parseblock(input, doc, page; keywords = false) + for (ex, str) in Utilities.parseblock(input, doc, page; keywords = false, raise=false) (value, success, backtrace, text) = Utilities.withoutput() do Core.eval(sandbox, ex) end diff --git a/src/Documenter.jl b/src/Documenter.jl index b2c7dac0a5..b99bf44034 100644 --- a/src/Documenter.jl +++ b/src/Documenter.jl @@ -906,8 +906,9 @@ function doctest( modules = modules, ) true - catch e - @error "Doctesting failed" e + catch err + @error "Doctesting failed" + showerror(stdout, err, catch_backtrace()) false finally rm(dir; recursive=true) diff --git a/src/Utilities/Utilities.jl b/src/Utilities/Utilities.jl index 93d3e54543..53325afa13 100644 --- a/src/Utilities/Utilities.jl +++ b/src/Utilities/Utilities.jl @@ -84,8 +84,12 @@ Returns a `Vector` of tuples `(expr, code)`, where `expr` is the corresponding e parsed from. The keyword argument `skip = N` drops the leading `N` lines from the input string. + +If `raise=false` is passed, the `Meta.parse` does not raise an exception on parse errors, +but instead returns an expression that will raise an error when evaluated. `parseblock` +returns this expression normally and it must be handled appropriately by the caller. """ -function parseblock(code::AbstractString, doc, file; skip = 0, keywords = true) +function parseblock(code::AbstractString, doc, file; skip = 0, keywords = true, raise=true) # Drop `skip` leading lines from the code block. Needed for deprecated `{docs}` syntax. code = string(code, '\n') code = last(split(code, '\n', limit = skip + 1)) @@ -102,7 +106,7 @@ function parseblock(code::AbstractString, doc, file; skip = 0, keywords = true) (QuoteNode(keyword), cursor + lastindex(line)) else try - Meta.parse(code, cursor) + Meta.parse(code, cursor; raise=raise) catch err push!(doc.internal.errors, :parse_error) @warn "failed to parse exception in $(Utilities.locrepr(file))" exception = err diff --git a/test/doctests/doctestapi.jl b/test/doctests/doctestapi.jl index 6945372ce7..3ccbf65613 100644 --- a/test/doctests/doctestapi.jl +++ b/test/doctests/doctestapi.jl @@ -97,6 +97,49 @@ module DocTest5 end end +""" +```jldoctest +julia> map(tuple, 1/(i+j) for i=1:2, j=1:2, [1:4;]) +ERROR: syntax: invalid iteration specification +``` +```jldoctest +julia> 1.2.3 +ERROR: syntax: invalid numeric constant "1.2." +``` +```jldoctest +println(9.8.7) +# output +ERROR: syntax: invalid numeric constant "9.8." +``` +```jldoctest +julia> Meta.ParseError("foo") +Base.Meta.ParseError("foo") + +julia> Meta.ParseError("foo") |> throw +ERROR: Base.Meta.ParseError("foo") +Stacktrace: +[...] +``` +""" +module ParseErrorSuccess end + +""" +```jldoctest +julia> map(tuple, 1/(i+j) for i=1:2, j=1:2, [1:4;]) +ERROR: syntax: invalid iteration specificationX +``` +""" +module ParseErrorFail end + +""" +```jldoctest +println(9.8.7) +# output +ERROR: syntax: invalid numeric constant "1.2." +``` +""" +module ScriptParseErrorFail end + @testset "Documenter.doctest" begin # DocTest1 run_doctest(nothing, [DocTest1]) do result, success, backtrace, output @@ -148,6 +191,20 @@ end @test success @test result isa Test.DefaultTestSet end + + # Parse errors in doctests (https://github.com/JuliaDocs/Documenter.jl/issues/1046) + run_doctest(nothing, [ParseErrorSuccess]) do result, success, backtrace, output + @test success + @test result isa Test.DefaultTestSet + end + run_doctest(nothing, [ParseErrorFail]) do result, success, backtrace, output + @test !success + @test result isa TestSetException + end + run_doctest(nothing, [ScriptParseErrorFail]) do result, success, backtrace, output + @test !success + @test result isa TestSetException + end end end # module diff --git a/test/doctests/fix/broken.md b/test/doctests/fix/broken.md index c1d46eb243..d4aa36e0bb 100644 --- a/test/doctests/fix/broken.md +++ b/test/doctests/fix/broken.md @@ -77,3 +77,7 @@ julia> 1 + 2 julia> 3 + 4 ``` +```jldoctest +julia> println(9.8.7) +ERROR: syntax: invalid numeric constant "1.2." +``` diff --git a/test/doctests/fix/fixed.md b/test/doctests/fix/fixed.md index d9f8b2d313..d790029f52 100644 --- a/test/doctests/fix/fixed.md +++ b/test/doctests/fix/fixed.md @@ -79,3 +79,7 @@ julia> 1 + 2 julia> 3 + 4 7 ``` +```jldoctest +julia> println(9.8.7) +ERROR: syntax: invalid numeric constant "9.8." +``` From 7118b2b54fd53b0883399c5c3393865bc13a4746 Mon Sep 17 00:00:00 2001 From: Morten Piibeleht Date: Tue, 9 Jul 2019 11:24:58 +1200 Subject: [PATCH 2/6] Add CHANGELOG --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8f3ed53c35..6b99d586b2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,6 +28,8 @@ * ![Bugfix][badge-bugfix] The HTML output now outputs HTML files for pages that are not referenced in the `pages` keyword too (Documenter finds them according to their extension). But they do exists outside of the standard navigation hierarchy (as defined by `pages`). This fixes a bug where these pages could still be referenced by `@ref` links and `@contents` blocks, but in the HTML output, the links ended up being broken. ([#1031][github-1031], [#1047][github-1047]) +* ![Bugfix][badge-bugfix] Doctesting now also handles doctests that contain invalid syntax and throw parsing errors. ([#487][github-487], [#1062][github-1062]) + * ![Experimental][badge-experimental] ![Feature][badge-feature] The current working directory when evaluating `@repl` and `@example` blocks can now be set to a fixed directory by passing the `workdir` keyword to `makedocs`. _The new keyword and its behaviour are experimental and not part of the public API._ ([#1013][github-1013], [#1025][github-1025]) ## Version `v0.22.5` @@ -288,6 +290,7 @@ * ![Bugfix][badge-bugfix] At-docs blocks no longer give an error when containing empty lines. ([#823][github-823], [#824][github-824]) [github-198]: https://github.com/JuliaDocs/Documenter.jl/issues/198 +[github-487]: https://github.com/JuliaDocs/Documenter.jl/issues/487 [github-511]: https://github.com/JuliaDocs/Documenter.jl/pull/511 [github-535]: https://github.com/JuliaDocs/Documenter.jl/issues/535 [github-697]: https://github.com/JuliaDocs/Documenter.jl/pull/697 @@ -365,6 +368,7 @@ [github-1037]: https://github.com/JuliaDocs/Documenter.jl/pull/1037 [github-1047]: https://github.com/JuliaDocs/Documenter.jl/pull/1047 [github-1054]: https://github.com/JuliaDocs/Documenter.jl/pull/1054 +[github-1062]: https://github.com/JuliaDocs/Documenter.jl/pull/1062 [documenterlatex]: https://github.com/JuliaDocs/DocumenterLaTeX.jl [documentermarkdown]: https://github.com/JuliaDocs/DocumenterMarkdown.jl From cfab3d71e1748847e49e8024a48efd43cc42b75d Mon Sep 17 00:00:00 2001 From: Morten Piibeleht Date: Tue, 9 Jul 2019 11:46:01 +1200 Subject: [PATCH 3/6] Pass emtpty backtrace for parse errors In the terminal, parse errors do not have a backtrace. But we still need to initialize the result.bt field. --- src/DocTests.jl | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/src/DocTests.jl b/src/DocTests.jl index 43a7979904..800b4daa08 100644 --- a/src/DocTests.jl +++ b/src/DocTests.jl @@ -209,9 +209,12 @@ function eval_repl(block, sandbox, meta::Dict, doc::Documents.Document, page) Core.eval(sandbox, Expr(:global, Expr(:(=), :ans, QuoteNode(value)))) result.value = value print(result.stdout, text) - if !success + if isa(ex, Expr) && ex.head === :error + @assert !success + result.bt = [] + break # don't evaluate further if there is a parse error + elseif !success result.bt = backtrace - break # don't evaluate further if there is an error (e.g. a parse error) end end checkresult(sandbox, result, meta, doc) @@ -234,7 +237,11 @@ function eval_script(block, sandbox, meta::Dict, doc::Documents.Document, page) end result.value = value print(result.stdout, text) - if !success + if isa(ex, Expr) && ex.head === :error + @assert !success + result.bt = [] + break # don't evaluate further if there is a parse error + elseif !success result.bt = backtrace break end From 6685a9ce365b3c6269e4cdf9978e6f3d28a7e1d1 Mon Sep 17 00:00:00 2001 From: Morten Piibeleht Date: Tue, 9 Jul 2019 14:33:19 +1200 Subject: [PATCH 4/6] Handle backtrace more properly The heuristic in error_to_string which removes the backtrace below the highest Core.eval call works in most cases, but will fail if makedocs/doctest gets called in a more complicated context, as there may sometimes be eval calls higher up in the stacktrace. --- src/DocTests.jl | 38 ++++++++++++++++++++++++++------------ test/doctests/doctests.jl | 14 ++++++++++++++ 2 files changed, 40 insertions(+), 12 deletions(-) diff --git a/src/DocTests.jl b/src/DocTests.jl index 800b4daa08..a4e55c7450 100644 --- a/src/DocTests.jl +++ b/src/DocTests.jl @@ -209,13 +209,11 @@ function eval_repl(block, sandbox, meta::Dict, doc::Documents.Document, page) Core.eval(sandbox, Expr(:global, Expr(:(=), :ans, QuoteNode(value)))) result.value = value print(result.stdout, text) - if isa(ex, Expr) && ex.head === :error - @assert !success - result.bt = [] - break # don't evaluate further if there is a parse error - elseif !success + if !success result.bt = backtrace end + # don't evaluate further if there is a parse error + isa(ex, Expr) && ex.head === :error && break end checkresult(sandbox, result, meta, doc) end @@ -237,11 +235,7 @@ function eval_script(block, sandbox, meta::Dict, doc::Documents.Document, page) end result.value = value print(result.stdout, text) - if isa(ex, Expr) && ex.head === :error - @assert !success - result.bt = [] - break # don't evaluate further if there is a parse error - elseif !success + if !success result.bt = backtrace break end @@ -317,14 +311,34 @@ function result_to_string(buf, value) end function error_to_string(buf, er, bt) - # Remove unimportant backtrace info. + # Remove unimportant backtrace info. TODO: this backtrace handling should maybe be done + # by Utilities.withoutput() already. + bt = remove_common_backtrace(bt, backtrace()) + # Remove everything below the last eval call (which should be the one in withoutput) index = findlast(ptr -> Base.ip_matches_func(ptr, :eval), bt) + bt = (index === nothing) ? bt : bt[1:(index - 1)] # Print a REPL-like error message. print(buf, "ERROR: ") - Base.invokelatest(showerror, buf, er, index === nothing ? bt : bt[1:(index - 1)]) + Base.invokelatest(showerror, buf, er, bt) return sanitise(buf) end +function remove_common_backtrace(bt, reference_bt) + cutoff = nothing + # We'll start from the top of the backtrace (end of the array) and go down, checking + # if the backtraces agree + for ridx in 1:length(bt) + # Cancel search if we run out the reference BT or find a non-matching one frames: + if ridx > length(reference_bt) || bt[length(bt) - ridx + 1] != reference_bt[length(reference_bt) - ridx + 1] + cutoff = length(bt) - ridx + 1 + break + end + end + # It's possible that the loop does not find anything, i.e. that all BT elements are in + # the reference_BT too. In that case we'll just return an empty BT. + bt[1:(cutoff === nothing ? 0 : cutoff)] +end + # Strip trailing whitespace from each line and return resulting string function sanitise(buffer) out = IOBuffer() diff --git a/test/doctests/doctests.jl b/test/doctests/doctests.jl index 2870d30d75..e85758eb08 100644 --- a/test/doctests/doctests.jl +++ b/test/doctests/doctests.jl @@ -213,4 +213,18 @@ rfile(filename) = joinpath(@__DIR__, "stdouts", filename) end end +using Documenter.DocTests: remove_common_backtrace +@testset "DocTest.remove_common_backtrace" begin + @test remove_common_backtrace([], []) == [] + @test remove_common_backtrace([1], []) == [1] + @test remove_common_backtrace([1,2], []) == [1,2] + @test remove_common_backtrace([1,2,3], [1]) == [1,2,3] + @test remove_common_backtrace([1,2,3], [2]) == [1,2,3] + @test remove_common_backtrace([1,2,3], [3]) == [1,2] + @test remove_common_backtrace([1,2,3], [2,3]) == [1] + @test remove_common_backtrace([1,2,3], [1,3]) == [1,2] + @test remove_common_backtrace([1,2,3], [1,2,3]) == [] + @test remove_common_backtrace([1,2,3], [0,1,2,3]) == [] +end + end # module From 0b461aa8ed0d97b0c02fb1ec470106e43ce6f826 Mon Sep 17 00:00:00 2001 From: Morten Piibeleht Date: Tue, 9 Jul 2019 14:40:53 +1200 Subject: [PATCH 5/6] Add more changelog --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6b99d586b2..e869b94198 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,6 +30,8 @@ * ![Bugfix][badge-bugfix] Doctesting now also handles doctests that contain invalid syntax and throw parsing errors. ([#487][github-487], [#1062][github-1062]) +* ![Bugfix][badge-bugfix] Stacktraces in doctests that throw an error are now filtered more thoroughly, fixing an issue where too much of the stacktrace was included when `doctest` or `makedocs` was called from a more complicated context. ([#1062][github-1062]) + * ![Experimental][badge-experimental] ![Feature][badge-feature] The current working directory when evaluating `@repl` and `@example` blocks can now be set to a fixed directory by passing the `workdir` keyword to `makedocs`. _The new keyword and its behaviour are experimental and not part of the public API._ ([#1013][github-1013], [#1025][github-1025]) ## Version `v0.22.5` From ca658c08113cef90658effcff92e2b91ad6d38b9 Mon Sep 17 00:00:00 2001 From: Morten Piibeleht Date: Wed, 10 Jul 2019 10:21:13 +1200 Subject: [PATCH 6/6] Remove doctest testing test for syntax errors On the nightly, syntax errors now get stacktraces, which means that their output is no longer deterministic. Hence we can't really compare against the fixed output. --- test/doctests/fix/broken.md | 4 ---- test/doctests/fix/fixed.md | 4 ---- 2 files changed, 8 deletions(-) diff --git a/test/doctests/fix/broken.md b/test/doctests/fix/broken.md index d4aa36e0bb..c1d46eb243 100644 --- a/test/doctests/fix/broken.md +++ b/test/doctests/fix/broken.md @@ -77,7 +77,3 @@ julia> 1 + 2 julia> 3 + 4 ``` -```jldoctest -julia> println(9.8.7) -ERROR: syntax: invalid numeric constant "1.2." -``` diff --git a/test/doctests/fix/fixed.md b/test/doctests/fix/fixed.md index d790029f52..d9f8b2d313 100644 --- a/test/doctests/fix/fixed.md +++ b/test/doctests/fix/fixed.md @@ -79,7 +79,3 @@ julia> 1 + 2 julia> 3 + 4 7 ``` -```jldoctest -julia> println(9.8.7) -ERROR: syntax: invalid numeric constant "9.8." -```