Minimal optional constraints

[juliusikkala]

trafficstars

This PR is limited to a very minimal implementation of optional constraints. I basically took this comment as a spec: https://github.com/shader-slang/slang/issues/7201#issuecomment-2902811333

To suit my needs, I'll need to make this work with generic params for structs, and with the if (let v = value as IThing) syntax. The reason why I "need" this feature, is that current Slang makes it very unwieldy to implement functions and structs with multiple optionally conforming parameters:

void func<A, B>(A a, B b)
    where optional A: IThing
    where optional B: IThing
{
    if (A is IThing)
    {
        a.doThing();
    }
    if (B is IThing)
    {
        b.doThing();
    }
}

// Current Slang would need 4 copies of this function, representing all combinations
// of parameters with and without the IThing conformance.

TODO:

• [x] Add where optional syntax
• [x] Allow calling function with and without conforming type
• [x] Make it work with is
• [x] Diagnose usage with as as an error
• [x] Make it work with structs
• [x] Allow using optional witness in if(a is Interface) blocks
• [ ] Come up with cleaner approach for checking NoneWitness in visitIsTypeExpr
• [x] Add tests
• [x] Add docs
• [x] Write spec proposal and open parallel PR to spec repo

[csyonghe]

I wonder if we can make is more semantically meaningful in that:

// Error, doThing is not a member of `a`.
a.doThing(); 

if (A is IThing)
{
     a.doThing(); // OK, lookup should find doThing through the witness
}

This maybe possible if we let lookup always look through optional witnesses. Then in a separate checking step in visitMemberExpr(), we verify if any dependent witness in the lookup is being verified in some if statement. If not, then it will be a type error.

[juliusikkala]

That would be really useful with inout parameters, the if-let syntax would not allow mutation. I'm definitely interested in trying that too, I'll add it to the TODO list.

[juliusikkala]

I got that working! IMO the as part is completely unnecessary now, as it's a strictly inferior way to access witnesses provided by optional constraints. The only upside I can think of would be consistency, but honestly I'd rather add an error and guide users to use a if (T is Interface) block instead.

[juliusikkala]

/format

[slangbot]

🌈 Formatted, please merge the changes from this PR

[juliusikkala]

Okay, I think this is ready for review now. Here's a brief walkthrough of the implementation:

Optional constraints are marked with a modifier in the AST (OptionalConstraintModifier). During checking, failing to adhere to an optional constraint is not an error. A dummy NoneWitness is provided in place of a real witness if necessary.

During checking, lookup results are filtered out if they reference an optional witness AND are not nested inside a matching if (T is IFoo) block (filterLookupResultByCheckedOptional).

Currently, NoneWitness lowers as IRWitnessTable(void, void). This is then detected when sequential IDs are being assigned, and a hardcoded ID of -1 is given to the NoneWitness.

The if (T is IFoo) checks work by testing if the sequential ID of the witness table for T is -1. An alternative to this hardcoded -1 would be to introduce some new IR instruction to do this check. I briefly tested that, but doing so adds quite a bit more complexity.

[juliusikkala]

The related feature proposal is still pending review. We may have to amend or remove this feature later, if the proposal eventually gets some feedback. I'm somewhat out-of-the-loop with the proposal process, so I'm not sure if it was OK to merge already. Hopefully it was.

[natevm]

My thoughts are that this needs testing in an active project to properly evaluate, and if I understand right, you are actively investing in something to that nature wrt Slang’s CPU backend. Merging this seems like it’ll unblock you from continuing your work.

I’ve also spoken to Yong about merging this PR, and it seemed he was ok with the idea, and had a similar impression as me. We also decided to delay the merge until after the latest VK release which occurred last Friday morning. So if there are any odd regressions that slipped under the radar, we should have ample time to find them.

I’d probably still treat this as an “alpha” which might be subject to change. The proposal repo can then serve as a means of getting the feature out of alpha.

[juliusikkala]

Yeah, I'm working on a small 3D puzzle game as a hobby project, with the goal for it to be 100% Slang. In that context, I've been testing this PR quite a lot already, I've just been using a branch on my fork to do that. In fact, I've now been able to focus on my own project for two full evenings without finding a single compiler bug! In any case, with this merged, it's a lot easier for me as I don't have to constantly maintain a with-all-fixes branch on my end :)