replace constructorof with something not UB (#102)

* replace constructorof with something not UB

It is confusing that `constructorof` is documented as returning the constructor of `T`, which is normally defined to be `T`, and then typically does not return `T`, and so does not usually construct `T`. However, this PR is intended just to remove the undefined behavior here of using an unsound generated function (examining mutable state with getfield).

* Update ConstructionBase.jl

* Update ConstructionBase.jl

* Update ConstructionBase.jl

* add pre to CI

* upd ci

* drop support for unmaintained versions of Julia

* delete remaining branching for unsupported versions of Julia

* remove pieces specific to old julia versions

---------

Co-authored-by: Jameson Nash <vtjnash@gmail.com>
Co-authored-by: Alexander Plavin <alexander@plav.in>

Author: 3 people

.github/workflows/CI.yml

@@ -14,19 +14,13 @@ jobs:
     strategy:
       matrix:
         version:
-          - '1.0'
-          - '1.6'
           - '1.10'
           - '1'
+          - 'pre'
         os:
           - ubuntu-latest
           - macOS-latest
           - windows-latest
-        exclude:
-          - os: macOS-latest # Apple Silicon
-            version: '1.0'
-          - os: macOS-latest # Apple Silicon
-            version: '1.6'
     steps:
       - uses: actions/checkout@v4
       - uses: julia-actions/setup-julia@v2

.github/workflows/Downstream.yml

@@ -12,7 +12,7 @@ jobs:
     strategy:
       fail-fast: false
       matrix:
-        julia-version: ['1.6', '1.10', '1']
+        julia-version: ['1.10', '1', 'pre']
         os: [ubuntu-latest]
         package:
           - Accessors
@@ -21,7 +21,7 @@ jobs:
           - Flatten
     steps:
       - uses: actions/checkout@v2
-      - uses: julia-actions/setup-julia@v1
+      - uses: julia-actions/setup-julia@v2
         with:
           version: ${{ matrix.julia-version }}
           arch: x64

Project.toml

@@ -19,7 +19,7 @@ ConstructionBaseStaticArraysExt = "StaticArrays"
 [compat]
 IntervalSets = "0.5, 0.6, 0.7"
 StaticArrays = "1"
-julia = "1"
+julia = "1.10"
 LinearAlgebra = "<0.0.1,1"
 
 [extras]

src/ConstructionBase.jl

@@ -26,10 +26,7 @@ for (name, path) in [
     end
 end
 
-@generated function constructorof(::Type{T}) where T
-    getfield(parentmodule(T), nameof(T))
-end
-
+constructorof(T::Type) = Base.typename(T).wrapper
 constructorof(::Type{<:Tuple}) = tuple
 constructorof(::Type{<:NamedTuple{names}}) where names =
     NamedTupleConstructor{names}()
@@ -48,40 +45,14 @@ getfields(x::NamedTuple) = x
 getproperties(o::NamedTuple) = o
 getproperties(o::Tuple) = o
 
-if VERSION >= v"1.7"
-    function check_properties_are_fields(obj)
-        # for ntuples of symbols `===` is semantically the same as `==`
-        # but triple equals is easier for the compiler to optimize, see #82
-        if propertynames(obj) !== fieldnames(typeof(obj))
-            error("""
-            The `$(nameof(typeof(obj)))` type defines custom properties: it has `propertynames` overloaded.
-            Please define `ConstructionBase.setproperties(::$(nameof(typeof(obj))), ::NamedTuple)` to set its properties.
-            """)
-        end
-    end
-else
-    function is_propertynames_overloaded(T::Type)::Bool
-        T <: NamedTuple && return false
-        which(propertynames, Tuple{T}).sig !== Tuple{typeof(propertynames), Any}
-    end
-
-    @generated function check_properties_are_fields(obj)
-        if is_propertynames_overloaded(obj)
-            return quote
-                T = typeof(obj)
-                msg = """
-                The function `Base.propertynames` was overloaded for type `$T`.
-                Please make sure the following methods are also overloaded for this type:
-                ```julia
-                ConstructionBase.setproperties
-                ConstructionBase.getproperties # optional in VERSION >= julia v1.7
-                ```
-                """
-                error(msg)
-            end
-        else
-            :(nothing)
-        end
+function check_properties_are_fields(obj)
+    # for ntuples of symbols `===` is semantically the same as `==`
+    # but triple equals is easier for the compiler to optimize, see #82
+    if propertynames(obj) !== fieldnames(typeof(obj))
+        error("""
+        The `$(nameof(typeof(obj)))` type defines custom properties: it has `propertynames` overloaded.
+        Please define `ConstructionBase.setproperties(::$(nameof(typeof(obj))), ::NamedTuple)` to set its properties.
+        """)
     end
 end
 
@@ -102,27 +73,13 @@ end
 # otherwise: throw an error
 tuple_or_ntuple(::Type, names, vals) = error("Only Int and Symbol propertynames are supported")
 
-if VERSION >= v"1.7"
-    function getproperties(obj)
-        fnames = propertynames(obj)
-        tuple_or_ntuple(fnames, getproperty.((obj,), fnames))
-    end
-    function getfields(obj::T) where {T}
-        fnames = fieldnames(T)
-        NamedTuple{fnames}(getfield.((obj,), fnames))
-    end
-else
-    @generated function getfields(obj)
-        fnames = fieldnames(obj)
-        fvals = map(fnames) do fname
-             Expr(:call, :getfield, :obj, QuoteNode(fname))
-        end
-        :(NamedTuple{$fnames}(($(fvals...),)))
-    end
-    function getproperties(obj)
-        check_properties_are_fields(obj)
-        getfields(obj)
-    end
+function getproperties(obj)
+    fnames = propertynames(obj)
+    tuple_or_ntuple(fnames, getproperty.((obj,), fnames))
+end
+function getfields(obj::T) where {T}
+    fnames = fieldnames(T)
+    NamedTuple{fnames}(getfield.((obj,), fnames))
 end
 
 ################################################################################
@@ -211,8 +168,4 @@ end
 include("nonstandard.jl")
 include("functions.jl")
 
-if !isdefined(Base, :get_extension)
-    include("../ext/ConstructionBaseLinearAlgebraExt.jl")
-end
-
 end # module

src/nonstandard.jl

@@ -36,5 +36,5 @@ linrange_constructor(start, stop, len, lendiv) = LinRange(start, stop, len)
 constructorof(::Type{<:LinRange}) = linrange_constructor
 
 ### Expr: args get splatted
-# ::Expr annotation is to make it type-stable on Julia 1.3-
+# ::Expr annotation is to make it type-stable on Julia 1.3-, probably not necessary anymore
 constructorof(::Type{<:Expr}) = (head, args) -> Expr(head, args...)::Expr

test/runtests.jl

@@ -341,15 +341,7 @@ Base.getproperty(obj::FieldProps, name::Symbol) = getproperty(getfield(obj, :com
     @test occursin("setproperties", msg)
     @test occursin("FieldProps", msg)
  # == FieldProps((a="aaa", b=:b)
-    if VERSION >= v"1.7"
-        @test getproperties(x) == (a=1, b=:b)
-    else
-        res = @test_throws ErrorException getproperties(x)
-        msg = sprint(showerror, res.value)
-        @test occursin("overload", msg)
-        @test occursin("getproperties", msg)
-        @test occursin("FieldProps", msg)
-    end
+    @test getproperties(x) == (a=1, b=:b)
 end
 
 
@@ -361,24 +353,20 @@ Base.getproperty(s::SProp, prop::Symbol) = "ps$prop"
 Base.getproperty(s::SProp, prop::Int) = "pi$prop"
 Base.getproperty(s::SProp, prop::String) = "pstr$prop"
 
-if VERSION >= v"1.7"
-    # automatic getproperties() supported only on 1.7+
+@testset "properties can be numbered" begin
+    @test getproperties(SProp((:a, :b))) === (a="psa", b="psb")
+    @test getproperties(SProp((1, 2))) === ("pi1", "pi2")
+    # what should it return?
+    @test_broken getproperties(SProp(("a", "b")))
 
-    @testset "properties can be numbered" begin
-        @test getproperties(SProp((:a, :b))) === (a="psa", b="psb")
-        @test getproperties(SProp((1, 2))) === ("pi1", "pi2")
-        # what should it return?
-        @test_broken getproperties(SProp(("a", "b")))
-
-        @test_throws ErrorException getproperties(SProp((1, :a)))
-    end
+    @test_throws ErrorException getproperties(SProp((1, :a)))
+end
 
-    @testset "propertynames can be a vector" begin
-        @test getproperties(SProp([:a, :b])) === (a="psa", b="psb")
-        @test getproperties(SProp(Symbol[])) === (;)
-        @test getproperties(SProp([1, 2])) === ("pi1", "pi2")
-        @test getproperties(SProp(Int[])) === ()
-    end
+@testset "propertynames can be a vector" begin
+    @test getproperties(SProp([:a, :b])) === (a="psa", b="psb")
+    @test getproperties(SProp(Symbol[])) === (;)
+    @test getproperties(SProp([1, 2])) === ("pi1", "pi2")
+    @test getproperties(SProp(Int[])) === ()
 end
 
 function funny_numbers(::Type{Tuple}, n)::Tuple
@@ -484,11 +472,7 @@ end
 @testset "no allocs S2" begin
     obj = S2(3, UInt32(5))
     @test 0 == hot_loop_allocs(constructorof, typeof(obj))
-    if VERSION < v"1.6"
-        @test 32 ≥ hot_loop_allocs(setproperties, obj, (; a = nothing, b = Int32(6)))
-    else
-        @test 0 == hot_loop_allocs(setproperties, obj, (; a = nothing, b = Int32(6)))
-    end
+    @test 0 == hot_loop_allocs(setproperties, obj, (; a = nothing, b = Int32(6)))
 end
 
 @testset "inference" begin
@@ -524,10 +508,8 @@ end
         @inferred getfields(nt)
 
         @inferred constructorof(typeof(nt))
-        if VERSION >= v"1.3"
-            content = funny_numbers(NamedTuple,n)
-            @inferred reconstruct(nt, content)
-        end
+        content = funny_numbers(NamedTuple,n)
+        @inferred reconstruct(nt, content)
         #no_allocs_test(nt, content)
         for k in 0:n
             nt2 = funny_numbers(NamedTuple, k)
@@ -549,12 +531,10 @@ end
     @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 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))
 
     @inferred getfields(funny_numbers(S,0))
     @inferred getfields(funny_numbers(S,1))
@@ -569,45 +549,43 @@ end
 
 using StaticArrays, IntervalSets
 
-if isdefined(Base, :get_extension)  # some 1.9 version
-    @testset "staticarrays" begin
-        sa = @SVector [2, 4, 6, 8]
-        sa2 = ConstructionBase.constructorof(typeof(sa))((3.0, 5.0, 7.0, 9.0))
-        @test sa2 === @SVector [3.0, 5.0, 7.0, 9.0]
-
-        ma = @MMatrix [2.0 4.0; 6.0 8.0]
-        ma2 = @inferred ConstructionBase.constructorof(typeof(ma))((1, 2, 3, 4))
-        @test ma2 isa MArray{Tuple{2,2},Int,2,4}
-        @test all(ma2 .=== @MMatrix [1 3; 2 4])
-
-        sz = SizedArray{Tuple{2,2}}([1 2;3 4])
-        sz2 = @inferred ConstructionBase.constructorof(typeof(sz))([:a :b; :c :d])
-        @test sz2 == SizedArray{Tuple{2,2}}([:a :b; :c :d])
-        @test typeof(sz2) <: SizedArray{Tuple{2,2},Symbol,2,2}
-
-        for T in (SVector, MVector)
-            @test @inferred(ConstructionBase.constructorof(T)((1, 2, 3)))::T == T((1, 2, 3))
-            @test @inferred(ConstructionBase.constructorof(T{3})((1, 2, 3)))::T == T((1, 2, 3))
-            @test @inferred(ConstructionBase.constructorof(T{3})((1, 2)))::T == T((1, 2))
-            @test @inferred(ConstructionBase.constructorof(T{3, Symbol})((1, 2, 3)))::T == T((1, 2, 3))
-            @test @inferred(ConstructionBase.constructorof(T{3, Symbol})((1, 2)))::T == T((1, 2))
-            @test @inferred(ConstructionBase.constructorof(T{3, X} where {X})((1, 2, 3)))::T == T((1, 2, 3))
-            @test @inferred(ConstructionBase.constructorof(T{3, X} where {X})((1, 2)))::T == T((1, 2))
-            @test @inferred(ConstructionBase.constructorof(T{X, Symbol} where {X})((1, 2, 3)))::T == T((1, 2, 3))
-        end
-
-        sv = SVector(1, 2)
-        @test SVector(3.0, 2.0) === @inferred setproperties(sv, x = 3.0)
-        @test SVector(3.0, 5.0) === @inferred setproperties(sv, x = 3.0, y = 5.0)
-        @test SVector(-1.0, -2.0) === @inferred setproperties(sv, data = (-1.0, -2))
-        @test_throws "does not have properties z" setproperties(sv, z = 3.0)
-        @test_throws "does not have properties z" setproperties(SVector(1, 2, 3, 4, 5), z = 3.0)
+@testset "staticarrays" begin
+    sa = @SVector [2, 4, 6, 8]
+    sa2 = ConstructionBase.constructorof(typeof(sa))((3.0, 5.0, 7.0, 9.0))
+    @test sa2 === @SVector [3.0, 5.0, 7.0, 9.0]
+
+    ma = @MMatrix [2.0 4.0; 6.0 8.0]
+    ma2 = @inferred ConstructionBase.constructorof(typeof(ma))((1, 2, 3, 4))
+    @test ma2 isa MArray{Tuple{2,2},Int,2,4}
+    @test all(ma2 .=== @MMatrix [1 3; 2 4])
+
+    sz = SizedArray{Tuple{2,2}}([1 2;3 4])
+    sz2 = @inferred ConstructionBase.constructorof(typeof(sz))([:a :b; :c :d])
+    @test sz2 == SizedArray{Tuple{2,2}}([:a :b; :c :d])
+    @test typeof(sz2) <: SizedArray{Tuple{2,2},Symbol,2,2}
+
+    for T in (SVector, MVector)
+        @test @inferred(ConstructionBase.constructorof(T)((1, 2, 3)))::T == T((1, 2, 3))
+        @test @inferred(ConstructionBase.constructorof(T{3})((1, 2, 3)))::T == T((1, 2, 3))
+        @test @inferred(ConstructionBase.constructorof(T{3})((1, 2)))::T == T((1, 2))
+        @test @inferred(ConstructionBase.constructorof(T{3, Symbol})((1, 2, 3)))::T == T((1, 2, 3))
+        @test @inferred(ConstructionBase.constructorof(T{3, Symbol})((1, 2)))::T == T((1, 2))
+        @test @inferred(ConstructionBase.constructorof(T{3, X} where {X})((1, 2, 3)))::T == T((1, 2, 3))
+        @test @inferred(ConstructionBase.constructorof(T{3, X} where {X})((1, 2)))::T == T((1, 2))
+        @test @inferred(ConstructionBase.constructorof(T{X, Symbol} where {X})((1, 2, 3)))::T == T((1, 2, 3))
     end
 
-    @testset "intervalsets" begin
-        @test constructorof(typeof(1..2))(0.5, 1.5) === 0.5..1.5
-        @test constructorof(typeof(OpenInterval(1, 2)))(0.5, 1.5) === OpenInterval(0.5, 1.5)
-        @test setproperties(1..2, left=0.0) === 0.0..2.0
-        @test setproperties(OpenInterval(1.0, 2.0), left=1, right=5) === OpenInterval(1, 5)
-    end
+    sv = SVector(1, 2)
+    @test SVector(3.0, 2.0) === @inferred setproperties(sv, x = 3.0)
+    @test SVector(3.0, 5.0) === @inferred setproperties(sv, x = 3.0, y = 5.0)
+    @test SVector(-1.0, -2.0) === @inferred setproperties(sv, data = (-1.0, -2))
+    @test_throws "does not have properties z" setproperties(sv, z = 3.0)
+    @test_throws "does not have properties z" setproperties(SVector(1, 2, 3, 4, 5), z = 3.0)
+end
+
+@testset "intervalsets" begin
+    @test constructorof(typeof(1..2))(0.5, 1.5) === 0.5..1.5
+    @test constructorof(typeof(OpenInterval(1, 2)))(0.5, 1.5) === OpenInterval(0.5, 1.5)
+    @test setproperties(1..2, left=0.0) === 0.0..2.0
+    @test setproperties(OpenInterval(1.0, 2.0), left=1, right=5) === OpenInterval(1, 5)
 end