nilable list can not be compared to nil

[agustech]

I have opened a ticked (#190) because this throws an unexpected (at least to me) exception

Code: Select all

class NilableListTest
   def main is shared
      ls = List<of int>?()
      if not ls == nil
         Console.writeLine("ok!")
      else
         ls = nil

UPDATE: you need to do it this way to make it work correctly:
ls as IList<of int>? = List<of int>()

so i have marked the ticket as invalid

[hopscc]

Hmm - thats interesting ( and surprising to me too)

NullReferenceException: Object reference not set to an instance of an object.

yes ?

I think thats still a bug - your workaround using explicit IList typing just switches the runtime to a different codepath that doesnt exhibit it...

I think theres some leakage between cobras notions of nilable and c#'s handling as implemented in the RT lib.
Adding null checks in Native.cs CobraImp.Equals(IList a, IList b) corrects for the surprise.

An equally valid ( and perhaps more cobra-ish) code workaround is to test for null with

Code: Select all

   ls = List<of int>?()
   if  ls
      Console.writeLine("ok!")


[agustech]

The exception raises at:
Sources/Cobra.Lang/Native.cs line 232

Code: Select all


   static public bool Equals(IList a, IList b) {
->   if (a.Count!= b.Count)
         return false;
      int count = a.Count;
      for (int i=0; i<count; i++) {
         if (!CobraImp.Equals(a[i], b[i]))
            return false;
      }
      return true;
   }


A quick hack would be to check for null before trying to get b.Count

[hopscc]

Heres a qandD patch that makes it work ( for Lists and Dicts at least - no tests and not run thru a full test cycle).

A better fix would probably be to check for comparison of nilable or nil types and vector to a CobraCore wrapper that did the nil checks before
deferring to the existing cobraCore Equals code (but I didnt do that )

[hopscc]

Chuck, Have you any comment on this?

[Charles]

The patch is this:

Code: Select all

if (a == null && b == null)
   return true;
if ((a == null && b != null) || (b == null && a != null))
        return false;

But I believe this would be more efficient and still correct:

Code: Select all

if (a == null) {
   if (b == null) return true;
   else return false;
} else if (b==null) return false;


Right?

Btw I had no idea you could say "x = Foo?()"!! I've been writing "x = Foo() to ?". So that's pretty wild for me to learn something new about Cobra. The first is not covered in the test suite, so we need to decide to keep it or not.

[hopscc]

Yeah thats the first change that made it not croak.

Your mod will save some comparisons but Its probably more efficient to follow the idea behind the comment about 'better fix' which was to only
make the null checks if either of the operands was nil or nillable
- shouldnt be necessary otherwise as the args passed to the RT method couldnt be nil then ( assuming the nil checking/guards in cobra are all good).

[Charles]

Well it looks like Cobra is not currently doing anything more sophisticated than invoking CobraImp.Equals(a,b) and it wouldn't hurt to make those methods more robust. For now I've done the guaranteed fix in changeset:2253. If we want to do an even more efficient approach at a later point, we can.

Btw this was probably never found before because most of us are using the "is" and "is not" operators for reference comparisons; or as hops pointed out, simply "if foo" for checking non-nil.

[agustech]

Too much C# in my brain anyway I think we should keep that syntax valid.

The patch is this:
Btw I had no idea you could say "x = Foo?()"!! I've been writing "x = Foo() to ?". So that's pretty wild for me to learn something new about Cobra. The first is not covered in the test suite, so we need to decide to keep it or not.

[hopscc]

I too think the syntax is/should be valid
its totally orthoganal to using ? for nillity ( ) elsewhere

Code: Select all

 def meth(x as Foo?)