replace constructorof with something not UB

[vtjnash]

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).

[rafaqz]

This didn't work out so well in the past:

#55

It would need to come with an assurance that its type stable and new julia versions wont break that. constructorof being type unstable has consequences for a lot of packages.

(constructorof doesn't necessarily return the constructor of T... but some function that will construct an object of T from the properties (usually modified somehow) of another constructed T. Which is often T. But often there is no actual constructor defined that does that, or extra type parameters are needed)

[KristofferC]

There is also UB at

ConstructionBase.jl/src/functions.jl

Line 11 in 72bb19d

T = getfield(parentmodule(F), nameof(F))

.

[vtjnash]

#55 does not appear to have anything at all to do with this, except for pattern matching on the text "generated", as that touches a different function for different reasons

@KristofferC that is good to know too. That one may be harder to make entirely safe, since it exists for the specific purpose of violating the safety of the compiler, but can perhaps have the same change made to it anyways

[rafaqz]

#55 just has the "removing generated function lead to type instability and bug reports" problem.

This generated function was also for performance at some stage - but that may have been driven by wanting to avoid .wrapper internals.

@vtjnash it would just be good to have confirmation that Base.typename(T).wrapper is always type stable.

[rafaqz]

Suggested change

	constructorof(T::Type) = Base.typename(T).wrapper
	constructorof(::Type{T}) where T = Base.typename(T).wrapper

Is there a reason why the where T was dropped as well?

[vtjnash]

In practice, the where just reduces the information inference is permitted to use about the argument during inlining

[rafaqz]

Ok, I guess I don't understand the scope of consequences of that.

From practical experience using where on types and functions like this often fixes type stability bugs.

(The reason for my concern is a small loss of type stability in constructorof could break the performance of 100s of packages)

[aplavin]

Could you please rebase on master? CI was cleaned up there to avoid almost all spurious test failures.

[vtjnash]

It appears that you have that ability disabled in settings

[nsajko]

Bump. On current v1.12 and v1.13, ConstructionBase.jl causes ungodly amounts of deprecation warnings to be printed. Furthermore, after JuliaLang/julia#58651 gets merged, and backported to v1.12, ConstructionBase will be completely broken.

[nsajko]

Also see: JuliaLang/julia#57701

[aplavin]

I guess a new PR with the same content is needed, as we cannot rebase this one and run tests? I have no idea what the issue with rebasing is...

[aplavin]

More generally, restrictions on generated functions are very unclear IMO.

the undefined behavior here of using an unsound generated function (examining mutable state with getfield).

Any generated function that calls any other generic function "examines mutable state" (the method table). Even some examples from Julia docs.

[vtjnash]

Any generated function that calls any other generic function "examines mutable state" (the method table)

Generated function restrictions are hard enough, so please don't just make more things up. The generated function execution world is a constant, which is an equivalent statement to saying that the method table is constant, so there is no mutable state in the method table.

[aplavin]

Then the restriction that generated functions must not use hasmethod seems strange. World age is fixed, so hasmethod result is constant, right?

[vtjnash]

This is not the right place to have this off-topic conversation, as it has not relevance to this PR. But anyways, there is no restriction on calling hasmethod. That was changed from always returning false (which seems of questionable usefulness but was not wrong) to returning a constant answer (which is of questionable value as to why someone would write a generated function which was guaranteed to return a constant), though I think some of the documentation updates were missed in f9e1c6cb5963

[aplavin]

Sorry for derailing, and thanks for clarifying that

there is no restriction on calling hasmethod.

I was just following the docs that say (and has been doing so for years):

Generated functions must not mutate or observe any non-constant global state (including, for example, IO, locks, non-local dictionaries, or using hasmethod).

Btw this PR was rebased by @nsajko as #102.

[jw3126]

Lets close this in favor of #102