IssueTracker

Is this really a P1? This seems less priority than the perf work that I am doing but if it is a P1 I will block that and work on this.

I'm happy to move it to a p2, but it seems like a pretty big correctness issue that we should take a look at in the near future. I'm also happy to help look at this myself if you can give me some pointers on what the issue may be

This is not simple as and will change once Jim lands the threading of the composer.

Sure thing, thanks for the context!

For anyone running into this issue, in the meantime the simple workaround is just consuming the ambient inside the body of the function, you don't explicitly need to use the returned value anywhere.

Jim and I discussed this this morning and the reason it fails is because we skip the calls to composable methods at the call-site the call itself. We restart composition inside the callee. Kotlin lowers the adding of the default parameter into a synthetic method that calls the original method resulting in the ambient read sitting between skipping and restarting. Modifying the ambient restarts the caller's scope but that will then skip the call because the parameters haven't changed (from the callers perspective). If we invalidated the callee's scope composition would restart after inside the original method using the old value of the ambient because the synthetic that encodes the read is skipped.

After Jim's change, we already planned to move the skipping logic into the function being called. We also need to route the $static property (see Leland's ADS talk for a description of what $static is) or we will compare too many values and the slot table will grow (also described in Leland's talk). As part of that lowering we can take over the default value lowering from Kotlin and calculate both the $default and $static sets. This fixes this problem because this would cause the caller to be invalidated and re-execute the call, re-read the ambient, and then call the original function which will skip if the ambient is the same or execute if the ambient value is changed.

Next we plan to change how ambients work. The ambient values should be a map of ambient keys to State objects. Providing an ambient should create a new set with the provided value added. This would allow us to delete almost all of the of the special ambient logic in the composer which will also speed up both reading and updating ambients at the cost of additional overhead tracking the reads.

The combination of the above will cause this bug will go away. Fixing it before these changes would more difficult and would involve much of the work we are already planning.

However, we need a test case that ensures we indeed fix this case. Louis, it would be helpful if you would create a test case in our unit tests that reproduces this (FcsCodeGen would be a good place to add it)? We should add @Ignore to the new test (as it will fail) with a reference to this bug in the description.