RFC: some type reflection improvements #22350

JeffBezanson · 2017-06-13T03:42:28Z

The definition of nfields has been bothering me, since it's a bit ambiguous whether it's telling you the number of fields of the object you pass, or if the object you pass is a type, the number of fields of instances of that type. This leads to some questionable and/or wrong results like:

julia> nfields(Tuple{T,T,T} where T)
2

(Since a UnionAll has 2 fields.)

I think there are ~~two~~ three possible approaches:

Make nfields refer only to the number of fields in the passed object, and add the not-too-beautifully-named type_nfields to get the field count of instances of a given type (when possible). This is what I've implemented here.
Combine both behaviors in nfields. This is similar to what we have now, except any argument that's a Type will give the field count of its instances, or an error if the type is too abstract. The downside is that there'd be no function that would tell you a UnionAll has 2 fields and a DataType has 16 fields. Granted, that is seldom needed, but it's still a bit confusing.
EDIT:
Have nfields accept only types (similar to fieldtype), and use nfields(typeof(x)) for instances.

sizeof has a similar problem, but in practice it's not nearly as bad since it's not useful to know the size in bytes of a Type. Therefore I think sizeof(::Type) can be safely interpreted as being about the type's instances. Here I just added some more errors, and allowed sizeof(::Symbol) since we allow write on symbols.

Other than that there is some simple wrapping of type field access for code in src/.

tkelman · 2017-06-13T12:46:15Z

base/exports.jl

@@ -994,6 +994,7 @@ export
    typeintersect,
    typejoin,
    widen,
+    type_nfields,


should be documented if exported

Also, if you're open to some name bikeshedding, perhaps ntypefields?

Yes I could very much use name suggestions, if folks prefer option 1.

iamed2 · 2017-06-13T21:52:34Z

Another option: have nfields only take types, and only return the number of fields of instances of that type. Then to get the number of fields of the object passed in you would call nfields∘typeof.

iamed2 · 2017-06-13T21:58:45Z

To expand on why I think that is better:

I think that nfields to get number of fields of the object passed in makes it seem like the number of fields is a property of the object itself, rather than its Julia representation.

struct Cat
    cute::Bool

    Cat() = Cat(true)
end

Cats (EDIT: the actual animals) don't have fields, but the Julia type Cat does.

JeffBezanson · 2017-06-13T22:08:29Z

Yes, that's also a good option. The only (very minor) issue with it is that we couldn't have types with variable numbers of fields. We want to have some kind of support for variable-size objects eventually. Now, I believe @yuyichao has a design that uses a fixed number of fields, but where some fields can have variable size, so we don't necessarily need variable field counts. But it's something to consider.

ararslan · 2017-06-13T22:12:02Z

Cats don't have fields

The instances do have fields, otherwise you couldn't access their fields as Cat().cute, for example. (Which is universally true, btw. 😉) That said, presumably the number of fields in an instance of a type is always equal to the number of fields of the type, so the distinction seems unimportant.

iamed2 · 2017-06-13T22:26:03Z

Sorry, I realize that was unclear, I meant actual cats not Cats.

JeffBezanson · 2017-06-13T22:43:07Z

fieldname, fieldnames, and fieldoffset have similar issues. All could be made type-only functions, except fieldoffset if objects can have internal variable-size fields. Maybe we can cross that bridge when we get there.

JeffBezanson · 2017-06-13T22:44:29Z

Hmm, there's a bit of a naming scheme there. Maybe the type-only version of nfields should be renamed fieldcount to go with those other functions?

ararslan · 2017-06-13T22:45:14Z

+1, I was actually going to suggest fieldcount earlier but I forgot

JeffBezanson · 2017-06-14T02:47:20Z

OK, I have used fieldcount for the type function. For now I've kept nfields for instances. It is very very heavily used and is pretty important to inference, so if we want to fully deprecate it to fieldcount(typeof(x)) that will take a bunch more work.

tkelman · 2017-06-16T05:42:08Z

base/exports.jl

@@ -983,6 +983,7 @@ export
    fieldoffset,
    fieldname,
    fieldnames,
+    fieldcount,


should be added to stdlib doc index

tkelman · 2017-06-16T05:43:55Z

base/replutil.jl

@@ -153,7 +153,7 @@ end

 function show(io::IO, ::MIME"text/plain", opt::JLOptions)
    println(io, "JLOptions(")
-    fields = fieldnames(opt)
+    fields = fieldnames(JLOptions)


reminding me that we have two nearly identical versions of this method...

tkelman · 2017-06-30T23:48:43Z

base/inference.jl

        if isType(x)
-            isleaftype(x.parameters[1]) && return Const(nfields(x.parameters[1]))
+            # TODO: remove with deprecation in builtins.c for nfields(::Type)


add a note in deprecated.jl or this will get missed

tkelman · 2017-06-30T23:51:49Z

did news addition get lost in a rebase?

JeffBezanson · 2017-07-01T05:08:21Z

Yep, looks like it. Found the missing commit locally.

tkelman

lgtm

vtjnash · 2017-07-01T17:17:32Z

base/reflection.jl

+"""
+function fieldcount(t::ANY)
+    if t isa UnionAll || t isa Union
+        p = ccall(:jl_argument_datatype, Ptr{Void}, (Any,), t)


would be cleaner to return the Julia sentinel (nothing) rather than the C sentinel

vtjnash · 2017-07-01T17:24:53Z

base/reflection.jl

@@ -265,7 +259,7 @@ false
 ```
 """
 isimmutable(x::ANY) = (@_pure_meta; (isa(x,Tuple) || !typeof(x).mutable))
-isstructtype(t::DataType) = (@_pure_meta; nfields(t) != 0 || (t.size==0 && !t.abstract))
+isstructtype(t::DataType) = (@_pure_meta; length(t.types) != 0 || (t.size==0 && !t.abstract))


the C definition has this same bug

A better definition is probably !abstract && !is_primitive_type.

that would certainly help with reviewing how those definitions intersect :)

vtjnash · 2017-07-01T17:28:35Z

base/show.jl

-            println(io)
-            print(io, indent, "  ", fields[idx], "::")
-            print(tvar_io, fieldtypes[idx])
+        if !x.abstract


can move this to the if check on L1253 with the rest of the similar tests

vtjnash · 2017-07-01T17:31:27Z

base/summarysize.jl

@@ -8,6 +8,7 @@ struct SummarySize
    chargeall::Any
 end

+_nfields(x::ANY) = length(typeof(x).types)


also remove along with deprecation?

vtjnash · 2017-07-01T17:37:03Z

src/builtins.c

-            // names===() and size > 0  =>  bitstype, size always known
-            if (dx->abstract || !jl_is_leaf_type(x))
+            jl_error("type does not have a fixed size");
+        if (!jl_is_primitivetype(dx)) {  // size of a primitive type is always known


this test is redundant with testing dx->layout

vtjnash · 2017-07-01T17:40:50Z

base/inference.jl

@@ -614,12 +614,14 @@ add_tfunc(Core.sizeof, 1, 1,
              x !== DataType && isleaftype(x) && return _const_sizeof(x)


isType(x) && return _const_sizeof(x.parameters[1])

This case isn't valid unless we define that T == S implies sizeof(T) === sizeof(S), which currently is not true. Either we need to use isConstType here (such that we know T == S implies T === S) or work out all of the equivalence classes that could occur (tempting, since we seem to be really close, but probably difficult to get right)

vtjnash · 2017-07-01T17:56:38Z

src/julia.h

@@ -928,7 +935,7 @@ STATIC_INLINE int jl_is_datatype_singleton(jl_datatype_t *d)
 STATIC_INLINE int jl_is_datatype_make_singleton(jl_datatype_t *d)
 {
    return (!d->abstract && jl_datatype_size(d) == 0 && d != jl_sym_type && d->name != jl_array_typename &&
-            d->uid != 0 && (d->name->names == jl_emptysvec || !d->mutabl));
+            d->uid != 0 && (jl_datatype_nfields(d) == 0 || !d->mutabl));


jl_datatype_nfields needs to examine layout, so it's less likely to be valid to use this predicate after this change (wasn't really valid before anyways though). Lets sink it into jltypes.c so that it isn't exported anymore.

vtjnash · 2017-07-01T18:01:17Z

test/reflection.jl

+@test_throws ErrorException sizeof(Union{Complex64,Complex128})
+@test_throws ErrorException sizeof(Union{Int8,UInt8})
+@test_throws ErrorException sizeof(AbstractArray)
+@test_throws ErrorException sizeof(Real)


use

@test_throws(ErrorException("argument is an abstract type; size is indeterminate"), sizeof(Real))

to make sure we aren't just throwing some random error by accident

JeffBezanson · 2017-07-02T04:26:28Z

@vtjnash Good suggestions. I implemented all of them, and also simplified the type instantiation code a bit. We were handling allocating singleton instances in about 3 places; I moved it all to jl_compute_field_offsets.

- deprecate `fieldnames(v)` to disambiguate whether it operates on types or their instances. - similarly, separate `nfields` for instances and `fieldcount` for types - more errors for abstract types in `sizeof` - allow `sizeof(::Symbol)` - use more abstract type property access in src/ - consolidate make_singleton logic to one place

JeffBezanson added the breaking This change will break code label Jun 13, 2017

tkelman reviewed Jun 13, 2017

View reviewed changes

ararslan added the needs docs Documentation for this change is required label Jun 13, 2017

JeffBezanson force-pushed the jb/nfields branch 2 times, most recently from 0bf4ef4 to 2d6c072 Compare June 14, 2017 02:45

JeffBezanson force-pushed the jb/nfields branch from 2d6c072 to ea0dbe3 Compare June 14, 2017 15:46

JeffBezanson added needs news A NEWS entry is required for this change and removed needs docs Documentation for this change is required labels Jun 14, 2017

tkelman reviewed Jun 16, 2017

View reviewed changes

JeffBezanson force-pushed the jb/nfields branch from ea0dbe3 to 0407be2 Compare June 16, 2017 19:03

JeffBezanson added deprecation This change introduces or involves a deprecation and removed needs news A NEWS entry is required for this change labels Jun 16, 2017

JeffBezanson force-pushed the jb/nfields branch from 0407be2 to 97ae4ec Compare June 30, 2017 22:12

tkelman reviewed Jun 30, 2017

View reviewed changes

JeffBezanson force-pushed the jb/nfields branch from 97ae4ec to 8a8575c Compare July 1, 2017 05:08

tkelman approved these changes Jul 1, 2017

View reviewed changes

vtjnash reviewed Jul 1, 2017

View reviewed changes

JeffBezanson force-pushed the jb/nfields branch from 8a8575c to 2d3d6bd Compare July 2, 2017 04:24

stevengj mentioned this pull request Jul 5, 2017

tuple type introspection via indexing/iteration #22687

Closed

matthieugomez mentioned this pull request Jul 6, 2017

nfield(x::DataType) JuliaData/NamedTuples.jl#33

Closed

JeffBezanson force-pushed the jb/nfields branch 2 times, most recently from 817c3b8 to 168733e Compare July 10, 2017 03:49

JeffBezanson force-pushed the jb/nfields branch from 168733e to bd18c6f Compare July 10, 2017 16:47

JeffBezanson merged commit f728d4e into master Jul 10, 2017

JeffBezanson deleted the jb/nfields branch July 10, 2017 21:41

yuyichao mentioned this pull request Jan 4, 2018

nfields works for AbstractTypes but fieldcount does not #25389

Closed

cormullion mentioned this pull request Mar 18, 2018

NEWS.md is getting a bit untidy #26508

Closed

goretkin mentioned this pull request Mar 3, 2020

deprecate f(instance::MyType) or warn on additional method, for f(x) = f(typeof(x)) #34985

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFC: some type reflection improvements #22350

RFC: some type reflection improvements #22350

JeffBezanson commented Jun 13, 2017 •

edited

Loading

tkelman Jun 13, 2017

ararslan Jun 13, 2017

JeffBezanson Jun 13, 2017

iamed2 commented Jun 13, 2017

iamed2 commented Jun 13, 2017 •

edited

Loading

JeffBezanson commented Jun 13, 2017

ararslan commented Jun 13, 2017

iamed2 commented Jun 13, 2017

JeffBezanson commented Jun 13, 2017

JeffBezanson commented Jun 13, 2017

ararslan commented Jun 13, 2017

JeffBezanson commented Jun 14, 2017

tkelman Jun 16, 2017

JeffBezanson Jun 16, 2017

tkelman Jun 16, 2017

tkelman Jun 30, 2017

JeffBezanson Jul 1, 2017

tkelman commented Jun 30, 2017

JeffBezanson commented Jul 1, 2017

tkelman left a comment

vtjnash Jul 1, 2017

vtjnash Jul 1, 2017

JeffBezanson Jul 1, 2017

vtjnash Jul 1, 2017

vtjnash Jul 1, 2017

vtjnash Jul 1, 2017

vtjnash Jul 1, 2017

vtjnash Jul 1, 2017

vtjnash Jul 1, 2017

vtjnash Jul 1, 2017

JeffBezanson commented Jul 2, 2017

		@@ -614,12 +614,14 @@ add_tfunc(Core.sizeof, 1, 1,
		x !== DataType && isleaftype(x) && return _const_sizeof(x)

RFC: some type reflection improvements #22350

RFC: some type reflection improvements #22350

Conversation

JeffBezanson commented Jun 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamed2 commented Jun 13, 2017

iamed2 commented Jun 13, 2017 • edited Loading

JeffBezanson commented Jun 13, 2017

ararslan commented Jun 13, 2017

iamed2 commented Jun 13, 2017

JeffBezanson commented Jun 13, 2017

JeffBezanson commented Jun 13, 2017

ararslan commented Jun 13, 2017

JeffBezanson commented Jun 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkelman commented Jun 30, 2017

JeffBezanson commented Jul 1, 2017

tkelman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeffBezanson commented Jul 2, 2017

JeffBezanson commented Jun 13, 2017 •

edited

Loading

iamed2 commented Jun 13, 2017 •

edited

Loading