Boxing struct as ref parameter sometimes skipped.

[iskiselev]

Looks like JSIL in translation doesn't box value sometimes. You can check it on AsyncAwait.cs.js (JSIL translation of test AsyncAwait.cs). Here is disasemlby from dotPeek:

      void IAsyncStateMachine.MoveNext()
      {
        string result;
        try
        {
          bool flag = true;
          TaskAwaiter awaiter;
          switch (this.\u003C\u003E1__state)
          {
            case -3:
              goto label_9;
            case 0:
              awaiter = this.\u003C\u003Eu__\u0024awaiter3;
              this.\u003C\u003Eu__\u0024awaiter3 = new TaskAwaiter();
              this.\u003C\u003E1__state = -1;
              break;
            case 1:
              awaiter = this.\u003C\u003Eu__\u0024awaiter3;
              this.\u003C\u003Eu__\u0024awaiter3 = new TaskAwaiter();
              this.\u003C\u003E1__state = -1;
              goto label_7;
            default:
              awaiter = this.task1.GetAwaiter();
              if (!awaiter.IsCompleted)
              {
                this.\u003C\u003E1__state = 0;
                this.\u003C\u003Eu__\u0024awaiter3 = awaiter;
                this.\u003C\u003Et__builder.AwaitUnsafeOnCompleted<TaskAwaiter, TestClass.\u003CAsyncMethod\u003Ed__2>(ref awaiter, ref this);
                flag = false;
                return;
              }
              else
                break;
          }
          awaiter.GetResult();
          awaiter = new TaskAwaiter();
          Console.WriteLine("After delay 1");
          awaiter = this.task2.GetAwaiter();
          if (!awaiter.IsCompleted)
          {
            this.\u003C\u003E1__state = 1;
            this.\u003C\u003Eu__\u0024awaiter3 = awaiter;
            this.\u003C\u003Et__builder.AwaitUnsafeOnCompleted<TaskAwaiter, TestClass.\u003CAsyncMethod\u003Ed__2>(ref awaiter, ref this);
            flag = false;
            return;
          }
label_7:
          awaiter.GetResult();
          awaiter = new TaskAwaiter();
          Console.WriteLine("After delay 2");
          result = "AsyncMethod result";
        }
        catch (Exception ex)
        {
          this.\u003C\u003E1__state = -2;
          this.\u003C\u003Et__builder.SetException(ex);
          return;
        }
label_9:
        this.\u003C\u003E1__state = -2;
        this.\u003C\u003Et__builder.SetResult(result);
      }

And here is JSIL translation:

  function $lAsyncMethod$gd__2_MoveNext ($exception) {
    var taskAwaiter = new JSIL.BoxedVariable(null);

    var $label1 = 0;
  $labelgroup1: 
    while (true) {
      switch ($label1) {
        case 0: /* $entry1 */ 
          try {

            var $label0 = 0;
          $labelgroup0: 
            while (true) {
              switch ($label0) {
                case 0: /* $entry0 */ 
                  switch (this.$state) {
                    case -3: 
                      $label1 = 1 /* goto IL_140 */ ;
                      continue $labelgroup1;

                    case 0: 
                      taskAwaiter.set(this.$);
                      this.$ = new ($T01())();
                      this.$state = -1;
                      $label0 = 1 /* goto IL_91 */ ;
                      continue $labelgroup0;

                    case 1: 
                      taskAwaiter.set(this.$);
                      this.$ = new ($T01())();
                      this.$state = -1;
                      $label0 = 2 /* goto IL_103 */ ;
                      continue $labelgroup0;

                  }
                  taskAwaiter.set($S00().CallVirtual("GetAwaiter", null, this.task1));
                  if (!taskAwaiter.get().get_IsCompleted()) {
                    this.$state = 0;
                    this.$ = taskAwaiter.get();
                    this.$builder.AwaitUnsafeOnCompleted$b2($T01(), $thisType)(/* ref */ taskAwaiter, new JSIL.BoxedVariable(this));
                    return;
                  }

                  $label0 = 1 /* goto IL_91 */ ;
                  continue $labelgroup0;
                case 1: /* IL_91 */ 
                  taskAwaiter.get().GetResult();
                  taskAwaiter.set(new ($T01())());
                  $T04().WriteLine("After delay 1");
                  taskAwaiter.set($S00().CallVirtual("GetAwaiter", null, this.task2));
                  if (!taskAwaiter.get().get_IsCompleted()) {
                    this.$state = 1;
                    this.$ = taskAwaiter.get();
                    this.$builder.AwaitUnsafeOnCompleted$b2($T01(), $thisType)(/* ref */ taskAwaiter, /* ref */ this);
                    return;
                  }

                  $label0 = 2 /* goto IL_103 */ ;
                  continue $labelgroup0;
                case 2: /* IL_103 */ 
                  taskAwaiter.get().GetResult();
                  taskAwaiter.set(new ($T01())());
                  $T04().WriteLine("After delay 2");
                  break $labelgroup0;

              }
            }
          } catch ($exception) {
            this.$state = -2;
            this.$builder.SetException($exception);
            return;
          }

          $label1 = 1 /* goto IL_140 */ ;
          continue $labelgroup1;
        case 1: /* IL_140 */ 
          this.$state = -2;
          $S01().CallVirtual("SetResult", null, this.$builder, "AsyncMethod result");

          break $labelgroup1;
      }
    }
  };

So, here is a problem:

170: this.$builder.AwaitUnsafeOnCompleted$b2($T01(), $thisType)(/* ref */ taskAwaiter, new JSIL.BoxedVariable(this));
184: this.$builder.AwaitUnsafeOnCompleted$b2($T01(), $thisType)(/* ref */ taskAwaiter, /* ref */ this);

There is no boxing on second call.

[iskiselev]

Due to this problem in #367 I've written workaround like:

26: awaiter = JSIL.BoxedVariable.$Is(awaiter) ? awaiter.get() : awaiter;

[iskiselev]

Here is very simple test case:

using System;

public static class Program {
    public static void Main (string[] args) {
        new Struct().SomeMethod();
    }

    public static void Method(ref Struct str)
    {
        Console.WriteLine(str.Field);
    }
}

public struct Struct
{
    public int Field;

    public void SomeMethod()
    {
        Program.Method(ref this);
        Program.Method(ref this);
    }
}

So, second pass ref this from struct reproduce error,

[iskiselev]

I found root cause of problem. There is next code that do JSExpression child replacement:

        public override void ReplaceChild (JSNode oldChild, JSNode newChild) {
            ... //Some checks skipped
            var expr = (JSExpression)newChild;

            for (int i = 0, c = Values.Length; i < c; i++) {
                if (Values[i] == oldChild)
                    Values[i] = expr;
            }
        }

So, this code use == operator for test of equality of old and child expression, but operator == doesn't overridden in JSNode or JSExpression, so it is simple ref equals. So, here should be used Values[i].Equals(oldChild) or operator == should be overridden. I suppose that same error presrve in other JSNode derived type (and maybe it is not single method with such error).

Source of this call is IntroduceVariableReferences.VisitNode, that use HashSet for storing expressions for replace (and hashset compare them with equals, so only one expression would be replaced).

[kg]

ReplaceChild is intended to use referential equality; none of the node types override ==. I hadn't thought about the fact that HashSet uses Equals, though... good catch.

[iskiselev]

So fix here will be something like:

protected readonly HashSet<JSPassByReferenceExpression> ReferencesToTransform = new HashSet<JSPassByReferenceExpression>(new ReferenceComparer<JSPassByReferenceExpression>());

Just a note: you define ReferenceComparer class twice, in JSIL.Threading and JSIL.Internal namespaces.

[kg]

Yeah, sounds right. Thanks for pointing out that there are two of them... wonder how that happened.

[iskiselev]

@kg, will you fix it yourself (maybe you want check same error in other places - you know better if you should) or should I prepare pull request myself?

[iskiselev]

Here is list of potential dangerous usage of HashSet (first column - class name where Equals defined):

JSOperator      Transforms\IntroduceEnumCasts.cs(41):            LogicalOperators = new HashSet<JSOperator>() {  
JSOperator      Transforms\IntroduceEnumCasts.cs(47):            BitwiseOperators = new HashSet<JSOperator>() {
JSVariable      Transforms\IntroduceVariableDeclarations.cs(11):        public readonly HashSet<JSVariable> ToDeclare = new HashSet<JSVariable>();
JSVariable      Transforms\IntroduceVariableDeclarations.cs(13):        public readonly HashSet<JSVariable> SeenAlready = new HashSet<JSVariable>();
JSVariable      Transforms\IntroduceVariableDeclarations.cs(14):        public readonly HashSet<JSVariable> CouldntDeclare = new HashSet<JSVariable>();
JSExpression    Transforms\IntroduceVariableReferences.cs(17):        protected readonly HashSet<JSPassByReferenceExpression> ReferencesToTransform = new HashSet<JSPassByReferenceExpression>();
JSVariable      Transforms\StaticAnalysis\EliminateSingleUseTemporaries.cs(17):        public readonly HashSet<JSVariable> EliminatedVariables = new HashSet<JSVariable>();
JSVariable      Transforms\StaticAnalysis\OptimizeArrayEnumerators.cs(16):        private readonly HashSet<JSVariable> EnumeratorsToKill = new HashSet<JSVariable>();
JSVariable      Transforms\StaticAnalysis\StaticAnalyzer.cs(258):                    var rightVars = new HashSet<JSVariable>(boe.Right.SelfAndChildrenRecursive.OfType<JSVariable>());

Maybe it will be a good idea to check all Dictionaries usage in same way (I searched "new HashSet" through solution, looking only on hash set without explicit comparator if class (or its parent) override Equals).

[iskiselev]

Here is same list for Dictionary:

JSVariable              JavascriptAstEmitter.cs(36):        protected readonly Dictionary<JSClosureVariable, string> AssignedClosureVariableNames = new Dictionary<JSClosureVariable, string>();
GenericTypeIdentifier   Transforms\CacheTypeExpressions.cs(30):            CachedTypes = new Dictionary<GenericTypeIdentifier, CachedTypeRecord>();
*  Transforms\IntroduceEnumCasts.cs(21):        public static readonly Dictionary<JSAssignmentOperator, JSBinaryOperator> ReverseCompoundAssignments = new Dictionary<JSAssignmentOperator, JSBinaryOperator> {
*  Transforms\SimplifyOperators.cs(17):        public readonly Dictionary<JSBinaryOperator, JSBinaryOperator> InvertedOperators = new Dictionary<JSBinaryOperator, JSBinaryOperator> {
*  Transforms\SimplifyOperators.cs(25):        public readonly Dictionary<JSBinaryOperator, JSAssignmentOperator> CompoundAssignments = new Dictionary<JSBinaryOperator, JSAssignmentOperator> {
*  Transforms\SimplifyOperators.cs(30):        public readonly Dictionary<JSBinaryOperator, JSUnaryOperator> PrefixOperators = new Dictionary<JSBinaryOperator, JSUnaryOperator> {
VariableCacheKey        Transforms\StaticAnalysis\AllocationHoisting.cs(68):            new Dictionary<VariableCacheKey, JSRawOutputIdentifier>();
RetargetKey             StaticAnalysis\EliminatePointlessRetargeting.cs(62):            SeenRetargetsInScope.Push(new Dictionary<RetargetKey, int>());
RetargetKey             StaticAnalysis\EliminatePointlessRetargeting.cs(74):            SeenRetargetsInScope.Push(new Dictionary<RetargetKey, int>());

With * I marked places, which looks safe for me - it is dictionary inited with predefined values and use only to read through other code.

[kg]

Thanks, I can go through and fix most of those. The async await test case was easy to fix, already got to that.

[kg]

Fixing all of those is actually quite hairy. :|