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

Base.@irrational should be warned to not be used outside Base and should maybe be redesigned #46531

Closed
KristofferC opened this issue Aug 29, 2022 · 2 comments · Fixed by #47103

Comments

@KristofferC
Copy link
Member

KristofferC commented Aug 29, 2022

Base.@irrational creates types of Irrational{:sym}. This means that there is no way to namespace new irrational values, they all share the same symbol slot in the Irrational type parameter.

This is a problem for packages that try to define new irrational numbers. If two packages define numbers with the same name but different values, it is undefined what value will actually be used which can be a quite severe bug.

I think we should state in the docstring that packages should not use this macro since it is currently so brittle.

It is also probably a good idea to think about a design that does not have this issue. Something with an abstract super type AbstractIrrational and having the macro create a new type that is a subtype of that seems like it should be workable:

abstract type AbstractIrrational end

struct Euler <: AbstractIrrational end
const euler = Euler()
@eval BigFloat(::Euler) = $(exp(big(1)))
Float64(::Euler) = 2.71828182845904523536

and then have methods defined on AbstractIrrational.

@devmotion
Copy link
Contributor

I tried to implement such a macro that creates subtypes of AbstractIrrational: JuliaMath/IrrationalConstants.jl#19 It's heavily based on the implementation of Base.@irrational and still needs more tests but at least simple tests already pass.

@KristofferC
Copy link
Member Author

Great initiative!

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

Successfully merging a pull request may close this issue.

2 participants