Code quality improvements driven by JETLS diagnostics

src/codeedges.jl

@@ -326,7 +326,6 @@ function direct_links!(cl::CodeLinks, src::CodeInfo)
             target = P(SSAValue(i), cl.ssapreds[i])
         elseif (lhs_rhs = get_lhs_rhs(stmt); lhs_rhs !== nothing)
             # An assignment
-            stmt = stmt::Expr
             lhs, rhs = lhs_rhs
             if @issslotnum(lhs)
                 lhs = lhs::AnySlotNumber
@@ -481,7 +480,7 @@ struct CodeEdges
     succs::Vector{Vector{Int}}
     byname::Dict{GlobalRef,Variable}
 end
-CodeEdges(n::Integer) = CodeEdges([Int[] for i = 1:n], [Int[] for i = 1:n], Dict{GlobalRef,Variable}())
+CodeEdges(n::Integer) = CodeEdges([Int[] for _ = 1:n], [Int[] for _ = 1:n], Dict{GlobalRef,Variable}())
 
 function Base.show(io::IO, edges::CodeEdges)
     println(io, "CodeEdges:")
@@ -526,7 +525,6 @@ function CodeEdges(src::CodeInfo, cl::CodeLinks)
     for (i, stmt) in enumerate(src.code)
         # Identify line predecessors for slots and named variables
         if (lhs_rhs = get_lhs_rhs(stmt); lhs_rhs !== nothing)
-            stmt = stmt::Expr
             lhs, _ = lhs_rhs
             # Mark predecessors and successors of this line by following ssas & named assignments
             if @issslotnum(lhs)
@@ -705,7 +703,7 @@ On return, the complete set of required statements will be marked `true`.
 
 `norequire` keyword argument specifies statements (represented as iterator of `Int`s) that
 should _not_ be marked as a requirement.
-For example, use `norequire = LoweredCodeUtils.exclude_named_typedefs(src, edges)` if you're
+For example, use `norequire = LoweredCodeUtils.exclude_named_typedefs(src)` if you're
 extracting method signatures and not evaluating new definitions.
 """
 function lines_required!(isrequired::AbstractVector{Bool}, src::CodeInfo, edges::CodeEdges,
@@ -715,7 +713,7 @@ function lines_required!(isrequired::AbstractVector{Bool}, src::CodeInfo, edges:
     return lines_required!(isrequired, objs, src, edges, controller; kwargs...)
 end
 
-function exclude_named_typedefs(src::CodeInfo, edges::CodeEdges)
+function exclude_named_typedefs(src::CodeInfo)
     norequire = BitSet()
     i = 1
     nstmts = length(src.code)
@@ -732,9 +730,11 @@ function exclude_named_typedefs(src::CodeInfo, edges::CodeEdges)
     return norequire
 end
 
-function lines_required!(isrequired::AbstractVector{Bool}, objs, src::CodeInfo, edges::CodeEdges,
-                         controller::SelectiveEvalController=SelectiveEvalController();
-                         norequire = ())
+function lines_required!(
+        isrequired::AbstractVector{Bool}, objs::Set{GlobalRef}, src::CodeInfo, edges::CodeEdges,
+        controller::SelectiveEvalController=SelectiveEvalController();
+        norequire = ()
+    )
     # A controller describes one particular slice. Recompute it from scratch so
     # callers can safely reuse the same object for another slice.
     empty!(controller.termination_points)
@@ -782,15 +782,20 @@ function lines_required!(isrequired::AbstractVector{Bool}, objs, src::CodeInfo,
     return isrequired
 end
 
-function add_requests!(isrequired, objs, edges::CodeEdges, norequire)
+function add_requests!(
+        isrequired::AbstractVector{Bool}, objs::Set{GlobalRef}, edges::CodeEdges,
+        norequire
+    )
     objsnew = Set{GlobalRef}()
     for obj in objs
         add_obj!(isrequired, objsnew, obj, edges, norequire)
     end
     return objsnew
 end
 
-function add_ssa_preds!(isrequired, src::CodeInfo, edges::CodeEdges, norequire)
+function add_ssa_preds!(
+        isrequired::AbstractVector{Bool}, src::CodeInfo, edges::CodeEdges, norequire
+    )
     changed = false
     for idx = 1:length(src.code)
         if isrequired[idx]
@@ -800,18 +805,22 @@ function add_ssa_preds!(isrequired, src::CodeInfo, edges::CodeEdges, norequire)
     return changed
 end
 
-function add_named_dependencies!(isrequired, edges::CodeEdges, objs, norequire)
+function add_named_dependencies!(
+        isrequired::AbstractVector{Bool}, edges::CodeEdges, objs::Set{GlobalRef}, norequire
+    )
     changed = false
     for (obj, uses) in edges.byname
         obj ∈ objs && continue
-        if any(view(isrequired, uses.succs))
+        if any(view(isrequired, uses.succs))::Bool
             changed |= add_obj!(isrequired, objs, obj, edges, norequire)
         end
     end
     return changed
 end
 
-function add_preds!(isrequired, idx, edges::CodeEdges, norequire)
+function add_preds!(
+        isrequired::AbstractVector{Bool}, idx::Int, edges::CodeEdges, norequire
+    )
     chngd = false
     preds = edges.preds[idx]
     for p in preds
@@ -823,18 +832,10 @@ function add_preds!(isrequired, idx, edges::CodeEdges, norequire)
     end
     return chngd
 end
-function add_succs!(isrequired, idx, edges::CodeEdges, succs, norequire)
-    chngd = false
-    for p in succs
-        isrequired[p] && continue
-        p ∈ norequire && continue
-        isrequired[p] = true
-        chngd = true
-        add_succs!(isrequired, p, edges, edges.succs[p], norequire)
-    end
-    return chngd
-end
-function add_obj!(isrequired, objs, obj::GlobalRef, edges::CodeEdges, norequire)
+function add_obj!(
+        isrequired::AbstractVector{Bool}, objs::Set{GlobalRef}, obj::GlobalRef,
+        edges::CodeEdges, norequire
+    )
     chngd = false
     for p in edges.byname[obj].preds
         p ∈ norequire && continue
@@ -1051,7 +1052,11 @@ end
 
 # New struct definitions, including their constructors, get spread out over many
 # statements. If we're evaluating any of them, it's important to evaluate *all* of them.
-function add_typedefs!(isrequired, src::CodeInfo, edges::CodeEdges, (typedef_blocks, typedef_names), norequire)
+function add_typedefs!(
+        isrequired, src::CodeInfo, edges::CodeEdges,
+        typedefs::Tuple{Vector{UnitRange{Int}},Vector{Symbol}},
+        norequire
+    )
     changed = false
     stmts = src.code
     defaultctors = Tuple{Int,BitSet}[]
@@ -1064,7 +1069,7 @@ function add_typedefs!(isrequired, src::CodeInfo, edges::CodeEdges, (typedef_blo
         stmt = stmts[idx]
         isrequired[idx] || (idx += 1; continue)
         intypedef = false
-        for (typedefr, typedefn) in zip(typedef_blocks, typedef_names)
+        for (typedefr, typedefn) in zip(typedefs...)
             if idx ∈ typedefr
                 ireq = view(isrequired, typedefr)
                 if !all(ireq)
@@ -1296,7 +1301,7 @@ function print_with_code(io::IO, src::CodeInfo, isrequired::AbstractVector{Bool}
     preprint(::IO) = nothing
     preprint(io::IO, idx::Int) = (c = isrequired[idx]; printstyled(io, lpad(idx, nd), ' ', c ? "t " : "f "; color = c ? :cyan : :plain))
     postprint(::IO) = nothing
-    postprint(::IO, idx::Int, bbchanged::Bool) = nothing
+    postprint(::IO, _idx::Int, _bbchanged::Bool) = nothing
 
     print_with_code(preprint, postprint, io, src)
 end

src/packagedef.jl

@@ -50,10 +50,10 @@ if ccall(:jl_generating_output, Cint, ()) == 1
     edges = CodeEdges(@__MODULE__, src)
     isrequired = lines_required(GlobalRef(@__MODULE__, :s), src, edges)
     lines_required(GlobalRef(@__MODULE__, :s), src, edges; norequire=())
-    lines_required(GlobalRef(@__MODULE__, :s), src, edges; norequire=exclude_named_typedefs(src, edges))
+    lines_required(GlobalRef(@__MODULE__, :s), src, edges; norequire=exclude_named_typedefs(src))
     for isreq in (isrequired, convert(Vector{Bool}, isrequired))
         lines_required!(isreq, src, edges; norequire=())
-        lines_required!(isreq, src, edges; norequire=exclude_named_typedefs(src, edges))
+        lines_required!(isreq, src, edges; norequire=exclude_named_typedefs(src))
     end
     frame = Frame(@__MODULE__, src)
     # selective_eval_fromstart!(frame, isrequired, true)

src/signatures.jl

@@ -331,7 +331,7 @@ function set_to_running_name!(interp::Interpreter, replacements::Dict{GlobalRef,
         throw(err)
     end
     replacements[callee] = cname
-    mi = methodinfos[cname] = methodinfos[callee]
+    methodinfos[cname] = methodinfos[callee]
     src = frame.framecode.src
     replacename!(src, callee=>cname) # the method itself
     return replacements
@@ -363,7 +363,7 @@ function _rename_framemethods!(interp::Interpreter, frame::Frame,
         end
     end
     for sc in selfcalls
-        linetop, linebody, callee, caller = sc.linetop, sc.linebody, sc.callee, sc.caller
+        linetop, callee = sc.linetop, sc.callee
         cname = get(replacements, callee, nothing)
         if cname !== nothing && cname !== callee
             replacename!(method_body(src.code[linetop])::CodeInfo, callee=>cname)
@@ -452,7 +452,7 @@ function replacename!(args::AbstractVector, pr)
             replacename!(a.val::Expr, pr)
         elseif a === oldname
             args[i] = newname
-        elseif a == oldname.name
+        elseif a === oldname.name
             args[i] = newname.name
         end
     end
@@ -570,7 +570,7 @@ By default the method will be defined (evaluated). You can prevent this by setti
 This is recommended if you are simply extracting signatures from code that has already been evaluated.
 """
 function methoddef!(interp::Interpreter, signatures::Vector{MethodInfoKey}, frame::Frame, @nospecialize(stmt), pc::Int; define::Bool=true)
-    framecode, pcin = frame.framecode, pc
+    framecode = frame.framecode
     if ismethod3(stmt)
         pc3 = pc
         arg1 = method_name(stmt)
@@ -601,7 +601,7 @@ function methoddef!(interp::Interpreter, signatures::Vector{MethodInfoKey}, fram
                 codeloc = codelocation(code, pc)
                 loc = linetable(code, codeloc)
                 ft = Base.unwrap_unionall((Base.unwrap_unionall(sigt)::DataType).parameters[1])
-                if !startswith(String((ft.name::Core.TypeName).name), "##")
+                if !startswith(String((ft.name::Core.TypeName).name), "##") && loc !== nothing
                     @warn "file $(loc.file), line $(loc.line): no method found for $sigt"
                 end
                 if pc == pc3
@@ -678,7 +678,7 @@ function methoddef!(interp::Interpreter, signatures::Vector{MethodInfoKey}, fram
     pc = frame.pc
     stmt = pc_expr(frame, pc)
     if !ismethod(stmt)
-        pc = next_until!(ismethod, interp, frame, true)
+        pc = next_until!(is_frame_at_method, interp, frame, true)
     end
     pc === nothing && error("pc at end of frame without finding a method")
     methoddef!(interp, signatures, frame, pc; define)
@@ -717,7 +717,7 @@ function _methoddefs!(interp::Interpreter, signatures::Vector{MethodInfoKey}, fr
     while pc !== nothing
         stmt = pc_expr(frame, pc)
         if !ismethod(stmt)
-            pc = next_until!(ismethod, interp, frame, true)
+            pc = next_until!(is_frame_at_method, interp, frame, true)
         end
         pc === nothing && break
         ret = methoddef!(interp, signatures, frame, pc; define)
@@ -728,11 +728,11 @@ end
 
 function is_self_call(@nospecialize(stmt), slotnames, argno::Integer=1)
     if isa(stmt, Expr)
-        if stmt.head == :call
+        if stmt.head === :call
             a = stmt.args[argno]
             if isa(a, SlotNumber) || isa(a, Core.SlotNumber)
                 sn = slotnames[a.id]
-                if sn == Symbol("#self#") || sn == Symbol("") # allow empty to fix https://github.com/timholy/CodeTracking.jl/pull/48
+                if sn === Symbol("#self#") || sn === Symbol("") # allow empty to fix https://github.com/timholy/CodeTracking.jl/pull/48
                     return true
                 end
             end

src/utils.jl

@@ -78,8 +78,8 @@ function getrhs(@nospecialize(stmt))
     return lhs_rhs === nothing ? stmt : lhs_rhs[2]
 end
 
-ismethod(frame::Frame)  = ismethod(pc_expr(frame))
-ismethod3(frame::Frame) = ismethod3(pc_expr(frame))
+is_frame_at_method(frame::Frame)  = ismethod(pc_expr(frame))
+is_frame_at_method3(frame::Frame) = ismethod3(pc_expr(frame))
 
 # Check if a call argument refers to Core.define_method
 function is_define_method_ref(@nospecialize(f))
@@ -104,9 +104,9 @@ function is_define_method_call_4arg(@nospecialize(stmt))
     return is_define_method_ref(stmt.args[1])
 end
 
-ismethod(stmt)  = isexpr(stmt, :method) || is_define_method_call_2arg(stmt) || is_define_method_call_4arg(stmt)
-ismethod1(stmt) = isexpr(stmt, :method, 1) || is_define_method_call_2arg(stmt)
-ismethod3(stmt) = isexpr(stmt, :method, 3) || is_define_method_call_4arg(stmt)
+ismethod(@nospecialize stmt)  = isexpr(stmt, :method) || is_define_method_call_2arg(stmt) || is_define_method_call_4arg(stmt)
+ismethod1(@nospecialize stmt) = isexpr(stmt, :method, 1) || is_define_method_call_2arg(stmt)
+ismethod3(@nospecialize stmt) = isexpr(stmt, :method, 3) || is_define_method_call_4arg(stmt)
 
 # Extract the "name" argument from a method-definition statement.
 # For Expr(:method, name, ...) it's args[1]; for define_method(mod, name, ...) it's args[3].
@@ -136,7 +136,7 @@ function method_body(@nospecialize(stmt))
     end
 end
 
-function ismethod_with_name(src, stmt, target::AbstractString; reentrant::Bool=false)
+function ismethod_with_name(src::CodeInfo, @nospecialize(stmt), target::AbstractString; reentrant::Bool=false)
     if reentrant
         name = stmt
     else

test/codeedges.jl

@@ -297,9 +297,8 @@ module ModSelective end
         isrequired = fill(false, length(src.code))
         targetidx = findlast(stmt -> Meta.isexpr(stmt, :call), src.code) # the second push!
         isrequired[targetidx] = true
-        lines_required!(isrequired, (GlobalRef(mod, :branch_value),), src, edges, controller)
-        selective_eval_fromstart!(
-            LoweredCodeUtils.RecursiveInterpreter(), frame, isrequired, controller, true)
+        lines_required!(isrequired, Set((GlobalRef(mod, :branch_value),)), src, edges, controller)
+        selective_eval_fromstart!(LoweredCodeUtils.RecursiveInterpreter(), frame, isrequired, controller, true)
         @test @invokelatest(mod.branch_value) == 1
         @test @invokelatest(mod.hits) == [2]
     end
@@ -507,7 +506,7 @@ module ModSelective end
     frame = Frame(ModEval, ex)
     src = frame.framecode.src
     edges = CodeEdges(ModEval, src)
-    isrequired = minimal_evaluation(@nospecialize(stmt)->(LoweredCodeUtils.ismethod3(stmt),false), src, edges; norequire=exclude_named_typedefs(src, edges))  # initially mark only the constructor
+    isrequired = minimal_evaluation(@nospecialize(stmt)->(LoweredCodeUtils.ismethod3(stmt),false), src, edges; norequire=exclude_named_typedefs(src))  # initially mark only the constructor
     bbs = CC.compute_basic_blocks(src.code)
     for (iblock, block) in enumerate(bbs.blocks)
         r = LoweredCodeUtils.rng(block)
@@ -545,15 +544,14 @@ module ModSelective end
     src = thk.args[1]
     edges = CodeEdges(Main, src)
     idx = findfirst(LoweredCodeUtils.ismethod, src.code)
-    lr = lines_required(idx, src, edges; norequire=exclude_named_typedefs(src, edges))
+    lr = lines_required(idx, src, edges; norequire=exclude_named_typedefs(src))
     idx = findfirst(@nospecialize(stmt)->Meta.isexpr(stmt, :(=)) && Meta.isexpr(stmt.args[2], :call) && is_global_ref(stmt.args[2].args[1], Core, :Box), src.code)
     @test lr[idx]
     # but make sure we don't break primitivetype & abstracttype (https://github.com/timholy/Revise.jl/pull/611)
     thk = Meta.lower(Main, quote
         primitive type WindowsRawSocket sizeof(Ptr) * 8 end
     end)
     src = thk.args[1]
-    edges = CodeEdges(Main, src)
     idx = findfirst(istypedef, src.code)
     r = LoweredCodeUtils.typedef_range(src, idx)
     # 1 before :latestworld, 2 after
@@ -592,14 +590,12 @@ module ModSelective end
         # CodeEdges
         edges = CodeEdges(Main, src)
         show(io, edges)
-        str = String(take!(io))
         LoweredCodeUtils.print_with_code(io, src, edges)
-        str = String(take!(io))
         # Works with Frames too
         frame = Frame(ModSelective, ex)
         edges = CodeEdges(ModSelective, frame.framecode.src)
         LoweredCodeUtils.print_with_code(io, frame, edges)
-        str = String(take!(io))
+        _ = String(take!(io))
 
         # display slot names
         ex = :(let