search

jkrumbiegel / GridLayoutBase.jl

MIT License
4 stars 13 forks source link

Type piracy in convert(...)? #59

Open frankier opened 5 days ago

frankier commented 5 days ago

This method:

Base.convert(::Type{SizeAttribute}, r::Real) = Float32(r)

which is the same as

convert(::Type{Union{Nothing, Float32, GridLayoutBase.Auto, GridLayoutBase.Fixed, GridLayoutBase.Relative}}, r::Real)

Seems to be type piracy, perhaps? For nothing, it seems to not make sense. For Float32, it seems to be included in the default definition:

https://github.com/JuliaLang/julia/blob/7a76e32c0e28133c3e229df7009c1eb7a6cc86d5/base/number.jl#L7

It causes invalidations together with this definition from Ratios.jl

https://github.com/timholy/Ratios.jl/blob/f5a7649bf3931088e72673ca8f6f43068b178e6e/src/Ratios.jl#L51

jkrumbiegel commented 4 days ago

Hm yes I agree that looks like piracy. Although Type is not covariant so I thought that using the parameter in this way, with GridLayoutBased owned types in it, it could never appear anywhere else so wouldn't conflict.

Maybe SizeAttribute should become a type instead, it's so long ago that I don't remember why I did it this way. But it's probably possible to change this without being breaking (SizeAttribute is not exported)

jkrumbiegel commented 2 days ago

Maybe pulling @timholy in here (as Ratios.jl is also your package but more because of your expertise with these topics). Are the convert signatures type piracy? The reason I did this was so that an Observable{SizeAttribute} would work correctly, I saw no other way to handle this at the time but to define convert for the union.