Document rationale for why short-circuiting operators don't fold RHS param values

[bradcray]

I was surprised to find that the following conditional is not folded as I would have expected:

config param flag = false;

if foo() && flag then   // *** conditional of interest ***
  bar();

writeln("OK!");

proc foo() {
  return true;
}

proc bar() param {
  compilerError("Shouldn't get here");
}

seemingly because of flag/false appearing as the second argument to &&. If the two arguments are swapped, things work as you'd expect.

[edit: In the comments below, @daviditen points out, compellingly, that this is by design so that the first expression is evaluated for side effects; I've re-purposed this issue to be about noting the rationale for this in the language spec given how many of us assumed it would be folded]

[edit: See also #20496, which is related]

Associated Test: test/param/shortCircuitParamRight.chpl #21332

[mppf]

@DanilaFe - IIRC you worked a bit on dyno's param folding. Could you check to see if this bug exists in our prototype dyno resolver?

[DanilaFe]

It sure does. I'll submit a fix PR.

[DanilaFe]

We do, however, have tests that make sure folding on the right doesn't occur...

  // 2nd argument param should not be folded
  QualifiedType qt = resolveTypeOfXInit(context,
                                      "var a: bool; var x = a || true;");
  assert(!qt.isParam() && !qt.hasParamPtr());
  assert(qt.type() == BoolType::get(context, 0));

[bradcray]

Oh, interesting. Best guess: In general, the RHS of a short-circuiting operator shouldn't be evaluated unless the LHS suggests that it needs to be. So maybe it was intentional to work hard not to evaluate the param if the RHS is an execution-time expression to ensure that the execution-time expression runs? But given that all params need to be evaluated at compile-time before an execution-time expression runs, it still seems a little odd to suggest deferring the folding of the conditional, particularly since param conditionals can be so powerful.

Who wrote the test?

If we decided that this was correct behavior for reasons that I'm not picking up on, it seems as though we might want to issue a warning saying "This conditional won't be folded at compile-time, but potentially could be if you swapped the order of the arguments"?

[DanilaFe]

I believe @mppf wrote the test (8da98467a901f1581b10be487a56610c8dc072e0). I agree with your assessment, Brad.

[daviditen]

Best guess: In general, the RHS of a short-circuiting operator shouldn't be evaluated unless the LHS suggests that it needs to be.

Yes, it was designed to do exactly this. The LHS must be evaluated before the RHS is considered. The LHS might have side effects that would be lost if the RHS caused the whole expression to be folded. The RHS is still a param, but the expression as a whole is not.

[DanilaFe]

I can make code like this issue a warning on the Dyno side, then.

[bradcray]

It'd be good to get some documentation into the spec indicating this behavior as well (if we don't already have it) and rationalizing it.

[daviditen]

I think this section describes the expected behavior fairly well, in particular the if isTrue(a) then isTrue(b) else false example.

https://chapel-lang.org/docs/master/language/spec/expressions.html#the-logical-and-operator

[bradcray]

I agree the definition covers this case accurately and sufficiently, but I still think that a seasoned Chapel programmer (like myself!) could look at if foo() && false and say "Well, this can clearly be known to be false at compile-time, so should be param-folded away" and be wrong about that. So I think adding a sentence noting that—due to these rewrite rules—param folding won't kick in when the LHS is a non-param expression even if the RHS is (or something like that) to make it even clearer / cover ourselves if someone (like me) balks in surprise.

[daviditen]

config const a = 100;
config param zero = 0;

if a * zero != 0 then
  compilerError("Didn't Fold!");

I don't find the lack of folding in the short-circuiting expression to be any more surprising than in a basic integer math expression like the code above. In fact I think this case is more surprising because there is no expectation of a short-circuit. I'm not opposed to the idea of adding something to clarify for the short-circuiting operators, but I also don't think what's there already is unclear.

[bradcray]

If you're arguing that we should add overloads of * that takes param 0's and fold them, I completely agree with you and would fully support your opening that issue and/or PR.

I also don't think what's there already is unclear.

I'm not saying the current text is unclear, I'm just saying that sometimes even when two clear rules (short-circuiting operator definitions, conditional folding) combine and result in a reasonable behavior, that doesn't mean that the intersection of the behavior is self-evident. In this case I'd say that it wasn't given that I opened this issue, nobody proactively pointed out my mistake, Michael seemed to agree with my thinking, Daniel and his implementation seemed to as well, and it took this conversation for us to realize that I was mistaken. That seems well worth having a note pointing out that even though a human may look at myVar && false or myVar || true and think "These can obviously be folded into the params false/true, they'd be wrong due to the need to evaluate the LHS for side-effects."

(it also raises an interesting question about whether the compiler could fold the cases where it proves the LHS is side-effect free, where I'd expect "yes", but I'm not as worried about that personally...)

[mppf]

I think tricky cases like this make great examples to explain how the (hopefully) clear rules are interacting.

[bradcray]

I returned to this issue for the first time in awhile, updating its title and description to reflect my sense that we should update the documentation to capture the rationale here.

That said, reading #20496 and thinking about this issue a bit more, I'm back to being unconvinced that the current behavior is correct. Specifically, I still buy @daviditen's assertion that we have to evaluate the LHS for side-effects before evaluating the RHS. But at the same time, there is no way that the result of the conditional expression can be true, whatever the LHS expression is. Therefore, it still seems suspect that we'd bother resolving the then conditional of the loop body, knowing that it can never execute. Put another way, though we have to evaluate the conditional expression at execution time, we still know what the param result must be. #20496 argues that we should compute the param but still evaluate the LHS for side-effects, and that sounds pretty reasonable (and less surprising than today's behavior) to me. Any counterarguments?