inference: minor follow-ups to #56299

- cosmetic changes (mostly)
- added some new tests that have been fixed by the PR
- remove the unnecessary `effect_free=ALWAYS_FALSE` taint for the
  fallback effects modeling of `Core.get_binding_type`

Author: aviatesk

base/compiler/abstractinterpretation.jl

@@ -2466,7 +2466,7 @@ function abstract_eval_replaceglobal!(interp::AbstractInterpreter, sv::AbsIntSta
     end
 end
 
-function args_are_actually_getglobal(argtypes)
+function argtypes_are_actually_getglobal(argtypes::Vector{Any})
     length(argtypes) in (3, 4) || return false
     M = argtypes[2]
     s = argtypes[3]
@@ -2506,21 +2506,21 @@ function abstract_call_known(interp::AbstractInterpreter, @nospecialize(f),
             return Future(abstract_eval_setglobalonce!(interp, sv, argtypes))
         elseif f === Core.replaceglobal!
             return Future(abstract_eval_replaceglobal!(interp, sv, argtypes))
-        elseif f === Core.getfield && args_are_actually_getglobal(argtypes)
+        elseif f === Core.getfield && argtypes_are_actually_getglobal(argtypes)
             return Future(abstract_eval_getglobal(interp, sv, argtypes))
-        elseif f === Core.isdefined && args_are_actually_getglobal(argtypes)
+        elseif f === Core.isdefined && argtypes_are_actually_getglobal(argtypes)
             exct = Bottom
             if length(argtypes) == 4
                 order = argtypes[4]
-                exct = global_order_exct(order, true, false)
-                if !(isa(order, Const) && get_atomic_order(order.val, true, false).x >= MEMORY_ORDER_UNORDERED.x)
+                exct = global_order_exct(order, #=loading=#true, #=storing=#false)
+                if !(isa(order, Const) && get_atomic_order(order.val, #=loading=#true, #=storing=#false).x >= MEMORY_ORDER_UNORDERED.x)
                     exct = Union{exct, ConcurrencyViolationError}
                 end
             end
             return Future(merge_exct(CallMeta(abstract_eval_isdefined(
                     interp,
-                    GlobalRef((argtypes[2]::Const).val,
-                              (argtypes[3]::Const).val),
+                    GlobalRef((argtypes[2]::Const).val::Module,
+                              (argtypes[3]::Const).val::Symbol),
                     sv),
                 NoCallInfo()), exct))
         elseif f === Core.get_binding_type
@@ -3048,7 +3048,7 @@ function abstract_eval_copyast(interp::AbstractInterpreter, e::Expr, vtypes::Uni
 end
 
 function abstract_eval_isdefined_expr(interp::AbstractInterpreter, e::Expr, vtypes::Union{VarTable,Nothing},
-                                 sv::AbsIntState)
+                                      sv::AbsIntState)
     sym = e.args[1]
     if isa(sym, SlotNumber) && vtypes !== nothing
         vtyp = vtypes[slot_id(sym)]

base/compiler/tfuncs.jl

@@ -2497,11 +2497,11 @@ function builtin_effects(𝕃::AbstractLattice, @nospecialize(f::Builtin), argty
     elseif f === getglobal
         2 ≤ length(argtypes) ≤ 3 || return EFFECTS_THROWS
         # Modeled more precisely in abstract_eval_getglobal
-        return Effects(EFFECTS_TOTAL; consistent=ALWAYS_FALSE, nothrow=false, inaccessiblememonly=ALWAYS_FALSE)
+        return generic_getglobal_effects
     elseif f === Core.get_binding_type
         length(argtypes) == 2 || return EFFECTS_THROWS
         # Modeled more precisely in abstract_eval_get_binding_type
-        return Effects(EFFECTS_TOTAL; effect_free=ALWAYS_FALSE, nothrow=get_binding_type_nothrow(𝕃, argtypes[1], argtypes[2]))
+        return Effects(EFFECTS_TOTAL; nothrow=get_binding_type_nothrow(𝕃, argtypes[1], argtypes[2]))
     elseif f === compilerbarrier
         length(argtypes) == 2 || return Effects(EFFECTS_THROWS; consistent=ALWAYS_FALSE)
         setting = argtypes[1]

test/compiler/effects.jl

@@ -357,18 +357,21 @@ end
 
 @test !Core.Compiler.builtin_nothrow(Core.Compiler.fallback_lattice, Core.get_binding_type, Any[Rational{Int}, Core.Const(:foo)], Any)
 
-# Nothrow for assignment to globals
+# effects modeling for assignment to globals
 global glob_assign_int::Int = 0
-f_glob_assign_int() = global glob_assign_int += 1
-let effects = Base.infer_effects(f_glob_assign_int, ())
+f_glob_assign_int() = global glob_assign_int = 1
+let effects = Base.infer_effects(f_glob_assign_int, (); optimize=false)
+    @test Core.Compiler.is_consistent(effects)
     @test !Core.Compiler.is_effect_free(effects)
     @test Core.Compiler.is_nothrow(effects)
 end
-# Nothrow for setglobal!
+# effects modeling for for setglobal!
 global SETGLOBAL!_NOTHROW::Int = 0
-let effects = Base.infer_effects() do
+let effects = Base.infer_effects(; optimize=false) do
         setglobal!(@__MODULE__, :SETGLOBAL!_NOTHROW, 42)
     end
+    @test Core.Compiler.is_consistent(effects)
+    @test !Core.Compiler.is_effect_free(effects)
     @test Core.Compiler.is_nothrow(effects)
 end