I'm happy to say I think I've found the cause of many a spurious NotImmutableErr!

The following func, despite being clearly immutable, throws a NotImmutableErr:

class Example {
  Void main() {
    obj := "wotever"
    if (false)
      obj = "wotever"
    func := |->| { obj.toStr }
    func.toImmutable
  }
}

sys::NotImmutableErr: Func
  fan.sys.Func.toImmutable (Func.java:34)
  fan.sys.FanObj.toImmutable (FanObj.java:122)
  afIoc::Example.main (Example.fan:8)

The Err is caused by the func referencing the wrapper object created by the second re-assignment. For the following works just fine:

Void main() {
  obj := "wotever"
  func := |->| { obj.toStr }
  func.toImmutable
}

(For those who don't know about wrapper objects, try debugging the program in F4 and inspecting the obj variable. Just make sure you have debug=true set in %FAN_HOME%/etc/sys/config.props)

Being a heavy user of immutable funcs(*) I've encountered a couple of these annoying NotImmutableErrs in the past. Only the offending code was always buried too deep to reproduce it in a simple test case. Instead, I found if I re-jigged the code a bit, it often sorted itself out!

But now I know what causes it and how to work around it! Yippee! I'm very happy!

(*) I know Brian hates me for this! :D

Promoted to ticket #2270 and assigned to brian

Interesting - checking the types of those variables you capture in the closure is little tricky. But I think in that case the type of obj is clearly the immutable class Str so it should work

Ticket cancelled

Actually I think I have it correct as it is now. This works is essentially the same as Java - as long as you only assign a variable once at declaration time its "effectively final". So we can mark the closure as immutable. But if the compiler detects you are assigning it after declaration, then its "effectively non-final" and I assume the variable might mutate after the closure has been created. You could potentially try and analyze the code paths, but that would be extremely difficult. So I think the effectively final is good way to keep it (especially now that Java has chosen the same approach)

Yeah, that seems fair enough. I see that whilst the Str itself is immutable, the reference to it is not. And java uses the final keyword for variables for this very scenario.

But I will ask, if at the time the NotImmutableErr is raised, there is any more contextual information that could be put into the description? For this Err is very subtle and not easy to spot. More information in the Err would be very useful.

I think:

A path analysis through any method can pretty easily determine whether a local captured by a closure can be mutated after the capture.

Doing this globally would be difficult, but all we need here is to track what happens to a local variable that would not normally escape the method it is created in, and ensure that:

(1) no path that creates the closure is able to assign to the local after the closure is created.

(2) You don't I think even need to require that the local has a compile-time constant value before the closure is created, though that would be true in this example and could be checked by similar path analysis.

Submethod calls don't need to be tracked because they will not capture a reference to the local. (If they do you can treat this as an assignment).

All this local tracking of paths is identical to what is done routinely by any compiler back-end for register assignment?

So: that is the way it looks to me but I'm very happy to be corrected if I'm wrong!

BTW - you can always get round this problem by using a new local:

class Example {
  Void main() {
    obj := "wotever"
    if (false)
      obj = "wotever"
    obj_copy := obj
    func := |->| { obj_copy.toStr }
    func.toImmutable
  }
}

It would be good to have a precise definition of this "single assignment is final" semantics in the documentation as part of a precise definition of what can be made immutable.

back to my imperfect understanding of Desktop.callAsync, which this example remind me of:

In the clock example callAsynch is invoked in the (maybe-non-UI-thread) actor Clock using update as the function passed to the UI for execution on the event loop. So according to the documentation update should be immutable, but according to the definition of immutable it is not (since it is a Func and has not been explicitly made immutable with toImmutable).

So, is this just that in this situation the function passed to callAsync should have immutable behaviour, with no enforcement, or is it that because the actor Clock is constructed in the UI thread it is therefore counted as being there?

BTW - you can always get round this problem by using a new local:

Yup, the work around is easy. I was happy to have identified the cause of the NotImmutableErr so I could work around it!

But the cause is difficult to spot, especially for a new comer (I'd imagine). Mainly because the code that causes the func to be mutable lies outside of the func. Not that I'm the brightest, but it took me a while to work it out.

It would be nice if the Err told us not just what is immutable (in this case, the func) but why!

A path analysis through any method can pretty easily determine...

I think there's a much easier way! Simply have the func check if any of its variable references are Wrappers or not! I assume it does this anyway, which is why it throws the NotImmutableErr.

A Wrapper is not an official class, more the way Fantom internals work. I only spotted them through debugging in F4. If you re-assign a method parameter or, as it seems, a variable later used in a func, then it is wrapped in another object and your code compiles to:

Void main() {
  obj := Wrapper()
  obj.val = "wotever"
  if (false)
    obj.val = "wotever"
  func := |->| { obj.val.toStr }
}

class Wrapper {  
  Obj? val
}

And it is the presence of this wrapper in a func that causes it to be immutable.

Of course, I expect there is a lot more to wrappers than this, and they may not be a suitable method for immutable detection.

Well, I'd expect the wrapper to be generated because of the closure - otherwise it would seem amazingly inefficient for every local updated in a method to go through a wrapper!

The point is that obj could be assigned a value from some future call of func, so it can't be kept local after its reference has "escaped".

In this case func cannot change the value of obj, so the wrapper is not needed, and cleverer compile-time analysis could determine this.

Ticket reopened

But I will ask, if at the time the NotImmutableErr is raised, there is any more contextual information that could be put into the description? For this Err is very subtle and not easy to spot. More information in the Err would be very useful.

Yeah I agree with that. Since we make it a runtime error we should try and make it really easy to figure out. I'll have to look at how that might work, maybe let function customize the NotImmutableErr raised with more explicit error message

all really good feedback slimerdude

Ticket resolved in 1.0.67

The compiler has been enhanced to generate a custom NotImmutableErr message per closure which specifies exactly which local variable is causing the problem.