Jeremie/cs module #1719
Jeremie/cs module #1719
Conversation
| @@ -129,14 +162,21 @@ public TableDeclaration(GeneratorAttributeSyntaxContext context) | |||
| throw new InvalidOperationException("Tagged enums cannot be tables."); | |||
| } | |||
|
|
|||
| var attrArgs = context.Attributes.Single().NamedArguments; | |||
| Visibility = context.TargetSymbol.DeclaredAccessibility switch { | |||
There was a problem hiding this comment.
Why is this restriction necessary / what happens if we don't check it ourselves?
There was a problem hiding this comment.
It's in the proposal that the generated types match the visibility of their source ones.
So if a table is internal, its handle view should also be internal, and so on.
|
Just to duplicate a note from DMs - I noticed a few things in the PR that would be caught by the codegen tests, and looks like the snapshots haven't been updated in the PR. Can you please run Also #1718 might help with some attribute parsing here. |
| @@ -77,6 +59,14 @@ public AlgebraicType GetAlgebraicType(ITypeRegistrar registrar) => | |||
| } | |||
| } | |||
|
|
|||
| public abstract class BaseReducerContext<DbView> : DbContext<DbView>, IReducerContext | |||
| where DbView : class, new() | |||
There was a problem hiding this comment.
Unless we have an interface to uphold, I'd rather not spell out extra constraints. In this case it doesn't seem to add much value in terms of checks.
| where DbView : class, new() | |
| where DbView : new() |
There was a problem hiding this comment.
I made it a class because it does carry state; the connection handle in this case. A struct could be copied/defaulted and introduce a silent null connection. This forces it to be a reference type, and given there's only one per application the distinction between value/ref is no longer relevant, I think.
There was a problem hiding this comment.
Well in this case Local is stateless, but the same base class has to support DbConnection on the client side.
| public abstract class BaseReducerContext<DbView> : DbContext<DbView>, IReducerContext | ||
| where DbView : class, new() | ||
| { | ||
| public Identity Sender => Runtime.SenderIdentity!; |
There was a problem hiding this comment.
Why are these set on the singleton now? What happens if someone wants to store a ReducerContext? It would now lose its own identity and point to the one from the currently running reducer instead.
I think those should be own fields, like before.
For Random we only used Runtime because we wanted to access the RNG anywhere in the codebase. It only needed reseeding, but wasn't actually tied to the reducer data - the result of RNG is non-deterministic by design, so returning data from the wrong context was not a concern, but for identity/address it will be.
Even for RNG that's changing in #1681 and they're all going to live on the context now.
There was a problem hiding this comment.
Reducer context is created once for the duration of the module, and @gefjon says each reducer invocation should be seen as its own isolated instance with no memory able to outlive that invocation.
Making them fields would need a new ReducerContext per reducer invocation, which means its DbView instance has to either also be created per invocation, or stored somewhere.
There was a problem hiding this comment.
I'm fine with either approaches; I guess it depends on what semantics we want for memory across reducer invocations, and if we want to leverage isolation in the impl like this ReducerContext does here.
| [AttributeUsage(AttributeTargets.Method, Inherited = false)] | ||
| public sealed class ReducerAttribute : Attribute | ||
| { | ||
| public string? Name; |
There was a problem hiding this comment.
Reducer kind being a special name was more of an implementation detail, and that will be changing in the new proposal (WIP in #1670) where ReducerKind is stored separately.
AFAIK the SDK proposal also doesn't allow custom #[spacetimedb::reducer(name = "...")], so we shouldn't be doing that in C# either - can you please revert this to a regular attribute parameter, so it would be easier to migrate to non-name-based ReducerKind in the future.
Updates runtime and codegen to support multiple tables per row type