Skip to content

Conversation

simeonschaub
Copy link
Collaborator

@simeonschaub simeonschaub commented Oct 27, 2021

Depends on JuliaLang/julia#42810 to pass. I did also observe some other
test failures on nightly I couldn't really explain.

@codecov
Copy link

codecov bot commented Oct 27, 2021

Codecov Report

Merging #498 (2231cda) into master (f365bcd) will increase coverage by 0.60%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #498      +/-   ##
==========================================
+ Coverage   86.13%   86.73%   +0.60%     
==========================================
  Files          12       12              
  Lines        2394     2488      +94     
==========================================
+ Hits         2062     2158      +96     
+ Misses        332      330       -2     
Impacted Files Coverage Δ
src/precompile.jl 0.00% <0.00%> (ø)
src/utils.jl 86.07% <0.00%> (+0.04%) ⬆️
src/builtins.jl 76.64% <0.00%> (+0.08%) ⬆️
src/optimize.jl 97.53% <0.00%> (+0.14%) ⬆️
src/localmethtable.jl 96.61% <0.00%> (+0.18%) ⬆️
src/construct.jl 91.26% <0.00%> (+0.18%) ⬆️
src/breakpoints.jl 95.10% <0.00%> (+0.25%) ⬆️
src/commands.jl 93.33% <0.00%> (+0.27%) ⬆️
src/packagedef.jl 91.48% <0.00%> (+0.58%) ⬆️
src/types.jl 74.52% <0.00%> (+0.83%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f365bcd...2231cda. Read the comment docs.

@KristofferC
Copy link
Member

I wonder how the resolver error on 1.0 started happening.

@simeonschaub
Copy link
Collaborator Author

That's my fault. I added DiffUtils as a test dependency to make future debugging easier, but forgot that it requires Julia >= 1.3

@aviatesk aviatesk closed this Dec 5, 2021
@aviatesk aviatesk reopened this Dec 5, 2021
@aviatesk aviatesk requested a review from timholy December 5, 2021 17:11
test/limits.jl Outdated
@test Aborted(frame, 1).at.file == Symbol("util.jl")
elseif isdefined(Base, :Experimental) &&
isdefined(Base.Experimental, Symbol("@force_compile"))
@test Aborted(frame, 1).at.file == Symbol("timing.jl")
Copy link
Member

Choose a reason for hiding this comment

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

Ooh, that's unfortunate. I don't immediately understand why it's different from the old heuristic, but I haven't poked at it. Can you file a Julia issue? I regard line-number issues as bugs.

Copy link
Member

Choose a reason for hiding this comment

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

Is this really bug? The line numbers appearing in lowered code seems just identical:

v1.7

julia> ex = Main, Base.parse_input_line("""
           x = 1
           for i = 1:10
               x += 1
           end
           let y = 0
               z = 5
           end
           if 2 > 1
               println("Hello, world!")
           end
           @elapsed sum(rand(5))
           """; filename="fake.jl"); Meta.lower(Main, ex)
:($(Expr(:toplevel, :(#= fake.jl:1 =#), :(x = 1), :(#= fake.jl:2 =#), :(for i = 1:10
      #= fake.jl:3 =#
      x += 1
  end), :(#= fake.jl:5 =#), :(let y = 0
      #= fake.jl:6 =#
      z = 5
  end), :(#= fake.jl:8 =#), :(if 2 > 1
      #= fake.jl:9 =#
      println("Hello, world!")
  end), :(#= fake.jl:11 =#), :($(Expr(Symbol("hygienic-scope"), quote
    #= timing.jl:297 =#
    while false
        #= timing.jl:297 =#
    end
    #= timing.jl:298 =#
    local t0 = time_ns()
    #= timing.jl:299 =#
    $(Expr(:escape, :(sum(rand(5)))))
    #= timing.jl:300 =#
    (time_ns() - t0) / 1.0e9
end, Base))))))

nightly

julia> ex = Base.parse_input_line("""
           x = 1
           for i = 1:10
               x += 1
           end
           let y = 0
               z = 5
           end
           if 2 > 1
               println("Hello, world!")
           end
           @elapsed sum(rand(5))
           """; filename="fake.jl"); Meta.lower(Main, ex)
:($(Expr(:toplevel, :(#= fake.jl:1 =#), :(x = 1), :(#= fake.jl:2 =#), :(for i = 1:10
      #= fake.jl:3 =#
      x += 1
      #= fake.jl:4 =#
  end), :(#= fake.jl:5 =#), :(let y = 0
      #= fake.jl:6 =#
      z = 5
  end), :(#= fake.jl:8 =#), :(if 2 > 1
      #= fake.jl:9 =#
      println("Hello, world!")
  end), :(#= fake.jl:11 =#), :($(Expr(Symbol("hygienic-scope"), quote
    #= timing.jl:356 =#
    $(Expr(Symbol("hygienic-scope"), :($(Expr(:meta, :force_compile))), Base.Experimental))
    #= timing.jl:357 =#
    local t0 = time_ns()
    #= timing.jl:358 =#
    $(Expr(:escape, :(sum(rand(5)))))
    #= timing.jl:359 =#
    (time_ns() - t0) / 1.0e9
end, Base))))))

The actual difference seems to be that v1.7 produces NewvarNode while nightly doesn't:

v1.7

julia> modexs = collect(ExprSplitter(Main, ex)); frame = Frame(modexs[5]...); frame.framecode.src
CodeInfo(
    @ fake.jl:11 within `top-level scope`
1 ─     Core.NewvarNode(:(#310#t0))
   ┌ @ timing.jl:297 within `macro expansion`
2 ┄│     goto #4 if not false
3 ─│     goto #2
   └
   ┌ @ timing.jl:298 within `macro expansion`
4 ─│     _J1 = ($(QuoteNode(time_ns)))()
│  └
│  ┌ @ timing.jl:299 within `macro expansion`
│  │     rand(5)
│  │     sum(%J5)
│  └
│  ┌ @ timing.jl:300 within `macro expansion`
│  │     ($(QuoteNode(time_ns)))()
│  │     ($(QuoteNode(-)))(%J7, _J1)
│  │     ($(QuoteNode(/)))(%J8, 1.0e9)
│  └
└──     return %J9
)

nightly

julia> modexs = collect(ExprSplitter(Main, ex)); frame = Frame(modexs[5]...); frame.framecode.src
CodeInfo(
    @ fake.jl:11 within `top-level scope`
   ┌ @ timing.jl:356 within `macro expansion`
1 ─│     $(Expr(:meta, :force_compile))
│  │ @ timing.jl:357 within `macro expansion`
│  │     _J1 = ($(QuoteNode(time_ns)))()
│  │ @ timing.jl:358 within `macro expansion`
│  │     rand(5)
│  │     sum(%J3)
│  │ @ timing.jl:359 within `macro expansion`
│  │     ($(QuoteNode(time_ns)))()
│  │     ($(QuoteNode(-)))(%J5, _J1)
│  │     ($(QuoteNode(/)))(%J6, 1.0e9)
│  └
└──     return %J7
)

So I'd say the first line number of that chunk was fake.jl sort of accidentally.

Copy link
Member

Choose a reason for hiding this comment

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

xref #500

isdefined(Base.Experimental, Symbol("@force_compile"))
Base.Experimental.@force_compile
end
a = (VecElement{Float64}(1.0), VecElement{Float64}(2.0))
Copy link
Member

Choose a reason for hiding this comment

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

You sure you need this? It didn't fail for me on nightly.

Copy link
Member

@aviatesk aviatesk Dec 5, 2021

Choose a reason for hiding this comment

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

Yeah, I couldn't reproduce the error within REPL, but I also confirmed Pkg.test will hit this line consistently without this change (maybe because the compilation heuristic used during Pkg.test is different from that within REPL?).
I think the root problem here is that @interpret doesn't configure enter_generated when calling enter_call_expr.

Copy link
Member

Choose a reason for hiding this comment

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

I'm OK with this if it's necessary.

@timholy
Copy link
Member

timholy commented Dec 6, 2021

I think one of the failures should be fixed by #500 instead, but otherwise I'm good with this.

@timholy
Copy link
Member

timholy commented Dec 6, 2021

Sorry for the rebase. Once that's done let's merge this.

@aviatesk aviatesk merged commit 716d7f6 into master Dec 6, 2021
@aviatesk aviatesk deleted the sds/fix_cov_test branch December 6, 2021 14:19
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.

4 participants