Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
frankier opened this issue Sep 26, 2024 · 2 comments
Open

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

frankier opened this issue Sep 26, 2024 · 2 comments

Comments

@frankier
Copy link

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
Copy link
Owner

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
Copy link
Owner

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants