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

typeintersect: fix UnionAll unaliasing bug caused by innervars. #53553

Merged
merged 2 commits into from
Mar 6, 2024

Conversation

N5N3
Copy link
Member

@N5N3 N5N3 commented Mar 2, 2024

The MWE in #53366 (comment) has been broken since v1.7.
I haven't checked which change caused the regression but more thorough UnionAll-renaming does fix it.

The 2nd commit fixes the wrapping order of innervars, as newly inserted innervar might depend on existing ones.

close #53621

@N5N3 N5N3 added types and dispatch Types, subtyping and method dispatch needs pkgeval Tests for all registered packages should be run with this change don't squash Don't squash merge labels Mar 2, 2024
@N5N3 N5N3 changed the title typeintersect: fix UnionAll unaliased bug caused by innervars. typeintersect: fix UnionAll unaliasing bug caused by innervars. Mar 2, 2024
@N5N3
Copy link
Member Author

N5N3 commented Mar 4, 2024

@nanosoldier runtests()

@topolarity
Copy link
Member

Thanks for the quick work, @N5N3!

I tried backporting this change to 1.10 and the patch applies (mostly) cleanly, but the test case is unfortunately still broken.

Would you be able to look into a backport and see what's going on?

@N5N3
Copy link
Member Author

N5N3 commented Mar 4, 2024

@topolarity which test case?
My local backport shows v1.10 is fine. (#53366 (comment) works correctly) And there should be no confusing conflict.

@topolarity topolarity added the backport 1.10 Change should be backported to the 1.10 release label Mar 4, 2024
@topolarity
Copy link
Member

topolarity commented Mar 4, 2024

Ah, my mistake - I managed to drop one of the commits when I was back-porting. Looking good here 👍

I can confirm that this resolves the original issue in #53366

src/subtype.c Show resolved Hide resolved
@N5N3 N5N3 added the backport 1.11 Change should be backported to release-1.11 label Mar 5, 2024
@nanosoldier
Copy link
Collaborator

The package evaluation job you requested has completed - possible new issues were detected.
The full report is available.

@gbaraldi
Copy link
Member

gbaraldi commented Mar 5, 2024

The crashes in pkgeval seem real.

@N5N3 N5N3 marked this pull request as draft March 5, 2024 23:07
@KristofferC KristofferC mentioned this pull request Mar 6, 2024
60 tasks
And ensures all innervar get checked.
@N5N3
Copy link
Member Author

N5N3 commented Mar 6, 2024

@nanosoldier runtests(["NSGAII", "UnitfulRecipes", "OutlierDetectionTrees", "MLJTSVDInterface", "SelfOrganizingMaps", "ImplicitPlots", "MLJDecisionTreeInterface", "OutlierDetectionNeighbors", "GPLinearODEMaker", "MLJClusteringInterface", "TidierCats", "Pyehtim", "GraviPet", "MLJGLMInterface", "Ipaper", "ScientificTypes", "MLJScientificTypes", "PlantRayTracer", "EconPDEs", "CategoricalDistributions", "MLJNaiveBayesInterface", "SkyDomes", "GeoTables", "AnyMOD", "MLJXGBoostInterface", "StatsLearnModels", "MLJEnsembles", "MCMCChains", "StatisticalMeasures", "MLJTestInterface", "ReducedComplexityModeling", "OutlierDetection", "OrdinalMultinomialModels", "GenerativeTopographicMapping", "MLJTuning", "DiffEqFinancial", "PredefinedDynamicalSystems", "GeneralizedSDistributions", "SensitivityRankCondition", "SchwarzChristoffel", "MLJTestIntegration", "EMpht", "ManifoldDiffEq", "DifferentialEquations", "LowRankIntegrators", "HetaSimulator", "Petri", "Bactos", "ChargeTransport", "EditorsRepo", "TSML", "SemanticTrajectories", "MINDFulMakie", "StateSpacePartitions", "FSimPlots", "NeuroAnalysis"])

@nanosoldier
Copy link
Collaborator

The package evaluation job you requested has completed - possible new issues were detected.
The full report is available.

@N5N3 N5N3 marked this pull request as ready for review March 6, 2024 21:50
@N5N3 N5N3 merged commit 56f1c8a into JuliaLang:master Mar 6, 2024
6 of 7 checks passed
@N5N3 N5N3 deleted the inter-fix branch March 6, 2024 21:57
@N5N3 N5N3 removed the needs pkgeval Tests for all registered packages should be run with this change label Mar 6, 2024
KristofferC pushed a commit that referenced this pull request Mar 7, 2024
…3553)

typeintersect: fix `UnionAll` unaliasing bug caused by innervars.
(cherry picked from commit 56f1c8a)
Drvi pushed a commit to RelationalAI/julia that referenced this pull request Mar 8, 2024
…liaLang#53553)

typeintersect: fix `UnionAll` unaliasing bug caused by innervars.
(cherry picked from commit 56f1c8a)
@KristofferC KristofferC mentioned this pull request Mar 12, 2024
25 tasks
KristofferC added a commit that referenced this pull request Mar 17, 2024
Backported PRs:
- [x] #39071 <!-- Add a lazy `logrange` function and `LogRange` type -->
- [x] #51802 <!-- Allow AnnotatedStrings in log messages -->
- [x] #53369 <!-- Orthogonalize re-indexing for FastSubArrays -->
- [x] #48050 <!-- improve `--heap-size-hint` arg handling -->
- [x] #53482 <!-- add IR encoding for EnterNode -->
- [x] #53499 <!-- Avoid compiler warning about redefining jl_globalref_t
-->
- [x] #53507 <!-- update staled `Core.Compiler.Effects` documentation
-->
- [x] #53408 <!-- task splitting: change additive accumulation to
multiplicative -->
- [x] #53523 <!-- add back an alias for `check_top_bit` -->
- [x] #53377 <!-- add _readdirx for returning more object info gathered
during dir scan -->
- [x] #53525 <!-- fix InteractiveUtils call in Base.runtests on failure
-->
- [x] #53540 <!-- use more efficient `_readdirx` for tab completion -->
- [x] #53545 <!-- use `_readdirx` for `walkdir` -->
- [x] #53551 <!-- revert "Add @create_log_macro for making custom styled
logging macros (#52196)" -->
- [x] #53554 <!-- Always return a value in 1-d circshift! of
abstractarray.jl -->
- [x] #53424 <!-- yet more atomics & cache-line fixes on work-stealing
queue -->
- [x] #53571 <!-- Update Documenter to v1.3 for inventory writing -->
- [x] #53403 <!-- Move parallel precompilation to Base -->
- [x] #53589 <!-- add back `unsafe_convert` to pointer for arrays -->
- [x] #53596 <!-- build: remove extra .a file -->
- [x] #53606 <!-- fix error path in `precompilepkgs` -->
- [x] #53004 <!-- Unexport with, at_with, and ScopedValue from Base -->
- [x] #53629 <!-- typo fix in scoped values docs -->
- [x] #53630 <!-- sroa: Fix incorrect scope counting -->
- [x] #53598 <!-- Use Base parallel precompilation to build stdlibs -->
- [x] #53649 <!-- precompilepkgs: package in boths deps and weakdeps are
in fact only weak -->
- [x] #53671 <!-- Fix bootstrap Base precompile in cross compile
configuration -->
- [x] #52125 <!-- Load Pkg if not already to reinstate missing package
add prompt -->
- [x] #53602 <!-- Handle zero on arrays of unions of number types and
missings -->
- [x] #53516 <!-- permit NamedTuple{<:Any, Union{}} to be created -->
- [x] #53643 <!-- Bump CSL to 1.1.1 to fix libgomp bug -->
- [x] #53679 <!-- move precompile workload back from Base -->
- [x] #53663 <!-- add isassigned methods for reinterpretarray -->
- [x] #53662 <!-- [REPL] fix incorrectly cleared line after completions
accepted -->
- [x] #53611 <!-- Linalg: matprod_dest for Diagonal and adjvec -->
- [x] #53659 <!-- fix #52025, re-allow all implicit pointer casts in
cconvert for Array -->
- [x] #53631 <!-- LAPACK: validate input parameters to throw informative
errors -->
- [x] #53628 <!-- Make some improvements to the Scoped Values
documentation. -->
- [x] #53655 <!-- Change tbaa of ptr_phi to tbaa_value  -->
- [x] #53391 <!-- Default to the medium code model in x86 linux -->
- [x] #53699 <!-- Move `isexecutable, isreadable, iswritable` to
`filesystem.jl` -->
- [x] #41232 <!-- Fix linear indexing for ReshapedArray if the parent
has offset axes -->
- [x] #53527 <!-- Enable analyzegc checks for try catch and fix found
issues -->
- [x] #52092 
- [x] #53682 <!-- Increase build precompilation -->
- [x] #53720 
- [x] #53553 <!-- typeintersect: fix `UnionAll` unaliasing bug caused by
innervars. -->

Contains multiple commits, manual intervention needed:
- [ ] #53305 <!-- Propagate inbounds in isassigned with CartesianIndex
indices -->

Non-merged PRs with backport label:
- [ ] #53736 <!-- fix literal-pow to return the right type when the base
is -1 -->
- [ ] #53707 <!-- Make ScopedValue public -->
- [ ] #53696 <!-- add invokelatest to on_done callback in bracketed
paste -->
- [ ] #53660 <!-- put Logging back in default sysimage -->
- [ ] #53509 <!-- revert moving "creating packages" from Pkg.jl -->
- [ ] #53452 <!-- RFC: allow Tuple{Union{}}, returning Union{} -->
- [ ] #53402 <!-- Add `jl_getaffinity` and `jl_setaffinity` -->
- [ ] #52694 <!-- Reinstate similar for AbstractQ for backward
compatibility -->
- [ ] #51928 <!-- Styled markdown, with a few tweaks -->
- [ ] #51816 <!-- User-themable stacktraces -->
- [ ] #51811 <!-- Make banner size depend on terminal size -->
- [ ] #51479 <!-- prevent code loading from lookin in the versioned
environment when building Julia -->
@KristofferC KristofferC removed the backport 1.11 Change should be backported to release-1.11 label Mar 18, 2024
KristofferC pushed a commit that referenced this pull request Mar 27, 2024
…3553)

typeintersect: fix `UnionAll` unaliasing bug caused by innervars.
(cherry picked from commit 56f1c8a)
KristofferC added a commit that referenced this pull request Apr 22, 2024
Backported PRs:
- [x] #50759 <!-- Fix outdated usage of scrubbing for log test failures
-->
- [x] #51830 <!-- Add version string to sysimg triple -->
- [x] #53273 <!-- [REPL] Fix typo in using/import completion -->
- [x] #53499 <!-- Avoid compiler warning about redefining jl_globalref_t
-->
- [x] #53424 <!-- yet more atomics & cache-line fixes on work-stealing
queue -->
- [x] #53596 <!-- build: remove extra .a file -->
- [x] #53516 <!-- permit NamedTuple{<:Any, Union{}} to be created -->
- [x] #53643 <!-- Bump CSL to 1.1.1 to fix libgomp bug -->
- [x] #53655 <!-- Change tbaa of ptr_phi to tbaa_value  -->
- [x] #53391 <!-- Default to the medium code model in x86 linux -->
- [x] #53809 <!-- Add missing GC_POP() in emit_cfunction -->
- [x] #53961 <!-- `LazyString` in `LinearAlgebra.checksquare` error
message -->
- [x] #52913 <!-- Added docstring for Artifacts.jl -->
- [x] #53553 <!-- typeintersect: fix `UnionAll` unaliasing bug caused by
@KristofferC KristofferC mentioned this pull request May 8, 2024
23 tasks
@KristofferC KristofferC removed the backport 1.10 Change should be backported to the 1.10 release label May 8, 2024
Drvi pushed a commit to RelationalAI/julia that referenced this pull request Jun 7, 2024
…liaLang#53553)

typeintersect: fix `UnionAll` unaliasing bug caused by innervars.
(cherry picked from commit 56f1c8a)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
don't squash Don't squash merge types and dispatch Types, subtyping and method dispatch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Assertion error in type intersection when testing NDTensors on 1.11
6 participants