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

Should fieldnames return a tuple rather than an array? #25327

Closed
nalimilan opened this issue Dec 29, 2017 · 11 comments
Closed

Should fieldnames return a tuple rather than an array? #25327

nalimilan opened this issue Dec 29, 2017 · 11 comments

Comments

@nalimilan
Copy link
Member

nalimilan commented Dec 29, 2017

It would sound natural to me for fieldnames to return a tuple. The current implementation returning an array implies an allocation even in cases where it could be avoided.

This could have more visible consequences now that we have NamedTuple, for which it is more common to extract the field names since they cannot be known in advance. EDIT: this comment doesn't seem accurate (see below)

@vtjnash
Copy link
Member

vtjnash commented Dec 29, 2017

Reflection on fieldnames is discouraged. (for NamedTuple, the keys should be extracted with the keys function).

@yurivish
Copy link
Contributor

Reflection on fieldnames is discouraged.

Could you clarify what you mean here? I'm not sure if you meant something specific to NamedTuples or if it's discouraged to use fieldnames for metaprogramming in general.

@nalimilan
Copy link
Member Author

Conversely, what are typical use cases for fieldnames?

@smldis
Copy link

smldis commented Dec 29, 2017

The small story about this is that the issue is coming from slack, a user posted it as a note and there where some thumbs up and no thumbs down. I would rather let some core developer discuss the consecuences (and tradeoffs) of a deprecation for 0.7 and a switch for 1.0 or for the next major api break.
Please don't be noisy on the topic since it is difficult to take care of such a breaking change at this precise time.

@andyferris
Copy link
Member

Reflection on fieldnames is discouraged.

If that's true in general (not specifically for named tuples) then we shouldn't be exporting it from Base, should we?

@vtjnash
Copy link
Member

vtjnash commented Dec 30, 2017

We export lots of reflection toys from Base. I suppose we could hide them under some sort of Reflection module, but many of them (isbits, names, fieldoffset, etc.) are pretty useful, and all are pretty varied.

(For NamedTuples, this is also just an artifact of their current implementation, not part of their stable API)

@yurivish
Copy link
Contributor

yurivish commented Dec 30, 2017

The reflection tools are very useful for building good tooling; it would be sad to see them go away.

Would maybe marking them "unstable" suffice, if the concern is that there are rough edges that would require breaking changes in a 1.x?

@nalimilan
Copy link
Member Author

These considerations don't seem to directly address the issue at hand, do they? What do we expect people to use fieldnames for exactly?

@smldis
Copy link

smldis commented Dec 31, 2017

Looking at master they are mostly used for testing and documentation.
As for other usecases:

julia master (cholmod.jl):

let offset = fieldoffset(C_Sparse{Float64}, findfirst(name -> name === :stype, fieldnames(C_Sparse{Float64})))

gadfly (chain method)

for name in fieldnames(Data)
    #more code
    getfield(data, name)
    #more code

So @vtjnash a Reflection module would be a nice occasion to put some engineering love in the julia reflection toolbox.
Is this feasible given the release and man power constraints, before or after 1.0?

The toolbox for now is composed by more than just: isbits, names, fieldoffset, fieldname,fieldnames, getfield, methods, methodswith, subtypes

The currently exported methods list is not structured ordered by usecase/functionality (the work could start from here right?)

# types
    convert,
    # getproperty,
    # setproperty!,
    fieldoffset,
    fieldname,
    fieldnames,
    fieldcount,
    isconcrete,
    oftype,
    promote,
    promote_rule,
    promote_type,
    subtypes,
    instances,
    supertype,
    typeintersect,
    typejoin,
    widen,

#= ... =#

# help and reflection
    apropos,
    edit,
    code_typed,
    code_warntype,
    code_lowered,
    code_llvm,
    code_native,
    fullname,
    functionloc,
    isconst,
    isinteractive,
    less,
    method_exists,
    methods,
    methodswith,
    module_name,
    module_parent,
    names,
    varinfo,
    versioninfo,
    which,
@isdefined,

If this is the case, I'd link the discussion to #20555.
If it is not this issue is still addressable by finding an answer to the issue title.

@JeffBezanson
Copy link
Member

I do think it would make sense for fieldnames to return a constant, immutable container. It tends to be small, and it would be weird to mutate it, so we might as well avoid the allocation.

@nalimilan
Copy link
Member Author

Opened #25725.

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

6 participants