[Bug]: improper interaction between forwarding and POI causes hysteresis

[DanilaFe]

I no longer have a single source for this claim, but Chapel's general philosophy about POI seems to be that POI functions should only be used when no non-POI functions are available. This prevents users of modules and libraries from modifying the libraries' internal behavior by defining more-specific overloads of internal helpers at instantiation points. See #15948 for a clarification.

This rule is load-bearing. After it was implemented in #16158, #16261 relied on this change to reduce the scope of necessary information required to cache generic instantiations. In particular, the assumption is: if an applicable function F is visible in the scope where another function Caller is defined, then Caller will always call that specific F, regardless of other POI candidates, even if they are more specific. Thus, for function cache entries, we only need to track called fn.s that are not visible in the instantiation's scope. This greatly reduces the work required to do instantiation bookkeeping.

Forwarding violates this assumption. First, note that forwarding fields are "ongoing", in that new methods defined on a forwareded type are always made available to its container. Thus, the following program resolves.

Working program demonstrating tertiary definitions being picked up by forwarding.

module A {
  record Inner {}
}

module B {
  import A.Inner;

  record Outer {
    forwarding var inner: Inner;
  }
}

module C {
  import A.Inner;
  import B.Outer;

  proc main() {
      proc Inner.foo() do return "Inner foo";
      var idk = (new Outer()).foo();
  }
}

Importantly, the compiler seems to consider all candidate methods for a type before moving on to forwarding, including POI candidates. Consider the following program:

module A {
  record Inner {
    proc foo() {
      return 42;
    }
  }
}

module B {
  import A.Inner;

  record Outer {
    forwarding var inner: Inner;
  }
}

module C {
  import B.Outer;

  record Outermost {
    forwarding var outer: Outer;
  }
}

module D {
  import A.Inner;
  import B.Outer;
  import C.Outermost;

  proc generic(x) {
    writeln(x.foo());
  }

  proc case1() {
    generic(new Outermost());
  }

  proc case2() {
    proc Outer.foo() do return "Outer foo";
    generic(new Outermost());
  }

  proc case3() {
    generic(new Outermost());
  }

  proc main() {
    case1();
    case2();
    case3();
  }
}

Here, there is a transitive forwarding chain, Outermost -> Outer -> Inner. The order in which candidates are checked is as follows:

• non-POI candidates for Outermost.
• POI candidates for Outermost.
• non-POI candidates for Outer.
• POI candidates for Outer.
• non-POI candidates for Inner
• POI candidates for Inner.

Notably, this allows a POI candidate for Outer to precede the non-POI candidate for Inner. This violates the property that POI candidates are always considered after non-POI canidates, which is required for instantiation caching to work. As a result, we can introduce order-sensitive cache entries. The above program prints 42 three times, invoking the Inner.foo overload. This is because the first call to generic uses the non-POI candidate acquired via transitive forwarding. By the (violated) assumption, the compiler can then re-use this instantiation for generic for case1 and case2. If I comment out case1, the compiler doesn't have a cache entry when resolving the call to generic in case2. Since POI candidates for Outer are checked earlier than non-POI candidates for Inner, without the cache, case2 resolves to a call to Outer.foo, which prints "Outer foo". This is then cached, and re-used in case3. The net result is that the program prints "Outer foo" twice, changing the output of case2 and case3 simply by removing an unrelated function call.

[DanilaFe]

In addition to the fact that properly implementing these rules requires heavyweight changes to function caching, I believe the current approach is against the spirit of our POI rules.

One reason we settled on the current POI rules (#15948) is that we wanted to prevent users from being able to modify the behavior of library code. Specifically, see:

module Lib {
  proc helper(x: integral) {}
  proc libFn(x) do helper(x);
}

module User {
  use Lib;
  libFn(42);
}

Even if the user defines a more specific procedure helper, which would be available to the call to libFn as a POI candidate, the goal of the existing POI rules is that libFn continues to behave as it did before:

module Lib {
  proc helper(x: integral) {}
  proc libFn(x) do helper(x);
}

module User {
  use Lib;
  proc helper(x: int) {return "I am not called because users shouldn't be able to modify a library's impl"; }
  libFn(42);
}

I believe that preferring POI candidates for Outer before considering non-POI candidates for Inner violates this behavior, and allows users to override "closed" implementations. This is visible in my example program, where, even though the Outermost type is defined and normally forwards the Inner.foo method (which yields 42), the user of the library can override this behavior by defining an overload of foo on Outer, causing Outermost to call a different method and even changing the return type to a string. I believe this is precisely the sort of thing that the design of POI priority rules was meant to avoid.