Add compiler warning for nil check on non-nilable class type?

[BryantLam]

Regarding nilability, should the compiler emit a warning that the nil-check is unnecessary? Maybe there's a reason I'm missing that the nil could e.g., be propagated somewhere despite the non-nilable class type.

class MyClass {}

proc main() {
    var c = new owned MyClass();

    if c != nil {
        writeln("not nil");
    }
}

[bradcray]

I believe you're right that the compiler could determine that the nil check is not necessary here. The type of c should be inferred to be MyClass so should be known never be nil.

[LouisJenkinsCS]

Why not just compile it away since this can be statically proven to always be true? I do not believe a warning is necessary, it's more "information" FYI type of information.

[bradcray]

Compile the conditional check away you mean and just leave the body? I think it could do that as well. Doing either of these things could be as simple as adding overloads along the lines of:

proc ==(c: object, d: _nilType) param {
  compilerWarning("Why do you even bother?");
  return false;
}

proc !=(c: object, d: _nilType) param {
  compilerWarning("Don't you have better things to test?");
  return true;
}

(and the reverse-order overloads as well).

[bradcray]

(One reason to potentially not issue the warning is that there may be generic programming situations where the warning wouldn't always occur, but would for certain instantiations, which would make it difficult to do much about other than to litter your code with more checks to stop the warnings (like if (c.type.notNilable() || c != nil) then ...).

[vasslitvinov]

We compare non-nilable things against nil in our domain maps. In the two cases below that I have seen -- Block and Cyclic -- the arrays are declared with non-nilable element types, however this is a (currently undetected) violation of typing rules because those arrays are initialized outside of the initializer:

proc CyclicDom.setup() {
  if locDoms(dist.targetLocDom.low) == nil {    // <----------- here
    coforall localeIdx in dist.targetLocDom {
      on dist.targetLocs(localeIdx) do
        locDoms(localeIdx) = new unmanaged LocCyclicDom(rank, idxType, dist.getChunk(whole, localeIdx));
    }
  } else {
    ...........
  }
}

// Analogously for BlockDom

There are also a couple of places in the internal modules. Just for the record - these probably should not affect the design:

////// _array._instance aka _value is commonly (always?) non-nilable

  inline proc =(ref a: [], b:[]) {
    if a.rank != b.rank then
      compilerError("rank mismatch in array assignment");

    if b._value == nil then                  // <-----------
      // This happens e.g. for 'new' on a record with an array field whose
      // default initializer is a forall expr. E.g. arrayInClassRecord.chpl.
      return;

    ........
  }

////// defaultDist._instance aka _value is of a non-nilable type.

  pragma "locale private"
  var defaultDist = new dmap(new unmanaged DefaultDist());

  proc chpl_defaultDistInitPrivate() {
    if defaultDist._value==nil {             // <-----------
      // FIXME [.......]
      const nd = new dmap(new unmanaged DefaultDist());
      __primitive("move", defaultDist, chpl__autoCopy(nd.clone()));
    }
  }

////// 'x' is a non-nilable formal

  // this version handles downcast to nilable unmanaged
  inline proc _cast(type t:unmanaged class?, x:borrowed class)
    where isProperSubtype(_to_nonnil(_to_borrowed(t)),x.type)
  {
    if x == nil {                           // <-----------
      return nil;
    }
    var tmp = __primitive("dynamic_cast", t, x);
    return _to_nilable(_to_unmanaged(tmp));
  }

To me intuitively, (a non-nilable class) == nil is in the same bucket as (a uint) < 0 and if false then ... . Currently we do not warn for these.