diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 450cb95cd..d64890a9f 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -729,10 +729,11 @@ The Direct2D authoring projects were removed before the first release and restor | Generated HLSL, a constructor in a static field initializer | Hands a user defined constructor call to an extension point whose default writes a default value, and overrides that default only in the rewriter for the shader body. An initializer holds zero where the same call one line away in the body constructs the value, and nothing is reported on either path: the generated HLSL still compiles, so the only sign is the number the shader computes | Imports the constructor the way the body does, the initializer rewriter handing the call to the one that rewrites bodies, a constructor declaration being a body. The arguments keep the rewriting the initializer gave them, the import reading them from the node it is handed rather than visiting them, and the local functions it lifts out are carried over the way an imported method's already are. The stubs are written among the type declarations, which both generators write ahead of the static fields, so the call is in scope where the initializer needs it, measured on the DXC path and on the FXC path alike; the value the field holds is read back from a device rather than inferred from the two paths writing the same source. The parameterless constructor a struct always has stays a default value, which is what it computes in C# as well. The extension point carries no default any more, one answering with a default value having computed something other than what the author wrote without saying so, so a rewriter that will not import a constructor states what it does instead | `b3bd8a3a`, `40fe41df`, `03433ef5`, `3220e6f4` | | Generated HLSL, a constructor written with an expression body | Reads the body of a constructor declaration without first turning an expression body into a block, which both of the other declarations it visits do, so importing one ends the generator with a null reference. The output of that generator is then dropped for the whole compilation unit, and what the author reads is a list of unrelated errors naming generated types that were never written. The two forms mean the same thing in C#, and the accepted set holds the arrow clause and the constructor declaration alike, so nothing had judged the shape refused | Turns the body into a block the way a method and a local function already are, from an override of the visit rather than from the caller, so that a route reaching a constructor declaration later is normalized as well. Only the body is: the import builds the declarations it writes from the parameter list and the body alone, and reads neither the modifiers nor the attribute lists, which is why a method drops its modifiers and a constructor drops nothing, an attribute list being dropped for every declaration by the walk the two share. Both entry points, the shader body and a static field initializer, were measured failing at that one line before the change and writing the same HLSL as a block body after it, on the Direct2D path as much as on the compute one. The generated sources of the whole solution were compared file by file across the change, and the only two that move are the pair that moves between two builds of the same commit anyway | `b576c5dc`, `207e40fc` | | Generated HLSL, a declaration carrying no body | Reads the body of a declaration without asking whether it has one. An extern declaration carries none, which C# reports only as a warning, and every route that reads a declaration meets it. A method and a local function are written out as declarations with no body and the shader compiler answers by naming generated code the author never wrote; a constructor and the entry point end the generator instead, which discards the descriptors for every shader in the compilation unit. Eleven routes were measured, which are the ones that write a declaration into the generated HLSL: the four imports, the entry point of each of the two generators, and a method of the shader itself or a local function, the last two whether or not they are called | Reports `CMPW0127`, or `CMPWD2D0099` on the Direct2D path, from the visit that normalizes the body of each of the three kinds a declaration can be, so the four imports and the entry point of each of the two generators are answered by one rule rather than by one rule per route. The declaration is given an empty body rather than the rewriting ending there: ending it discards the output for the whole compilation unit, which is the failure the report replaces, and the report is an error, so what the empty body produces never reaches the shader compiler. The helper that normalizes a body already stated it always returns a block body, and it now keeps that for a declaration carrying none as well. A declaration split into parts is unaffected, the implementing part being the one that is read. What is reported follows what is written out rather than what is declared, so a member of an external type the shader never reaches is left alone, which three routes were measured for. A partial declaration with no implementing part is outside this: C# refuses one carrying accessibility modifiers and erases one that does not, so neither reaches the rewriting | `a86382b6`, `56f951ff`, `9957e455` | -| Generated HLSL, a static field initializer reaching itself | Rewrites the initializer of an external static field and adds the entry to the collection of static field definitions after the rewriting finishes, where the two other collections a rewriting can return to claim their entry before it. An initializer that reaches the field it initializes therefore finds no entry, rewrites the field a second time and adds it, and the outer rewriting adds the same key again when it returns. The generator faults with an argument exception, which discards the descriptors for every shader in the compilation unit and leaves the author with errors that name none of this. The two routes that close the cycle are a method and a constructor the initializer imports | Claims the entry before rewriting, the way the other two collections do, and treats a return to a claimed entry as a cycle, reporting `CMPW0124` on the compute path and `CMPWD2D0096` on the Direct2D one. Claiming alone was measured to be insufficient: the fault stops, no diagnostic is reported, and the shader compiler accepts generated HLSL that reads a global static through a function reading it back, so the fault would become a silently different value, which is the worse of the two in the order this tree keeps. The mark is an empty type declaration, which widens the type alias for a static field; the two places that read it both read an array built after every rewriting finishes, and building both generators over the widened alias reports no warning. One signature wrote the collection as a raw tuple rather than as the alias, which the widening turns into a nullability error, so it is aligned with the other three. The report is located on the read that closes the cycle and not on the field declaration: a cycle closed from two imported declarations is two places the author has to change, and one report on the declaration names neither of them. A static field of the shader is excluded by every call site here too, and is answered on routes of its own, which a row of its own records. An initializer reading a static field directly reaches the report as well, that read being imported the way the shader body imports one. | `da160eb1`, `fc0676b9`, `a1a12303`, `fabf4202` | +| Generated HLSL, a static field initializer reaching itself | Rewrites the initializer of an external static field and adds the entry to the collection of static field definitions after the rewriting finishes, where the two other collections a rewriting can return to claim their entry before it. An initializer that reaches the field it initializes therefore finds no entry, rewrites the field a second time and adds it, and the outer rewriting adds the same key again when it returns. The generator faults with an argument exception, which discards the descriptors for every shader in the compilation unit and leaves the author with errors that name none of this. The two routes that close the cycle are a method and a constructor the initializer imports | Claims the entry before rewriting, the way the other two collections do, and treats a return to a claimed entry as a cycle, reporting `CMPW0124` on the compute path and `CMPWD2D0096` on the Direct2D one. Claiming alone was measured to be insufficient: the fault stops, no diagnostic is reported, and the shader compiler accepts generated HLSL that reads a global static through a function reading it back, so the fault would become a silently different value, which is the worse of the two in the order this tree keeps. The mark is an empty type declaration, which widens the type alias for a static field; the two places that read it both read an array built after every rewriting finishes, and building both generators over the widened alias reports no warning. One signature wrote the collection as a raw tuple rather than as the alias, which the widening turns into a nullability error, so it is aligned with the other three. The read that closes the cycle is reported by the walk over what every initializer reaches, which the row on a static field accessed before its initializer has run describes, and which names the read rather than the field declaration: a cycle closed from two imported declarations is two places the author has to change. The claim stays, being what stops the fault, and a read returning to a claimed entry is written out under the name the entry already holds. A static field of the shader is excluded by every call site here too, and is answered on routes of its own, which a row of its own records. An initializer reading a static field directly reaches the claim as well, that read being imported the way the shader body imports one. | `da160eb1`, `fc0676b9`, `a1a12303`, `fabf4202`, `6d4e2354` | | Generated HLSL, static field reads in an initializer | Rewrites a static field initializer with a rewriter that answers for a constant and for nothing else a field can be. A read of a static field declared outside the shader is written out under the name the author wrote, naming a type the generated HLSL never declares, and a read of the shader's own static field through the type name keeps the type name on it. The shader body imports the first and drops the type name from the second, one line away. The initializer of an imported field is rewritten by the same rewriter, so the body meets this as well as soon as an imported field's own initializer reads another one by name alone. Nothing is reported on any of these paths, the shader compiler answering instead by naming generated code the author never wrote. The fields the body does import are written after the ones the shader type declares, and among themselves in whatever order the collection holding them enumerates, so a read that resolved to an import would still name a declaration written after it | Reads a static field the way the body does, handing the read to the rewriter that imports every other declaration an initializer reaches, the way the import of a constructor is already handed to it, and carrying back the local functions that import lifted out. A field of the shader type is written under the name it has in the shader rather than imported, which is what the body does with it. A read written by name alone is intercepted before the member access path, an identifier being where a read of a field of the enclosing type arrives, and a read the import declines falls back to what the base rewriter would have done, which keeps a constant and an enum member on the path they already had. Every static field, imported or declared on the shader type, is written as one sequence ordered by how many had finished when its own rewriting did, which is after every field its initializer reached, so each is written after the ones it names; the count is carried on the model for a static field rather than left to the enumeration order of the collection. Writing the imported ones as a block ahead of the declared ones was measured to be insufficient, a field declared on the shader type and read from an imported initializer landing after the read. The import claims its entry before rewriting, so an initializer reaching the field it initializes through one of these reads is answered by `CMPW0124`, or `CMPWD2D0096` on the Direct2D path, rather than by a fault | `3bc1bcca`, `19fe5d51` | | Generated HLSL, a variable a static field initializer would have to declare | Writes an out argument into a call in a static field initializer as the author wrote it, declaration and all, so the generated HLSL declares a variable inside an argument list and the shader compiler rejects it while naming generated code. A discarded out argument answers the same way, the discard reaching the generated HLSL as an identifier nothing declares. The shader body declares the variable at the start of the body and passes it to the call, so where the author writes the call decides whether it works | Reports `CMPW0126`, or `CMPWD2D0098` on the Direct2D path, at the declaration and at the discard alike. The two reporting sites are the two places the rewriter for a body introduces such a variable, so what is refused is what that rewriter hoists rather than the shapes that happened to be measured. Giving the variable a global static of its own instead was rejected: one storage would be shared by every invocation of the shader, and HLSL leaves the order of global static initializers undefined, which is the hazard `CMPW0124` already refuses. The report names the source the author wrote, where the failure it replaces named the shader type and quoted a line of generated HLSL | `121de6e8` | -| Generated HLSL, a static field of the shader reaching itself | Rewrites the initializer of a static field of the shader without claiming an entry for it, the collection the other rewritings claim in holding the fields of other types, and leaves a static method of the shader for the generator to write out, which it does before it rewrites any initializer. An initializer that reaches the field it initializes therefore finds nothing on any of its routes: nothing is reported, the generator does not fault, and the shader compiler accepts generated HLSL that reads a global through a function reading it back, so the shader computes a value C# never produces. Reading the field by its own name and reading it through the shader type differ only in that the second writes the type name into the generated HLSL, which the compiler then rejects while naming generated code | Claims the entry for a field of the shader as well, in the processor that rewrites an initializer rather than in the caller that rewrites an external one, and releases it once the initializer is rewritten, the collection being the external definitions a generator writes out on top of the fields of the shader it gathers itself. The read is answered where it is written: by name alone in the shared rewriter, and through the shader type in each of the two rewriters, neither of which reaches the other's site. A cycle closing through a static method of the shader reaches none of those, that method being rewritten before any field is claimed, which was measured rather than read, so what such a call reaches is walked instead. The walk returns at once unless a claim is outstanding, which leaves alone the ordinary shape of a method reading a static field, and it resolves only the three kinds a read or a call is written as, a read written as a member access resolving on the access alone, so one read stays one report. What it reaches is every static method a reached declaration calls and the initializer of every static field it reads, neither of which is a shape a rewriting passes through while the claim is held: a call leaving the shader type is written out before any claim is taken, and a second field is rewritten while the claim is on the field being read rather than on the field the read closes the cycle on. Two fields whose initializers reach each other through a static method are answered this way. Two that read each other directly are not, that shape being refused by the shader compiler instead, one of the two reading a global declared after it where a read reaching it through a function finds a forward declaration. The type symbol for the shader moves to the type both rewriters derive from, the shared logic having to tell a member of the shader from a member of any other type, and each of the two having held its own copy | `41027c2f`, `18e3c03d` | +| Generated HLSL, a static field of the shader reaching itself | Rewrites the initializer of a static field of the shader without claiming an entry for it, the collection the other rewritings claim in holding the fields of other types, and leaves a static method of the shader for the generator to write out, which it does before it rewrites any initializer. An initializer that reaches the field it initializes therefore finds nothing on any of its routes: nothing is reported, the generator does not fault, and the shader compiler accepts generated HLSL that reads a global through a function reading it back, so the shader computes a value C# never produces. Reading the field by its own name and reading it through the shader type differ only in that the second writes the type name into the generated HLSL, which the compiler then rejects while naming generated code | Reports the read from a walk over what every initializer reaches, run once after the initializers are rewritten, which the row on a static field accessed before its initializer has run describes. Nothing is claimed for a field of the shader and nothing is reported as an initializer is rewritten: a rewriting does not pass through every declaration an initializer reaches, a declaration being imported once and a method of the shader never, which is what left routes unanswered while the read was reported where it was rewritten. Before the walk, the read was answered where it was written, by name alone in the shared rewriter and through the shader type in each of the two rewriters, and a call to a static method of the shader was walked from the rewriting site, which reached neither a declaration the body or an earlier initializer had imported first, nor a declaration in the middle of its own import, nor a field of another type the walk found. Two fields whose initializers reach each other through a static method are answered as a read C# performs too early rather than as a cycle, C# running the first initializer to completion before the second starts. The type symbol for the shader moves to the type both rewriters derive from, the shared logic having to tell a member of the shader from a member of any other type, and each of the two having held its own copy | `41027c2f`, `18e3c03d`, `6d4e2354` | +| Generated HLSL, a static field accessed before its initializer has run | Writes an access to a static field into the generated HLSL as it stands wherever the access is performed, and writes the static fields out in an order derived from what each initializer reached. C# runs the static field initializers of a type once, in the order the fields are declared, and a field of that type whose turn has not come yet holds the default value of its type: a read of it is that default, and a write to it is discarded when its initializer runs, whether the initializer reaches the field directly, through a method of the shader, through an imported declaration, or from the initializer of an imported field. The generated HLSL reproduces neither: the shader compiler folds the initializer the later field carries when it can, so the shader computes the initialized value where C# computes the default and keeps a write C# discards, leaves the read undefined otherwise, and does not compile the direct read at all, naming a global declared later in generated code. A field read through a function before its initializer has run was measured to compute 10 on DXC and FXC alike where C# computes 0, with neither compiler reporting anything, and a write through a function to a later field carrying a constant initializer was measured to stay on DXC where C# computes the initializer's value, FXC refusing that write while naming generated code. An initializer reaching back into the field it initializes is the same read, and the cycle rows above answered it only where a rewriting passed through the read while the field was claimed | Reports `CMPW0124`, or `CMPWD2D0096` on the Direct2D path, at the access, naming the field accessed and the initializer that performs the access too early. The report comes from one walk over every static field the generated HLSL declares, the shader's own and the imported ones alike, run once after the initializers are rewritten: from each initializer it follows the method, local function or constructor a call resolves to and the initializer of a static field of another type, whose initializers C# runs when that type is first touched, and reports a read or a write of a field of the initializer's own type whose turn has not come and that carries an initializer, the field being initialized among them for a read. A field carrying no initializer holds the default value of its type in C# and in the generated HLSL alike, and a write to the field being initialized is overwritten by its initializer in both, so neither is reported; both were measured on DXC. A write is a simple assignment to the field or an out argument, and every other reference reads the field, a compound assignment and a ref argument among them. A field of the same type is not walked into, its initializer having run already or not running until its own turn, a local function is walked only through a call, C# not running one that is never called, and an access two initializers both perform too early is reported once, for the first of the two in declaration order, so every access is one report. The walk does not follow the order of the statements it passes, so a read after a write in the same declaration is reported like any other. The same descriptor answers for an initializer reaching itself, that being the first field whose turn has not come, so the identifier the earlier rows introduced widens rather than a second one being added, and its message names both fields. The walk resolves semantic information only for the kinds an access, a call or a construction can be written as, which is the pre-filter the rewriting uses too | `6d4e2354` | | Generated HLSL, the requirements a shader raises | Keeps what a rewriting gathers on the rewriter that gathered it, and carries it out by hand wherever one rewriter creates another. Which of those places carry anything is left to a hook the specialized rewriter may implement, and a hook nothing implements is silent. On the Direct2D path the scene position requirement is raised inside the rewriter for a called method, an instance method, a constructor, the initializer of an external static field, an initializer of the shader itself, and a declaration imported by one, and reaches the generator from none of them, so `CMPWD2D0045` answers only for a call written in the shader body. The generated HLSL then reads the position without the define its header needs, and the author is left with the failure FXC raises against generated code, which names a line of generated source and not the attribute to add. On the compute path the three initializer routes lose the requirement that a dispatch cover whole thread groups, so a range that cuts a group in half is accepted for a shader that waits for one. Two of the hooks are declared and implemented with no call site anywhere in the tree | Holds the requirements in one instance per shader, handed to every rewriter created for it, so a requirement is raised where it is found and read where the shader is written, with no path in between having to carry it. Which requirements exist still differs between the two generators, so they are declared in a partial declaration each one carries, the way the rewriters themselves already are, and a rewriter that is not handed them does not compile. The two hooks with no call site are removed along with the carrying they existed for, and so is the suppression that hid them: measured over both generators, that rule had nothing else to report, so a hook declared and never called now stops the build. The other rule in that suppression is kept, one field being assigned and never read where the shared file compiles into the Direct2D generator, and what it hides is written beside it. The hook that raises a requirement from a mapped call moves to the type both rewriters derive from: what a call requires does not depend on whether it was written in a body or in an initializer, and declaring it twice was the shape that left one of the two uncalled. The sampler requirement is left where it is, a texture being refused as a static field and as a parameter alike, so only a rewriting of the shader's own members can raise it, which was measured rather than read. Measured on both paths over the six places a rewriter creates another one and the one each generator creates for a static field of the shader itself | `44f39c86`, `6b8f4af7`, `f937b857`, `33756710` | | Generated HLSL, primary constructors | Refuses the construction of a type declaring a primary constructor, which is deliberate, the captures of such a constructor reaching the members of the type in ways the rewriting cannot follow. It reports the diagnostic for a constructor with no source to analyze, because the check that reaches it is whether a constructor declaration can be found, and a primary constructor has none of its own. The author reads a reason that is visibly not the one, the source being right there, and the same refusal covers a constructor from another assembly, for which the reason is correct | Reports `CMPW0120`, or `CMPWD2D0093` on the Direct2D path, when the constructor is declared in source, and keeps the older diagnostic for one that is not. The two are told apart by whether the constructor has any declaring syntax reference at all, a primary constructor having one on the type declaration. The refusal itself is unchanged, and so is the shader type's own primary constructor, whose captures become the shader fields | `00f95213` | | Generated HLSL, generic local functions | Lifts a local function to a top level HLSL function whether or not it is called, carrying its type parameter list with it, and asks whether a method is generic only where a call is rewritten. A generic local function that is never called reaches the HLSL compiler with `` on it, and one that is called is reported against the call rather than against the declaration the author has to change | The declaration is refused, so the report names it, and the lifted function, which still carries its type parameter list, never reaches the shader compiler. The call site leaves a local function alone, because the declaration has already answered and two reports would name two places for one cause | `e364c7e4` | diff --git a/src/ComputeWeave.D2D1.SourceGenerators/D2DPixelShaderDescriptorGenerator.HlslSource.cs b/src/ComputeWeave.D2D1.SourceGenerators/D2DPixelShaderDescriptorGenerator.HlslSource.cs index 2ec1ef9e2..05604e6a9 100644 --- a/src/ComputeWeave.D2D1.SourceGenerators/D2DPixelShaderDescriptorGenerator.HlslSource.cs +++ b/src/ComputeWeave.D2D1.SourceGenerators/D2DPixelShaderDescriptorGenerator.HlslSource.cs @@ -373,6 +373,15 @@ private static ImmutableArray GetStaticFields( } } + // An access C# performs before the field's initializer has run is reported once every initializer + // is rewritten (same as in the DX12 generator) + HlslDefinitionsSyntaxProcessor.ReportStaticFieldAccessesBeforeInitialization( + structDeclarationSymbol, + staticFieldDefinitions, + semanticModel, + diagnostics, + token); + // One sequence ordered by when each field finished (same as in the DX12 generator) HlslStaticField[] fields = [.. declared.WrittenSpan, .. staticFieldDefinitions.Values]; diff --git a/src/ComputeWeave.D2D1.SourceGenerators/Diagnostics/DiagnosticDescriptors.cs b/src/ComputeWeave.D2D1.SourceGenerators/Diagnostics/DiagnosticDescriptors.cs index 7845e0110..be027bc28 100644 --- a/src/ComputeWeave.D2D1.SourceGenerators/Diagnostics/DiagnosticDescriptors.cs +++ b/src/ComputeWeave.D2D1.SourceGenerators/Diagnostics/DiagnosticDescriptors.cs @@ -1447,19 +1447,19 @@ partial class DiagnosticDescriptors helpLinkUri: "https://github.com/routersys/ComputeWeave"); /// - /// Gets a for a static field initializer reaching the field it initializes. + /// Gets a for a static field accessed before its initializer has run. /// - /// Format: "The static field {0} is initialized from an expression that reads it back, directly or through a declaration that initializer reaches". + /// Format: "The static field {0} is accessed while the initializer of {1} is running, before its own initializer has run (C# reads the default value of its type there and discards a write, which the generated HLSL does not reproduce)". /// /// - public static readonly DiagnosticDescriptor CyclicStaticFieldInitializer = new( + public static readonly DiagnosticDescriptor StaticFieldAccessedBeforeInitialization = new( id: "CMPWD2D0096", - title: "Static field initializer reaching the field it initializes", - messageFormat: "The static field {0} is initialized from an expression that reads it back, directly or through a declaration that initializer reaches", + title: "Static field accessed before its initializer has run", + messageFormat: "The static field {0} is accessed while the initializer of {1} is running, before its own initializer has run (C# reads the default value of its type there and discards a write, which the generated HLSL does not reproduce)", category: "ComputeWeave.D2D1.Shaders", defaultSeverity: DiagnosticSeverity.Error, isEnabledByDefault: true, - description: "C# runs the initializer once, and where the cycle closes it reads the field as its default value, so the field holds whatever that produces. HLSL has no defined order for global static initializers, so the value the shader computes is not the one C# computes, and the shader compiler accepts the source without saying anything: the generated HLSL was measured to compile with the cycle written out as it stands. Before this report the generator faulted instead, adding the same key to the collection of static field definitions twice, which discards the descriptors for every shader in the compilation unit and leaves the author with errors that name none of this.", + description: "C# runs the static field initializers of a type once, in the order the fields are declared. A field whose turn has not come yet holds the default value of its type: a read of it is that default, and a write to it is discarded when its initializer runs. Both happen when an initializer reaches a field of the same type declared after it, directly or through any declaration it reaches, and the read happens when an initializer reaches back into the field it initializes. The generated HLSL reproduces neither: the shader compiler folds the initializer of a constant declared later, so the read is the initialized value, and refuses the other forms, so the shader either computes a value C# never produces or fails against generated code the author never wrote. A field carrying no initializer holds the default value in both and is not reported, and neither is a write to the field being initialized, which its initializer overwrites in both. Before this report an initializer reaching itself faulted the generator instead, adding the same key to the collection of static field definitions twice, which discards the descriptors for every shader in the compilation unit and leaves the author with errors that name none of this.", helpLinkUri: "https://github.com/routersys/ComputeWeave"); /// diff --git a/src/ComputeWeave.SourceGeneration.Hlsl/SyntaxProcessors/HlslDefinitionsSyntaxProcessor.cs b/src/ComputeWeave.SourceGeneration.Hlsl/SyntaxProcessors/HlslDefinitionsSyntaxProcessor.cs index 3d19a534e..f0df6370b 100644 --- a/src/ComputeWeave.SourceGeneration.Hlsl/SyntaxProcessors/HlslDefinitionsSyntaxProcessor.cs +++ b/src/ComputeWeave.SourceGeneration.Hlsl/SyntaxProcessors/HlslDefinitionsSyntaxProcessor.cs @@ -12,6 +12,7 @@ using Microsoft.CodeAnalysis; using Microsoft.CodeAnalysis.CSharp; using Microsoft.CodeAnalysis.CSharp.Syntax; +using Microsoft.CodeAnalysis.Operations; using static ComputeWeave.SourceGeneration.Diagnostics.DiagnosticDescriptors; using static Microsoft.CodeAnalysis.CSharp.SyntaxFactory; @@ -117,19 +118,6 @@ public static bool TryGetStaticField( token.ThrowIfCancellationRequested(); - // A field of the shader is claimed before its initializer is rewritten, the way the caller that - // rewrites an external one already claims its entry, so that a read of the field reaching back into - // its own initializer finds a claim rather than an identifier written out as it stands. The claim is - // released below, this collection being the external definitions a generator writes out on top of - // the fields of the shader it gathers itself, which this field is already one of. The order a - // completed entry carries is never read for this one, the claim being released before that - bool isShaderStaticField = SymbolEqualityComparer.Default.Equals(fieldSymbol.ContainingType, structDeclarationSymbol); - - if (isShaderStaticField) - { - staticFieldDefinitions.Add(fieldSymbol, (name, null, null, 0)); - } - // Create the rewriter to use, which is also returned to callers so they can extract the local // functions an initializer lifted out. What the initializer requires of the shader is raised // into the shared requirements instead, so a caller has nothing to read back out for that. @@ -150,11 +138,6 @@ public static bool TryGetStaticField( token.ThrowIfCancellationRequested(); - if (isShaderStaticField) - { - _ = staticFieldDefinitions.Remove(fieldSymbol); - } - assignmentExpression = processedDeclaration?.NormalizeWhitespace(eol: "\n").ToFullString(); return true; @@ -356,4 +339,177 @@ public static int GetStaticFieldOrder(IDictionary return order; } + + /// + /// Reports every access to a static field that C# performs before the initializer of that field has run. + /// + /// The type symbol for the shader type. + /// The collection of discovered static field definitions. + /// The instance for the type to process. + /// The collection of produced instances. + /// The used to cancel the operation, if needed. + /// + /// + /// C# runs the static field initializers of a type once, in the order the fields are declared. A field of + /// that type whose initializer has not run yet holds the default value of its type, so a read of it from the + /// initializer being run, or from anything that initializer reaches, is that default, and a write to it is + /// discarded when its initializer runs. The generated HLSL reproduces neither: the shader compiler folds + /// the initializer the field carries when it can, so the read is the initialized value and the write stays, + /// leaves the read undefined otherwise, and does not compile a direct read of a global declared later. The + /// field being initialized is the first one whose turn has not come, so an initializer reaching back into + /// itself is the same case, except for a write, which the initializer overwrites in the generated HLSL as + /// well. A field carrying no initializer holds the default value in both and is not part of this. + /// + /// + /// The walk starts from the initializer of every static field the generated HLSL declares, the ones of the + /// shader and the imported ones alike, and follows every declaration it reaches: the method, local function + /// or constructor a call resolves to, and the initializer of a static field of another type, whose + /// initializers C# runs when that type is first touched. A field of the same type is not walked into, its + /// initializer having run already or not running until its own turn, and a local function is walked only + /// through a call, C# not running one that is never called. An access two initializers both perform too + /// early is one place to change, so it is reported once, for the first of the two in declaration order. + /// The walk does not follow the order of the statements it passes, so a read after a write in the same + /// declaration is reported like any other. + /// + /// + /// This runs once after the initializers are rewritten rather than as they are, because a rewriting does + /// not pass through every declaration an initializer reaches: a declaration is imported once, so one the + /// body or an earlier initializer imported is not rewritten again, and a method of the shader is never + /// imported. Semantic information is resolved only for the kinds an access, a call or a construction can + /// be written as, an access written as a member access resolving on the access alone. + /// + /// + public static void ReportStaticFieldAccessesBeforeInitialization( + INamedTypeSymbol structDeclarationSymbol, + IDictionary staticFieldDefinitions, + SemanticModelProvider semanticModel, + ImmutableArrayBuilder diagnostics, + CancellationToken token) + { + Dictionary> initializers = new(SymbolEqualityComparer.Default); + + // An access is one place the author has to change, so one reached from two initializers is reported + // once, naming the first of the two in declaration order + HashSet reportedAccesses = []; + + // The static fields of a type that carry an initializer, in the order C# runs those + List<(IFieldSymbol Field, ExpressionSyntax Initializer)> GetInitializers(INamedTypeSymbol type) + { + if (!initializers.TryGetValue(type, out List<(IFieldSymbol Field, ExpressionSyntax Initializer)>? order)) + { + order = []; + + foreach (ISymbol member in type.GetMembers()) + { + if (member is IFieldSymbol { IsImplicitlyDeclared: false, IsStatic: true, IsConst: false } field && + field.TryGetSyntaxNode(token, out VariableDeclaratorSyntax? declarator) && + declarator.Initializer is { } initializer) + { + order.Add((field, initializer.Value)); + } + } + + initializers.Add(type, order); + } + + return order; + } + + // A simple assignment to the field and an out argument write it without reading it + static bool IsWrite(IFieldReferenceOperation reference) + { + return reference.Parent switch + { + ISimpleAssignmentOperation assignment => ReferenceEquals(assignment.Target, reference), + IArgumentOperation { Parameter.RefKind: RefKind.Out } => true, + _ => false + }; + } + + // The imported fields are walked in the declaration order of their type too, so that an access two of + // them perform too early names the first of the two + HashSet importedFields = new(staticFieldDefinitions.Keys, SymbolEqualityComparer.Default); + HashSet importedTypes = new(SymbolEqualityComparer.Default); + List<(IFieldSymbol Field, ExpressionSyntax Initializer)> roots = [.. GetInitializers(structDeclarationSymbol)]; + + foreach (IFieldSymbol importedField in staticFieldDefinitions.Keys) + { + if (importedTypes.Add(importedField.ContainingType)) + { + roots.AddRange(GetInitializers(importedField.ContainingType).Where(root => importedFields.Contains(root.Field))); + } + } + + foreach ((IFieldSymbol field, ExpressionSyntax initializer) in roots) + { + token.ThrowIfCancellationRequested(); + + // The fields of the type whose initializer has not run when this one runs: this field and the ones after it + HashSet pending = new( + GetInitializers(field.ContainingType).SkipWhile(candidate => !SymbolEqualityComparer.Default.Equals(candidate.Field, field)).Select(candidate => candidate.Field), + SymbolEqualityComparer.Default); + HashSet visited = new(SymbolEqualityComparer.Default); + + WalkReachedNodes(initializer); + + void WalkReachedDeclaration(ISymbol symbol, SyntaxNode? root) + { + if (root is not null && visited.Add(symbol)) + { + WalkReachedNodes(root); + } + } + + void WalkReachedNodes(SyntaxNode root) + { + // The initializer can be the access or the call itself, so the root counts as a reached node too. + // A local function is entered from a call to it, the way a method is, rather than from the body + // it is declared in, that body not running it unless it calls it + foreach (SyntaxNode node in root.DescendantNodesAndSelf(descendant => descendant is not LocalFunctionStatementSyntax || ReferenceEquals(descendant, root))) + { + token.ThrowIfCancellationRequested(); + + if (node is not + (IdentifierNameSyntax or + MemberAccessExpressionSyntax or + InvocationExpressionSyntax or + BaseObjectCreationExpressionSyntax)) + { + continue; + } + + switch (semanticModel.For(node).GetOperation(node, token)) + { + case IFieldReferenceOperation { Field: { IsStatic: true, IsConst: false } reachedField } reference: + if (pending.Contains(reachedField)) + { + // A write to the field being initialized is overwritten by its initializer in + // the generated HLSL as well, so it is the one access that is not reported + if ((!IsWrite(reference) || !SymbolEqualityComparer.Default.Equals(reachedField, field)) && + reportedAccesses.Add(node)) + { + diagnostics.Add(StaticFieldAccessedBeforeInitialization, node, reachedField, field); + } + } + else if (!SymbolEqualityComparer.Default.Equals(reachedField.ContainingType, field.ContainingType) && + reachedField.TryGetSyntaxNode(token, out VariableDeclaratorSyntax? reachedDeclarator)) + { + WalkReachedDeclaration(reachedField, reachedDeclarator.Initializer?.Value); + } + + break; + case IInvocationOperation { TargetMethod: { MethodKind: MethodKind.LocalFunction } localFunction }: + WalkReachedDeclaration(localFunction, localFunction.TryGetSyntaxNode(token, out LocalFunctionStatementSyntax? localFunctionStatement) ? localFunctionStatement : null); + break; + case IInvocationOperation { TargetMethod: { } method }: + WalkReachedDeclaration(method, method.TryGetSyntaxNode(token, out MethodDeclarationSyntax? methodDeclaration) ? methodDeclaration : null); + break; + case IObjectCreationOperation { Constructor: { } constructor }: + WalkReachedDeclaration(constructor, constructor.TryGetSyntaxNode(token, out ConstructorDeclarationSyntax? constructorDeclaration) ? constructorDeclaration : null); + break; + } + } + } + } + } } \ No newline at end of file diff --git a/src/ComputeWeave.SourceGeneration.Hlsl/SyntaxRewriters/HlslSourceRewriter.Diagnostics.cs b/src/ComputeWeave.SourceGeneration.Hlsl/SyntaxRewriters/HlslSourceRewriter.Diagnostics.cs index a98244dcc..634f4e452 100644 --- a/src/ComputeWeave.SourceGeneration.Hlsl/SyntaxRewriters/HlslSourceRewriter.Diagnostics.cs +++ b/src/ComputeWeave.SourceGeneration.Hlsl/SyntaxRewriters/HlslSourceRewriter.Diagnostics.cs @@ -1,4 +1,3 @@ -using System.Collections.Generic; using ComputeWeave.SourceGeneration.Extensions; using ComputeWeave.SourceGeneration.Mappings; using Microsoft.CodeAnalysis; @@ -104,153 +103,6 @@ protected void ReportUnmappedMemberAccess(MemberAccessExpressionSyntax node, IMe } } - /// - /// Reports a read of a static field of the shader that its own initializer has reached. - /// - /// The read that closes the cycle, which the report is located on. - /// The field being read. - /// - /// - /// An entry with no type declaration is a field whose initializer is still being rewritten, so reaching it - /// means the initializer reads the field it writes. HLSL reads such a global as uninitialized, where C# - /// defines the same read as the default value of the type, so the two languages compute different values. - /// - /// - /// Only a field of the shader is answered here. A field of any other type is answered where an access to - /// an external static field is rewritten, and what reaches this site instead is left to the shader - /// compiler the way it already is: an initializer reading such a field by its name alone, and a read the - /// walk finds while following what a call reaches. - /// - /// - protected void ReportCyclicStaticFieldInitializer(SyntaxNode node, IFieldSymbol fieldSymbol) - { - if (IsClaimedStaticField(fieldSymbol)) - { - Diagnostics.Add(CyclicStaticFieldInitializer, node, fieldSymbol); - } - } - - /// - /// Reports the cycles an initializer closes through a static method of the shader. - /// - /// The called method, which is left for the generator to write out. - /// - /// - /// A static method of the shader is not imported by either rewriter, the generator writing those out - /// through a path of its own, and that path runs before any static field is claimed. No rewriting of such - /// a body therefore passes through the reporting sites while an initializer holds its claim, so what the - /// call reaches is walked here instead of being reported as it is rewritten. - /// - /// - /// The walk answers nothing unless a claim is outstanding, which is the case only while an initializer is - /// being rewritten. A method reading a static field is otherwise the ordinary shape this leaves alone. - /// - /// - /// What the walk reaches is every static method a reached declaration calls and the initializer of every - /// static field it reads, other than the initializer the walk was entered from. A call leaving the shader - /// type closes the cycle through a declaration of another type, and a field read closes it through the - /// initializer of a second field, neither of which is a shape a rewriting passes through while the claim - /// is held: the one is written out before any claim is taken, and the other is rewritten while the claim - /// is on the field being read rather than on the field the read closes the cycle on. - /// - /// - protected void ReportCyclicStaticFieldInitializerThroughShaderMethod(IMethodSymbol method) - { - if (!HasClaimedStaticField()) - { - return; - } - - HashSet visitedMethods = new(SymbolEqualityComparer.Default); - HashSet visitedFields = new(SymbolEqualityComparer.Default); - - WalkReachedMethod(method); - - void WalkReachedMethod(IMethodSymbol target) - { - if (!visitedMethods.Add(target) || - !target.TryGetSyntaxNode(CancellationToken, out MethodDeclarationSyntax? declaration)) - { - return; - } - - WalkReachedNodes(declaration); - } - - void WalkReachedField(IFieldSymbol target) - { - // The claimed field is the one whose initializer is being rewritten, so walking it would read the - // expression the rewriting is already on, and the read that closes the cycle is reported by then - if (IsClaimedStaticField(target) || - !visitedFields.Add(target) || - !target.TryGetSyntaxNode(CancellationToken, out VariableDeclaratorSyntax? declarator) || - declarator.Initializer is null) - { - return; - } - - WalkReachedNodes(declarator.Initializer.Value); - } - - void WalkReachedNodes(SyntaxNode root) - { - // An initializer can be the read or the call itself, so the root counts as a reached node too - foreach (SyntaxNode reachedNode in root.DescendantNodesAndSelf()) - { - CancellationToken.ThrowIfCancellationRequested(); - - // Only the three kinds a field read or a call can be written as are resolved, semantic - // information for every node being expensive enough that the rewriting pre-filters for it too. - // A read written as a member access resolves on the access alone, the name under it carrying - // no operation of its own, which was measured, so one read stays one report - if (reachedNode is not (IdentifierNameSyntax or MemberAccessExpressionSyntax or InvocationExpressionSyntax)) - { - continue; - } - - switch (SemanticModel.For(reachedNode).GetOperation(reachedNode, CancellationToken)) - { - case IFieldReferenceOperation { Field.IsStatic: true } fieldOperation: - ReportCyclicStaticFieldInitializer(reachedNode, fieldOperation.Field); - WalkReachedField(fieldOperation.Field); - break; - case IInvocationOperation { TargetMethod.IsStatic: true } invocationOperation: - WalkReachedMethod(invocationOperation.TargetMethod); - break; - } - } - } - } - - /// - /// Checks whether a static field is the one whose initializer is being rewritten. - /// - /// The field to check. - /// Whether is a field of the shader with no type declaration yet. - private bool IsClaimedStaticField(IFieldSymbol fieldSymbol) - { - return SymbolEqualityComparer.Default.Equals(fieldSymbol.ContainingType, ShaderType) && - StaticFieldDefinitions.TryGetValue(fieldSymbol, out HlslStaticField fieldInfo) && - fieldInfo.TypeDeclaration is null; - } - - /// - /// Checks whether any static field is claimed, meaning some initializer is being rewritten. - /// - /// Whether any entry in has no type declaration yet. - private bool HasClaimedStaticField() - { - foreach (HlslStaticField staticField in StaticFieldDefinitions.Values) - { - if (staticField.TypeDeclaration is null) - { - return true; - } - } - - return false; - } - /// /// Reports an element access that every mapping has declined, when HLSL has no indexer for the target. /// diff --git a/src/ComputeWeave.SourceGeneration.Hlsl/SyntaxRewriters/HlslSourceRewriter.cs b/src/ComputeWeave.SourceGeneration.Hlsl/SyntaxRewriters/HlslSourceRewriter.cs index fcb6eb99a..b43ec7d32 100644 --- a/src/ComputeWeave.SourceGeneration.Hlsl/SyntaxRewriters/HlslSourceRewriter.cs +++ b/src/ComputeWeave.SourceGeneration.Hlsl/SyntaxRewriters/HlslSourceRewriter.cs @@ -520,18 +520,12 @@ private ExpressionSyntax VisitConvertedConditionalArm(ExpressionSyntax node, Exp // and member access expressions, as those will be handled separately. Doing so avoids unnecessarily // retrieving semantic information for every identifier, which would otherwise be fairly expensive. if (node.Parent is not (InvocationExpressionSyntax or MemberAccessExpressionSyntax) && - SemanticModel.For(node).GetOperation(node, CancellationToken) is IFieldReferenceOperation operation) + SemanticModel.For(node).GetOperation(node, CancellationToken) is IFieldReferenceOperation operation && + operation.Field.IsConst && + operation.Type!.TypeKind != TypeKind.Enum && + TryRewriteConstantReference(operation.Field, out IdentifierNameSyntax? constantName)) { - if (operation.Field.IsConst && - operation.Type!.TypeKind != TypeKind.Enum && - TryRewriteConstantReference(operation.Field, out IdentifierNameSyntax? constantName)) - { - return constantName; - } - - // A field of the shader written by its name alone reaches no other reporting site: the rewriter for - // the body leaves it to this one, and the rewriter for an initializer maps constants alone - ReportCyclicStaticFieldInitializer(node, operation.Field); + return constantName; } return updatedNode; diff --git a/src/ComputeWeave.SourceGeneration.Hlsl/SyntaxRewriters/ShaderSourceRewriter.cs b/src/ComputeWeave.SourceGeneration.Hlsl/SyntaxRewriters/ShaderSourceRewriter.cs index 4f3d6ab62..722ebb4ba 100644 --- a/src/ComputeWeave.SourceGeneration.Hlsl/SyntaxRewriters/ShaderSourceRewriter.cs +++ b/src/ComputeWeave.SourceGeneration.Hlsl/SyntaxRewriters/ShaderSourceRewriter.cs @@ -472,7 +472,7 @@ private void MergeLocalFunctions(ShaderSourceRewriter rewriter) SemanticModel.For(node).GetOperation(node, CancellationToken) is IFieldReferenceOperation { Field.IsStatic: true } operation && !SymbolEqualityComparer.Default.Equals(operation.Field.ContainingType, ShaderType)) { - return ImportExternalStaticField(node, null, operation.Field) ?? base.VisitIdentifierName(node); + return ImportExternalStaticField(null, operation.Field) ?? base.VisitIdentifierName(node); } return base.VisitIdentifierName(node); @@ -530,14 +530,12 @@ public override SyntaxNode VisitMemberAccessExpression(MemberAccessExpressionSyn // is often the case if they're accessed from external types. So we just map the name and use that expression. if (SymbolEqualityComparer.Default.Equals(fieldOperation.Field.ContainingType, ShaderType)) { - ReportCyclicStaticFieldInitializer(node, fieldOperation.Field); - _ = HlslKnownKeywords.TryGetMappedName(fieldOperation.Field.Name, out string? mappedFieldName); return IdentifierName(mappedFieldName ?? fieldOperation.Field.Name); } - return ImportExternalStaticField(node, updatedNode, fieldOperation.Field); + return ImportExternalStaticField(updatedNode, fieldOperation.Field); } } @@ -767,10 +765,6 @@ public override SyntaxNode VisitMemberAccessExpression(MemberAccessExpressionSyn return updatedNode.WithExpression(IdentifierName(methodIdentifier)); } - - // A static method of the shader is left for the generator's own path, so a cycle an - // initializer closes through one is answered by walking it (see HlslSourceRewriter) - ReportCyclicStaticFieldInitializerThroughShaderMethod(method); } else { @@ -996,27 +990,17 @@ ArgumentSyntax[] ExtractArguments() /// /// Imports a static field declared in an external type and rewrites the read of it as needed. /// - /// The the access was written as, used as location. /// The rewritten to fall back to, if there is one. - /// The instance for . + /// The instance for the field being accessed. /// The rewritten static field expression. - /// - /// The location is taken from the node the author wrote and not from the field symbol, so that a cycle - /// closed from two declarations names both of the reads rather than naming the field declaration twice. - /// [return: NotNullIfNotNull(nameof(updatedNode))] - internal SyntaxNode? ImportExternalStaticField(SyntaxNode node, SyntaxNode? updatedNode, IFieldSymbol fieldSymbol) + internal SyntaxNode? ImportExternalStaticField(SyntaxNode? updatedNode, IFieldSymbol fieldSymbol) { + // A field with an entry is written out under the name the entry holds. An entry with no type + // declaration is one still being rewritten, so the initializer has reached the field it initializes, + // which is reported once every initializer is rewritten (see HlslDefinitionsSyntaxProcessor) if (StaticFieldDefinitions.TryGetValue(fieldSymbol, out HlslStaticField fieldInfo)) { - // An entry with no type declaration is one still being rewritten, so the initializer has - // reached the field it initializes. HLSL has no defined order for its global static - // initializers, so the value the shader reads here is not the one C# computes - if (fieldInfo.TypeDeclaration is null) - { - Diagnostics.Add(CyclicStaticFieldInitializer, node, fieldSymbol); - } - return IdentifierName(fieldInfo.Name); } @@ -1024,7 +1008,7 @@ ArgumentSyntax[] ExtractArguments() string name = fieldSymbol.GetFullyQualifiedMetadataName().ToHlslIdentifierName(); // Claim the entry before rewriting, the way an imported method and constructor already do, so that - // an initializer reaching itself is seen above rather than adding the same key a second time + // an initializer reaching itself finds the entry above rather than adding the same key a second time StaticFieldDefinitions.Add(fieldSymbol, (name, null, null, 0)); // Execute the same logic as for shader static fields, to process them and extract the relevant info diff --git a/src/ComputeWeave.SourceGeneration.Hlsl/SyntaxRewriters/StaticFieldRewriter.cs b/src/ComputeWeave.SourceGeneration.Hlsl/SyntaxRewriters/StaticFieldRewriter.cs index d25fc305d..438965b18 100644 --- a/src/ComputeWeave.SourceGeneration.Hlsl/SyntaxRewriters/StaticFieldRewriter.cs +++ b/src/ComputeWeave.SourceGeneration.Hlsl/SyntaxRewriters/StaticFieldRewriter.cs @@ -108,16 +108,12 @@ public override SyntaxNode VisitMemberAccessExpression(MemberAccessExpressionSyn { if (SymbolEqualityComparer.Default.Equals(staticFieldOperation.Field.ContainingType, ShaderType)) { - // An initializer naming the shader to reach the field it writes closes the cycle here, - // this being the only site a qualified read of it reaches - ReportCyclicStaticFieldInitializer(node, staticFieldOperation.Field); - _ = HlslKnownKeywords.TryGetMappedName(staticFieldOperation.Field.Name, out string? mappedFieldName); return IdentifierName(mappedFieldName ?? staticFieldOperation.Field.Name); } - return ImportExternalStaticField(node, updatedNode, staticFieldOperation.Field); + return ImportExternalStaticField(updatedNode, staticFieldOperation.Field); } if (HlslKnownProperties.TryGetMappedName(operation.Member.ToDisplayString(), out string? mapping)) @@ -150,7 +146,7 @@ public override SyntaxNode VisitMemberAccessExpression(MemberAccessExpressionSyn SemanticModel.For(node).GetOperation(node, CancellationToken) is IFieldReferenceOperation { Field.IsStatic: true } operation && !SymbolEqualityComparer.Default.Equals(operation.Field.ContainingType, ShaderType)) { - return ImportExternalStaticField(node, null, operation.Field) ?? base.VisitIdentifierName(node); + return ImportExternalStaticField(null, operation.Field) ?? base.VisitIdentifierName(node); } return base.VisitIdentifierName(node); @@ -159,16 +155,15 @@ public override SyntaxNode VisitMemberAccessExpression(MemberAccessExpressionSyn /// /// Imports a static field declared outside the shader, through the rewriter that imports every other declaration. /// - /// The the read was written as, used as location. /// The rewritten to fall back to, if there is one. - /// The instance for . + /// The instance for the field being accessed. /// The rewritten static field expression. [return: NotNullIfNotNull(nameof(updatedNode))] - private SyntaxNode? ImportExternalStaticField(SyntaxNode node, SyntaxNode? updatedNode, IFieldSymbol fieldSymbol) + private SyntaxNode? ImportExternalStaticField(SyntaxNode? updatedNode, IFieldSymbol fieldSymbol) { ShaderSourceRewriter shaderSourceRewriter = CreateImportRewriter(); - SyntaxNode? rewrittenNode = shaderSourceRewriter.ImportExternalStaticField(node, updatedNode, fieldSymbol); + SyntaxNode? rewrittenNode = shaderSourceRewriter.ImportExternalStaticField(updatedNode, fieldSymbol); MergeImportedLocalFunctions(shaderSourceRewriter); @@ -251,15 +246,11 @@ public override SyntaxNode VisitMemberAccessExpression(MemberAccessExpressionSyn // A static method with no mapping is imported by rewriting its declaration, the same way the // shader body imports one. HLSL accepts a call in a static field initializer because every // forward declaration is written ahead of the static fields. A method on the shader type is - // left alone for the generator to write out, so what it reaches is walked for a cycle instead. - if (method.IsStatic) + // left alone, as the generator writes those out through its own path. + if (method.IsStatic && + !SymbolEqualityComparer.Default.Equals(ShaderType, method.ContainingType)) { - if (!SymbolEqualityComparer.Default.Equals(ShaderType, method.ContainingType)) - { - return VisitImportedStaticMethodInvocation(node, updatedNode, method); - } - - ReportCyclicStaticFieldInitializerThroughShaderMethod(method); + return VisitImportedStaticMethodInvocation(node, updatedNode, method); } } diff --git a/src/ComputeWeave.SourceGenerators/ComputeShaderDescriptorGenerator.HlslSource.cs b/src/ComputeWeave.SourceGenerators/ComputeShaderDescriptorGenerator.HlslSource.cs index 2060c844e..236bc75d3 100644 --- a/src/ComputeWeave.SourceGenerators/ComputeShaderDescriptorGenerator.HlslSource.cs +++ b/src/ComputeWeave.SourceGenerators/ComputeShaderDescriptorGenerator.HlslSource.cs @@ -412,6 +412,15 @@ private static ImmutableArray GetStaticFields( } } + // An access C# performs before the field's initializer has run is reported once every initializer + // is rewritten, the walk covering the declarations a rewriting does not pass through again + HlslDefinitionsSyntaxProcessor.ReportStaticFieldAccessesBeforeInitialization( + structDeclarationSymbol, + staticFieldDefinitions, + semanticModel, + diagnostics, + token); + // The fields are written as one sequence, ordered by when each finished, which is after every // field its own initializer reached. A declared field and an imported one that finished with the // same count keep this order, the import having finished after the declared field did diff --git a/src/ComputeWeave.SourceGenerators/Diagnostics/DiagnosticDescriptors.cs b/src/ComputeWeave.SourceGenerators/Diagnostics/DiagnosticDescriptors.cs index ed0d35644..2e6fb6e18 100644 --- a/src/ComputeWeave.SourceGenerators/Diagnostics/DiagnosticDescriptors.cs +++ b/src/ComputeWeave.SourceGenerators/Diagnostics/DiagnosticDescriptors.cs @@ -1641,19 +1641,19 @@ partial class DiagnosticDescriptors helpLinkUri: "https://github.com/routersys/ComputeWeave"); /// - /// Gets a for a static field initializer reaching the field it initializes. + /// Gets a for a static field accessed before its initializer has run. /// - /// Format: "The static field {0} is initialized from an expression that reads it back, directly or through a declaration that initializer reaches". + /// Format: "The static field {0} is accessed while the initializer of {1} is running, before its own initializer has run (C# reads the default value of its type there and discards a write, which the generated HLSL does not reproduce)". /// /// - public static readonly DiagnosticDescriptor CyclicStaticFieldInitializer = new( + public static readonly DiagnosticDescriptor StaticFieldAccessedBeforeInitialization = new( id: "CMPW0124", - title: "Static field initializer reaching the field it initializes", - messageFormat: "The static field {0} is initialized from an expression that reads it back, directly or through a declaration that initializer reaches", + title: "Static field accessed before its initializer has run", + messageFormat: "The static field {0} is accessed while the initializer of {1} is running, before its own initializer has run (C# reads the default value of its type there and discards a write, which the generated HLSL does not reproduce)", category: "ComputeWeave.Shaders", defaultSeverity: DiagnosticSeverity.Error, isEnabledByDefault: true, - description: "C# runs the initializer once, and where the cycle closes it reads the field as its default value, so the field holds whatever that produces. HLSL has no defined order for global static initializers, so the value the shader computes is not the one C# computes, and the shader compiler accepts the source without saying anything: the generated HLSL was measured to compile with the cycle written out as it stands. Before this report the generator faulted instead, adding the same key to the collection of static field definitions twice, which discards the descriptors for every shader in the compilation unit and leaves the author with errors that name none of this.", + description: "C# runs the static field initializers of a type once, in the order the fields are declared. A field whose turn has not come yet holds the default value of its type: a read of it is that default, and a write to it is discarded when its initializer runs. Both happen when an initializer reaches a field of the same type declared after it, directly or through any declaration it reaches, and the read happens when an initializer reaches back into the field it initializes. The generated HLSL reproduces neither: the shader compiler folds the initializer the field carries when it can, so the read is the initialized value and the write stays, leaves the read undefined otherwise, and does not compile a direct read of a global declared later at all, so the shader either computes a value C# never produces or fails against generated code the author never wrote. A field carrying no initializer holds the default value in both and is not reported, and neither is a write to the field being initialized, which its initializer overwrites in both. Before this report an initializer reaching itself faulted the generator instead, adding the same key to the collection of static field definitions twice, which discards the descriptors for every shader in the compilation unit and leaves the author with errors that name none of this.", helpLinkUri: "https://github.com/routersys/ComputeWeave"); /// diff --git a/tests/ComputeWeave.D2D1.Tests.SourceGenerators/Test_D2DPixelShaderDescriptorGenerator_Diagnostics.cs b/tests/ComputeWeave.D2D1.Tests.SourceGenerators/Test_D2DPixelShaderDescriptorGenerator_Diagnostics.cs index d2f4f868a..722fe9c9c 100644 --- a/tests/ComputeWeave.D2D1.Tests.SourceGenerators/Test_D2DPixelShaderDescriptorGenerator_Diagnostics.cs +++ b/tests/ComputeWeave.D2D1.Tests.SourceGenerators/Test_D2DPixelShaderDescriptorGenerator_Diagnostics.cs @@ -1060,8 +1060,8 @@ public float4 Execute() /// that the method calls. /// /// - /// The shader's own method is written out before any field is claimed, so the call it makes is rewritten - /// while nothing is claimed. The walk answering this is shared with the compute generator, and each of the + /// The shader's own method is written out before any initializer is rewritten, so only a walk over what + /// the initializer reaches sees the read. The walk is shared with the compute generator, and each of the /// two carries the descriptor under its own identifier, so a row on one of them says nothing about the /// other. /// @@ -1100,14 +1100,14 @@ public float4 Execute() } /// - /// A cycle two fields of the shader close between their initializers, the first reaching the second - /// through a static method of the shader and the second reading the first back. + /// Two fields of the shader whose initializers reach each other, the first reaching the second through a + /// static method of the shader and the second reading the first back. /// /// - /// Only the field being initialized is claimed, so the initializer of the second field is what has to be - /// walked to reach the read that closes the cycle. The walk is shared with the compute generator, and each - /// of the two carries the descriptor under its own identifier, so a row on one of them says nothing about - /// the other. + /// C# runs the first initializer to completion before the second one starts, so what it reads through the + /// method is the second field before its initializer has run. The walk is shared with the compute + /// generator, and each of the two carries the descriptor under its own identifier, so a row on one of them + /// says nothing about the other. /// [TestMethod] public void AStaticFieldOfTheShaderReachedThroughAnotherFieldIsDiagnosed() @@ -1140,6 +1140,85 @@ public float4 Execute() CSharpGeneratorTest.VerifyDiagnosticIsReported(source, "CMPWD2D0096"); } + /// + /// A cycle a field of the shader closes through a method the shader body imported first. A declaration is + /// imported once, so the read closing the cycle is not rewritten again when the initializer calls it. + /// + /// + /// The walk is shared with the compute generator, and each of the two carries the descriptor under its own + /// identifier, so a row on one of them says nothing about the other. + /// + [TestMethod] + public void AStaticFieldOfTheShaderReachedThroughAMethodTheBodyImportedIsDiagnosed() + { + const string source = """ + using ComputeWeave; + using ComputeWeave.D2D1; + using float4 = global::ComputeWeave.Float4; + + namespace MyNamespace; + + internal static class Helper + { + public static float Go() => MyShader.Value * 2; + } + + [D2DInputCount(0)] + [D2DShaderProfile(D2D1ShaderProfile.PixelShader50)] + [D2DGeneratedPixelShaderDescriptor] + internal readonly partial struct MyShader : ID2D1PixelShader + { + internal static readonly float Value = Helper.Go(); + + public float4 Execute() + { + return Helper.Go() + Value; + } + } + """; + + CSharpGeneratorTest.VerifyDiagnosticIsReported(source, "CMPWD2D0096"); + } + + /// + /// A static field of the shader read through a static method of the shader from the initializer of a field + /// declared ahead of it. C# runs that initializer before the field's own, and reads the default value. + /// + /// + /// The walk is shared with the compute generator, and each of the two carries the descriptor under its own + /// identifier, so a row on one of them says nothing about the other. + /// + [TestMethod] + public void AStaticFieldOfTheShaderReadBeforeItsInitializerHasRunIsDiagnosed() + { + const string source = """ + using ComputeWeave; + using ComputeWeave.D2D1; + using float4 = global::ComputeWeave.Float4; + + namespace MyNamespace; + + [D2DInputCount(0)] + [D2DShaderProfile(D2D1ShaderProfile.PixelShader50)] + [D2DGeneratedPixelShaderDescriptor] + internal readonly partial struct MyShader : ID2D1PixelShader + { + private static readonly float Value = Twice(); + + private static readonly float Base = 5.0f; + + private static float Twice() => Base * 2; + + public float4 Execute() + { + return Value; + } + } + """; + + CSharpGeneratorTest.VerifyDiagnosticIsReported(source, "CMPWD2D0096"); + } + /// /// A static method of the shader reading a static field of it, reached from the body rather than from that /// field's initializer, which is the ordinary shape the walk answering the rows above runs over. diff --git a/tests/ComputeWeave.Tests.SourceGenerators/Shaders/ShaderGeneratorDiagnosticTests.cs b/tests/ComputeWeave.Tests.SourceGenerators/Shaders/ShaderGeneratorDiagnosticTests.cs index c54fc8969..37b60b4c5 100644 --- a/tests/ComputeWeave.Tests.SourceGenerators/Shaders/ShaderGeneratorDiagnosticTests.cs +++ b/tests/ComputeWeave.Tests.SourceGenerators/Shaders/ShaderGeneratorDiagnosticTests.cs @@ -1,4 +1,5 @@ using System.Linq; +using System.Text.RegularExpressions; using ComputeWeave.SourceGenerators; using ComputeWeave.Tests.SourceGenerators.Helpers; using Microsoft.CodeAnalysis; @@ -385,7 +386,8 @@ public void Execute() /// /// A field of the shader is reached on paths of its own: neither rewriter imports the shader's own /// declarations, and the generator writes its static methods out before it rewrites any initializer, so - /// there is a row per way the read can be written and per kind of declaration the initializer reaches. + /// there is a row per way the read can be written and per kind of declaration the initializer reaches. The + /// last row reaches it through a constructor, which the walk follows the way it follows a method. /// [TestMethod] [DataRow( @@ -443,9 +445,8 @@ public void AStaticFieldOfTheShaderReachingItselfIsDiagnosed(string assemblyName /// /// /// The read is written as a member access on the shader type, that being the only name an external - /// declaration can reach the field by. The two rows are different reporting sites: the rewriter for a body - /// answers the first as it rewrites the import, and the walk answers the second, the shader's own method - /// not being imported by either rewriter. + /// declaration can reach the field by. The second row passes back through a static method of the shader, + /// which is never imported, so the walk over what the initializer reaches is the only thing that sees it. /// [TestMethod] [DataRow( @@ -491,10 +492,9 @@ public void Execute() /// that the method calls. The second row imports that declaration from the initializer as well. /// /// - /// The shader's own method is written out before any field is claimed, so the call it makes is rewritten - /// while nothing is claimed and reports nothing. The second row is what a claim alone cannot answer: the - /// initializer imports the same declaration, and an import is rewritten once, so by the time the claim is - /// taken the read that closes the cycle has already been written out. + /// The shader's own method is written out before any initializer is rewritten, and the declaration of the + /// other type is imported once, from that method, so the read closing the cycle passes through no rewriting + /// of the initializer on either row: only a walk over what the initializer reaches sees it. /// [TestMethod] [DataRow( @@ -524,16 +524,15 @@ internal static class Helper } /// - /// A cycle two or more fields close between their initializers, the first reaching the second through a - /// static method of the shader and the second reading the first back. + /// Two or more fields of the shader whose initializers reach each other, the first reaching the second + /// through a static method of the shader and the second reading the first back. /// /// - /// Only the field being initialized is claimed, so the read of the second field reports nothing and the - /// initializer of that field is what has to be walked to reach the read that closes the cycle. These are - /// the shapes the shader compiler accepts: the read of a field declared later goes through a function, - /// which is forward declared, where reading it directly does not. The last row leaves the shader type, - /// where nothing claims the field whose initializer closes the cycle, that initializer having been - /// written out before the claim on the field being read. + /// C# runs the first initializer to completion before the second one starts, so what it reads through the + /// method is the second field before its initializer has run, and that read is the one report: the second + /// initializer then reads a field whose initializer has already run. These are the shapes the shader + /// compiler accepts: the read of a field declared later goes through a function, which is forward declared, + /// where reading it directly does not, so nothing but this report tells the author. /// [TestMethod] [DataRow( @@ -568,33 +567,44 @@ internal static class Helper private static float Second() => Other * 2; """)] - [DataRow( - "ShaderStaticFieldCycleThroughAnImportedFieldTests", - """ - internal static class Helper - { - public static readonly float Amount = Shader.First * 2; - } - """, - """ - internal static readonly float First = Twice(); - - internal static float Twice() => Helper.Amount; - """)] public void AStaticFieldOfTheShaderReachedThroughAnotherFieldIsDiagnosed(string assemblyName, string declarations, string members) { AssertReportsAt(ShaderWithStaticFields(members, "this.buffer[0] = First;", declarations), assemblyName, "CMPW0124", 1); } + /// + /// A field of the shader and a field of another type whose initializers reach each other. + /// + /// + /// Whichever type C# initializes first, its initializer starts the other one and reads its own field back + /// through it before that initializer has run, so each of the two reads is the read a run of the program + /// performs too early, and both are reported. Neither of the two is the shape one type can fix alone. + /// + [TestMethod] + public void AStaticFieldOfTheShaderAndAnImportedFieldReachingEachOtherAreDiagnosed() + { + const string Declarations = """ + internal static class Helper + { + public static readonly float Amount = Shader.First * 2; + } + """; + + const string Members = """ + internal static readonly float First = Twice(); + + internal static float Twice() => Helper.Amount; + """; + + AssertReportsAt(ShaderWithStaticFields(Members, "this.buffer[0] = First;", Declarations), "ShaderStaticFieldCycleThroughAnImportedFieldTests", "CMPW0124", 2); + } + /// /// A cycle the initializer of a field of the shader closes twice over, once by reading the field itself /// and once through a static method of the shader that reads it back. /// /// - /// The two reads are answered by different sites, the one in the initializer as it is rewritten and the - /// one in the method by the walk, and each is a place the author has to change. The walk does not reach - /// the first a second time: the field whose initializer it would walk into is the one being initialized, - /// which it leaves to the rewriting it was entered from. + /// Each read is a place the author has to change, so each is reported, and at a location of its own. /// [TestMethod] public void AStaticFieldOfTheShaderReachingItselfTwiceIsDiagnosedAtEachRead() @@ -609,16 +619,17 @@ public void AStaticFieldOfTheShaderReachingItselfTwiceIsDiagnosedAtEachRead() } /// - /// Two fields the walk reaches whose initializers read each other. Neither is the field being - /// initialized, so neither closes a cycle on it, and the walk has to stop rather than follow them round. + /// A field reaching a pair of fields whose initializers read each other. C# runs the three initializers in + /// order, so the first reads the second before its turn and the second reads the third before its turn, + /// while the third reads a field whose initializer has already run. /// /// - /// The set is not pinned as a whole here. The pair reads a global declared after it, which the shader - /// compiler refuses under an identifier of its own, and what this row holds is that the walk terminates - /// and reports no cycle for the field it was entered for. + /// The pair read directly used to be left to the shader compiler, which refuses the read of a global + /// declared later under an identifier of its own, naming generated code. The two reads C# performs too + /// early are reported, and the one it does not perform too early is not. /// [TestMethod] - public void APairOfStaticFieldsTheWalkReachesIsNotFollowedRound() + public void APairOfStaticFieldsReadingEachOtherIsDiagnosedAtTheReadsPerformedTooEarly() { const string Members = """ private static readonly float Value = Twice(); @@ -630,17 +641,18 @@ public void APairOfStaticFieldsTheWalkReachesIsNotFollowedRound() private static float Twice() => Left * 2; """; - AssertReportsAt(ShaderWithStaticFields(Members), "ShaderStaticFieldPairReachedByTheWalkTests", "CMPW0124", 0); + AssertReportsAt(ShaderWithStaticFields(Members), "ShaderStaticFieldPairReachedByTheWalkTests", "CMPW0124", 2); } /// - /// The shapes the walk goes over without reaching a cycle: a declaration of another type called past the - /// shader's own method, a static field of another type, and a second field of the shader. + /// The shapes the walk goes over without reaching a read performed too early: a declaration of another + /// type called past the shader's own method, a static field of another type, an earlier field of the + /// shader read through the method, and a later field of the shader carrying no initializer, which holds + /// the default value of its type in C# and in the generated HLSL alike. /// /// - /// A field of another type is left alone rather than walked, its initializer being rewritten under a claim - /// of its own, and a field of the shader is walked into an initializer that reads nothing back. Every row - /// reaches the walk, so a walk reporting on what it reaches rather than on the claim refuses all three. + /// Every row reaches the walk, so a walk reporting on what it reaches rather than on what C# reads before + /// its initializer has run refuses all four. /// [TestMethod] [DataRow( @@ -670,12 +682,22 @@ internal static class Helper private static float Twice() => Helper.Amount * 2; """)] [DataRow( - "ShaderStaticFieldReachingASecondFieldTests", + "ShaderStaticFieldReachingAnEarlierFieldTests", + "", + """ + private static readonly float Base = 5.0f; + + private static readonly float Value = Twice(); + + private static float Twice() => Base * 2; + """)] + [DataRow( + "ShaderStaticFieldReachingALaterFieldWithoutAnInitializerTests", "", """ private static readonly float Value = Twice(); - private static readonly float Base = 5.0f; + private static float Base; private static float Twice() => Base * 2; """)] @@ -689,9 +711,8 @@ public void AStaticFieldOfTheShaderReachingDeclarationsWithoutACycleIsNotDiagnos /// /// /// The first row is what separates a cycle from the ordinary shape: the same method reads the same field, - /// and only the initializer reaching that method makes it one. All three are refused by a report that does - /// not read the claim, and the last one by a claim that outlives the initializer it was taken for, so they - /// are what keeps the refusal to the field being initialized while its initializer is being rewritten. + /// and only the initializer reaching that method makes it one. All three are refused by a walk that reports + /// what it reaches rather than what C# reads before its initializer has run. /// [TestMethod] [DataRow( @@ -723,6 +744,335 @@ public void AStaticFieldOfTheShaderWithoutACycleIsNotDiagnosed(string assemblyNa AssertReportsNothing(ShaderWithStaticFields(members, body), assemblyName); } + /// + /// A cycle a field closes through a declaration something else imported first: the shader body, an earlier + /// field, or the very import that reached the field. A declaration is imported once, so the read closing + /// the cycle is not rewritten again when the initializer calls it, and only a walk over what the + /// initializer reaches sees it. + /// + /// + /// The rows are the declaration kinds an initializer can reach, a static method, an instance method and a + /// constructor, and the places that import one first. The last row calls the same method twice from one + /// initializer, which is one read and so one report. + /// + [TestMethod] + [DataRow( + "ShaderStaticFieldCycleThroughAMethodTheBodyImportedTests", + "internal static class Helper { public static float Go() => Shader.Value * 2; }", + "internal static readonly float Value = Helper.Go();", + "this.buffer[0] = Helper.Go() + Value;", + 1)] + [DataRow( + "ShaderStaticFieldCycleThroughAMethodAnEarlierFieldImportedTests", + "internal static class Helper { public static float Go() => Shader.Second * 2; }", + """ + internal static readonly float First = Helper.Go(); + + internal static readonly float Second = Helper.Go(); + """, + "this.buffer[0] = First + Second;", + 1)] + [DataRow( + "ShaderStaticFieldCycleThroughAConstructorTheBodyImportedTests", + """ + internal struct Box + { + public float Amount; + + public Box(float amount) + { + Amount = amount + Shader.Value; + } + + public static float Read(Box box) => box.Amount; + } + """, + "internal static readonly float Value = Box.Read(new Box(1.0f));", + "this.buffer[0] = Box.Read(new Box(1.0f)) + Value;", + 1)] + [DataRow( + "ShaderStaticFieldCycleThroughAnInstanceMethodTheBodyImportedTests", + """ + internal struct Box + { + public float Amount; + + public Box(float amount) + { + Amount = amount; + } + + public float Read() => Amount + Shader.Value; + } + """, + "internal static readonly float Value = new Box(1.0f).Read();", + "this.buffer[0] = new Box(1.0f).Read() + Value;", + 1)] + [DataRow( + "ShaderStaticFieldCycleThroughAMethodCalledTwiceTests", + "internal static class Helper { public static float Go() => Shader.Value * 2; }", + "internal static readonly float Value = Helper.Go() + Helper.Go();", + "this.buffer[0] = Value;", + 1)] + public void AStaticFieldReachedThroughADeclarationImportedEarlierIsDiagnosed(string assemblyName, string declarations, string members, string body, int expectedCount) + { + AssertReportsAt(ShaderWithStaticFields(members, body, declarations), assemblyName, "CMPW0124", expectedCount); + } + + /// + /// A field of another type whose initializer calls the method that reached it, that method being in the + /// middle of its own import when the field is imported, so its body is not rewritten again either. + /// + [TestMethod] + public void AnExternalStaticFieldReachedThroughTheMethodImportingItIsDiagnosed() + { + const string Declarations = """ + internal static class Helper + { + public static readonly float Value = Go(); + + public static float Go() => Value * 2; + } + """; + + AssertReportsAt(ShaderWithStaticFields("", "this.buffer[0] = Helper.Go();", Declarations), "ExternalStaticFieldCycleThroughTheImportingMethodTests", "CMPW0124", 1); + } + + /// + /// A static field read by an initializer that C# runs before the initializer of that field: a field of + /// the same type declared after the one being initialized, read directly, through a method of the shader, + /// through an imported method, or from the initializer of an imported field. + /// + /// + /// C# reads the default value of the type there. The shader compiler folds the initializer the later field + /// carries when the read goes through a function, so the generated HLSL computes the initialized value + /// instead, and it does not compile the direct read at all, naming generated code. The report names the + /// read and the initializer that performs it too early. + /// + [TestMethod] + [DataRow( + "ShaderStaticFieldReadingALaterFieldDirectlyTests", + "", + """ + private static readonly float Value = Base * 2; + + private static readonly float Base = 5.0f; + """)] + [DataRow( + "ShaderStaticFieldReadingALaterFieldThroughItsOwnMethodTests", + "", + """ + private static readonly float Value = Twice(); + + private static readonly float Base = 5.0f; + + private static float Twice() => Base * 2; + """)] + [DataRow( + "ShaderStaticFieldReadingALaterFieldThroughAnImportedMethodTests", + "internal static class Helper { public static float Go() => Shader.Base * 2; }", + """ + internal static readonly float Value = Helper.Go(); + + internal static readonly float Base = 5.0f; + """)] + [DataRow( + "ShaderStaticFieldReadingALaterFieldFromAnImportedInitializerTests", + "internal static class Helper { public static readonly float Doubled = Shader.Base * 2; }", + """ + internal static readonly float Value = Helper.Doubled; + + internal static readonly float Base = 5.0f; + """)] + [DataRow( + "ExternalStaticFieldReadingALaterFieldDirectlyTests", + """ + internal static class Helper + { + public static readonly float Value = Base * 2; + + public static readonly float Base = 5.0f; + } + """, + "internal static readonly float Value = Helper.Value;")] + [DataRow( + "ExternalStaticFieldReadingALaterFieldThroughAMethodTests", + """ + internal static class Helper + { + public static readonly float Value = Twice(); + + public static readonly float Base = 5.0f; + + public static float Twice() => Base * 2; + } + """, + "internal static readonly float Value = Helper.Value;")] + public void AStaticFieldReadBeforeItsInitializerHasRunIsDiagnosed(string assemblyName, string declarations, string members) + { + AssertReportsAt(ShaderWithStaticFields(members, declarations: declarations), assemblyName, "CMPW0124", 1); + } + + /// + /// A read inside a local function of a method the initializer reaches. C# runs the local function only + /// when the method calls it, so the read is performed too early only then, whereas the generated HLSL + /// lifts the function out whether or not it is called. + /// + [TestMethod] + [DataRow("ShaderStaticFieldReadInsideACalledLocalFunctionTests", "return Read();", true)] + [DataRow("ShaderStaticFieldReadInsideAnUncalledLocalFunctionTests", "return 2.0f;", false)] + public void AStaticFieldReadInsideALocalFunctionIsDiagnosedWhenTheFunctionIsCalled(string assemblyName, string body, bool isCalled) + { + string members = $$""" + private static readonly float Value = Twice(); + + private static float Twice() + { + static float Read() => Value * 2; + + {{body}} + } + """; + + if (isCalled) + { + AssertReportsAt(ShaderWithStaticFields(members), assemblyName, "CMPW0124", 1); + } + else + { + AssertReportsNothing(ShaderWithStaticFields(members), assemblyName); + } + } + + /// + /// A static field written by an initializer that C# runs before the initializer of that field, through an + /// assignment or an out argument in a method of the shader. C# discards the write when the initializer of + /// the field runs, whereas the shader compiler folds that initializer into the declaration and keeps the + /// write. + /// + [TestMethod] + [DataRow( + "ShaderStaticFieldWritingALaterFieldTests", + """ + private static readonly float Value = Init(); + + private static float Base = 5.0f; + + private static float Init() + { + Base = 2.0f; + + return 1.0f; + } + """)] + [DataRow( + "ShaderStaticFieldWritingALaterFieldThroughAnOutArgumentTests", + """ + private static readonly float Value = Init(); + + private static float Base = 5.0f; + + private static void Set(out float target) + { + target = 2.0f; + } + + private static float Init() + { + Set(out Base); + + return 1.0f; + } + """)] + public void AStaticFieldWrittenBeforeItsInitializerHasRunIsDiagnosed(string assemblyName, string members) + { + AssertReportsAt(ShaderWithStaticFields(members), assemblyName, "CMPW0124", 1); + } + + /// + /// The writes C# and the generated HLSL agree on: one to a later field carrying no initializer, which both + /// keep, and one to the field being initialized, through an assignment or an out argument, which both lose + /// to the initializer running after it. + /// + /// + /// The set is pinned empty rather than the identifier counted, so the rows also show the shader compiles. + /// + [TestMethod] + [DataRow( + "ShaderStaticFieldWritingALaterFieldWithoutAnInitializerTests", + """ + private static readonly float Value = Init(); + + private static float Base; + + private static float Init() + { + Base = 2.0f; + + return 1.0f; + } + """)] + [DataRow( + "ShaderStaticFieldWritingItselfTests", + """ + private static float Value = Init(); + + private static float Init() + { + Value = 2.0f; + + return 1.0f; + } + """)] + [DataRow( + "ShaderStaticFieldWritingItselfThroughAnOutArgumentTests", + """ + private static float Value = Init(); + + private static void Set(out float target) + { + target = 2.0f; + } + + private static float Init() + { + Set(out Value); + + return 1.0f; + } + """)] + public void AStaticFieldWrittenWhereCSharpAndTheGeneratedHlslAgreeIsNotDiagnosed(string assemblyName, string members) + { + AssertReportsNothing(ShaderWithStaticFields(members), assemblyName); + } + + /// + /// The message names the field accessed and the initializer performing the access, in that order, each + /// access being attributed to the initializer whose walk reached it. + /// + /// + /// The count and the location cannot tell the two names apart, so the messages are read. + /// + [TestMethod] + public void AStaticFieldAccessIsAttributedToTheInitializerPerformingIt() + { + const string Members = """ + private static readonly float First = Second + 1; + + private static readonly float Second = Third * 2; + + private static readonly float Third = 5.0f; + """; + + string[] attributions = [.. RunDiagnostics(ShaderWithStaticFields(Members, "this.buffer[0] = First;"), "ShaderStaticFieldAccessAttributionTests") + .Where(static diagnostic => diagnostic.Id == "CMPW0124") + .Select(static diagnostic => Regex.Match(diagnostic.GetMessage(), @"field \S*?(\w+) is accessed while the initializer of \S*?(\w+) is running")) + .Select(static match => $"{match.Groups[1].Value} in {match.Groups[2].Value}") + .Order()]; + + Assert.AreEqual("Second in First, Third in Second", string.Join(", ", attributions)); + } + private static string ShaderWithStaticFields(string members, string body = "this.buffer[0] = Value;", string declarations = "") { return $$""" @@ -867,14 +1217,27 @@ public void Execute() """; } + /// + /// Asserts that an identifier is reported a given number of times, each at a location of its own. + /// + /// + /// A report is one read the author has to change, so two reports at one location would be one read + /// reported twice, which is what a count alone cannot tell from two reads. + /// private static void AssertReportsAt(string source, string assemblyName, string expectedId, int expectedCount) { - string[] actualIds = Run(source, assemblyName); + Diagnostic[] actualDiagnostics = RunDiagnostics(source, assemblyName); + Diagnostic[] expectedDiagnostics = [.. actualDiagnostics.Where(diagnostic => diagnostic.Id == expectedId)]; + + Assert.AreEqual( + expectedCount, + expectedDiagnostics.Length, + $"{expectedId} is not reported {expectedCount} time(s): {string.Join(", ", actualDiagnostics.Select(static diagnostic => diagnostic.Id).Order())}"); Assert.AreEqual( expectedCount, - actualIds.Count(id => id == expectedId), - $"{expectedId} is not reported {expectedCount} time(s): {string.Join(", ", actualIds)}"); + expectedDiagnostics.Select(static diagnostic => diagnostic.Location.SourceSpan).Distinct().Count(), + $"{expectedId} is reported more than once at one location: {string.Join(", ", expectedDiagnostics.Select(static diagnostic => diagnostic.Location.GetLineSpan().StartLinePosition))}"); } private static void AssertReports(string source, string assemblyName, string expectedId) @@ -901,6 +1264,12 @@ private static void AssertIsNotHandedToTheCompiler(string source, string assembl } private static string[] Run(string source, string assemblyName) + { + // Not made distinct, so that a row can assert how many times an identifier was reported + return [.. RunDiagnostics(source, assemblyName).Select(static diagnostic => diagnostic.Id).Order()]; + } + + private static Diagnostic[] RunDiagnostics(string source, string assemblyName) { CSharpCompilation compilation = CompilationHelper.CreateCompilation( [source], @@ -911,7 +1280,6 @@ private static string[] Run(string source, string assemblyName) Assert.IsNull(result.Exception, result.Exception?.ToString()); - // Not made distinct, so that a row can assert how many times an identifier was reported - return [.. result.Diagnostics.Select(static diagnostic => diagnostic.Id).Order()]; + return [.. result.Diagnostics]; } } diff --git a/tests/ComputeWeave.Tests.SourceGenerators/Shaders/StaticFieldInitializerTests.cs b/tests/ComputeWeave.Tests.SourceGenerators/Shaders/StaticFieldInitializerTests.cs index 554e3964c..4a2cd7726 100644 --- a/tests/ComputeWeave.Tests.SourceGenerators/Shaders/StaticFieldInitializerTests.cs +++ b/tests/ComputeWeave.Tests.SourceGenerators/Shaders/StaticFieldInitializerTests.cs @@ -637,8 +637,8 @@ public void AnExternalStaticFieldReadByNameAloneIsImported() } /// - /// A read that closes a cycle is answered by the report the import already carries, rather than by the - /// fault that claiming the entry exists to stop. + /// A read that closes a cycle is answered by a report rather than by the fault that claiming the entry + /// exists to stop, the import writing the read out under the claimed name and the walk reporting it. /// [TestMethod] public void AReadThatClosesACycleIsReported()