Add some aliasing warnings to docstrings for mutating functions in Base #50824
Conversation
|
Should |
|
I would even change should to must. |
Looked for it in the code base but the search didn't yield anything. Perhaps it doesn't exist 🤯 |
done |
|
Is "alias with" a self-explanatory phrase? Not a native speaker, but I would understand this better if it said "share memory with" or something similar. |
I took the formulation from |
|
Replacement done, I think this is ready for review |
|
I agree with adding warnings to the docstrings. However, I'm not sure what to do with julia> x = [1, 2];
julia> map!(x -> 2x, x, x); x
2-element Vector{Int64}:
2
4In fact, there is a unit test for that use case. |
|
I just took a cue from #50814 but it's easy to remove. Should I leave |
|
Good catch. julia> x = ones(Int,4); @views map!(+, x[begin+1:end], x[begin+1:end], x[begin:end-1]); x
4-element Vector{Int64}:
1
2
3
4We need to specify (or non-specify) an evaluation order for Either of these would mean that We should also consider the behavior of |
|
I actually had a PR (#47012) for the
|
Yeah, that might be the safest thing to do for now. |
|
The "...intended to operate without making any allocations" clause seems redundant and buries the key point. How about sticking to |
|
I removed the redundancy and fixed the |
|
Gonna use @jakobnissen's suggestion and run filter(map(String, names(Base))) do i
endswith(i, '!')
endto see if I missed a few spots. |
|
OK just went through the whole list, see the first message. If anyone objects to my selection of "aliasing-vulnerable" mutating functions, let them speak now or forever hold their peace |
|
Can this be backported to 1.10? If 1.10 becomes an LTS release, it might be nice to have these changes in the docs. |
Co-authored-by: Cameron Bieganek <8310743+CameronBieganek@users.noreply.github.com>
Don't ask me, I'm just visiting around these parts 🤣 |
|
Question for triage: Should we add a boilerplate warning to every mutating function that can misbehave when passed aliased inputs? |
Backported PRs: - [x] #48625 <!-- add replace(io, str, patterns...) --> - [x] #48387 <!-- Improve documentation of sort-related functions --> - [x] #48363 <!-- Revise sort.md and docstrings in sort.jl --> - [x] #48977 <!-- Update SparseArrays.jl stdlib for SuiteSparse 7 --> - [x] #50719 <!-- fix `CyclePadding(::DataType)` --> - [x] #50694 <!-- inference: permit recursive type traits --> - [x] #50860 <!-- Add `Base.get_extension` to docs/API --> - [x] #50594 <!-- Disallow non-index Integer types in isassigned --> - [x] #50802 <!-- Makes IntrusiveLinkedListSynchronized mutable to avoid allocation on poptask --> - [x] #50858 <!-- Add a `threadpool` parameter to `Channel` constructor --> - [x] #50874 <!-- Restrict COFF to a single thread when symbol count is high --> - [x] #50822 <!-- Add default method for setmodifiers! --> - [x] #50730 <!-- Fix integer overflow in `isapprox` --> - [x] #50850 <!-- Remove weird Rational dispatch and add pi functions to list --> - [x] #50809 <!-- Limit type-printing in MethodError --> - [x] #50915 <!-- Add note the `Task` about sticky bit --> - [x] #50929 <!-- when widening tuple types in tmerge, only widen the complex parts --> - [x] #50928 <!-- Bump JuliaSyntax to 0.4.6 --> - [x] #50959 <!-- Update libssh2 patches --> - [x] #50823 <!-- Make ranges more robust with unsigned indexes. --> - [x] #48542 <!-- Add docs on task-specific buffering using multithreading --> - [x] #50912 <!-- Separate foreign threads into a :foreign threadpool --> - [x] #51010 <!-- Add ORIGIN to SuiteSparse rpath on Linux/FreeBSD --> - [x] #50753 <!-- cat: remove unused promote_eltype methods that confuse inference --> - [x] #51027 <!-- Implement realloc accounting correctly --> - [x] #51019 <!-- fix a case of potentially use of undefined variable when handling error in distributed message processing --> - [x] #51039 <!-- Revert "Optimize findall(f, ::AbstractArray{Bool}) (#42202)" --> - [x] #51036 <!-- add missing invoke edge for nospecialize targets --> - [x] #51042 <!-- inference: fix return_type_tfunc modeling of concrete functions --> - [x] #51114 <!-- Workaround upstream FreeBSD issue #272992 --> - [x] #50892 <!-- Add `JL_DLLIMPORT` to `small_typeof` declaration --> - [x] #51154 <!-- broadcast: use recursion rather than ntuple to map over a tuple --> - [x] #51153 <!-- fix debug typo in "add missing invoke edge for nospecialize targets (#51036)" --> - [x] #51222 <!-- Check again if the tty is open inside the IO lock --> - [x] #51236 <!-- Add lock around uv_unref during init --> - [x] #51243 <!-- GMP: Gracefully handle more overflows. --> - [x] #51254 <!-- Ryu: make sure adding zeros does not overwrite trailing dot --> - [x] #51175 <!-- shorten stale_age for cachefile lock --> - [x] #51300 <!-- fix method definition error for bad vararg --> - [x] #51307 <!-- fix force-throw ctrl-C on Windows --> - [x] #51303 <!-- ensure revising structs is safe --> - [x] #51393 - [x] #51403 Need manual backport: - [x] #51009 <!-- fix #50562, regression in `in` of tuple of Symbols --> - [x] #51053 <!-- Bump Statistics stdlib --> - [x] #51013 <!-- fix #50709, type bound error due to free typevar in sparam env --> - [x] #51305 <!-- reduce test time for rounding and floatfuncs --> Contains multiple commits, manual intervention needed: - [ ] #50663 <!-- Fix Expr(:loopinfo) codegen --> - [ ] #51035 <!-- refactor GC scanning code to reflect jl_binding_t are now first class --> - [ ] #51092 <!-- inference: fix bad effects for recursion --> - [x] #51247 <!-- Check if malloc has succeeded before incrementing gc stats --> - [x] #51294 <!-- use LibGit2_jll for LibGit2 library --> Non-merged PRs with backport label: - [ ] #51132 <!-- Handle `AbstractQ` in concatenation --> - [x] #51029 <!-- testdefs: make sure that if a test set changes the active project, they change it back when they're done --> - [ ] #50919 <!-- Code loading: do the "skipping mtime check for stdlib" check regardless of the value of `ispath(f)` --> - [ ] #50824 <!-- Add some aliasing warnings to docstrings for mutating functions --> - [x] #50385 <!-- Precompile pidlocks: add to NEWS and docs --> - [ ] #49805 <!-- Limit TimeType subtraction to AbstractDateTime -->
|
@LilithHafner did this come up in triage? Honestly at this point I'm happy to revert to whatever state of the PR is deemed more mergeable |
|
No. Last Traige we only talked about the |
Yes |
|
|
Triage says merge! |
LilithHafner
added
status:merge me
LilithHafner
removed
the
status:merge me
|
Thanks @gdalle! Sorry I made this process so long. |
|
Don't apologize, thorough reviews are better than no reviews :) |
Backported PRs: - [x] #50932 <!-- types: fix hash values of Vararg --> - [x] #50975 <!-- Use rr-safe `nopl; rdtsc` sequence --> - [x] #50989 <!-- fix incorrect results in `expm1(::Union{Float16, Float32})` --> - [x] #51284 <!-- Avoid infinite loop when doing SIGTRAP in arm64-apple --> - [x] #51332 <!-- Add s4 field to Xoshiro --> - [x] #51397 <!-- call Pkg precompile hook in latest world --> - [x] #51405 <!-- Remove fallback that assigns a module to inlined frames. --> - [x] #51491 <!-- Throw clearer ArgumentError for strip with two string args --> - [x] #51531 <!-- fix `_tryonce_download_from_cache` (busybox.exe download error) --> - [x] #51541 <!-- Fix string index error in tab completion code --> - [x] #51530 <!-- Don't mark nonlocal symbols as hidden --> - [x] #51557 <!-- Fix last startup & shutdown precompiles --> - [x] #51512 <!-- avoid limiting Type{Any} to Type --> - [x] #51595 <!-- reset `maxprobe` on `empty!` --> - [x] #51582 <!-- Aggressive constprop in LinearAlgebra.wrap --> - [x] #51592 <!-- correctly track element pointer in heap snapshot --> - [x] #51326 <!-- complete false & true more generally as vals --> - [x] #51376 <!-- make `hash(::Xoshiro)` compatible with `==` --> - [x] #51557 <!-- Fix last startup & shutdown precompiles --> - [x] #51845 - [x] #51840 - [x] #50663 <!-- Fix Expr(:loopinfo) codegen --> - [x] #51863 <!-- LLVM 15.0.7-9 --> Contains multiple commits, manual intervention needed: - [ ] #51035 <!-- refactor GC scanning code to reflect jl_binding_t are now first class --> - [ ] #51092 <!-- inference: fix bad effects for recursion --> Non-merged PRs with backport label: - [ ] #51479 <!-- prevent code loading from lookin in the versioned environment when building Julia --> - [ ] #51414 <!-- improvements on GC scheduler shutdown --> - [ ] #51366 <!-- Handle infix operators in REPL completion --> - [ ] #50919 <!-- Code loading: do the "skipping mtime check for stdlib" check regardless of the value of `ispath(f)` --> - [ ] #50824 <!-- Add some aliasing warnings to docstrings for mutating functions in Base --> - [ ] #49805 <!-- Limit TimeType subtraction to AbstractDateTime -->
KristofferC
added
backport 1.10
Backported PRs: - [x] #51213 <!-- Wait for other threads to finish compiling before exiting --> - [x] #51520 <!-- Make allocopt respect the GC verifier rules with non usual address spaces --> - [x] #51598 <!-- Use a simple error when reporting sysimg load failures. --> - [x] #51757 <!-- fix parallel peakflop usage --> - [x] #51781 <!-- Don't make pkgimages global editable --> - [x] #51848 <!-- allow finalizers to take any locks and yield during exit --> - [x] #51847 <!-- add missing wait during Timer and AsyncCondition close --> - [x] #50824 <!-- Add some aliasing warnings to docstrings for mutating functions in Base --> - [x] #51885 <!-- remove chmodding the pkgimages --> - [x] #50207 <!-- [devdocs] Improve documentation about building external forks of LLVM --> - [x] #51967 <!-- further fix to the new promoting method for AbstractDateTime subtraction --> - [x] #51980 <!-- macroexpand: handle const/atomic struct fields correctly --> - [x] #51995 <!-- [Artifacts] Pass artifacts dictionary to `ensure_artifact_installed` dispatch --> - [x] #52098 <!-- Fix errors in `sort` docstring --> - [x] #52136 <!-- Bump JuliaSyntax to 0.4.7 --> - [x] #52140 <!-- Make c func `abspath` consistent on Windows. Fix tracking path conversion. --> - [x] #52009 <!-- fix completion that resulted in startpos of 0 for `\\ --> - [x] #52192 <!-- cap the number of GC threads to number of cpu cores --> - [x] #52206 <!-- Make have_fma consistent between interpreter and compiled --> - [x] #52027 <!-- fix Unicode.julia_chartransform for Julia 1.10 --> - [x] #52217 <!-- More helpful error message for empty `cpu_target` in `Base.julia_cmd` --> - [x] #51371 <!-- Memoize `cwstring` when used for env lookup / modification on Windows --> - [x] #52214 <!-- Turn Method Overwritten Error into a PrecompileError -- turning off caching --> - [x] #51895 <!-- Devdocs on fixing precompile hangs, take 2 --> - [x] #51596 <!-- Reland "Don't mark nonlocal symbols as hidden"" --> - [x] #51834 <!-- [REPLCompletions] allow symbol completions within incomplete macrocall expression --> - [x] #52010 <!-- Revert "Support sorting iterators (#46104)" --> - [x] #51430 <!-- add support for async backtraces of Tasks on any thread --> - [x] #51471 <!-- Fix segfault if root task is NULL --> - [x] #52194 <!-- Fix multiversioning issues caused by the parallel llvm work --> - [x] #51035 <!-- refactor GC scanning code to reflect jl_binding_t are now first class --> - [x] #52030 <!-- Bump Statistics --> - [x] #52189 <!-- codegen: ensure i1 bool is widened to i8 before storing --> - [x] #52228 <!-- Widen diagonal var during `Type` unwrapping in `instanceof_tfunc` --> - [x] #52182 <!-- jitlayers: replace sharedbytes intern pool with one that respects alignment --> Contains multiple commits, manual intervention needed: - [ ] #51092 <!-- inference: fix bad effects for recursion --> Non-merged PRs with backport label: - [ ] #52196 <!-- Fix creating custom log level macros --> - [ ] #52170 <!-- fix invalidations related to `ismutable` --> - [ ] #51479 <!-- prevent code loading from lookin in the versioned environment when building Julia -->
KristofferC
removed
the
backport 1.10
Functions like
sum!(B, A)have undefined behavior whenAandBshare memory. We might fix that in the long run, but in the short run, doc warnings are better than nothing.Related issues:
map!#50814See also: https://discourse.julialang.org/t/did-julia-community-do-something-to-improve-its-correctness/102515/
accumulate!all!any!append!(unsure)asyncmap!broadcast!circcopy!circshift!clamp!conj!copy!copyto!count!cumprod!cumsum!delete!deleteat!digits!empty!extrema!fill!filter!findmax!findmin!get!hex2bytes!insert!intersect!invpermute!keepat!kron!map!maximum!merge!(unsure)mergewith!(unsure)minimum!modifyproperty!partialsort!partialsortperm!permute!permutedims!pop!popat!popfirst!prepend!(unsure)prod!push!pushfirst!put!read!readbytes!replace!replaceproperty!resize!reverse!setdiff!setindex!setproperty!sizehint!sort!sortperm!splice!sum!swapproperty!symdiff!take!union!unique!unsafe_copyto!unsafe_store!