Should Chapel consider formal constness when disambiguating calls?

[dlongnecke-cray]

Should this program compile? If so, what overload of foo should
be selected?

record rec { var x = 0; }

proc foo(const ref r: rec) const ref { return r.x; }
proc foo(ref r: rec) ref { return r.x; }

proc main() {
  var r = new rec();
  const ref rx = r;
  foo(rx);
}

Today this program is a compilation error. The compiler selects the
ref overload of foo and then complains about passing rx
to a ref formal.

As part of an outcome of the Reflection module review, I've been
trying to unify getField and getFieldRef, where the second
procedure only differs from the first in that it takes the first
formal by ref and returns by ref.

I was surprised to see the compiler prefer the ref overload always,
and wrote another sample program (given above), in which the same
behavior occurs.

To me, it seems intuitive that the program should compile and the
const ref overload of foo should be selected.

Implementation-wise, I think the compiler should not even consider
the ref overload of foo as a candidate for this call, on the
basis that the actual is marked const ref.

Note that a limited variation of this disambiguation already occurs
today for method calls:

record rec { var x = 0; }

proc const ref rec.foo() const ref { writeln('const ref'); return x; }
proc ref rec.foo() ref { writeln('ref'); return x; }

proc main() {
  var r = new rec();
  const ref rx = r;
  rx.foo();
}

But outside of receivers in method calls, the spec makes no mention
of constness when selecting candidates for a call:

https://chapel-lang.org/docs/language/spec/procedures.html?highlight=call%20disambiguation#function-resolution

[dlongnecke-cray]

@DanilaFe @jabraham17 Both pointed out that today, the spec takes a clear stance on not disambiguating overloads based on formal argument intents.

In which case, I think it would be good if the example program was an ambiguity error, instead of preferring the ref overload.

[DanilaFe]

I believe this is a bug; the spec (and the overload selection mechanism in production) says these functions are of equal specificity. The conditions for the ref overload to be preferred aren't met (it's not used as an lvalue, it's not assigned to a ref variable, etc.), so that overload shouldn't be picked.

I believe the bug is somewhere around lines 4356 and 4503 in functionResolution.cpp.

In particular, a ContextCall was created (indicating overload selection hasn't concluded), but the "normal checks" run, including a constness checker. The ref version of the function is always selected for the constness checking if available (line 4232 + line 4331).

[mppf]

Note that a limited variation of this disambiguation already occurs today for method calls:

Not so sure why this case works when the other does not, but I don't think it's due to disambiguation itself. The return intent overloading is a later process & I think that's working differently here.

To me, it seems intuitive that the program should compile and the const ref overload of foo should be selected.

Agreed, and I agree with @DanilaFe in that I think this issue is a bug. In particular, I think it's specific to the case when the result of foo is not captured or used in any way.

Implementation-wise, I think the compiler should not even consider the ref overload of foo as a candidate for this call, on the basis that the actual is marked const ref.

I'm open to this, but it would be a language change, rather than an implementation one, because it will impact what the spec describes in the function resolution section. I know that historically we argued that the intents are irrelevant for candidate and overload selection; however that's not true -- they are relevant in various ways (e.g. only a type actual can pass to a type formal) and as a result I don't think it would be a big leap to include const-ness as a consideration. Even ref vs const ref is relevent today, in terms of allowable implicit conversions (but it does not yet consider if the actual is const/const ref as you are proposing here).

[dlongnecke-cray]

Right, I think this should be a compiler error under the current language rules for one reason or another.

Assigning to a const ref var doesn't work - the compiler still picks the ref overload.

proc main() {
  var r = new rec();
  const ref rx = r;
  const ref f = foo(rx); // Now we have a target, we can disambiguate the RI? Nope...
}

I'm not entirely convinced that this should work either. It seems super subtle that two functions that differ only in formal intents can be differentiated solely by RIO.

In the case where no context is available for the call, it should definitely be an ambiguity error. But IMO, it should also be an ambiguity error even if there is context available. Or perhaps a new class of error or something.

---

I'm open to this, but it would be a language change, rather than an implementation one, because it will impact what the spec describes in the function resolution section.

That's correct, I'm advocating for a language change here.

I understand that between value, ref, const ref overloads, generic instantiation, implicit conversion the candidate selection rules can get super subtle and confusing. So my hope is that maybe we can just keep this about const ref vs ref intent at first (and maybe later const/cvref as well) and maybe just view this as an extension of method receiver intents.

[mppf]

@bradcray - I ran into this issue recently. This issue is proposing a language change and I'm not sure it's received attention.

Do you think we should consider the language change here? That is, making something that is some sort of const passed to a ref intent formal cause the function to be not a candidate? I would presume that the same would apply to out / inout.

As it is now, it's a candidate but you would get a const-checking error if the candidate is selected.

[bradcray]

That is, making something that is some sort of const passed to a ref intent formal cause the function to be not a candidate?

This part seems reasonable proposal to me; the obvious hesitation is what it implies for 2.0.

However, it seems to me that it would effectively be useless for cases like the OP if we didn't also have a rule saying something like a var/ref/modifiable actual prefers the ref formal over the const ref formal. Otherwise, it seems as though it would be passable to either and would/should result in an ambiguity (making the ref overload pointless since there'd be no way to call it). So it feels like more than changing the candidate selection rule, but also changing the disambiguation rules to make it work. Does that sound right? And are you proposing or open to that as well?

[mppf]

However, it seems to me that it would effectively be useless for cases like the OP if we didn't also have a rule saying something like a var/ref/modifiable actual prefers the ref formal over the const ref formal. Otherwise, it seems as though it would be passable to either and would/should result in an ambiguity (making the ref overload pointless since there'd be no way to call it). So it feels like more than changing the candidate selection rule, but also changing the disambiguation rules to make it work. Does that sound right? And are you proposing or open to that as well?

I'd expect the return intent overloading to cover this case (as it does today), and so I'd limit the proposal to simply discarding candidates with ref/out/inout formals when given something const. I'm not seeing a need for additional changes to disambiguation to support it (at least, not with the assumption that we will keep return intent overloading in the language).

[bradcray]

I'm not seeing how return intent overloading comes into play, so am either missing something or not being clear. Trying to clarify in case it's the second, my interpretation of your previous comment is that in a situation like:

proc foo(ref x: int) { writeln("In ref foo()"); }
proc foo(const ref x: int) { writeln("In const ref foo()"); }

calls like:

param answer = 42;
const answerToo = answer;
const ref answerRef = answer;
foo(42);
foo(answer);
foo(answerToo);
foo(answerRef);

will all go to the second overload since the first isn't a candidate (since the actual can't be modified). But wouldn't calls like the following:

var answer = 42;
ref answerRef = answer;
foo(answer);
foo(answerToo);

result in ambiguity warnings since they could dispatch to either overload, unless we had a "prefers ref" rule? Or are you saying that return intent overloading has some sort of "prefers ref arguments" rule as part of it already?

[I can see that if the call also involved return intent overloading and the callsites selected between those options then things might happen to work out. But in cases like the OP where the return value is dropped on the floor, would they? And/or are you suggesting that we'd want to say that the only way to do this sort of argument intent overloading is if there also happens to be return intent overloading?]

[mppf]

calls like [const examples] will all go to the second overload since the first isn't a candidate (since the actual can't be modified).

Right

But wouldn't calls like [var examples] result in ambiguity warnings since they could dispatch to either overload, unless we had a "prefers ref" rule?

Right, they would be ambiguous if return intent overloading is not in play

Or are you saying that return intent overloading has some sort of "prefers ref arguments" rule as part of it already?

Nope

And/or are you suggesting that we'd want to say that the only way to do this sort of argument intent overloading is if there also happens to be return intent overloading?

Yes, this is what I'm arguing, partly from a viewpoint of not wanting to change more than we have to.

The example in the OP had return intent overloading as well:

proc foo(const ref r: rec) const ref { return r.x; }
proc foo(ref r: rec) ref { return r.x; }

But in cases like the OP where the return value is dropped on the floor, would they?

FWIW, I think it's a bug to pick the ref return intent overload for something like foo(rx); where the result is dropped on the floor. My reading of the spec section https://chapel-lang.org/docs/language/spec/procedures.html#choosing-return-intent-overloads-based-on-calling-context indicates that dropping the result on the floor will cause the value version to be used (or const ref if there is only ref and const ref return intent overloads). So I think this is a compiler bug (and IMO we had some agreement about that in the early comments on this issue).

But, to be clear, I think to meet what @dlongnecke-cray has been asking for here, yes, we would need a disambiguation change. I just don't think we should go that far at this point with 2.0 since IMO it completely replaces return intent overloading with something else that might have unknown problems when applied to our standard library.

[bradcray]

I definitely feel more nervous about relying on return intent overloading to make const vs. const ref overloading meaningful than I would about just applying such a rule more uniformly. To a large extent because I think it will make these cases feel asymmetrical. In part because of my discomfort with return intent overloading in general (since it feels a bit more like taking the output context into consideration rather than just the input context). But I agree that it is a more minor change and wouldn't block it over that.

In any case, I agree that ref formals shouldn't be considered candidates for non-ref actuals.

[mppf]

It sounds like we have some agreement on a starting point.

However, I'd like to dig a little deeper into your concern and whether or not it's possible to address it. Suppose we did something to address your concern in the previous comment (where I think maybe you meant const ref vs ref?). How could we do it in a way that does not disrupt return intent overloading?

In other words, how/why can we make

proc foo(ref x: int) { writeln("In ref foo()"); }
proc foo(const ref x: int) { writeln("In const ref foo()"); }

var x: int;
foo(x); // want it to choose the `ref` version rather than be ambiguous?

not result in ambiguity without disrupting the similar return intent overloading case:

proc foo(const ref r: rec) const ref { return r.x; }
proc foo(ref r: rec) ref { return r.x; }

var x = ...;
foo(x) = 4; // chooses ref overload
writeln(foo(x)); // chooses const ref overload

?

The idea of adjusting the disambiguation rules to prefer a ref formal given both const ref and ref formals of the same type would break the return intent overloading pattern.

[bradcray]

Hmm, that's a good point. I don't know if I believe the following, but just to wrestle with it before giving up:

• For the foo(x) = 4; case, if we considered the returned value "just another argument to consider" then we could say "x is OK with either const ref or ref" while " = 4" needs a ref return type. So the ref overload is the only one that makes sense for that context and will be used.
• For the writeln(foo(x));, we could say "x is OK with either" and the writeln() is OK with either as well, so maybe we just use the const version since const is a less invasive/impactful + more optimizable intent? (I realize this is the opposite of what I was saying above in terms of preferring ref for a var argument).
• So then that raises the question "Well, then when would we ever prefer the ref overload? And I guess that supports your point that perhaps we never would unless there was a return intent difference (or potentially another argument constraint difference) that pushed us in that direction.
• Yet that still seems like a different conclusion than saying that my non-returning foo() overloads would be an ambiguity (which is what I thought we would be doing if the return intents didn't differ. I.e., if I bought this line of reasoning so far, it would suggest that we'd prefer the const ref version there as well. But of course, that means that the ref overload would never get used.

So maybe the right thing to do in such a case (at least in the long-term) is to warn when we have an overload like mine in which one of the options will never be preferred? (i.e., "warning proc foo(ref x: int) { } will never be called because proc foo(const ref x: int) { } is always a more attractive match.").

[mppf]

I've created a draft PR that makes the simple change under discussion here over at PR #24097.

[mppf]

I ran into an interesting related case this morning while adding some testing for some operations on param complex. Here is a reproducer that doesn't have anything to do with complex:

// A simpler program intended to simulate a problem
// facing complex.

proc ref int.foo() ref {
  return this;
}
proc int.foo() {
  return this;
}
proc main() {
  1.foo(); // error: const actual is passed to 'ref' formal 'this' of foo()
}

We can view this program as either:

• another case that the proposed change in Reject candidates that would cause constness errors #24097 would address, or
• a different bug in return intent overloading that can be separately resolved

[bradcray]

If I'm understanding correctly, this seems sufficiently similar to me that it'd be nice to address here. Specifically, I'm thinking that for the direction we're pursuing for normal arguments, the first wouldn't be a candidate (because 1 can't be passed by ref), so it would be eliminated and the second would be the only reasonable match, so we'd take it. Are there reasons not to consider it another instance of the same issue?

[mppf]

@bradcray - it's certainly related, and yes, PR #24097 should fix the issue in the new code sample.

OTOH, I think we could fix it in a different way if we chose not to proceed with PR #24097. In particular, I expect that it's possible to fix return-intent overloading in this case to behave better. Point being, I don't think that the new example helps much in making a decision about PR #24097.

Either way, I think we should include it in our efforts to improve the situation.

[mppf]

Here is another reproducer for a problem that I observed when working with complex re and im:

proc ref int.foo() ref {
  return this;
}
proc int.foo() {
  return this;
}
proc identity(arg) {
  return arg;
}
proc main() {
  var v = 1;
  identity(v).foo(); // error: non-lvalue actual is passed to 'ref' formal 'this' of foo()
}

The issue is also present when using a record:

record R { var x: int; }

proc ref R.foo() ref {
  return this;
}
proc R.foo() {
  return this;
}
proc identity(arg) {
  return arg;
}
proc main() {
  var v = new R(1);
  identity(v).foo(); // error: non-lvalue actual is passed to 'ref' formal 'this' of foo()
}

As with the other new example, these could be solved by the proposal in PR #24097 but there are presumably other paths to solving it as well.

[mppf]

PR #24097 remains a proposal under discussion & we'll release 2.0 in the meantime. As a result, we have to either convince ourselves that the change in PR #24097 is non-breaking; or we need to conclude that we can't move the language in that direction.

[bradcray]

As a result, we have to either convince ourselves that the change in PR #24097 is non-breaking; or we need to conclude that we can't move the language in that direction.

Or we use it as motivation or fodder for a future Chapel 3.0 release.

[vasslitvinov]

Why would the user want to write an overload that takes a ref in addition to an overload that takes a const ref if there is no return intent overload? I suggest that this is a pattern that we do not support.

[Added] Thinking about #25343, for the purposes of this discussion we will treat formals of the type c_ptr as ref formals and c_ptrConst formals as const ref formals. Analogously, c_ptr actuals are akin to things that can be taken a ref of whereas c_ptrConst things are not.