PartialStruct tmerge fails to converge #43784
Comments
JeffBezanson
added
the
bug
When `tmerge` is applied to `Const`/`PartialStruct` and a field is
`Const` in one of the types and `Union{}` (i.e. undef) and the other,
the resulting field type must not be `Const` (as the resursively called
`tmerge` produces).
Fixes #43784.
When `tmerge` is applied to `Const`/`PartialStruct` and a field is
`Const` in one of the types and `Union{}` (i.e. undef) in the other,
the resulting field type must not be `Const` (as the resursively called
`tmerge` produces).
Fixes #43784.
Co-authored-by: Shuhei Kadowaki <40514306+aviatesk@users.noreply.github.com>
|
Sigh ... this issue just caused me half my Sunday in a slightly different presentation ... let's get it fixed! |
Co-authored-by: Shuhei Kadowaki <40514306+aviatesk@users.noreply.github.com>
When `tmerge` is applied to `Const`/`PartialStruct` and a field is
`Const` in one of the types and `Union{}` (i.e. undef) in the other,
the resulting field type must not be `Const` (as the resursively called
`tmerge` produces).
Fixes #43784.
|
I see #43812 is merged now, which is probably good since it fixed the immediate issue, but from discussion with @vtjnash this morning, we're not convinced, it's the correct solution, so re-opening this to track hopefully coming up with something better (cc @martinholters @aviatesk). Notes from our discussion this morning: Point 1: A complicating issue here is that currently we have !⊑(a, b), i.e. we don't consider Union{} in a PartialStruct field to be a refinement of Point 2: Because of (1), prior to #43812, tmerge as violating its spec. It should always produce a valid upper bound over the inference lattice, but currently it did not. Point 3: #43784 fixed this specific case, but it is not generally sufficient as the same issue can happen with PartialStructs that don't have Point 4: Because of Point 1 and Point 3, if we were to fix the thing identified in Point 1, the fix from #43812 would become ineffective, since tmerge would always return the larger of the two types. |
|
For point 3, I propose we could use here a simplified function tmerge_fast(fielda, fieldb, reftype)
is_lattice_equal(fielda, fieldb) && return fielda
tn = _typename(widentype(fielda))
if tn isa Const && tn === _typename(widentype(fieldb))
return typeintersect(reftype, (tn.val::TypeName).wrapper)
end
return reftype
end
......
fieldx = tmerge_fast(fielda, fieldb, fieldtype(ty, n))
......This is not monotonic (since we completely ignore the possibility of them being subtypes), but neither is |
…#43812) When `tmerge` is applied to `Const`/`PartialStruct` and a field is `Const` in one of the types and `Union{}` (i.e. undef) in the other, the resulting field type must not be `Const` (as the resursively called `tmerge` produces). Fixes JuliaLang#43784.
Previously we assumed only union type could have complexity that violated the tmerge lattice requirements, but other types can have that too. This lets us fix an issue with the PartialStruct comparison failing for undefined fields, mentioned in #43784.
…#43812) When `tmerge` is applied to `Const`/`PartialStruct` and a field is `Const` in one of the types and `Union{}` (i.e. undef) in the other, the resulting field type must not be `Const` (as the resursively called `tmerge` produces). Fixes JuliaLang#43784.
Previously we assumed only union type could have complexity that violated the tmerge lattice requirements, but other types can have that too. This lets us fix an issue with the PartialStruct comparison failing for undefined fields, mentioned in #43784.
Previously we assumed only union type could have complexity that violated the tmerge lattice requirements, but other types can have that too. This lets us fix an issue with the PartialStruct comparison failing for undefined fields, mentioned in #43784.
* inference: fix tmerge lattice over issimpleenoughtype Previously we assumed only union type could have complexity that violated the tmerge lattice requirements, but other types can have that too. This lets us fix an issue with the PartialStruct comparison failing for undefined fields, mentioned in #43784. * inference: refine PartialStruct lattice tmerge Be more aggressive about merging fields to greatly accelerate convergence, but also compute anyrefine more correctly as we do now elsewhere (since #42831, a121721) Move the tmeet algorithm, without changes, since it is a precise lattice operation, not a heuristic limit like tmerge. Close #43784
* inference: fix tmerge lattice over issimpleenoughtype Previously we assumed only union type could have complexity that violated the tmerge lattice requirements, but other types can have that too. This lets us fix an issue with the PartialStruct comparison failing for undefined fields, mentioned in #43784. * inference: refine PartialStruct lattice tmerge Be more aggressive about merging fields to greatly accelerate convergence, but also compute anyrefine more correctly as we do now elsewhere (since #42831, a121721) Move the tmeet algorithm, without changes, since it is a precise lattice operation, not a heuristic limit like tmerge. Close #43784 (cherry picked from commit ff88fa4)
Original text:
I just opened a PR to
Measurements.jland found that CI is hanging on Julia nightly: https://github.com/JuliaPhysics/Measurements.jl/runs/4649844099?check_suite_focus=true. I just recompiled latestmasterof Julia and found that simple things likehang
Originally posted by @giordano in #43561 (comment)
Initial analysis and MWE:
The problem here is that this
tmergemay fail to converge for recursive structs (e.g. ifmayinlineallocis false):Then in a while loop, we keep wrapping it such that we never converge this lattice:
The text was updated successfully, but these errors were encountered: