typeintersect: conservative typevar subtitution during finish_unionall#54465
typeintersect: conservative typevar subtitution during finish_unionall#54465N5N3 merged 2 commits intoJuliaLang:masterfrom
finish_unionall#54465Conversation
This commit adds a nothrow path for type instantiation, which eliminates the bad `Union` elements from the result rather than returns the bottom type. close JuliaLang#54404
|
This looks like a good direction. But it seems like we still may be left with some ambiguity of whether the intersection/application returned Bottom exactly, or whether it failed, which could have minor implications? I think we should merge this, as it is clearly an improvement already. Should we later consider using a different sentinel for the failure (eg an Exception object or a NULL)? |
Yes, that should also help to remove the special handling of Vararg Tuple. |
vtjnash
left a comment
There was a problem hiding this comment.
Got a few minutes free to review, and just had a couple questions about cleanups
| JL_CATCH { | ||
| *btemp->ub = jl_substitute_var_nothrow(iub, vb->var, varval); | ||
| if (*btemp->ub == NULL) | ||
| res = jl_bottom_type; |
There was a problem hiding this comment.
Does *btemp->ub need to be assigned non-NULL here also?
| JL_TRY { | ||
| check_datatype_parameters(tn, iparams, ntp); |
There was a problem hiding this comment.
Looks like it would be an easy cleanup now to remove this try/catch and thread the nothrow into this call too
| if (pi == NULL) { | ||
| if (i == ntp-1 && jl_is_vararg(elt)) { | ||
| t = NULL; | ||
| break; | ||
| } | ||
| else { | ||
| pi = jl_bottom_type; | ||
| } | ||
| } |
| JL_TRY { | ||
| lb = inst_type_w_(ua->var->lb, env, stack, check, 0); |
There was a problem hiding this comment.
Why do you use try/catch instead of passing nothrow here?
There was a problem hiding this comment.
As we don't want a imprecise lb or invarient parameter. It seems bad to allow Val{Union{Int,Complex{T}}} to be instantiated as Val{Int}.
Though perhaps adding another level of control would be better.
| if (v->N) // This branch should never throw. | ||
| N = inst_type_w_(v->N, env, stack, check, 0); |
There was a problem hiding this comment.
FWIW, this branch does potentially throw
julia> Tuple{Vararg{Int,Union{N,Complex{N}}}} where N
NTuple{Union{Complex{N}, N}, Int64} where N
julia> ans{String}
ERROR: TypeError: in Complex, in T, expected T<:Real, got Type{String}
Stacktrace:
[1] top-level scope
@ REPL[6]:1
There was a problem hiding this comment.
My bad, shouldn't assume invalid type don't exist. (IIRC, only Int and TypeVar with no constraint could be used as vararg length)
| JL_TRY { | ||
| jl_value_t *pi = inst_type_w_(elt, env, stack, check, 0); | ||
| iparams[i] = pi; | ||
| bound |= (pi != elt); | ||
| } | ||
| JL_CATCH { | ||
| if (!nothrow) jl_rethrow(); | ||
| t = NULL; | ||
| } | ||
| if (t == NULL) break; |
There was a problem hiding this comment.
Similarly here, why does it use try/catch instead of setting nothrow directly?
…ation (#54514) Adopt suggestions from #54465 (review) and fix various added regession & residual MWE.
…ation (#54514) Adopt suggestions from #54465 (review) and fix various added regession & residual MWE. (cherry picked from commit af545b9)
…ation (#54514) Adopt suggestions from #54465 (review) and fix various added regession & residual MWE. (cherry picked from commit af545b9)
Backported PRs: - [x] #53665 <!-- use afoldl instead of tail recursion for tuples --> - [x] #53976 <!-- LinearAlgebra: LazyString in interpolated error messages --> - [x] #54005 <!-- make `view(::Memory, ::Colon)` produce a Vector --> - [x] #54010 <!-- Overload `Base.literal_pow` for `AbstractQ` --> - [x] #54069 <!-- Allow PrecompileTools to see MI's inferred by foreign abstract interpreters --> - [x] #53750 <!-- inference correctness: fields and globals can revert to undef --> - [x] #53984 <!-- Profile: fix heap snapshot is valid char check --> - [x] #54102 <!-- Explicitly compute stride in unaliascopy for SubArray --> - [x] #54070 <!-- Fix integer overflow in `skip(s::IOBuffer, typemax(Int64))` --> - [x] #54013 <!-- Support case-changes to Annotated{String,Char}s --> - [x] #53941 <!-- Fix writing of AnnotatedChars to AnnotatedIOBuffer --> - [x] #54137 <!-- Fix typo in docs for `partialsortperm` --> - [x] #54129 <!-- use correct size when creating output data from an IOBuffer --> - [x] #54153 <!-- Fixup IdSet docstring --> - [x] #54143 <!-- Fix `make install` from tarballs --> - [x] #54151 <!-- LinearAlgebra: Correct zero element in `_generic_matvecmul!` for block adj/trans --> - [x] #54213 <!-- Add `public` statement to `Base.GC` --> - [x] #54222 <!-- Utilize correct tbaa when emitting stores of unions. --> - [x] #54233 <!-- set MAX_OS_WRITE on unix --> - [x] #54255 <!-- fix `_checked_mul_dims` in the presence of 0s and overflow. --> - [x] #54259 <!-- Fix typo in `readuntil` --> - [x] #54251 <!-- fix typo in gc_mark_memory8 when chunking a large array --> - [x] #54276 <!-- Fix solve for complex `Hermitian` with non-vanishing imaginary part on diagonal --> - [x] #54248 <!-- ensure package callbacks are invoked when no valid precompile file exists for an "auto loaded" stdlib --> - [x] #54308 <!-- Implement eval-able AnnotatedString 2-arg show --> - [x] #54302 <!-- Specialised substring equality for annotated strs --> - [x] #54243 <!-- prevent `package_callbacks` to run multiple time for a single package --> - [x] #54350 <!-- add a precompile signature to Artifacts code that is used by JLLs --> - [x] #54331 <!-- correctly track freed bytes in jl_genericmemory_to_string --> - [x] #53509 <!-- revert moving "creating packages" from Pkg.jl --> - [x] #54335 <!-- When accessing the data pointer for an array, first decay it to a Derived Pointer --> - [x] #54239 <!-- Make sure `fieldcount` constant-folds for `Tuple{...}` --> - [x] #54288 - [x] #54067 - [x] #53715 <!-- Add read/write specialisation for IOContext{AnnIO} --> - [x] #54289 <!-- Rework annotation ordering/optimisations --> - [x] #53815 <!-- create phantom task for GC threads --> - [x] #54130 <!-- inference: handle `LimitedAccuracy` in `handle_global_assignment!` --> - [x] #54428 <!-- Move ConsoleLogging.jl into Base --> - [x] #54332 <!-- Revert "add unsetindex support to more copyto methods (#51760)" --> - [x] #53826 <!-- Make all command-line options documented in all related files --> - [x] #54465 <!-- typeintersect: conservative typevar subtitution during `finish_unionall` --> - [x] #54514 <!-- typeintersect: followup cleanup for the nothrow path of type instantiation --> - [x] #54499 <!-- make `@doc x` work without REPL loaded --> - [x] #54210 <!-- attach finalizer in `mmap` to the correct object --> - [x] #54359 <!-- Pkg REPL: cache `pkg_mode` lookup --> Non-merged PRs with backport label: - [ ] #54471 <!-- Actually setup jit targets when compiling packageimages instead of targeting only one --> - [ ] #54457 <!-- Make `String(::Memory)` copy --> - [ ] #54323 <!-- inference: fix too conservative effects for recursive cycles --> - [ ] #54322 <!-- effects: add new `@consistent_overlay` macro --> - [ ] #54191 <!-- make `AbstractPipe` public --> - [ ] #53957 <!-- tweak how filtering is done for what packages should be precompiled --> - [ ] #53882 <!-- Warn about cycles in extension precompilation --> - [ ] #53707 <!-- Make ScopedValue public --> - [ ] #53452 <!-- RFC: allow Tuple{Union{}}, returning Union{} --> - [ ] #53402 <!-- Add `jl_getaffinity` and `jl_setaffinity` --> - [ ] #53286 <!-- Raise an error when using `include_dependency` with non-existent file or directory --> - [ ] #52694 <!-- Reinstate similar for AbstractQ for backward compatibility --> - [ ] #51479 <!-- prevent code loading from lookin in the versioned environment when building Julia -->
Backported PRs: - [x] #54010 <!-- Overload `Base.literal_pow` for `AbstractQ` --> - [x] #54143 <!-- Fix `make install` from tarballs --> - [x] #54151 <!-- LinearAlgebra: Correct zero element in `_generic_matvecmul!` for block adj/trans --> - [x] #54233 <!-- set MAX_OS_WRITE on unix --> - [x] #54251 <!-- fix typo in gc_mark_memory8 when chunking a large array --> - [x] #54363 <!-- typeintersect: fix another stack overflow caused by circular constraints --> - [x] #54497 <!-- Make TestLogger thread-safe (introduce a lock) --> - [x] #53796 <!-- Add a missing doc --> - [x] #54465 <!-- typeintersect: conservative typevar subtitution during `finish_unionall` --> - [x] #54514 <!-- typeintersect: followup cleanup for the nothrow path of type instantiation --> Need manual backport: - [ ] #52505 <!-- fix alignment of emit_unbox_store copy --> - [ ] #53373 <!-- fix sysimage-native-code=no option with pkgimages --> - [ ] #53815 <!-- create phantom task for GC threads --> - [ ] #53984 <!-- Profile: fix heap snapshot is valid char check --> - [ ] #54276 <!-- Fix solve for complex `Hermitian` with non-vanishing imaginary part on diagonal --> Contains multiple commits, manual intervention needed: - [ ] #52854 <!-- Change to streaming out the heap snapshot data --> - [ ] #53218 <!-- Fix interpreter_exec.jl test --> - [ ] #53833 <!-- Profile: make heap snapshots viewable in vscode viewer --> - [ ] #54303 <!-- LinearAlgebra: improve type-inference in Symmetric/Hermitian matmul --> - [ ] #52694 <!-- Reinstate similar for AbstractQ for backward compatibility --> Non-merged PRs with backport label: - [ ] #54471 <!-- Actually setup jit targets when compiling packageimages instead of targeting only one --> - [ ] #53452 <!-- RFC: allow Tuple{Union{}}, returning Union{} --> - [ ] #51479 <!-- prevent code loading from lookin in the versioned environment when building Julia -->
…ll` (JuliaLang#54465) This commit adds a nothrow path for type instantiation, which eliminates the bad `Union` elements from the result rather than returns the bottom type. close JuliaLang#54404
…ation (JuliaLang#54514) Adopt suggestions from JuliaLang#54465 (review) and fix various added regession & residual MWE.
This commit adds a nothrow path for type instantiation, which eliminates the bad
Unionelements from the result rather than returns the bottom type.close #54404