Chapel: `canResolve` giving compilation error on unbounded ranges.

[krishnadey30]

Steps to Reproduce

Source Code:

use Reflection;
var x = ..6;
var y = ..7;
writeln(canResolve(">",x, y));

Output

error: parallel iteration is not supported over unbounded ranges

[LouisJenkinsCS]

I think this can be fixed by just overloading <, >, and other relational operators on ranges.

[bradcray]

I believe that the compiler's implementation of canResolve() should stop resolving and return false upon hitting compilerError()s (and should probably squash, but continue, for compilerWarnings(). @mppf, what do you think?

Then there's also a bit of a question here in my mind about whether canResolve() should take things like promotion (or coercion?) into effect. I think "yes" is probably the right default answer, but maybe there should be optional flags that let users turn those choices off?

[bradcray]

I think this can be fixed by just overloading <, >, and other relational operators on ranges.

I believe that would work around the issue here, but I think it's a problem that canResolve() would generate an error message rather than returning true / false.

[mppf]

I believe that the compiler's implementation of canResolve() should stop resolving and return false upon hitting compilerError()s (and should probably squash, but continue, for compilerWarnings(). @mppf, what do you think?

On the surface that seems reasonable. However I'd anticipate challenges there. In particular if you try to run canResolve and it instantiates a generic function - then if the compilerWarning is squashed you won't see it when you call the function.

In some ways it reminds me of the issue we faced with errors in standalone iterators. Compilation errors there caused surprising behavior when they were ignored (and just disabled the standalone iterator). It seems ignoring errors would just put us back in this situation when using Reflection for the same pattern.

For these reasons, I'd prefer canResolve does not change error/warning reporting.

However I think it's reasonable for it to take promotion / coercion into account.

[bradcray]

For these reasons, I'd prefer canResolve does not change error reporting.

This seems surprising to me, though granted I haven't used reflection much in other languages, and I think I may be missing some subtlety that you're anticipating. Specifically, I imagine a typical pattern to be:

if (canResolve("myCall", ...)) then
  myCall(...);
else
  workAroundLackOfMyCall();

So for a case like this where the call is resolvable, but only to an overload that was only provided to generate a better user-facing error message than "unresolved call...", it seems odd to me that we wouldn't want to return false rather than the current behavior (which I'd describe as "not only can you not make that call, you can't even ask whether that call can be made without exiting the compiler."). Exiting the compiler seems like pretty bad behavior for a query that may not even know all the signatures it's going to be called with when the user writes their code.

As far as implementation, I have a vague recollection of some code in the resolution module that treats "Are we really resolving" vs. "Are we checking to see whether we can resolve" cases differently, so was imagining that the latter case would cause resolution of compilerError() calls to short-circuit the attempt-to-resolve resolution, returning false ("no, I can't successfully resolve that") while the former case would still resolve it as ever.

One other thought: Maybe the behavior isn't whether or not we hit a compilerError(), but whether or not we hit an overload that was introduced simply to generate an error message (which we sometimes label with a pragma in the internal modules); though I think if this is what we decide, we'd want to make that into a user-facing feature and apply it much more consistently to error overloads. That said, I worry that this approach wouldn't be as general since many times such error cases are handled with param conditionals within otherwise working functions rather than through distinct overloads.

In particular if you try to run canResolve and it instantiates a generic function - then if the compilerWarning is squashed you won't see it when you call the function.

By "squashed" I didn't mean "removed from the AST for all time" but more like "when attempting to be resolved for a canResolve() call, would not generate output to the console." Then, since we'd presumably return true for such a case (yes, we can resolve that without breaking compilation), when the user did call it (in their then branch say), I was imagining they'd still get the warning.

[mppf]

By "squashed" I didn't mean "removed from the AST for all time" but more like "when attempting to be resolved for a canResolve() call, would not generate output to the console." Then, since we'd presumably return true for such a case (yes, we can resolve that without breaking compilation), when the user did call it (in their then branch say), I was imagining they'd still get the warning.

Right, I was pointing out this presents an implementation challenge because it's not how the compiler thinks about these things today.

One other thought: Maybe the behavior isn't whether or not we hit a compilerError(), but whether or not we hit an overload that was introduced simply to generate an error message (which we sometimes label with a pragma in the internal modules); though I think if this is what we decide, we'd want to make that into a user-facing feature and apply it much more consistently to error overloads. That said, I worry that this approach wouldn't be as general since many times such error cases are handled with param conditionals within otherwise working functions rather than through distinct overloads.

I think this is interesting to me.

Going back to your sketch of typical usage:

if (canResolve("myCall", ...)) then
  myCall(...);
else
  workAroundLackOfMyCall();

Now depending on the situation, the person compiling might have created myCall themselves; or it might be something in a library that they have no control over. If they created myCall and it has a type error in it (say), I think it'd be better to report the error immediately. But if it's in a library that they have no control over, and it has a type error in it, they might want to use the workAroundLackOfMyCall path.

[lydia-duncan]

If they created myCall and it has a type error in it (say), I think it'd be better to report the error immediately.

I disagree - I can't really think of a situation where a user would be uncertain about if a call will work and would want canResolve to do anything other than return true or false (except maybe to also throw an error if one occurred so they can catch it and propagate that information, but they can accomplish that by making the call in the else branch if they really need to).

[lydia-duncan]

By asking canResolve, you want to know "yes or no" and be able to respond appropriately to either answer

[mppf]

I disagree - I can't really think of a situation where a user would be uncertain about if a call will work and would want canResolve to do anything other than return true or false (except maybe to also throw an error if one occurred so they can catch it and propagate that information, but they can accomplish that by making the call in the else branch if they really need to).

This is exactly the situation that came up with standalone iterators. The canResolve code is in the library somewhere, and the user is trying to implement something satisfying an optional interface. In Brad's example, the user is trying to implement myCall and the canResolve check for it is in some library somewhere.

Here, if canResolve returns false when the user has a type error in myCall, the user will probably observe that things "work" but are slower than expected (e.g. iterator not parallel, sort not using radix sort, ....). I'd rather have an immediate compilation error in that setting.

[bradcray]

I think it'd be better to report the error immediately

It's not clear to me why it would be better to have it report the error on the canResolve() line rather than the one following it?

I think this is interesting to me.

Here's a (real!) example of why (with a bit more thought) I think this should be done on the calls to compilerError() and not on the "error overload procedure" granularity. Sometimes such error overloads are separate functions, like this one:

  proc chpl_count_help(r:range(?), i)
    where r.boundedType == BoundedRangeType.boundedNone
  {
    compilerError("count operator is not defined for unbounded ranges");
  }

and sometimes such functions are tagged with "compiler generated" or "last resort" pragmas to indicate that they are error overloads and should be user-overridable. But sometimes they're not, as in the case for iteration:

  pragma "no doc"
  iter range.these(param tag: iterKind) where tag == iterKind.standalone &&
                                              !localeModelHasSublocales
  {
    if ! isBoundedRange(this) {
      compilerError("parallel iteration is not supported over unbounded ranges");
    }
   ...

and I think it would be regrettable to have different behavior in these two cases or to require users to take one approach or the other for handling errors (I feel like I've seen cases where either approach seems preferable stylistically).

[mppf]

The fundamental problem here is this - when there is an error in generic code, is it the fault of the generic code author or of the code using it? interfaces/concepts solves that problem neatly. I view anything else with Reflection as part of an interim strategy.

It's not clear to me why it would be better to have it report the error on the canResolve() line rather than the one following it?

We're talking about this:

if (canResolve("myCall", ...)) then
  myCall(...);
else
  workAroundLackOfMyCall();

If the error isn't reported on the canResolve line, canResolve will return false, and the call myCall on the next line won't have the opportunity to report an error.

[lydia-duncan]

Here, if canResolve returns false when the user has a type error in myCall, the user will probably observe that things "work" but are slower than expected (e.g. iterator not parallel, sort not using radix sort, ....). I'd rather have an immediate compilation error in that setting.

That seems like something that perhaps should have been solved with a warning when using the slower version. E.g.

if (canResolve("myCall", ...)) then
  myCall(...);
else {
  if (debugWarning)
    writeln("Warning: using work-around, could not resolve <intended call here>");
  workAroundLackOfMyCall();
}

I don't think it is canResolve's fault that the if/else statement didn't correctly diagnose the problem.

[bradcray]

Three thoughts:

• We could have canResolve() take an optional boolean indicating what the return value should be when hitting compilerError()s so that a user could choose which behavior they want.

• That said, my proposal was meant to be very specific to compilerError() calls themselves whereas I think the problems with "tryToken" were much more awful because any issue the compiler encountered would cause the code to be rejected, not just the question of whether or not the function call could be made.

• That said, I now wonder based on this discussion whether canResolve() is already doing too much by looking into the body of a given function. Should it just be checking to see whether the function call can be made, and not be trying to resolve its body as well? (in which case it would return true for a case like this and then the compilerError() would not be reached until the call was then attempted). (though then, for a case like @krishnadey30's, I think he would be frustrated with the behavior and want the bool of the first bullet to change the behavior maybe?)

[mppf]

That seems like something that perhaps should have been solved with a warning when using the slower version.

This doesn't work when the generic library is intended to operate without an optional interface. We get that with standalone iterators (can fall back on leader/follower) and with sorting (if radix sorting functions are not available, fall back on comparison sorting). I don't think a warning is appropriate in either of those concrete cases.

[bradcray]

I don't think a warning is appropriate in either of those concrete cases.

Only loosely related, but I've long imagined / wished for a --performance-hints flag that would opt into having the compiler (or maybe even executed code) generate messages for things that seem suboptimal and potentially improved. For these cases, I could imagine the compiler saying "This loop could use a standalone iterator, but one wasn't found; consider adding one for improved performance" or "Sort being implemented using comparison sorting; opt into radix sorting by providing xyz." Somehow, it seems I've never managed to open an issue for this, though.

[krishnadey30]

As a user, I feel that at the end I should be receiving a false as I wanted to check if the given method can be called with these arguments. A user is checking this because they don't want their program to get a compilation error upon using the method with these arguments directly. Here if we got a compilation error that means we can't use the method with these arguments and we should tell the user the same by returning a false.

[ben-albrecht]

Here, if canResolve returns false when the user has a type error in myCall, the user will probably observe that things "work" but are slower than expected (e.g. iterator not parallel, sort not using radix sort, ....). I'd rather have an immediate compilation error in that setting.

That seems like something that perhaps should have been solved with a warning when using the slower version

I don't think a warning is appropriate in either of those concrete cases.

I agree that a warning by default would not be appropriate here. However, I think an opt-in warning like @lydia-duncan's debugWarning example or @bradcray's --performance-hints suggestion would be appropriate, and would help users better understand what their program is doing.

We could have canResolve() take an optional boolean indicating what the return value should be when hitting compilerError()s so that a user could choose which behavior they want.

👍

[mppf]

I think it'd be reasonable for canResolve to handle calls to compilerError encountered when trying to resolve something by returning false and not reporting the error.

The original comment I made only applies to the case in which the function being resolved generates a warning. However I suspect that is not the problem we have here. In particular, for the error case, there is no need to report an error again that canResolve ignored when handling the pattern

if (canResolve("myCall", ...)) then
  myCall(...);
else
  workAroundLackOfMyCall();

[ben-albrecht]

To reiterate the original motivating use case for this feature in a little more detail:

Suppose you have a library function that needs to compare 2 completely generic values:

proc isEqual(a, b) {
 // ...
}

It would be nice to do return a == b;, however this will result in a compiler error if non-comparable types are passed, e.g. string and complex(128).

Additionally, you cannot depend on checking that the types are equal (a.type == b.type), because some types are coercible into others, such as int(32) -> int(64). You also cannot rely on checking if the types are coercible to each other (isCoercible()), because some types are not coercible yet have comparison operations defined for them, such as arrays of int(32) and int(64).

Something like isComparable() would solve this problem, but I suspect that implementation would require the functionality proposed in this issue as well.

[mppf]

That said, I now wonder based on this discussion whether canResolve() is already doing too much by looking into the body of a given function. Should it just be checking to see whether the function call can be made, and not be trying to resolve its body as well?

Actually, the original example in this issue was perhaps more of an exception, implementation-wise. The implementation of promotion involves fully resolving the promoted function. In contrast, the following example does not fully resolve the function in the process of running canResolve:

use Reflection;

proc f() {
  compilerError("I am an error");
}

proc main() {
  if canResolve("f") {
    f(); // error goes away if this line is commented out
  }
}

That means that resolving this issue probably involves 3 changes:

1. Fully resolve any functions called in canResolve, recursively
2. Keep track that we are in canResolve and hide errors encountered
3. If a function with a compilerError is called again, later, re-issue that error

[mppf]

That said, my proposal was meant to be very specific to compilerError() calls themselves whereas I think the problems with "tryToken" were much more awful because any issue the compiler encountered would cause the code to be rejected, not just the question of whether or not the function call could be made.

There are many different kinds of error messages the compiler might produce during resolution besides compilerError. Provided that we can raise these errors again when the function is called again outside of canResolve, I think that we should allow any of these errors to cause canResolve to return false.

Consider for example this example:

use Reflection;
class MyClass { };
proc f() {
  var x: owned MyClass; // error: variable of non-nilable class type not inited
}
proc main() {
  if canResolve("f") {
    writeln("I could resolve f");
  }
}

I would like this to not print out I could resolve f and instead have canResolve("f") return false. I would like for any errors encountered when resolving f to be printed out at such time as a call to f is made outside of a canResolve.

[mppf]

But, I wonder if the direction in the above comment will just recreate the issues with tryToken...

[mppf]

PR #14849 makes the adjustment to resolve this issue. It attempts to resolve the body of the function (but not necessarily all called functions). It only applies special handling to compilerError calls encountered (so will still emit type errors etc). We may revisit either of those choices but I wanted to get far enough on this to make progress on getting types to be able to opt out of init= and have isCopyable return false for them (for #13600 and the related issue #8065).

[mppf]

@krishnadey30 - I've just merged a fix to this but I wasn't quite sure what the expected behavior was to the future test/library/packages/UnitTest/AssertGreater/UnEqualRangeGreater.chpl. If you are able to investigate and update that future as appropriate it would be much appreciated.