Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AssertionError(msg="Union{} typed field should be strictly undefined") #57673

Closed
lgoettgens opened this issue Mar 7, 2025 · 20 comments · Fixed by #57720 · May be fixed by #57764
Closed

AssertionError(msg="Union{} typed field should be strictly undefined") #57673

lgoettgens opened this issue Mar 7, 2025 · 20 comments · Fixed by #57720 · May be fixed by #57764
Assignees
Labels
bug Indicates an unexpected problem or unintended behavior compiler:inference Type inference
Milestone

Comments

@lgoettgens
Copy link
Contributor

lgoettgens commented Mar 7, 2025

The Oscar.jl tests seem to be running into this assertion now after #57541 tried to fix oscar-system/Oscar.jl#4648:

 From worker 4:	Internal error: during type inference of
      From worker 4:	(::Type{Oscar.IntersectionTheory.AbstractVarietyMap{V1, V2} where V2<:Oscar.IntersectionTheory.AbstractVarietyT where V1<:Oscar.IntersectionTheory.AbstractVarietyT})(Oscar.IntersectionTheory.AbstractVariety, Oscar.IntersectionTheory.AbstractVariety, Array{Oscar.MPolyQuoRingElem{Oscar.MPolyDecRingElem{Nemo.QQFieldElem, Nemo.QQMPolyRingElem}}, 1}, AbstractAlgebra.Generic.FunctionalMap{Oscar.MPolyQuoRing{Oscar.MPolyDecRingElem{Nemo.QQFieldElem, Nemo.QQMPolyRingElem}}, Oscar.MPolyDecRing{Nemo.QQFieldElem, Nemo.QQMPolyRing}})
      From worker 4:	Encountered unexpected error in runtime:
      From worker 4:	AssertionError(msg="`Union{}` typed field should be strictly undefined")
      From worker 4:	PartialStruct at ./coreir.jl:62

https://github.com/oscar-system/Oscar.jl/actions/runs/13611555768/job/38059503585#step:11:677

Originally posted by @benlorenz in #57541 (comment)

cc @benlorenz @fingolfin

ping @aviatesk @serenity4 @topolarity as you guys worked on #57541

@nsajko nsajko added bug Indicates an unexpected problem or unintended behavior compiler:inference Type inference labels Mar 7, 2025
@serenity4
Copy link
Contributor

The following case seems to still be unsupported:

julia> struct A{T}; x::Any; y::T; A{T}(x) where {T} = new{T}(x); end

julia> Core.PartialStruct(A{Union{}}, Union{Nothing,Bool}[false, nothing], Any[Int, Union{}])
ERROR: AssertionError: `Union{}` typed field should be strictly undefined
Stacktrace:
 [1] Core.PartialStruct(typ::Type, undefs::Vector{Union{Nothing, Bool}}, fields::Vector{Any})
   @ Base ./coreir.jl:62

Perhaps we should adjust the check to also allow undef === nothing, instead of requiring undef to be true for Union{} fields.

While a field with type Union{} implies undef === true, it seems a bit too much to me to require all callsites to check for Union{} in every field type to determine whether the corresponding undef entry should be set to true before constructing a PartialStruct.

@aviatesk
Copy link
Member

I'm open to loosening the assertion if we need to, but I'd really like to understand why it's failing in the first place. When I ran the Oscar tests locally, I hit an inference stack overflow error and couldn't even get to the point where the assertion fails. Do we have a reliable way to reproduce this?

@serenity4
Copy link
Contributor

I can try to reproduce and see if we can get to an understanding about what's going on

@serenity4
Copy link
Contributor

serenity4 commented Mar 10, 2025

Here it goes:

julia> using Oscar, Oscar.IntersectionTheory

julia> code_typed(degeneracy_locus, (Oscar.IntersectionTheory.AbstractBundle{Oscar.IntersectionTheory.AbstractVariety}, Oscar.IntersectionTheory.AbstractBundle{Oscar.IntersectionTheory.AbstractVariety}, Int64))

ERROR: AssertionError: `Union{}` typed field should be strictly undefined
Stacktrace:
  [1] Core.PartialStruct(typ::Type, undefs::Vector{Union{Nothing, Bool}}, fields::Vector{Any})
    @ Base ./coreir.jl:62
  [2] Core.PartialStruct(::Compiler.InferenceLattice{Compiler.ConditionalsLattice{…}}, typ::Any, undefs::Vector{Union{…}}, fields::Vector{Any})
    @ Compiler ./../usr/share/julia/Compiler/src/typelattice.jl:765
  [3] form_partially_defined_struct(𝕃ᵢ::Compiler.InferenceLattice{Compiler.ConditionalsLattice{Compiler.PartialsLattice{Compiler.ConstsLattice}}}, obj::Any, name::Any)
    @ Compiler ~/.julia/juliaup/julia-nightly/share/julia/Compiler/src/abstractinterpretation.jl:2162
  [4] abstract_call_builtin(interp::Compiler.NativeInterpreter, f::Core.Builtin, ::Compiler.ArgInfo, sv::Compiler.InferenceState)
    @ Compiler ~/.julia/juliaup/julia-nightly/share/julia/Compiler/src/abstractinterpretation.jl:2118
  [5] abstract_call_known(interp::Compiler.NativeInterpreter, f::Any, arginfo::Compiler.ArgInfo, si::Compiler.StmtInfo, sv::Compiler.InferenceState, max_methods::Int64)
    @ Compiler ~/.julia/juliaup/julia-nightly/share/julia/Compiler/src/abstractinterpretation.jl:2664
  [...] (truncated)

It errors when constructing a PartialStruct with

objt0 = Oscar.MPolyAnyMap{<:AbstractAlgebra.MPolyRing{T}, Union{}, Nothing} where T<:AbstractAlgebra.FieldElem
undefs = Union{Nothing, Bool}[0, 0, 0, 0, nothing, nothing, 0]
fields = Any[AbstractAlgebra.MPolyRing{T} where T<:AbstractAlgebra.FieldElem, Union{}, Nothing, Vector, Any, Vector{Int64}, Dict{Symbol, Any}]

where the Oscar.MPolyAnyMap type is defined here: https://github.com/oscar-system/Oscar.jl/blob/43891614c618991a89f198068f4d514d89a3e3d6/src/Rings/MPolyMap/Types.jl#L4-L42

We can see it being instantiated with a C = Union{} type parameter, and because the inner constructor calls new with 4 values here, datatype_min_ninitialized returns 4 and therefore partialstruct_init_undefs fills in the corresponding undefs entries with false.

Therefore, the fix I suggested would not even be enough. Even if we may wonder why it occurs, isn't that technically a valid Julia type that we may see in inference? (even if it would never be constructed at runtime)

I guess a possible fix would be to look at fields in partialstruct_init_undefs and set the corresponding entry to true for Union{} fields. In theory it might be enough so long as any construction of PartialStruct uses that function to define undefs.

@aviatesk
Copy link
Member

Thanks so much for creating the reproducer!

From your explanation, it seems like the inference for new(OscarMPolyAnyMap{...}, ...) is skipping the check at

at = tmeet(𝕃ᵢ, at, ft)
at === Bottom && return RTEffects(Bottom, TypeError, EFFECTS_THROWS)
for some reason.

Because of that, an object type that shouldn't exist at runtime is being propagated around. This is likely causing form_partially_defiend_struct, which expects a valid object type, to throw an error.

However, I still can't reproduce the issue on my end though:

julia> using Oscar, Oscar.IntersectionTheory
  ___   ____   ____    _    ____
 / _ \ / ___| / ___|  / \  |  _ \   |  Combining ANTIC, GAP, Polymake, Singular
| | | |\___ \| |     / _ \ | |_) |  |  Type "?Oscar" for more information
| |_| | ___) | |___ / ___ \|  _ <   |  Manual: https://docs.oscar-system.org
 \___/ |____/ \____/_/   \_\_| \_\  |  Version 1.3.0

julia> code_typed(degeneracy_locus, (Oscar.IntersectionTheory.AbstractBundle{Oscar.IntersectionTheory.AbstractVariety}, Oscar.IntersectionTheory.AbstractBundle{Oscar.IntersectionTheory.AbstractVariety}, Int64))
Warning: detected a stack overflow; program state may be corrupted, so further execution might be unreliable.
ERROR: StackOverflowError:
  • Julia version: Version 1.13.0-DEV.193 (2025-03-11)
  • Oscar version: 1.3.0

For fixing the root cause, I think abstract_eval_new should correctly return Union{} in cases like this, since there might be other places that expect runtime-valid PartialStruct. What do you think?

@fingolfin
Copy link
Member

@aviatesk that's odd. We had to apply a change to our code to work around a stack overflow in code inference in Julia (yet another regression on master), but this workaround should be in the Oscar 1.3.0 release... But perhaps just in case, try with latest Oscar master?

@fingolfin fingolfin added this to the 1.12 milestone Mar 11, 2025
@aviatesk
Copy link
Member

Sorry, it looks like the issue was caused by changes I had locally (I didn't notice because those changes didn't cause any error when running the base tests).

I looked into the problem, and there's nothing wrong with abstract_eval_new itself. It seems like Union{} can be contained within Type{...}, and form_partially_defined_struct needs to be adjusted to avoid creating a runtime invalid PartialStruct in those cases.

@serenity4
Copy link
Contributor

serenity4 commented Mar 11, 2025

I still can't reproduce the issue

perhaps just in case, try with latest Oscar master

It worked for me with Oscar master and a 3-day old Julia nightly version, in case that helps.

I managed to reproduce the pattern that generates an invalid type:

julia> struct A{C<:Union{Int,Float64}} val::C end

julia> f(a::A{<:String}) = a
f (generic function with 1 method)

julia> code_typed(f, (A,))
1-element Vector{Any}:
 CodeInfo(
1return a
) => A{Union{}}

which happens in Oscar when dispatching to this function with an instance of this type, which eventually branches on isdefined here which then leads to inference building a PartialStruct, triggering the error. Essentially this:

# make it mutable with at least one possibly undefined field so we later form a PartialStruct
julia> mutable struct MA{C<:Union{Int,String}} a::C; b::Any; MA(a::T) where {T} = new{T}(a); end

julia> f(a::MA{<:Symbol}) = isdefined(a, :b) ? invokelatest(identity, a) : a.a
f (generic function with 1 method)

julia> code_typed(f, (MA,))
ERROR: AssertionError: `Union{}` typed field should be strictly undefined
Stacktrace:
  [1] Core.PartialStruct(typ::Type, undefs::Vector{Union{Nothing, Bool}}, fields::Vector{Any})
    @ Base ./coreir.jl:62
  [2] Core.PartialStruct(::Compiler.InferenceLattice{Compiler.ConditionalsLattice{…}}, typ::Any, undefs::Vector{Union{…}}, fields::Vector{Any})
    @ Compiler ~/.julia/juliaup/julia-nightly/share/julia/Compiler/src/typelattice.jl:765
  [3] form_partially_defined_struct(𝕃ᵢ::Compiler.InferenceLattice{Compiler.ConditionalsLattice{Compiler.PartialsLattice{Compiler.ConstsLattice}}}, obj::Any, name::Any)
    @ Compiler ~/.julia/juliaup/julia-nightly/share/julia/Compiler/src/abstractinterpretation.jl:2164
  [4] abstract_call_builtin(interp::Compiler.NativeInterpreter, f::Core.Builtin, ::Compiler.ArgInfo, sv::Compiler.InferenceState)
    @ Compiler ~/.julia/juliaup/julia-nightly/share/julia/Compiler/src/abstractinterpretation.jl:2121
  [5] abstract_call_known(interp::Compiler.NativeInterpreter, f::Any, arginfo::Compiler.ArgInfo, si::Compiler.StmtInfo, sv::Compiler.InferenceState, max_methods::Int64)
  [...]

IIUC the root cause is that Union{} is the only type that satisfies <:Symbol and <:Union{Int,Float64}, and is therefore assumed when specializing the method f to a UnionAll A. Is that something we could change?

aviatesk added a commit that referenced this issue Mar 11, 2025
Specifically, when `form_partially_defined_struct` tries to create a
`PartialStruct`, this commit adds extra checks to prevent creating it
if it doesn't exist at runtime.
While it would be better to avoid propagating these runtime-invalid
object types altogether, that would require a major overhaul of the
dispatch system. As a fix within the current dispatch system, this
commit is proposed.

- closes #57673

Co-authored-by: Cédric Belmant <[email protected]>
@aviatesk
Copy link
Member

Yea, it seems like the dispatch can sometimes narrow down signatures in a way that creates types that are invalid at runtime.

I'm not an expert on the dispatch system, but ideally, we should prevent dispatch from matching when it would create these runtime-invalid types. I'm not sure if that's even possible, though.

As a temporary fix, I've created #57720.

@vtjnash, we're discussing whether we can prevent method matching in cases where it would generate runtime-invalid object types like below. My understanding is that it's quite difficult to do (since it means the dispatch system needs to recognize min-field-count for each possible object types). What are your thoughts?

julia> mutable struct Issue57673{C<:Union{Int,Float64}}
           c::C
           d
           Issue57673(c::C, d) where C = new{C}(c, d)
           Issue57673(c::C) where C = new{C}(c)
       end

julia> function issue57673(x::Issue57673{<:AbstractString})
           x
       end
issue57673 (generic function with 1 method)

julia> methods(issue57673, (Issue57673,)) # we ideally want to have method error for this case?
# 1 method for generic function "issue57673" from Main:
 [1] issue57673(x::Issue57673{<:AbstractString})
     @ REPL[41]:1

julia> code_typed(issue57673, (Issue57673,); optimize=false)
1-element Vector{Any}:
 CodeInfo(
1%1 = x::Issue57673{Union{}}
└──      return %1
) => Issue57673{Union{}}

@vtjnash
Copy link
Member

vtjnash commented Mar 11, 2025

This is about inference, so dispatch has nothing to do with this issue. We have that predicate already,

julia> typeintersect(Issue57673{<:AbstractString}, Issue57673)
Issue57673{Union{}}

julia> Base.Compiler.has_concrete_subtype(ans)
false

julia> Base.Compiler.valid_as_lattice(Issue57673{Union{}}, true)
false

and we already forbid inference from exploring those methods itself (unless you force it with code_typed, as it is generally legal to ask nonsense queries of code_typed, though inference reserve the right to return garbage out)

all(@nospecialize(x) -> isvarargtype(x) || valid_as_lattice(x, true), sigtuple.parameters) ||
return Future(MethodCallResult(Union{}, Any, EFFECTS_THROWS, nothing, false, false)) # catch bad type intersections early

And that is also called from here:

function abstract_eval_new(interp::AbstractInterpreter, e::Expr, sstate::StatementState,
                           sv::AbsIntState)
    𝕃ᵢ = typeinf_lattice(interp)
    rt, isexact = instanceof_tfunc(abstract_eval_value(interp, e.args[1], sstate, sv), true)

It is a conservative query of course, but it isn't immediately obvious how it would have missed such an easy case where the field type is simply ::Union{} and allowed that to propagate in this example

@serenity4
Copy link
Contributor

serenity4 commented Mar 11, 2025

Thanks for the input. It seems that we may have a bug then:

julia> T = Oscar.MPolyAnyMap{<:Oscar.MPolyRing{<:Oscar.FieldElem}, Union{}, Nothing}
Oscar.MPolyAnyMap{<:MPolyRing{<:FieldElem}, Union{}, Nothing}

julia> fieldtype(T, 2)
Union{}

julia> Compiler.valid_as_lattice(T, true)
true

where has_concrete_subtype returns true, with

julia> Compiler.unwrap_unionall(T).flags
0x0021

@serenity4
Copy link
Contributor

serenity4 commented Mar 11, 2025

Here is a smaller example which captures the same behavior:

julia> struct A{X,Y}
         x::X
         y::Y
       end

julia> Compiler.valid_as_lattice(A{Int,Union{}}, true)
false

julia> Compiler.valid_as_lattice(A{<:Int,Union{}}, true)
true

julia> Base.unwrap_unionall(A{Int,Union{}}).flags
0x0052

julia> Base.unwrap_unionall(A{<:Int,Union{}}).flags
0x0021

The second call should return false, right?

@vtjnash
Copy link
Member

vtjnash commented Mar 12, 2025

I suspect it hasn't yet computed the fieldtypes for that UnionAll, so it is unaware yet as to whether they are possible

@aviatesk
Copy link
Member

It is a conservative query of course, but it isn't immediately obvious how it would have missed such an easy case where the field type is simply ::Union{} and allowed that to propagate in this example

Actually abstract_eval_new isn't causing the problem this time. The issue is simply that abstract_call_method allows introducing these invalid object types at runtime.
So the new test example I added in #57720 doesn't require nonsense reflection calls.

So I agree with Cédric making the checks that abstract_call_method uses more reliable would solve the core problem.

@aviatesk
Copy link
Member

I ran Oscar's test suite on the latest nightly build and confirmed that the error reported in this issue no longer occurs at least. There still seem to be other assertion errors, but they appear unrelated to this particular issue.

@serenity4
Copy link
Contributor

serenity4 commented Mar 12, 2025

I suspect it hasn't yet computed the fieldtypes for that UnionAll, so it is unaware yet as to whether they are possible

Field types are already precomputed here for this reason:

datatype_fieldtypes(x) # force computation of has_concrete_subtype to be updated now

But even after computing a Union{} in the field types, the has_concrete_subtype flag is still set (while it should no longer be):

julia> T = A{<:Int,Union{}}
A{<:Int64, Union{}}

julia> T0 = Base.unwrap_unionall(T)
A{var"#s4"<:Int64, Union{}}

julia> Base.datatype_fieldtypes(T0)
svec(var"#s4"<:Int64, Union{})

julia> Compiler.has_concrete_subtype(T0)
true

julia> T0.flags
0x0021

@vtjnash
Copy link
Member

vtjnash commented Mar 12, 2025

That isn't very useful to be there, since we are often unlikely to get there, bailing here instead:

else if (!jl_has_fixed_layout(dt))

@serenity4
Copy link
Contributor

Would you suggest to move the computation of st->has_concrete_subtype out of jl_compute_field_offsets and make it more explicit? Perhaps as an uncached ccallable function that we call during valid_as_lattice?
This isn't a part of the code I'm familiar with, and there seems to be some caching involved (based on layout from what I gather).

@vtjnash
Copy link
Member

vtjnash commented Mar 12, 2025

It can probably be moved above that conditional, so that we always call it just after computing the field types

@serenity4
Copy link
Contributor

I prototyped a fix in #57764, comments are welcome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Indicates an unexpected problem or unintended behavior compiler:inference Type inference
Projects
None yet
6 participants