Fix poorly defined behavior when choosing certain function overloads.

[marsupial]

Description

OSL currently does not define how function overloads are chosen very well.
As such oslc can choose different overloads depending on the order of declaration.

This attempts to define how overloads are chosen in a more concrete way by scoring:

1. all arguments match without conversion
2. prefer promotion of int -> float over float -> int
3. use return type to resolve any possible ambiguities

Tests

testsuite/function-overloads

Checklist:

• I have read the contribution guidelines.
• I have previously submitted a Contributor License Agreement.
• I have updated the documentation, if applicable.
• I have ensured that the change is tested somewhere in the testsuite (adding new test cases if necessary).
• My code follows the prevailing code style of this project.

[lgritz]

This is nice work. I think it LGTM, but it's another one where I feel like I really need to test it thoroughly and it would be risky for me to merge it while I'm on vacation, since there is the potential to subtly break people's shaders. Ugh, if only this had arrived last week! I think the prudent thing to do now, since it's nobody's emergency to get this immediately, is to merge it right after Jan 1 when we're all around and able to jump on any problems.

[marsupial]

No rush, I'll see if I can find the time to re-add the old lookup code and warn/error when they resolve differently. Performance shouldn't be affected much as the newer resolution only traverses the list once.

[marsupial]

So what is the verdict on the possible change of behavior?
Generating a warning on change in resolutions was fairly trivial, but the nature of the problem lead to more significant changes of the test-suite (allowing multiple outputs for one test).
Worth pursuing or just let dead dogs lie?

[lgritz]

Now that I've integrated it, it will be in the next build of our internal renderer, which will cause it to get tested on an extensive and complex set of shaders in the shading department's testsuite. If it causes no trouble there, I think we can just let it lie. If they yell at me, then we can return to this topic and add some kind of warning.

[marsupial]

Two issues whose behavior needs to be defined:

A. Considering the following declarations:

color smoothstep (color edge0, color edge1, color in);
vector smoothstep (vector edge0, vector edge1, vector in);

The spec seems to allow for resolution based on return type, so the following is ok:

color c = smoothstep(point(0), point(1), point(0.5));
vector v = smoothstep(point(0), point(1), point(0.5));

But if the return value is ignored, there is ambiguity that cannot be resolved.
Should those cases error? If not what should happen?

smoothstep(point(0), point(1), point(0.5)); // Error ?
(vector) smoothstep(point(0), point(1), point(0.5)); // Ok ?

B. Similarly, should overload by return-type only be disallowed?

color ambiguous();
vector ambiguous();

[lgritz]

A. So the question is, there is no exact match, but there are more than one possible matches allowing type coercion.

I think that certainly triple-to-triple should certainly be preferred over float-to-triple. Spatial triple (point/vector/normal) to spatial triple coercion should be preferred over spatial triple to color and vice versa.

So the case that's left is that it's spatial-to-spatial, only two out of the three are defined (not smart), and it's not the one we passed!

I'm ok with that being an error, saying that it's ambiguous. I'd also be ok with a warning and just choose arbitrarily (including order of declaration, though I know the point of this patch was to eliminate declaration order dependency).

B. I think this should be allowed. The coercion and/or resolving of ambiguities should work similarly to parameters.

---

While I like your attempt to make an objective scoring system that did not depend on order of declaration, the fact is that in practice, what we were doing was working, inasmuch as people were writing large scale shader libraries and not bumping into ambiguity problems that were causing any real trouble.

I suspect that this was because almost always, the different triple versions of a function behave identically. So I bet people tend to either declare just one triple version (and let the type coercion handle all the cases), or declare all of them. The ambiguous case of defining only a few of them, probably goes unnoticed because most of the time the different versions do the same thing anyway.

---

I also just now noticed that in your original description of this PR, you mention "float->int" substitutions. But those should not be allowed, i.e., is should never allow a function overload to resolve in such a way that a passed float gets coerced to be an int, because it's not legal to say "int = float" in OSL. But you can have "float=int", so it's ok to pass an int where a float was expected (though it should prefer a polymorphic variant where it really expects an int, to one that expects a float and coerces the int).

[marsupial]

For clarification, is disallowing float->int part of the spec (ala GLSL), or just unimplemented?
I'm not seeing it in the docs.

---

I had the same thought on preferring spatial vs. color and shouldn't be too difficult.

---

I'm not sure if the second paragraph is advocating letting things remain as is, so a quick defense:
I get that this issue hasn't caused trouble for you, but still feel it's important for two reasons.

1. I am bumping up against it wanting a overload that actually does exhibit differing behavior between floats/ triples.
2. Fixing it before you guys eventually bump into it would seem prudent.
3. I'm finding cases of what looks both to be incorrect and sub-optimal code being generated:

float clampa(float v, float mn, float mx) {
    printf("clamp.float\n");
    return 0;
}
color clampa(color v, color mn, color mx) {
    printf("clamp.color\n");
    return 0;
}
shader test() {
    clampa(0.2, 0.0, 1.0);
    clampa(0.2, 0, 1);
}

outputs a seemingly incorrect:

clamp.float
clamp.color

I think the float variant should be chosen for both cases, choosing the color variant can easily generate 9 unnecessary loads and 3 times the number of operations from the function's body.

[marsupial]

Solved the late issues discovered, but looks like GitHub can't re-open if a PR has been merged.
Moved to #844.

[lgritz]

It's purposeful that floats can't be assigned to ints. You are right that it isn't spelled out carefully in the docs (it does say that you can assign an int to a float). I'll clarify the docs to explain clearly what types may be assigned each other.

I wasn't advocating letting things remain as-is! I like the idea of removing the order-dependencies to resolving ambiguities (and perhaps warning about any remaining ambiguities). I was just pointing out that (a) it has worked for a long time, so there's no rush, we can proceed carefully on this; (b) if in the end we have to make some "arbitrary choices" in the scoring of edge cases to remove ambiguities (such as declaring that vector has higher valance to coerced types than normal), then so be it, because users have already shown a high ability to avoid the ambiguities in practical situations.

[marsupial]

Ok great. So I think exporting OSL_LEGACY_FUNCTION_RESOLUTION=err per 42611e8 and running against what I understand is a larger internal test-suite should verify/clarify any further edge cases.

---

About float->int, is the reasoning for users to be explicit about truncation or just an implementation detail? The reason I ask is the initializer lists PR has a commit that allows float->int to be code-gened without going through a temporary (though the language aspect of that is not affected).

[lgritz]

float->int : Yes, the reasoning was to force them to be explicit about truncation, or simply to avoid situations where they are accidentally passing a float to someplace where non-integer values aren't meaningful. Whereas the other way (passing an int to a float function) seems to me that implicitly converting the int to float is fine and probably won't break the expectations or semantics of the function.

[aghiles]

Chiming in a bit late into the conversation but it happens that I am playing right now in our Maya hyper shade translator and I was wondering if a "tuple"/"triple" type would do us a favour ? We could then really write functions that do not care if it's a point/vector/normal or even color. This is mainly for utility nodes. One particular example right now is the "bridge" nodes that we have to create because OSL can't connect components yet (if we want to connect the R component into a float we need an intermediate node). One can see it as a base class for all these ... tuples :)

Might be overkill but wanted to mention this anyway.

[marsupial]

As of now function you can write functions that don't care about triple type.
The only thing this changes in regard to that is that conversion between spatial triples are preferred over spatial <-> color.

Seems we're running into similar problems, and voting up/commenting some other PR's might push things forward.

#836 adds named component access to triples, which is a precursor to having a more generic vector type that would encompass vec2, vec3, & vec4 and conversions between them. Unclear if it the later bits are desired by @lgritz.

#801 allows a triple's component to be connected (either as src or dst) to a single float or another triple's component (precisely because the problem you mention).