#797 Setting const fields with reflection

ivan Mon 19 Oct 2009

I have the following class:

const class Bar
{
  new make(|This t|? f := null) { f?.call(this) }
  const Int a := 4
}

Then, if I create it like this:

Bar() { a = 5} 

everything works fine, however the following code fails with ReadonlyErr:

Bar() { Bar#a[it] = 5 }

Same for:

Bar() { it.trap("a", [5]) }

So, it seems that it is impossible to write for example custom deserialization of const objects Is there any workaround?

brian Tue 20 Oct 2009

I think this raises an excellent point which sort of got lost in the "great and never ending constructor brainstorming".

What I do today is some compiler magic to have the it-block maintain a flag that can be used to check if we're in a constructor before setting const fields. However, I never enhanced this to include custom deserialization or reflection.

It is critical that reflection not be allowed to set const fields after construction. This would enable all sorts of nasty concurrency issues that Fan tries so hard to control with its type system.

But I would welcome ideas on how to handle this issue. Any proposals?

qualidafial Tue 20 Oct 2009

Use a thread (actor?) local identity set to keep track of all objects with const fields which are under construction. In these class's constructors having at least one Func argument, synthetically generate code to add the obj to the construction list, and to remove it at the end of the constructor. Change the runtime const field assignment check to verify the obj is in the construction list and throw ImmutableErr if not.

ivan Tue 20 Oct 2009

Yes, this compiler magic works when we use direct assignment, if I remove constructor in the example above (so this-block becomes with-block) I see err stack trace like this:

sys::ConstErr: socketTest::Bar
  fan.sys.Func$Indirect1.checkInCtor (Func.java:147)
  socketTest::Main.main (Main.fan:12)
  fan.sys.FanObj.with (FanObj.java:143)
  fan.sys.FanObj.with (FanObj.java:134)

However, when using a reflection error is the folowing:

sys::ReadonlyErr: Cannot set const field socketTest::Bar.a
  fan.sys.Field.set (Field.java:90)
  fan.sys.Field.set (Field.java:79)
  socketTest::Main.main (Main.fan:12)
  fan.sys.FanObj.with (FanObj.java:143)
  fan.sys.FanObj.with (FanObj.java:134)
  socketTest::Main.main (Main.fan:12)

So failure happens in java Field.set, which is always called with checkConst=true. ObjectDecoder calls this method with checkConst=false

Ideally there should be the same mechanism for all three cases above. Also, having public java method which allows setting const fields is potential danger - I can write java code like this:

public class FieldSetter {
  public static void set(Object instance, Object value, String type, String field)   {
        Type t = Type.find(type);
        Field f = t.field(field);
        f.set(instance, value, false);

    }
}

and then use it via JavaFFI to set any const field at any time. I think qualidafial's solution should work - there should be some internal mechanism which allows to know if object's construction in progress. Another idea - to have some const mixin with methods like beginMake, endMake which will do some similar magic

brian Tue 20 Oct 2009

Promoted to ticket #797 and assigned to brian

I don't really have any idea how this should be done.

But I think this is an action item we need to track.

I would say the requirement is this: there must be a way to set const fields via reflection during object construction.

brian Tue 20 Oct 2009

Renamed from Problem with setting const fields with reflection to Setting const fields with reflection

brian Sat 21 Nov 2009

OK, my simple fix was to enhance the existing sys::Type.make with an extra Str:Obj? parameter which was a map of fields to set.

The problem with that approach is that takes away the ability of the constructor to perform its designed construction checks. Furthermore, if the class was designed with const fields and not a ctor that took an it-block, then that probably means you shouldn't be able to use reflection to set those const fields.

So I think the answer is that reflection should just normal Type.make, but that we want some factory which will create an it-block that does the reflective set.

For example:

const class Point
{
  new make(|This| f) { f(this); checkFields() }
  const Int x
  const Int y
}

What I am thinking is something like this:

f := Type.makeFieldSetFunc(["x":3, "y":4])
pt := Point#.make([f])

So basically we are creating a reflective it-block passed to the normal ctor.

What do you guys think of that?

Yuri Strot Sat 21 Nov 2009

Yep, it should work. Just one question: is there any way to reflect custom constructor? For example, if we have:

const class Point
{
  new make(|This| f) { f(this); checkFields() }
  new makeSym(Int x) { this.x = x; this.y = x; checkFields() }
  const Int x
  const Int y
}

I believe Point#.makeSym(5) or something like this should also be possible. Otherwise it still looks incomplete.

ivan Sat 21 Nov 2009

Brian, I think this solution is fine and flexible enough! One note - probably it is better to use Field:Obj? instead of Str:Obj?. Of course, Str:Obj? is more convenient for writing deserialization code and more compact, but does not seem very safe

Yuri, It looks like you can call non-default constructor as any other method via reflection:

fansh> Range#makeExclusive.call(1,2)
1..<2

brian Mon 23 Nov 2009

Yep, it should work. Just one question: is there any way to reflect custom constructor?

Ivan answered this, but to elaborate - constructors are reflected as Methods and calling them creates a new object. So reflection (like the calling syntax) doesn't make a distinction between factory methods and constructors.

Brian, I think this solution is fine and flexible enough! One note - probably it is better to use Field:Obj? instead of Str:Obj?.

That is probably a good idea - in case you have already looked up the Fields or want to use Field literals it will be a little faster.

Yuri Strot Tue 24 Nov 2009

Thanks guys. It didn't work for me before because of my mistake.

constructors are reflected as Methods and calling them creates a new object

That's fine. Anyway when this difference import we can use sys::Slot.isCtor.

brian Thu 26 Nov 2009

Ticket resolved in 1.0.48

I added a new factory method Field.makeSetFunc which lets you create an it-block to pass to a constructor for reflectively setting const fields - changeset.

**
** Construct a function which sets zero or more fields on a target
** object.  The function can be passed to a constructor which
** takes an it-block to reflectively set const fields.  Example:
**
**   const class Foo
**   {
**     new make(|This|? f := null) { f?.call(this) }
**     const Int x
**   }
**
**   f := Field.makeSetFunc([Foo#x: 7])
**   Foo foo := Foo#.make([f])
**
static |Obj| makeSetFunc(Field:Obj? vals)

Yuri Strot Tue 8 Dec 2009

Just to clarify: are there are still no way to create custom |This| function for const classes?

For example, let's extend Foo example:

const class Foo
{
  new make(|This|? f := null) { f?.call(this) }
  new makeCopy(Foo foo, |This|? f := null) { x = foo.x; f?.call(this) }
  This copy(|This|? change) { makeCopy(this, change) }

  const Int x
}

Now, we can do something like this:

foo1 := Foo { x = 10 }
foo2 := foo1.copy { x++ }
echo(foo2.x) // 11

But we can't create such |This| function separately, right?

brian Tue 8 Dec 2009

But we can't create such |This| function separately, right?

No, not today. The problem is that we need some indication about whether to allow a closure to set const fields. Most of the time we want a compile-time error. So the trick is figuring out when to say that a closure should allow setting const fields, but adding additional checks for runtime. Right now only happens for constructors.

We could potentially allow that compiler behavior using a facet, something like:

@allowConstSets
This copy(|This|? change) { ... }

Yuri Strot Tue 8 Dec 2009

Actually, I don't know how but my example compiles and works without any facets.

The problem I mentioned is following:

foo1 := Foo { x = 10 }
foo2 := foo1.copy { x++ } // No problem, foo2.x == 11
f := |Foo f| { f.x++ } // Compile error: Cannot set const field 'x' outside of constructor
foo3 := foo1.copy(f)

brian Tue 8 Dec 2009

Actually that is the correct behavior - the rule was any it-block can set a const field with runtime checking (not necessarily just on ctor) - see docLang

So a non-it-block closure will receive a compile error trying to set a const field. Not perfect, but seems a pragmatic design. We assume it-blocks are used to configure const field, but we assume closures are not and we should error at compile time.

Yuri Strot Tue 8 Dec 2009

Okay thanks. It looks fine.

By the way, there is "if if" misprint in the docLang

Login or Signup to reply.