Merge branch 'master' into fieldvalues

Author: jw3126

.github/workflows/CI.yml

@@ -10,12 +10,14 @@ jobs:
     name: Julia ${{ matrix.version }} - ${{ matrix.os }} - ${{ matrix.arch }}
     runs-on: ${{ matrix.os }}
     strategy:
-      fail-fast: true
+      fail-fast: false
       matrix:
         version:
           - '1.0'
           - '1.3'
           - '1.5'
+          - '1.6'
+          - '1'
           - 'nightly'
         os:
           - ubuntu-latest

.github/workflows/Downstream.yml

@@ -0,0 +1,52 @@
+name: IntegrationTest
+on:
+  push:
+  pull_request:
+
+jobs:
+  test:
+    name: ${{ matrix.package.repo }}/${{ matrix.julia-version }}
+    runs-on: ${{ matrix.os }}
+    strategy:
+      fail-fast: false
+      matrix:
+        julia-version: [1.6, 1, nightly]
+        os: [ubuntu-latest]
+        package:
+          - {repo: JuliaObjects/Accessors.jl}
+          - {repo: JuliaFolds/BangBang.jl}
+          - {repo: jw3126/Setfield.jl}
+          - {repo: rafaqz/Flatten.jl}
+    steps:
+      - uses: actions/checkout@v2
+      - uses: julia-actions/setup-julia@v1
+        with:
+          version: ${{ matrix.julia-version }}
+          arch: x64
+      - uses: julia-actions/julia-buildpkg@latest
+      - name: Clone Downstream
+        uses: actions/checkout@v2
+        with:
+          repository: ${{ matrix.package.repo }}
+          path: downstream
+      - name: Load this and run the downstream tests
+        shell: julia --color=yes --project=downstream {0}
+        run: |
+          using Pkg
+          try
+            # force it to use this PR's version of the package
+            Pkg.develop(PackageSpec(path="."))  # resolver may fail with main deps
+            Pkg.update()
+            Pkg.test(coverage=true)  # resolver may fail with test time deps
+          catch err
+            err isa Pkg.Resolve.ResolverError || rethrow()
+            # If we can't resolve that means this is incompatible by SemVer and this is fine
+            # It means we marked this as a breaking change, so we don't need to worry about
+            # Mistakenly introducing a breaking change, as we have intentionally made one
+            @info "Not compatible with this release. No problem." exception=err
+            exit(0)  # Exit immediately, as a success
+          end
+      - uses: julia-actions/julia-processcoverage@v1
+      - uses: codecov/codecov-action@v1
+        with:
+          file: lcov.info

Project.toml

@@ -1,7 +1,7 @@
 name = "ConstructionBase"
 uuid = "187b0558-2788-49d3-abe0-74a17ed4e7c9"
 authors = ["Takafumi Arakaki", "Rafael Schouten", "Jan Weidner"]
-version = "1.3.0"
+version = "1.3.1"
 
 [deps]
 LinearAlgebra = "37e2e46d-f89d-539d-b4ee-838fcccc9c8e"

src/ConstructionBase.jl

@@ -37,14 +37,10 @@ constructorof(::Type{<:NamedTuple{names}}) where names =
 
 struct NamedTupleConstructor{names} end
 
-@generated function (::NamedTupleConstructor{names})(args...) where names
-    quote
-        Base.@_inline_meta
-        $(NamedTuple{names, Tuple{args...}})(args)
-    end
+@inline function (::NamedTupleConstructor{names})(args...) where names
+    NamedTuple{names}(args)
 end
 
-
 ################################################################################
 #### getfields
 ################################################################################
@@ -53,13 +49,40 @@ getfields(x::NamedTuple) = x
 @generated function getfields(obj)
     fnames = fieldnames(obj)
     fvals = map(fnames) do fname
-        Expr(:call, :getfield, :obj, QuoteNode(fname))
+         Expr(:call, :getfield, :obj, QuoteNode(fname))
+    end
+    :(NamedTuple{$fnames}(($(fvals...),)))
+end
+getproperties(o::NamedTuple) = o
+getproperties(o::Tuple) = o
+if VERSION >= v"1.7"
+    function getproperties(obj)
+        fnames = propertynames(obj)
+        NamedTuple{fnames}(getproperty.(Ref(obj), fnames))
+    end
+else
+    function getproperties(obj)
+        check_properties_are_fields(obj)
+        getfields(obj)
+    end
+    @generated function check_properties_are_fields(obj)
+        if which(propertynames, Tuple{obj}).sig != Tuple{typeof(propertynames), Any}
+            # custom propertynames defined for this type
+            return quote
+                msg = """
+                Different fieldnames and propertynames are only supported on Julia v1.7+.
+                For older julia versions, consider overloading
+                `ConstructionBase.getproperties(obj::$(typeof(obj))`.
+                See also https://github.com/JuliaObjects/ConstructionBase.jl/pull/60.
+                """
+                error(msg)
+            end
+        else
+            :()
+        end
     end
-    fvals = Expr(:tuple, fvals...)
-    :(NamedTuple{$fnames}($fvals))
 end
 
-getproperties(o) = getfields(o)
 ################################################################################
 ##### setproperties
 ################################################################################
@@ -94,9 +117,8 @@ function validate_setproperties_result(
 end
 @noinline function validate_setproperties_result(nt_new, nt_old, obj, patch)
     O = typeof(obj)
-    P = typeof(patch)
     msg = """
-    Failed to assign properties $(fieldnames(P)) to object with fields $(fieldnames(O)).
+    Failed to assign properties $(propertynames(patch)) to object with properties $(propertynames(obj)).
     You may want to overload
     ConstructionBase.setproperties(obj::$O, patch::NamedTuple)
     ConstructionBase.getproperties(obj::$O)
@@ -148,6 +170,7 @@ setproperties_object(obj, patch::Tuple{}) = obj
     obj = $obj
     patch = $patch
     """
+    throw(ArgumentError(msg))
 end
 setproperties_object(obj, patch::NamedTuple{()}) = obj
 function setproperties_object(obj, patch)

test/runtests.jl

@@ -69,6 +69,7 @@ end
     @test setproperties((a=1,), ()) === (a=1,)
     @test setproperties((a=1,), NamedTuple()) === (a=1,)
     @test setproperties(AB(1,2), ()) === AB(1,2)
+    @test_throws ArgumentError setproperties(AB(1,2), (10,))
     @test setproperties(AB(1,2), NamedTuple()) === AB(1,2)
 
     @test setproperties(AB(1,2), (a=2, b=3))   === AB(2,3)
@@ -262,7 +263,31 @@ end
     end
 end
 
-function funny_numbers(n)::Tuple
+# example of a struct with different fields and properties
+struct FieldProps{NT <: NamedTuple{(:a, :b)}}
+    components::NT
+end
+
+Base.propertynames(obj::FieldProps) = (:a, :b)
+Base.getproperty(obj::FieldProps, name::Symbol) = getproperty(getfield(obj, :components), name)
+ConstructionBase.constructorof(::Type{<:FieldProps}) = (a, b) -> FieldProps((a=a, b=b))
+
+@testset "use properties, not fields" begin
+    x = FieldProps((a=1, b=:b))
+    if VERSION >= v"1.7"
+        @test getproperties(x) == (a=1, b=:b)
+        @test setproperties(x, a="aaa") == FieldProps((a="aaa", b=:b))
+        VERSION >= v"1.8-dev" ?
+            (@test_throws "Failed to assign properties (:c,) to object with properties (:a, :b)" setproperties(x, c=0)) :
+            (@test_throws ArgumentError setproperties(x, c=0))
+    else
+        @test_throws ErrorException getproperties(x)
+        @test_throws ErrorException setproperties(x, a="aaa")
+        @test_throws ErrorException setproperties(x, c=0)
+    end
+end
+
+function funny_numbers(::Type{Tuple}, n)::Tuple
     types = [
         Int128, Int16, Int32, Int64, Int8,
         UInt128, UInt16, UInt32, UInt64, UInt8,
@@ -272,47 +297,129 @@ function funny_numbers(n)::Tuple
 end
 
 function funny_numbers(::Type{NamedTuple}, n)::NamedTuple
-    t = funny_numbers(n)
+    t = funny_numbers(Tuple,n)
     pairs = map(1:n) do i
         Symbol("a$i") => t[i]
     end
     (;pairs...)
 end
 
-for n in [0,1,20,40]
+abstract type S end
+Sn_from_n = Dict{Int,Type}()
+for n in [0,1,10,20,40]
     Sn = Symbol("S$n")
     types = [Symbol("A$i") for i in 1:n]
     fields = [Symbol("a$i") for i in 1:n]
     typed_fields = [:($ai::$Ai) for (ai,Ai) in zip(fields, types)]
-    @eval struct $(Sn){$(types...)}
+    @eval struct $(Sn){$(types...)} <: S
         $(typed_fields...)
     end
-    @eval funny_numbers(::Type{$Sn}) = ($Sn)(funny_numbers($n)...)
+    @eval Sn_from_n[$n] = $Sn
+end
+function funny_numbers(::Type{S}, n)::S
+    fields = funny_numbers(Tuple, n)
+    Sn_from_n[n](fields...)
+end
+
+reconstruct(obj, content) = constructorof(typeof(obj))(content...)
+
+function write_output_to_ref!(f, out_ref::Ref, arg_ref::Ref)
+    arg = arg_ref[]
+    out_ref[] = f(arg)
+    out_ref
+end
+function write_output_to_ref!(f, out_ref::Ref, arg_ref1::Ref, arg_ref2::Ref)
+    arg1 = arg_ref1[]
+    arg2 = arg_ref2[]
+    out_ref[] = f(arg1,arg2)
+    out_ref
+end
+function hot_loop_allocs(f::F, args...) where {F}
+    # we want to test that f(args...) does not allocate 
+    # when used in hot loops
+    # however a naive @allocated f(args...)
+    # will not be representative of what happens in an inner loop
+    # Instead it will sometimes box inputs/outputs
+    # and report too many allocations
+    # so we use Refs to minimize inputs and outputs
+    out_ref = Ref(f(args...))
+    arg_refs = map(Ref, args)
+    write_output_to_ref!(f, out_ref, arg_refs...)
+    out_ref = typeof(out_ref)() # erase out_ref so we can assert work was done later
+    # Avoid splatting args... which also results in undesired allocs
+    allocs = if length(arg_refs) == 1
+        r1, = arg_refs
+        @allocated write_output_to_ref!(f, out_ref, r1)
+    elseif length(arg_refs) == 2
+        r1,r2 = arg_refs
+        @allocated write_output_to_ref!(f, out_ref, r1, r2)
+    else
+        error("TODO too many args")
+    end
+    @assert out_ref[] == f(args...)
+    return allocs
+end
+
+@testset "no allocs $T" for T in [Tuple, NamedTuple, S]
+    @testset "n = $n" for n in [0,1,10,20]
+        obj = funny_numbers(T, n)
+        new_content = funny_numbers(Tuple, n)
+        @test 0 == hot_loop_allocs(constructorof, typeof(obj))
+        @test 0 == hot_loop_allocs(reconstruct, obj, new_content)
+        @test 0 == hot_loop_allocs(getproperties, obj)
+        @test 0 == hot_loop_allocs(getfields, obj)
+        patch_sizes = [0,1,n÷3,n÷2,n]
+        patch_sizes = min.(patch_sizes, n)
+        patch_sizes = unique(patch_sizes)
+        for k in patch_sizes
+            patch = if T === Tuple
+                funny_numbers(Tuple, k)
+            else
+                funny_numbers(NamedTuple, k)
+            end
+            @test 0 == hot_loop_allocs(setproperties, obj, patch)
+        end
+    end
 end
 
 @testset "inference" begin
     @testset "Tuple n=$n" for n in [0,1,2,3,4,5,10,20,30,40]
-        t = funny_numbers(n)
+        t = funny_numbers(Tuple,n)
         @test length(t) == n
         @test getproperties(t) === t
         @inferred getproperties(t)
+        @test getfields(t) === t
+        @inferred getfields(t)
+        @inferred constructorof(typeof(t))
+        content = funny_numbers(Tuple,n)
+        @inferred reconstruct(t, content)
         for k in 0:n
-            t2 = funny_numbers(k)
-            @inferred setproperties(t, t2)
+            t2 = funny_numbers(Tuple,k)
             @test setproperties(t, t2)[1:k] === t2
             @test setproperties(t, t2) isa Tuple
             @test length(setproperties(t, t2)) == n
             @test setproperties(t, t2)[k+1:n] === t[k+1:n]
+            @inferred setproperties(t, t2)
         end
     end
-    @inferred getproperties(funny_numbers(100))
-    @inferred setproperties(funny_numbers(100), funny_numbers(90))
+    @inferred getproperties(funny_numbers(Tuple,100))
+    @inferred setproperties(funny_numbers(Tuple,100), funny_numbers(Tuple,90))
+
     @testset "NamedTuple n=$n" for n in [0,1,2,3,4,5,10,20,30,40]
         nt = funny_numbers(NamedTuple, n)
         @test nt isa NamedTuple
         @test length(nt) == n
         @test getproperties(nt) === nt
         @inferred getproperties(nt)
+        @test getfields(nt) === nt
+        @inferred getfields(nt)
+
+        @inferred constructorof(typeof(nt))
+        if VERSION >= v"1.3"
+            content = funny_numbers(NamedTuple,n)
+            @inferred reconstruct(nt, content)
+        end
+        #no_allocs_test(nt, content)
         for k in 0:n
             nt2 = funny_numbers(NamedTuple, k)
             @inferred setproperties(nt, nt2)
@@ -325,18 +432,27 @@ end
     @inferred getproperties(funny_numbers(NamedTuple, 100))
     @inferred setproperties(funny_numbers(NamedTuple, 100), funny_numbers(NamedTuple, 90))
 
+    @inferred setproperties(funny_numbers(S,0), funny_numbers(NamedTuple, 0))
+    @inferred setproperties(funny_numbers(S,1), funny_numbers(NamedTuple, 1))
+    @inferred setproperties(funny_numbers(S,20), funny_numbers(NamedTuple, 18))
+    @inferred setproperties(funny_numbers(S,40), funny_numbers(NamedTuple, 38))
+    @inferred constructorof(S0)
+    @inferred constructorof(S1)
+    @inferred constructorof(S20)
+    @inferred constructorof(S40)
+    if VERSION >= v"1.3"
+        @inferred reconstruct(funny_numbers(S,0) , funny_numbers(Tuple,0))
+        @inferred reconstruct(funny_numbers(S,1) , funny_numbers(Tuple,1))
+        @inferred reconstruct(funny_numbers(S,20), funny_numbers(Tuple,20))
+        @inferred reconstruct(funny_numbers(S,40), funny_numbers(Tuple,40))
+    end
 
-    @inferred setproperties(funny_numbers(S0), funny_numbers(NamedTuple, 0))
-    @inferred setproperties(funny_numbers(S1), funny_numbers(NamedTuple, 1))
-    @inferred setproperties(funny_numbers(S20), funny_numbers(NamedTuple, 18))
-    @inferred setproperties(funny_numbers(S40), funny_numbers(NamedTuple, 38))
-    @inferred getproperties(funny_numbers(S0))
-    @inferred getproperties(funny_numbers(S1))
-    @inferred getproperties(funny_numbers(S20))
-    @inferred getproperties(funny_numbers(S40))
-
-    @inferred getfields(funny_numbers(S0))
-    @inferred getfields(funny_numbers(S1))
-    @inferred getfields(funny_numbers(S20))
-    @inferred getfields(funny_numbers(S40))
+    @inferred getfields(funny_numbers(S,0))
+    @inferred getfields(funny_numbers(S,1))
+    @inferred getfields(funny_numbers(S,20))
+    @inferred getfields(funny_numbers(S,40))
+    @inferred getproperties(funny_numbers(S,0))
+    @inferred getproperties(funny_numbers(S,1))
+    @inferred getproperties(funny_numbers(S,20))
+    @inferred getproperties(funny_numbers(S,40))
 end