Engine.SetValue with null strings crashes with NullReferenceException since 3.0.0-beta-2045

[christianrondeau]

Problem:

For example, this code:

var engine = new Engine();
engine.SetValue("x", (string)null);

will throw, because SetValue calls JsString.Create directly which doesn't make null checks.

Fix:

I see two potential fixes for that:

1. Make JsString.Create return a JsValue that could either be Null or the JsString, this would always work but may be troublesome
2. Simply add an if statement in Engine.SetValue(string, string) to either set a JsString or null. This is safer but other places may also be affected that I didn't notice.

Since:

3.0.0-beta-2045 (commit 6839807)

In any case the fix is dead simple, I just want to make sure I'm not missing something this change did intentionally.

[lahma]

I guess nullable annotations were expected to be respected to cover this, but of course consuming code can force anything in.

The second option seems reasonable. First option can affect performance and cause changes to be required on callsites.

[christianrondeau]

All right, I'll do that (maybe today). We have Nullable enabled almost everywhere ;)

[christianrondeau]

That was fast, thanks <3