Array(::AbstractRange) should return an Array#50568
Conversation
vtjnash
left a comment
There was a problem hiding this comment.
I don't see any methods for AbstractRange which don't return an Array. Indeed, I would have assumed that was the expected API for vcat. But I can see an argument that this is more reliable.
|
Currently, |
|
Since this may be seen as a bugfix (returning the incorrect type silently), I'm adding backport labels |
|
Can we just delete these methods and have it use |
|
Currently, there are some tests for Personally, I think those tests are abusing the type system (they should be |
|
If there are no further comments, would this be good to merge? |
Backported PRs: - [x] #50411 <!-- Fix weird dispatch of * with zero arguments --> - [x] #50202 <!-- Remove dynamic dispatch from _wait/wait2 --> - [x] #50064 <!-- Fix numbered prompt with input only with comment --> - [x] #50026 <!-- Store heapsnapshot files in tempdir() instead of current directory --> - [x] #50402 <!-- Add CPU feature helper function --> - [x] #50387 <!-- update newpages pointer after actually sweeping pages --> - [x] #50424 <!-- avoid potential type-instability in _replace_(str, ...) --> - [x] #50444 <!-- Optimize getfield lowering to avoid boxing in some cases --> - [x] #50474 <!-- docs: Fix a `!!! note` which was miscapitalized --> - [x] #50466 <!-- relax assertion involving pg->nold to reflect that it may be a bit in… --> - [x] #50490 <!-- Fix compat annotation for italic printstyled --> - [x] #50488 <!-- fix typo in `Base.isassigned` with `Tridiagonal` --> - [x] #50476 <!-- Profile: Add specifying dir for `take_heap_snapshot` and handling if current dir is unwritable --> - [x] #50461 <!-- fix typo in the --gcthreads argument description --> - [x] #50528 <!-- ssair: Correctly handle stmt insertion at end of basic block --> - [x] #50533 <!-- ensure internal_obj_base_ptr checks whether objects past freelist pointer are in freelist --> - [x] #49322 <!-- improve cat design / performance --> - [x] #50540 <!-- gc: remove over-eager assertion --> - [x] #50542 <!-- gf: remove unnecessary assert cycle==depth --> - [x] #50559 <!-- Expand kwcall lowering positional default check to vararg --> - [x] #50058 <!-- Add unwrapping mechanism for triangular mul and solves --> - [x] #50551 <!-- typeintersect: also record chained `innervars` --> - [x] #50552 <!-- read(io, Char): fix read with too many leading ones --> - [x] #50541 <!-- precompile: ensure globals are not accidentally created where disallowed --> - [x] #50576 <!-- use atomic compare exchange when setting the GC mark-bit --> - [x] #50578 <!-- gf: make method overwrite/delete an error during precompile --> - [x] #50516 <!-- Fix visibility of assert on GCC12/13 --> - [x] #50597 <!-- Fix memory corruption if task is launched inside finalizer --> - [x] #50591 <!-- build: fix various makefile bugs --> - [x] #50599 <!-- faster invalid object lookup in conservative gc --> - [x] #50634 <!-- 🤖 [master] Bump the SparseArrays stdlib from b4b0e72 to 99c99b4 --> - [x] #50639 <!-- Backport LLVM patches to fix various issues. --> - [x] #50546 <!-- Revert storage of method instance in LineInfoNode --> - [x] #50631 <!-- Shift DCE pass to optimize imaging mode code better --> - [x] #50525 <!-- only check that values are finite in `generic_lufact` when `check=true` --> - [x] #50587 <!-- isassigned for ranges with BigInt indices --> - [x] #50144 <!-- Page based heap size heuristics --> Need manual backport: - [ ] #50595 <!-- Rename ENV variable `JULIA_USE_NEW_PARSER` -> `JULIA_USE_FLISP_PARSER` --> Non-merged PRs with backport label: - [ ] #50637 <!-- Remove SparseArrays legacy code --> - [ ] #50618 <!-- inference: continue const-prop' when concrete-eval returns non-inlineable --> - [ ] #50598 <!-- only limit types in stack traces in the REPL --> - [ ] #50594 <!-- Disallow non-index Integer types in isassigned --> - [ ] #50568 <!-- `Array(::AbstractRange)` should return an `Array` --> - [ ] #50523 <!-- Avoid generic call in most cases for getproperty --> - [ ] #50172 <!-- print feature flags used for matching pkgimage -->
Currently, `Array(r::AbstractRange)` falls back to `vcat(r)`, but certain ranges may choose to specialize `vcat(r::AbstractRange)` to not return an `Array`. This PR ensures that `Array(r)` always returns an `Array`. At present, there's some code overlap with `vcat` (just above the `Array` method added in this PR). Perhaps some of these may be replaced by `unsafe_copyto!`, but the tests for ranges include some special cases that don't support `getindex`, which complicates things a bit. I've not done this for now. In any case, the common bit of code is pretty simple, so perhaps the duplication is harmless. (cherry picked from commit 3cc0590)
Currently, `Array(r::AbstractRange)` falls back to `vcat(r)`, but certain ranges may choose to specialize `vcat(r::AbstractRange)` to not return an `Array`. This PR ensures that `Array(r)` always returns an `Array`. At present, there's some code overlap with `vcat` (just above the `Array` method added in this PR). Perhaps some of these may be replaced by `unsafe_copyto!`, but the tests for ranges include some special cases that don't support `getindex`, which complicates things a bit. I've not done this for now. In any case, the common bit of code is pretty simple, so perhaps the duplication is harmless. (cherry picked from commit 3cc0590)
Backported PRs: - [x] #50637 <!-- Remove SparseArrays legacy code --> - [x] #50665 <!-- print `@time` msg into print buffer --> - [x] #50523 <!-- Avoid generic call in most cases for getproperty --> - [x] #50635 <!-- `versioninfo()`: include build info and unofficial warning --> - [x] #50670 <!-- Make reinterpret specialize fully. --> - [x] #50666 <!-- include `--pkgimage=no` caches for stdlibs --> - [x] #50765 - [x] #50764 - [x] #50768 - [x] #50767 - [x] #50618 <!-- inference: continue const-prop' when concrete-eval returns non-inlineable --> - [x] #50689 <!-- Attach `tanpi` docstring to method --> - [x] #50671 <!-- Fix rdiv of complex lhs by real factorizations --> - [x] #50598 <!-- only limit types in stack traces in the REPL --> - [x] #50766 <!-- Don't partition alwaysinline functions --> - [x] #50771 <!-- re-allow non-string values in ENV `get!` --> - [x] #50682 <!-- Add fallback if we have make a weird GC decision. --> - [x] #50781 <!-- fix `bit_map!` with aliasing --> - [x] #50172 <!-- print feature flags used for matching pkgimage --> - [x] #50844 <!-- Bump OpenBLAS binaries to use the new GEMM multithreading threshold --> - [x] #50826 <!-- Update dependency builds --> - [x] #50845 <!-- fix #50438, use default pool for at-threads --> - [x] #50568 <!-- `Array(::AbstractRange)` should return an `Array` --> - [x] #50655 <!-- fix hashing regression. --> - [x] #50779 <!-- Minor refactor to image generation --> - [x] #50791 <!-- Make symbols internal in jl_create_native, and only externalize them when partitioning --> - [x] #50724 <!-- Merge opaque closure modules with the rest of the workqueue --> - [x] #50738 <!-- Add alignment to constant globals --> - [x] #50871 <!-- macOS: Don't inspect dead threadtls during exception handling. --> Need manual backport: Contains multiple commits, manual intervention needed: Non-merged PRs with backport label: - [ ] #50850 <!-- Remove weird Rational dispatch and add pi functions to list --> - [ ] #50823 <!-- Make ranges more robust with unsigned indexes. --> - [ ] #50809 <!-- Limit type-printing in MethodError --> - [ ] #50663 <!-- Fix Expr(:loopinfo) codegen --> - [ ] #50594 <!-- Disallow non-index Integer types in isassigned --> - [ ] #50385 <!-- Precompile pidlocks: add to NEWS and docs --> - [ ] #49805 <!-- Limit TimeType subtraction to AbstractDateTime -->
Backported PRs: - [x] #47782 <!-- Generalize Bool parse method to AbstractString --> - [x] #48634 <!-- Remove unused "deps" mechanism in internal sorting keywords [NFC] --> - [x] #49931 <!-- Lock finalizers' lists at exit --> - [x] #50064 <!-- Fix numbered prompt with input only with comment --> - [x] #50474 <!-- docs: Fix a `!!! note` which was miscapitalized --> - [x] #50516 <!-- Fix visibility of assert on GCC12/13 --> - [x] #50635 <!-- `versioninfo()`: include build info and unofficial warning --> - [x] #49915 <!-- Revert "Remove number / vector (#44358)" --> - [x] #50781 <!-- fix `bit_map!` with aliasing --> - [x] #50845 <!-- fix #50438, use default pool for at-threads --> - [x] #49031 <!-- Update inference.md --> - [x] #50289 <!-- Initialize prev_nold and nold in gc_reset_page --> - [x] #50559 <!-- Expand kwcall lowering positional default check to vararg --> - [x] #49582 <!-- Update HISTORY.md for `DelimitedFiles` --> - [x] #50341 <!-- invokelatest docs should say not exported before 1.9 --> - [x] #50525 <!-- only check that values are finite in `generic_lufact` when `check=true` --> - [x] #50444 <!-- Optimize getfield lowering to avoid boxing in some cases --> - [x] #50523 <!-- Avoid generic call in most cases for getproperty --> - [x] #50860 <!-- Add `Base.get_extension` to docs/API --> - [x] #50164 <!-- codegen: handle dead code with unsafe_store of FCA pointers --> - [x] #50568 <!-- `Array(::AbstractRange)` should return an `Array` --> - [x] #50871 <!-- macOS: Don't inspect dead threadtls during exception handling. --> Need manual backport: - [ ] #48542 <!-- Add docs on task-specific buffering using multithreading --> - [ ] #50591 <!-- build: fix various makefile bugs --> Non-merged PRs with backport label: - [ ] #50842 <!-- Avoid race conditions with recursive rm --> - [ ] #50823 <!-- Make ranges more robust with unsigned indexes. --> - [ ] #50663 <!-- Fix Expr(:loopinfo) codegen --> - [ ] #49716 <!-- Update varinfo() docstring signature --> - [ ] #49713 <!-- prevent REPL from erroring in numbered mode in some situations --> - [ ] #49573 <!-- Implement jl_cpu_pause on PPC64 --> - [ ] #48726 <!-- fix macro expansion of property destructuring --> - [ ] #48642 <!-- Use gc alloc instead of alloc typed in lowering --> - [ ] #48183 <!-- Don't use pkgimage for package if any includes fall in tracked path for coverage or alloc tracking --> - [ ] #48050 <!-- improve `--heap-size-hint` arg handling --> - [ ] #47615 <!-- Allow threadsafe access to buffer of type inference profiling trees -->
Currently,
Array(r::AbstractRange)falls back tovcat(r), but certain ranges may choose to specializevcat(r::AbstractRange)to not return anArray. This PR ensures thatArray(r)always returns anArray.At present, there's some code overlap with
vcat(just above theArraymethod added in this PR). Perhaps some of these may be replaced byunsafe_copyto!, but the tests for ranges include some special cases that don't supportgetindex, which complicates things a bit. I've not done this for now. In any case, the common bit of code is pretty simple, so perhaps the duplication is harmless.