Initial work on property declarations (#1410)

* Initial work on property declarations

Introduction
============

The main feature added here is support for `property` declarations, which provide a nicer experience for working with getter/setter pairs.

If existing code had something like this:

```hlsl
struct Sphere
{
	float4 centerAndRadius; // xyz: center, w: radius

	float3 getCenter() { return centerAndRadius.xyz; }
	void setCenter(float3 newValue) { centerAndRadius.xyz = newValue; }

	// similarly for radius...
}
void someFunc(in out Sphere s)
{
	float3 c = s.getCenter();
	s.setCenter(c + offset);
}
```

It can be expressed instead using a `property` declaration for `center`:

```hlsl
struct Sphere
{
	float4 centerAndRadius; // xyz: center, w: radius

	property center : float3
	{
		get { return centerAndRadius.xyz; }
		set(newValue) { centerAndRadius.xyz = newValue; }
	}

	// similarly for radius...
}
void someFunc(in out Sphere s)
{
	float3 c = s.center;
	s.center = c + offset;
}
```

The benefits at the declaration site aren't that signficiant (e.g., in the example above we actually have slightly more lines of code), but the improvement in code clarity for users is significant.

Having `property` declarations should also make it easier to migrate from a simple field to a property with more complex logic without having to first abstract the use-site code using a getter and setter.

An important future benefit of `property` syntax will be if we allow `interface`s to include `property` requirements, and then also allow those requirements to be satisfied by ordinary fields in concrete types.

Subscripts
----------

The Slang compiler already has limited (stdlib-use-only) support for `__subscript` declarations, which are conceptually similar to `operator[]` from the C++ world, but are expressed in a way that is more in line with `subscript` declarations in Swift. A `SubscriptDecl` in the AST contains zero or more `AccessorDecl`s, which correspond to the `get` and `set` clauses inside the original declaration (there is also a case for a `__ref` accessor, to handle the case where access needs to return a single address/reference that can be atomically mutated).

A major goal of the implementation here is to re-use as much of the infrastructure as possible for `__subscript` declarations when implementing `property` declarations.

Nonmutating Setters
-------------------

One additional thing added in this change is the ability to mark a `set` accessor on either a subscript or a property as `[nonmutating]`, and indeed all of the existing `set` accessors declared in the stdlib have been marked this way.

The need for this modifier is a bit subtle. If we think about a typical subscript or property:

```hlsl
struct MyThing
{
    int f;
    property p : int
    {
        get { return f; }
        set(newValue) { f = newValue; }
    }
}
```

it is clear we want the `set` accessor to translate to output HLSL as something like:

```
void MyThing_p_set(inout MyThing this, int newValue)
{
    this.f = newValue;
}
```

Note how the implicit `this` parameter is `inout` even though we didn't mark anything as `[mutating]`. This is the obvious thing a user would expect us to generate given a property declaration.

Now consider a case like the following:

```hlsl
struct MyThing
{
    RWStructuredBuffer<int> storage;

    property p : int
    {
        get { return storage[0]; }
        set(newValue) { storage[0] = newValue; }
    }
}
```

This new declaration doesn't require (or want) an `inout` `this` parameter at all:

```
void MyThing_p_set(MyThing this, int newValue)
{
    this.storage[0] = newValue;
}
```

In fact, given the limitations in the current Slang compiler around functions that return resource types (or use them for `inout` parameters), we can only support a `set` operation like this if we can ensure that the `this` parameter is considered to be `in` instead of `inout`. This is exactly the behavior we allow users to opt into with a `[nonmutating] set` declaration.

All of the subscript operations in the stdlib today have `set` accessors that don't actually change the value of `this` that they act on (e.g., storing into a `RWStructuredBuffer` using its `operator[]` doesn't change the value of the `RWStructuredBuffer` variable -- just its contents).

We'd gotten away without this detail so far just because `set` accessors were only being declared in the stdlib and they were all implicitly `[nonmutating]` anyway, so it never surfaced as an issue that the code we generated assumed a setter wouldn't change `this`.

Implementation
==============

Parser and AST
--------------

Adding a new AST node for `PropertyDecl` and the relevant parsing logic was mostly straightforward. The biggest change was allowing a `set` declaration to introduce an explicit name for the parameter that represents the new value to be set.

This change also adds a `[nonmutating]` attribute as a dual to `[mutating]`, for reasons I will get to later.

Semantic Checking
-----------------

The `getTypeForDeclRef` logic was updated to allow references to `property` declarations.

Some of the semantic checking work for subscripts was pulled out into re-usable subroutines to allow it to be shared by `__subscript` and `property` declarations.

The checking of accessor declarations, which sets their result type based on the type of the outer `__subscript` was changed to also handle an outer `property`.

Some special-case logic was added for checking of `set` declarations to make sure that their parameter is given the expected type.

Some logic around deciding whether or not `this` is mutable had to be updated to correctly note that `this` should be mutable by default in a `set` accessor, with an explicit `[nonmutating]` modifier required to opt out of this default. (This is the inverse of how a typical method or `get` accessor works).

IR Lowering
-----------

The good news is that after IR lowering, access to properties turns into ordinary function calls (equivalent to what hand-written getters and setters would produce), so that subsequent compiler steps (including all the target-specific emit logic) doesn't have to care about the new feature.

The bad news is that adding `property` declarations has revealed a few holes in how IR lowering was handling `__subscript` declarations and their accessors, so that it didn't trivially work for the new case as-is.

The IR lowering pass already has the `LoweredValInfo` type that abstractly represents a value that resulted from lowering some AST code to the IR. One of the cases of `LoweredValInfo` was `BoundSubscript` that represented an expression of the form `baseVal[someIndex]` where the AST-level expression referenced a `__subscript` declaration. The key feature of `BoundSubscript` is that it avoided deciding whether to invoke the getter, the setter, or both "too early" and instead tried to only invoke the expected/required operations on-demand.

This change generalizes `BoundSubscript` to handle `property` references as well, so it changes to `BoundStorage`. Making the type handle user-defined property declarations required fixing a bunch of issues:

* When building up argument lists in the IR, we need to know whether an argument corresponds to an `in` or an `out`/`inout` parameter, to decide whether to pass the value directly or a pointer to the value. Some of the logic in the lowering pass had been playing fast and loose with this, so this change tries to make sure that whenever we care computing a list of `IRInst*` that represent the arguments to a call we have the information about the corresponding parameter.

* Similarly, when emitting a call to an accessor in the IR, the information about the expected type of the callee was missing/unavailable, and the code was incorrectly building up the expected type of the callee based on the types of the arguments at the call site. The logic has been changed so that we can extract the expected signature of an accessor (how it will be translated to the IR) using the same logic that is used to produce the actual `IRFunc` for the accessor (so hopefully both will always agree).

* Dealing with `in` vs. `inout` differences around parameters means also dealing with the "fixup" code that is used to assign from the temporary used to pass an `inout` argument back into the actual l-value expression that was used. That logic has all been hoisted out of the expression visitor(s) and into the global scope.

Future Work
===========

The entire approach to handling l-values in the IR lowering pass is broken, and it is in need a of a complete rewrite based on new first-principles design goals. While something like `LoweredValInfo` is decent for abstracting over the easy cases of r-values, addresses, and a few complicated l-value cases like swizzling, it just doesn't scale to highly abstract l-values like we get from `__subcript` and `property` declarations, nor other corner cases of l-values that we need to handle (e.g., passing an `int` to an `inout float` parameter is allowed in HLSL, and performs conversions in both directions!).

It Should be Easy (TM) to extend the logic that tries to synthesize an interface conformance witness method when there isn't an exact match to also support synthesizing a property declaration (plus its accessors) to witness a required property when the type has a field of the same name/type.

* fixup: pedantic template parsing error (thanks, clang!)

* fixup: cleanups and review feedback

* Removed some `#ifdef`'d out code from merge change

* Added proper diagnostics for accessor parameter constraints, which led to some fixes/refactorings

* Added a test case for the accessor-related diagnostics

1 files changed, 64 insertions, 16 deletions

diff --git a/source/slang/slang-parser.cpp b/source/slang/slang-parser.cpp
index c939e0a38..ed20e0d2e 100644
--- a/source/slang/slang-parser.cpp
+++ b/source/slang/slang-parser.cpp
@@ -208,6 +208,10 @@ namespace Slang
         Parser*			parser,
         ContainerDecl*	containerDecl);
 
+    static void parseModernParamList(
+        Parser*         parser,
+        CallableDecl*   decl);
+
     //
 
     static void Unexpected(
@@ -1208,7 +1212,7 @@ namespace Slang
     }
 
     static void parseParameterList(
-        Parser*                 parser,
+        Parser*         parser,
         CallableDecl*    decl)
     {
         parser->ReadToken(TokenType::LParent);
@@ -2645,6 +2649,7 @@ namespace Slang
         Modifiers modifiers = ParseModifiers(parser);
 
         AccessorDecl* decl = nullptr;
+        auto loc = peekToken(parser).loc;
         if( AdvanceIf(parser, "get") )
         {
             decl = parser->astBuilder->create<GetterDecl>();
@@ -2662,9 +2667,24 @@ namespace Slang
             Unexpected(parser);
             return nullptr;
         }
+        decl->loc = loc;
 
         AddModifiers(decl, modifiers.first);
 
+        parser->PushScope(decl);
+
+        // A `set` declaration should support declaring an explicit
+        // name for the parameter representing the new value.
+        //
+        // We handle this by supporting an arbitrary parameter list
+        // on any accessor, and then assume that semantic checking
+        // will diagnose any cases that aren't allowed.
+        //
+        if(parser->tokenReader.peekTokenType() == TokenType::LParent)
+        {
+            parseModernParamList(parser, decl);
+        }
+
         if( parser->tokenReader.peekTokenType() == TokenType::LBrace )
         {
             decl->body = parser->parseBlockStatement();
@@ -2674,25 +2694,14 @@ namespace Slang
             parser->ReadToken(TokenType::Semicolon);
         }
 
+        parser->PopScope();
+
+
         return decl;
     }
 
-    static NodeBase* ParseSubscriptDecl(Parser* parser, void* /*userData*/)
+    static void parseStorageDeclBody(Parser* parser, ContainerDecl* decl)
     {
-        SubscriptDecl* decl = parser->astBuilder->create<SubscriptDecl>();
-        parser->FillPosition(decl);
-        parser->PushScope(decl);
-
-        // TODO: the use of this name here is a bit magical...
-        decl->nameAndLoc.name = getName(parser, "operator[]");
-
-        parseParameterList(parser, decl);
-
-        if( AdvanceIf(parser, TokenType::RightArrow) )
-        {
-            decl->returnType = parser->ParseTypeExp();
-        }
-
         if( AdvanceIf(parser, TokenType::LBrace) )
         {
             // We want to parse nested "accessor" declarations
@@ -2708,6 +2717,25 @@ namespace Slang
 
             // empty body should be treated like `{ get; }`
         }
+    }
+
+    static NodeBase* ParseSubscriptDecl(Parser* parser, void* /*userData*/)
+    {
+        SubscriptDecl* decl = parser->astBuilder->create<SubscriptDecl>();
+        parser->FillPosition(decl);
+        parser->PushScope(decl);
+
+        // TODO: the use of this name here is a bit magical...
+        decl->nameAndLoc.name = getName(parser, "operator[]");
+
+        parseParameterList(parser, decl);
+
+        if( AdvanceIf(parser, TokenType::RightArrow) )
+        {
+            decl->returnType = parser->ParseTypeExp();
+        }
+
+        parseStorageDeclBody(parser, decl);
 
         parser->PopScope();
         return decl;
@@ -2718,6 +2746,25 @@ namespace Slang
         return parser->ReadToken(tokenType).type == tokenType;
     }
 
+    static NodeBase* ParsePropertyDecl(Parser* parser, void* /*userData*/)
+    {
+        PropertyDecl* decl = parser->astBuilder->create<PropertyDecl>();
+        parser->FillPosition(decl);
+        parser->PushScope(decl);
+
+        decl->nameAndLoc = expectIdentifier(parser);
+
+        if( expect(parser, TokenType::Colon) )
+        {
+            decl->type = parser->ParseTypeExp();
+        }
+
+        parseStorageDeclBody(parser, decl);
+
+        parser->PopScope();
+        return decl;
+    }
+
     static void parseModernVarDeclBaseCommon(
         Parser*             parser,
         VarDeclBase* decl)
@@ -5254,6 +5301,7 @@ namespace Slang
         DECL(extension,       ParseExtensionDecl);
         DECL(__init,          parseConstructorDecl);
         DECL(__subscript,     ParseSubscriptDecl);
+        DECL(property,        ParsePropertyDecl);
         DECL(interface,       parseInterfaceDecl);
         DECL(syntax,          parseSyntaxDecl);
         DECL(attribute_syntax,parseAttributeSyntaxDecl);