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, 196 insertions, 14 deletions

diff --git a/source/slang/slang-check-decl.cpp b/source/slang/slang-check-decl.cpp
index 600d456b8..e4cf40122 100644
--- a/source/slang/slang-check-decl.cpp
+++ b/source/slang/slang-check-decl.cpp
@@ -78,9 +78,23 @@ namespace Slang
 
         void visitConstructorDecl(ConstructorDecl* decl);
 
+        void visitAbstractStorageDeclCommon(ContainerDecl* decl);
+
         void visitSubscriptDecl(SubscriptDecl* decl);
 
+        void visitPropertyDecl(PropertyDecl* decl);
+
+
+            /// Get the type of the storage accessed by an accessor.
+            ///
+            /// The type of storage is determined by the parent declaration.
+        Type* _getAccessorStorageType(AccessorDecl* decl);
+
+            /// Perform checks common to all types of accessors.
+        void _visitAccessorDeclCommon(AccessorDecl* decl);
+
         void visitAccessorDecl(AccessorDecl* decl);
+        void visitSetterDecl(SetterDecl* decl);
     };
 
     struct SemanticsDeclRedeclarationVisitor
@@ -379,6 +393,30 @@ namespace Slang
             qualType.isLeftValue = isLValue;
             return qualType;
         }
+        else if( auto propertyDeclRef = declRef.as<PropertyDecl>() )
+        {
+            // Access to a declared `property` is similar to
+            // access to a variable/field, except that it
+            // is mediated through accessors (getters, seters, etc.).
+
+            QualType qualType;
+            qualType.type = getType(astBuilder, propertyDeclRef);
+
+            bool isLValue = false;
+
+            // If the property has any declared accessors that
+            // can be used to set the property, then the resulting
+            // expression behaves as an l-value.
+            //
+            if(propertyDeclRef.getDecl()->getMembersOfType<SetterDecl>().isNonEmpty())
+                isLValue = true;
+            if(propertyDeclRef.getDecl()->getMembersOfType<RefAccessorDecl>().isNonEmpty())
+                isLValue = true;
+
+            qualType.isLeftValue = isLValue;
+            return qualType;
+
+        }
         else if( auto enumCaseDeclRef = declRef.as<EnumCaseDecl>() )
         {
             QualType qualType;
@@ -3652,19 +3690,19 @@ namespace Slang
         checkCallableDeclCommon(decl);
     }
 
-    void SemanticsDeclHeaderVisitor::visitSubscriptDecl(SubscriptDecl* decl)
+    void SemanticsDeclHeaderVisitor::visitAbstractStorageDeclCommon(ContainerDecl* decl)
     {
-        decl->returnType = CheckUsableType(decl->returnType);
-
-        // If we have a subscript declaration with no accessor declarations,
+        // If we have a subscript or property declaration with no accessor declarations,
         // then we should create a single `GetterDecl` to represent
         // the implicit meaning of their declaration, so:
         //
         //      subscript(uint index) -> T;
+        //      property x : Y;
         //
         // becomes:
         //
         //      subscript(uint index) -> T { get; }
+        //      property x : Y { get; }
         //
 
         bool anyAccessors = decl->getMembersOfType<AccessorDecl>().isNonEmpty();
@@ -3677,29 +3715,173 @@ namespace Slang
             getterDecl->parentDecl = decl;
             decl->members.add(getterDecl);
         }
+    }
+
+    void SemanticsDeclHeaderVisitor::visitSubscriptDecl(SubscriptDecl* decl)
+    {
+        decl->returnType = CheckUsableType(decl->returnType);
+
+        visitAbstractStorageDeclCommon(decl);
 
         checkCallableDeclCommon(decl);
     }
 
-    void SemanticsDeclHeaderVisitor::visitAccessorDecl(AccessorDecl* decl)
+    void SemanticsDeclHeaderVisitor::visitPropertyDecl(PropertyDecl* decl)
     {
-        // An accessor must appear nested inside a subscript declaration (today),
-        // or a property declaration (when we add them). It will derive
-        // its return type from the outer declaration, so we handle both
-        // of these checks at the same place.
-        auto parent = decl->parentDecl;
-        if (auto parentSubscript = as<SubscriptDecl>(parent))
+        decl->type = CheckUsableType(decl->type);
+        visitAbstractStorageDeclCommon(decl);
+    }
+
+    Type* SemanticsDeclHeaderVisitor::_getAccessorStorageType(AccessorDecl* decl)
+    {
+        auto parentDecl = decl->parentDecl;
+        if (auto parentSubscript = as<SubscriptDecl>(parentDecl))
         {
             ensureDecl(parentSubscript, DeclCheckState::CanUseTypeOfValueDecl);
-            decl->returnType = parentSubscript->returnType;
+            return parentSubscript->returnType;
+        }
+        else if (auto parentProperty = as<PropertyDecl>(parentDecl))
+        {
+            ensureDecl(parentProperty, DeclCheckState::CanUseTypeOfValueDecl);
+            return parentProperty->type.type;
         }
-        // TODO: when we add "property" declarations, check for them here
+        else
+        {
+            return getASTBuilder()->getErrorType();
+        }
+    }
+
+    void SemanticsDeclHeaderVisitor::_visitAccessorDeclCommon(AccessorDecl* decl)
+    {
+        // An accessor must appear nested inside a subscript or property declaration.
+        //
+        auto parentDecl = decl->parentDecl;
+        if (as<SubscriptDecl>(parentDecl))
+        {}
+        else if (as<PropertyDecl>(parentDecl))
+        {}
         else
         {
             getSink()->diagnose(decl, Diagnostics::accessorMustBeInsideSubscriptOrProperty);
         }
+    }
 
-        checkCallableDeclCommon(decl);
+    void SemanticsDeclHeaderVisitor::visitAccessorDecl(AccessorDecl* decl)
+    {
+        _visitAccessorDeclCommon(decl);
+
+        // Note: This subroutine is used by both `get`
+        // and `ref` accessors, but is bypassed by
+        // `set` accessors (which use `visitSetterDecl`
+        // intead).
+
+        // Accessors (other than setters) don't support
+        // parameters.
+        //
+        if( decl->getParameters().getCount() != 0 )
+        {
+            getSink()->diagnose(decl, Diagnostics::nonSetAccessorMustNotHaveParams);
+        }
+
+        // By default, the return type of an accessor is treated as
+        // the type of the abstract storage location being accessed.
+        //
+        // A `ref`  accessor currently relies on this logic even though
+        // it isn't quite correct, because we don't have support
+        // for by-reference return values today. This is a non-issue
+        // for now because we don't support user-defined `ref`
+        // accessors yet.
+        //
+        // TODO: Once we can support the by-reference return value
+        // correctly *or* we can move to something like a coroutine-based
+        // `modify` accessor (a la Swift), we should split out
+        // handling of `RefAccessorDecl` and only use this routine
+        // for `GetterDecl`s.
+        //
+        decl->returnType.type = _getAccessorStorageType(decl);
+    }
+
+    void SemanticsDeclHeaderVisitor::visitSetterDecl(SetterDecl* decl)
+    {
+        // Make sure to invoke the common checking logic for all accessors.
+        _visitAccessorDeclCommon(decl);
+
+        // A `set` accessor always returns `void`.
+        //
+        decl->returnType.type = getASTBuilder()->getVoidType();
+
+        // A setter always receives a single value representing
+        // the new value to set into the storage.
+        //
+        // The user may declare that parameter explicitly and
+        // thereby control its name, or they can declare no
+        // parmaeters and allow the compiler to synthesize one
+        // names `newValue`.
+        //
+        ParamDecl* newValueParam = nullptr;
+        auto params = decl->getParameters();
+        if( params.getCount() >= 1 )
+        {
+            // If the user declared an explicit parameter
+            // then that is the one that will represent
+            // the new value.
+            //
+            newValueParam = params.getFirst();
+
+            if( params.getCount() > 1 )
+            {
+                // If the user declared more than one explicit
+                // parameter, then that is an error.
+                //
+                getSink()->diagnose(params[1], Diagnostics::setAccessorMayNotHaveMoreThanOneParam);
+            }
+        }
+        else
+        {
+            // If the user didn't declare any explicit parameters,
+            // then we create an implicit one and add it into
+            // the AST.
+            //
+            newValueParam = m_astBuilder->create<ParamDecl>();
+            newValueParam->nameAndLoc.name = getName("newValue");
+            newValueParam->nameAndLoc.loc = decl->loc;
+
+            newValueParam->parentDecl = decl;
+            decl->members.add(newValueParam);
+        }
+
+        // The new-value parameter is expected to have the
+        // same type as the abstract storage that the
+        // accessor is setting.
+        //
+        auto newValueType = _getAccessorStorageType(decl);
+
+        // It is allowed and encouraged for the programmer
+        // to leave off the type on the new-value parameter,
+        // in which case we will set it to the expected
+        // type automatically.
+        //
+        if( !newValueParam->type.exp )
+        {
+            newValueParam->type.type = newValueType;
+        }
+        else
+        {
+            // If the user *did* give the new-value parameter
+            // an explicit type, then we need to check it
+            // and then enforce that it matches what we expect.
+            //
+            auto actualType = CheckProperType(newValueParam->type);
+
+            if(as<ErrorType>(actualType))
+            {}
+            else if(actualType->equals(newValueType))
+            {}
+            else
+            {
+                getSink()->diagnose(newValueParam, Diagnostics::setAccessorParamWrongType, newValueParam, actualType, newValueType);
+            }
+        }
     }
 
     GenericDecl* SemanticsVisitor::GetOuterGeneric(Decl* decl)