Regression in passing variable state to OneOf()

[richardbuckle]

EDDI version in which issue is found

3.5.3-b6

Steps to reproduce

1. I found this because the "Bodies to map" script was misbehaving but have managed to isolate it.
2. Pick an unused script such as "Asteroid cracked" and set it to:

{set x to "candidate"}
{OneOf("{x} for mapping")}

Expected

Output is "candidate for mapping".

Observed

Output is "for mapping".

Investigation

This is a regression from the stable branch.

[richardbuckle]

Raising this one to show stopper. Lots of scripts broken.

[Tkael]

Cottle Versioning Guide:

From 1.4.* to 1.5.*

• Cottle function delegates now receive a IReadOnlyList instead of their mutable equivalent.

From 1.5.* to 1.6.*

• All functions should be constructed using methods from Function static class.

From 1.6.* to 2.0.*

• Specialized value classes (e.g. Values.FunctionValue) are deprecated, use Value.From* static construction methods instead (e.g. Value.FromFunction).

FINDING: Cottle function delegates are now in IReadOnlyList format and consequently values must be passed when recursing.

Native .NET functions

The callback you’ll pass to Function takes multiple arguments:

• First argument is always an internal state that must be forwarded to any nested function call ;
• Next arguments are either a list of values (for functions accepting variable number of arguments) or separate scalar values (for functions accepting a fixed number of arguments) received as arguments when invoking the function ;
• Last argument, for non-pure functions only, is a TextWriter instance open to current document output.

Suspected: We are not properly passing the internal state to the nested recursion.

[Tkael]

Cottle source code is available here for reference: https://github.com/r3c/cottle/tree/master/src

[dauthiatull]

I think this is also affecting the docking granted and landing pad report.
{SetState('eddi_context_landing_pad_pad', event.landingpad)} doesnt seem to set the variable to the landing pad number so that the pad report script just reads
landing pad is at , as you enter with the green lights on your right.
there is no direction to the pad

[richardbuckle]

TL:DR we need to revert and ask them to add support for our use case.

Cottle function delegates now receive a IReadOnlyList instead of their mutable equivalent

turned out to be a false lead. The real killer is:

IStore replaced by immutable IContext interface for rendering documents.

It's not just the interface that's immutable, it's the underlying object. What this in fact means is that the IContext we create never receives any changes when local and global variables etc are defined in the script, as used to be the case. This immediately kills the possibility of recursively resolving subexpressions stone dead.

The affected functions are:

• F()
• OneOf()
• Occasionally()
• Transmit()

I have been all over the Cottle codebase, inspected objects in the debugger, and satisfied myself that there's nothing we can do, not even by using "dirty tricks" such as reaching into private members etc. The new evaluation code simply isn't structured in a way that facilitates recursive C# functions (oddly enough, recursive Cottle functions like the factorial() demo in the docs are fine).

In the short term, I believe our best option is to revert the entire change and go back to Cottle 1.4.0.3.

In the medium term, I note their speedy and friendly response to TK's issue r3c/cottle#26, and that was without even mentioning that we are our antecedent are #2 and #3 in the Nuget "top 3 GitHub repositories that depend on Cottle"

So I am hopeful that if we ask nicely and explain our use case, they will hear us.

I will revert the change and cherry-pick in the new tests plus some minor renamings and call-graphs simplifications that I saw. I'm not up for a big refactoring of the ScriptResolver right now: we need to focus on getting to a stable build.

Then I shall write up an issue on the Cottle repo very and make it as open and friendly as possible.
I shall probably give an implementation of OneOf() in Cottle. That will make it clear that it is legitimate and worthy of a native C# implementation.

[richardbuckle]

Pushed branch hotfix/revert-Cottle-Rollbar-from-3.5.3b4 re this.

[jangliss]

I can open a separate issue if it's different, but discovered this evening when doing FSS and identifying planets or moons that are unique/different/special/etc, it skips the adjective. For example "moon 1a is a with an inclined orbit". This is using 3.5.3-b6.

[Tkael]

That's the same issue, but thanks for the report.

[dauthiatull]

should we just uninstall this and install 3.5.3b4? then

[richardbuckle]

should we just uninstall this and install 3.5.3b4? then

@dauthiatull for now, yep, revert to 3.5.3b4. We are currently play-testing a fix.

[richardbuckle]

Fixed in 4007beb

[richardbuckle]

Fixed in 3.5.3-b7.