JuliaLang / julia

The Julia Programming Language
https://julialang.org/
MIT License
44.94k stars 5.42k forks source link

Inconsistent behaviour of `hasproperty` with Modules #47150

Open tecosaur opened 1 year ago

tecosaur commented 1 year ago
Julia 1.8.2, Linux

I feel like the behavior of {has,get}{property,field} is a bit suspect when it comes to Modules. The following example should be sufficiently illustrative.

julia> hasproperty(Main, :DataFrames)
false

julia> getproperty(Main, :DataFrames)
DataFrames

julia> hasfield(Main, :DataFrames)
ERROR: MethodError: no method matching hasfield(::Module, ::Symbol)
Closest candidates are:
  hasfield(::Type, ::Symbol) at reflection.jl:215
Stacktrace:
 [1] top-level scope
   @ REPL[26]:1

julia> getfield(Main, :DataFrames)
DataFrames
jishnub commented 1 year ago

Related: https://github.com/JuliaLang/julia/issues/46857

KristofferC commented 1 year ago

For modules, I think it makes more sense to use isdefined.

oxinabox commented 1 year ago

I feel liked getfield has always been a bit of a pun when used on modules. But while we support it, it kins of seems like we should complete the metaphor with the others from {has,get}{property,field} Or deprecate it and provide an alternative

simeonschaub commented 1 year ago

Or deprecate it and provide an alternative

That's basically what #44231 did and why we now have getglobal. We don't really want to add a loud deprecation warning though since it's too commonly used and the benefits of the change aren't worth the annoyance of a ton of warnings

oxinabox commented 1 year ago

Makes sense. We really need to sort out deprecating warnings that warn only for deprecations that are directly your fault.

IMO we can close this as resolved

ericphanson commented 1 month ago

I don't think this is resolved since getglobal only replaces getfield. The docstring for getglobal recommends getproperty, so getproperty definitely doesn't seem to be deprecated, but getproperty is still in contradiction to hasproperty for modules, so there is still an issue.

ericphanson commented 1 month ago

it is valid to add hasproperty(mod::Module, x::Symbol) = isdefined(mod, x) ? That could at least bring hasproperty and getproperty into alignment.

Or should getproperty be deprecated for modules also? (e.g. at least remove it from the getglobal docstring)

vtjnash commented 1 month ago

Seems valid, though I think it may be isresolved and ispublic?