Open
Conversation
WalkthroughAdds opt-in constraint/validation handling and MethodImpl metadata to the newtype generator: new attribute members, model and extractor changes to record validation-method metadata and locations, generator diagnostics for malformed/multiple validators, constraint-aware code emission (constructor validation/throwing, DEBUG gating), tests, and analyzer release metadata. Changes
Sequence DiagramsequenceDiagram
participant SourceGenerator as Code Generator
participant AttrParser as Attribute Parser
participant ModelExtractor as Model Extractor
participant CodeGen as Constructor CodeGen
participant Diagnostics as Diagnostic Emitter
SourceGenerator->>AttrParser: Parse `newtype` attribute (includes Options/MethodImpl)
AttrParser->>ModelExtractor: Provide attribute args & aliased type
ModelExtractor->>ModelExtractor: Locate validation method, build ConstraintModel (name, IncludeInRelease, valid/multiple, location)
ModelExtractor->>SourceGenerator: Return AliasModel with ConstraintModel & ExtractedOptions
alt Constraints requested and validator invalid/multiple
SourceGenerator->>Diagnostics: Emit NEWTYPE001 / NEWTYPE002
SourceGenerator-->>SourceGenerator: Short-circuit generation for this type
else
SourceGenerator->>CodeGen: Request constructor emission for AliasModel
alt Use constraints and valid validator
CodeGen->>CodeGen: Emit multi-line ctor, call validator, throw if invalid (DEBUG gating / IncludeInRelease)
else
CodeGen->>CodeGen: Emit simple single-line value constructor
end
CodeGen->>SourceGenerator: Return generated source
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
NewType.Generator/AliasModel.cs (1)
11-64:⚠️ Potential issue | 🟡 MinorUpdate the model summary and validation flag naming.
The summary still says there are no Roslyn types, but
Location?is now part of the model. Also,validValidationMethodbreaks PascalCase consistency; considerHasValidValidationMethod(update usages accordingly).✏️ Suggested doc tweak
-/// Contains only strings, bools, plain enums, and EquatableArrays — no Roslyn symbols. +/// Contains only strings, bools, plain enums, EquatableArrays, and Location for diagnostics.
🤖 Fix all issues with AI agents
In `@NewType.Generator/AliasCodeGenerator.cs`:
- Around line 141-173: When _model.IncludeConstraints is true, forwarded
constructors emitted by AppendForwardedConstructor/for each ctor in
_model.ForwardedConstructors currently assign _value directly and bypass
validation; modify AppendForwardedConstructor so forwarded constructors route to
the validated primary constructor (the ctor taking _model.AliasedTypeFullName
and running AppendConstraintChecker) or explicitly call AppendConstraintChecker
before assigning _value, ensuring every path that sets _value (including
constructors generated for ForwardedConstructors) enforces the same IsValid
checks for _model.TypeName.
In `@NewType.Generator/AliasGenerator.cs`:
- Around line 128-142: The diagnostic is being reported after emitting the
generated file; move the validation check so that you call
context.ReportDiagnostic(...) (using MissingIsValidMethodDiagnostic,
model.Location, model.TypeName, model.AliasedTypeFullName) and return if
model.IncludeConstraints && !model.validValidationMethod before calling
context.AddSource(...) (the current call that constructs fileName and invokes
context.AddSource with SourceText.From(source, Encoding.UTF8)); alternatively,
when the validation fails simply skip the context.AddSource(...) path so no
source is emitted for that model.
In `@NewType.Generator/AliasModelExtractor.cs`:
- Around line 365-379: HasValidIsValid currently detects a matching IsValid
method but does not verify it's static, which lets an instance method slip
through and causes generated code that calls IsValid(value) to fail to compile;
update HasValidIsValid to also check that the found IMethodSymbol.IsStatic is
true when selecting members from targetType via
AliasModel.ConstraintValidationMethodSymbol (and keep the other checks: return
type boolean and single parameter type equals aliasedType) so only static
validation methods are accepted.
In `@NewType.Generator/AnalyzerReleases.Unshipped.md`:
- Around line 1-8: The release entry for NEWTYPE001 uses category `<Undetected>`
but the DiagnosticDescriptor in AliasGenerator (check the
DiagnosticDescriptor/constructor in AliasGenerator.cs where the category is set
to "Unknown") declares the category as "Unknown"; update the Unshipped release
table row for NEWTYPE001 in NewType.Generator/AnalyzerReleases.Unshipped.md to
use `Unknown` as the Category so the table reads: NEWTYPE001 | `Unknown` | Error
| AliasGenerator.
🧹 Nitpick comments (5)
NewType.Generator/AliasAttributeSource.cs (1)
38-79: ExposeConstraintOptionson the non-generic attribute for parity.The non-generic
newtypeAttributeis the fallback API; withoutConstraintOptions, constraint-enabled aliases can’t be expressed there. Consider adding the same property to keep feature parity.🧩 Suggested addition (non-generic attribute)
public NewtypeOptions Options { get; set; } +/// <summary>Controls which constraint features the generator emits.</summary> +public NewtypeConstraintOptions ConstraintOptions { get; set; } + /// <summary> /// Overrides the MethodImplOptions applied to generated members. /// Default is <see cref="global::System.Runtime.CompilerServices.MethodImplOptions.AggressiveInlining"/>.NewType.Generator/AliasGenerator.cs (1)
15-24: Consider a more descriptive diagnostic category.The category
"Unknown"at line 20 is not informative. Roslyn diagnostic categories help users filter and organize warnings/errors. Consider using a category like"NewType"or"CodeGeneration"to align with the analyzer's purpose.Suggested change
- category: "Unknown", + category: "NewType",NewType.Tests/ConstraintValidationTests.cs (2)
7-19: Add explicit assertions to verify test intent.These tests rely on "no exception thrown" as the success criterion, but this is implicit. Consider adding explicit assertions to make the test intent clear and catch potential regressions (e.g., if the constructor becomes a no-op).
Suggested improvement
[Fact] public void Direction_Valid() { //doesn't throw var dir = new Direction(new Vector2(0, 1)); + Assert.Equal(new Vector2(0, 1), dir.Value); } [Fact] public void Direction_ValidImplicit() { //doesn't throw Direction dir = new Vector2(0, 1); + Assert.Equal(new Vector2(0, 1), dir.Value); }
33-39: Add assertion to verify the operation result.Similar to the construction tests, this test should assert on the result value to verify the operation completed correctly, not just that it didn't throw.
Suggested improvement
[Fact] public void Direction_ForwardedOperation_ValidResult() { var dir1 = new Direction(new Vector2(0, 1f)); var dir2 = new Direction(new Vector2(0, 1f)); var dir3 = dir1 * dir2; + // Assert expected result of multiplication + Assert.NotNull(dir3); }NewType.Generator/AliasModelExtractor.cs (1)
60-62: Minor: trailing whitespace.Lines 61-62 have trailing whitespace. Consider removing for cleaner formatting.
Suggested fix
var useConstraints = (allOptions.ConstraintOptions & OptionsUseConstraints) != 0; - - + return new AliasModel(
tvenclovas96
force-pushed
the
constraints
branch
from
af180e1 to
7faa613
Compare
Author
|
You are absolutely correct Mr. AI! (Mostly) |
Author
|
Thinking if a separate attribute on the validation method would be more ergonomic/intuitive/clear what is happening 🤔 |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
NewType.Generator/AliasAttributeSource.cs (1)
94-120:⚠️ Potential issue | 🟡 MinorNon-generic attribute is missing
ConstraintOptionsproperty.The generic
newtypeAttribute<T>(line 78) includes the newConstraintOptionsproperty, but the non-genericnewtypeAttributeclass does not. This inconsistency means users of the non-generic attribute cannot enable constraint validation.Proposed fix
/// <summary>Controls which features the generator emits.</summary> public NewtypeOptions Options { get; set; } + /// <summary>Controls which constraint features the generator emits.</summary> + public NewtypeConstraintOptions ConstraintOptions { get; set; } + /// <summary> /// Overrides the MethodImplOptions applied to generated members.
🤖 Fix all issues with AI agents
In `@NewType.Generator/AliasCodeGenerator.cs`:
- Around line 150-163: The constructor body indentation is inconsistent: in
AliasCodeGenerator.cs when _model.IncludeConstraints is true you append the
assignment using three SingleIndent units which ignores memberIndent and
mis-indents types without namespaces; change the code that writes the body (the
AppendConstraintChecker call and the line writing "_value = value;") to build
indentation relative to memberIndent (e.g., append memberIndent then one
SingleIndent before body lines) instead of using SingleIndent repeated three
times so the body is always indented exactly one level deeper than memberIndent;
keep AppendConstraintChecker("SingleIndent", "value") usage but ensure the
subsequent "_value = value;" write uses memberIndent + SingleIndent.
- Around line 690-705: The forwarded constructor uses hardcoded SingleIndent
repetitions which don't respect the current member indentation; change both
branches to build indentation from the existing indent variable plus
SingleIndent(s) instead of appending SingleIndent three times. Specifically, in
the block that calls AppendConstraintChecker and sets _value, replace the
_sb.Append(SingleIndent).Append(SingleIndent).Append(SingleIndent)... call with
something that starts from indent and then appends the needed SingleIndent(s)
(for example _sb.Append(indent).Append(SingleIndent).Append(SingleIndent) before
writing the "_value" assignment), and in the else branch replace the single-line
AppendLine with a properly indented multi-line emission that uses indent +
SingleIndent(s) so the forwarded constructor matches the same member-indent
level; reference AppendConstraintChecker, _sb, indent, SingleIndent,
_model.TypeName and _model.AliasedTypeFullName to locate the lines to change.
- Around line 728-743: AppendConstraintChecker currently hardcodes triple
indentation by calling .Append(indent) three times; instead use the passed-in
indent as the correct "body" indent: emit the if line with a single
.Append(indent).AppendLine($"if (!IsValid({valueName}))"), and emit the throw
line with the body indent plus one more indent (e.g.
.Append(indent).Append(indent).AppendLine(...)) so nesting is correct; keep the
`#if` DEBUG guard logic and the same exception text referencing _model.TypeName
and _model.AliasedTypeMinimalName and write to _sb as before.
🧹 Nitpick comments (6)
NewType.Generator/AliasGenerator.cs (1)
15-24: Consider using a more descriptive diagnostic category.The diagnostic category is set to
"Unknown"(line 20). Using a more specific category like"NewType"or"CodeGeneration"would improve discoverability when filtering diagnostics.Proposed fix
private static readonly DiagnosticDescriptor MissingIsValidMethodDiagnostic = new DiagnosticDescriptor( id: "NEWTYPE001", title: "Missing validation method", messageFormat: $"Type '{{0}}' uses constraints but does not define a compatible validation method. Expected signature: 'bool {AliasModel.ConstraintValidationMethodSymbol}({{1}})'.", - category: "Unknown", + category: "NewType", DiagnosticSeverity.Error, isEnabledByDefault: true, description: "A constraint-enabled wrapped type must define a validation method." );NewType.Generator/AliasModel.cs (2)
47-49: Naming convention:validValidationMethodshould beValidValidationMethod.The property uses camelCase (
validValidationMethod) instead of PascalCase, which is the standard C# convention for public properties/fields.Proposed fix
bool IncludeConstraints, bool DebugOnlyConstraints, - bool validValidationMethod, + bool ValidValidationMethod,Note: This requires updating all usages in
AliasCodeGenerator.csandAliasGenerator.csas well.
19-21:Locationstores a Roslyn symbol reference.The doc comment (line 9) states "no Roslyn symbols," but
Locationis a Roslyn type. While this is necessary for diagnostic reporting and is acceptable, it's worth noting this deviation from the stated design goal. Consider updating the comment to reflect this exception.NewType.Tests/ConstraintValidationTests.cs (3)
1-3: Consider adding explicitusing System;directive.
InvalidOperationException(used in assertions at lines 31, 37, 43, 60) is in theSystemnamespace. This will compile with implicit usings enabled, but adding an explicitusing System;improves clarity and prevents issues if implicit usings are disabled.using System.Numerics; +using System; using newtype.tests; using Xunit;
7-26: Use discard pattern for "doesn't throw" tests to suppress unused variable warnings.The variables
dirin these tests are intentionally unused—their purpose is to verify construction doesn't throw. Using the discard pattern_ = ...makes this intent explicit and avoids compiler warnings.♻️ Proposed fix
[Fact] public void Direction_Valid() { //doesn't throw - var dir = new Direction(new Vector2(0, 1)); + _ = new Direction(new Vector2(0, 1)); } [Fact] public void Direction_ValidImplicit() { //doesn't throw - Direction dir = new Vector2(0, 1); + Direction _ = new Vector2(0, 1); } [Fact] public void Direction_Valid_Forwarded() { //doesn't throw - var dir = new Direction(0, 1); + _ = new Direction(0, 1); }
46-52: Discard unuseddir3variable or add an assertion.Similar to the earlier tests,
dir3is unused. Either discard it to clarify intent or add an assertion to verify the result.♻️ Proposed fix
[Fact] public void Direction_ForwardedOperation_ValidResult() { var dir1 = new Direction(new Vector2(0, 1f)); var dir2 = new Direction(new Vector2(0, 1f)); - var dir3 = dir1 * dir2; + _ = dir1 * dir2; }
Comment on lines
150
to
163
| if (_model.IncludeConstraints) | ||
| { | ||
| _sb.AppendLine($"{memberIndent}public {_model.TypeName}({_model.AliasedTypeFullName} value)"); | ||
| _sb.Append(memberIndent).Append('{').AppendLine(); | ||
|
|
||
| AppendConstraintChecker(SingleIndent, "value"); | ||
| _sb.Append(SingleIndent).Append(SingleIndent).Append(SingleIndent).AppendLine("_value = value;"); | ||
|
|
||
| _sb.Append(memberIndent).Append('}').AppendLine(); | ||
| } | ||
| else | ||
| { | ||
| _sb.AppendLine($"{memberIndent}public {_model.TypeName}({_model.AliasedTypeFullName} value) => _value = value;"); | ||
| } |
There was a problem hiding this comment.
Inconsistent indentation in generated constructor body.
The _value = value; assignment (line 156) uses three SingleIndent levels hardcoded, but memberIndent already accounts for namespace depth. When a namespace is present, memberIndent is 8 spaces, and the body should be indented one level deeper (12 spaces). However, SingleIndent * 3 always produces 12 spaces regardless of namespace presence.
For types without a namespace, the member indent is 4 spaces, so body lines should be 8 spaces, but this code emits 12.
Proposed fix
if (_model.IncludeConstraints)
{
_sb.AppendLine($"{memberIndent}public {_model.TypeName}({_model.AliasedTypeFullName} value)");
- _sb.Append(memberIndent).Append('{').AppendLine();
-
- AppendConstraintChecker(SingleIndent, "value");
- _sb.Append(SingleIndent).Append(SingleIndent).Append(SingleIndent).AppendLine("_value = value;");
-
- _sb.Append(memberIndent).Append('}').AppendLine();
+ _sb.AppendLine($"{memberIndent}{{");
+
+ var bodyIndent = memberIndent + SingleIndent;
+ AppendConstraintChecker(bodyIndent, "value");
+ _sb.AppendLine($"{bodyIndent}_value = value;");
+
+ _sb.AppendLine($"{memberIndent}}}");
}🤖 Prompt for AI Agents
In `@NewType.Generator/AliasCodeGenerator.cs` around lines 150 - 163, The
constructor body indentation is inconsistent: in AliasCodeGenerator.cs when
_model.IncludeConstraints is true you append the assignment using three
SingleIndent units which ignores memberIndent and mis-indents types without
namespaces; change the code that writes the body (the AppendConstraintChecker
call and the line writing "_value = value;") to build indentation relative to
memberIndent (e.g., append memberIndent then one SingleIndent before body lines)
instead of using SingleIndent repeated three times so the body is always
indented exactly one level deeper than memberIndent; keep
AppendConstraintChecker("SingleIndent", "value") usage but ensure the subsequent
"_value = value;" write uses memberIndent + SingleIndent.
Author
|
After playing around with it, using an attribute felt nicer |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
NewType.Generator/AliasGenerator.cs (1)
140-158: Consider using a more specific diagnostic category.Both
ValidatorInvalidDiagnosticandValidatorMultipleDiagnosticusecategory: "Unknown"which appears to be a placeholder. Consider using a descriptive category like"NewtypeGenerator"or"Validation"for better diagnostic organization and filtering.📝 Proposed fix
private static readonly DiagnosticDescriptor ValidatorInvalidDiagnostic = new( id: "NEWTYPE001", title: "Malformed validation method", messageFormat: "Incorrectly formed validation method for type '{0}'. Expected signature: 'bool {1}({2})'.", - category: "Unknown", + category: "NewtypeGenerator", DiagnosticSeverity.Error, isEnabledByDefault: true ); private static readonly DiagnosticDescriptor ValidatorMultipleDiagnostic = new( id: "NEWTYPE002", title: "Multiple validators", messageFormat: "Only a single validation method should be used", - category: "Unknown", + category: "NewtypeGenerator", DiagnosticSeverity.Error, isEnabledByDefault: true );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@NewType.Generator/AliasGenerator.cs` around lines 140 - 158, The diagnostics ValidatorInvalidDiagnostic and ValidatorMultipleDiagnostic currently use a placeholder category ("Unknown"); update their category argument to a descriptive string such as "NewtypeGenerator" or "Validation" to improve filtering and organization — locate the two static fields ValidatorInvalidDiagnostic and ValidatorMultipleDiagnostic in AliasGenerator.cs and replace the category value for each DiagnosticDescriptor construction with the chosen specific category.NewType.Generator/AliasModel.cs (1)
58-59:ConstraintOptionsfield inExtractedOptionsappears unused.
ExtractedOptionscontains aConstraintOptionsfield, but looking atAliasGenerator.ExtractNamedArguments(lines 85-106), this field is always set toDefaultConstraintOptions(0) and is never populated from the attribute's named arguments. Additionally,AliasModelExtractor.Extractdoesn't appear to useallOptions.ConstraintOptions.If this is placeholder for future functionality, consider adding a TODO comment. Otherwise, it should be removed to avoid confusion.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@NewType.Generator/AliasModel.cs` around lines 58 - 59, The ConstraintOptions field on the ExtractedOptions record struct is unused; remove it to avoid confusion: delete the ConstraintOptions parameter from the ExtractedOptions declaration and update all call sites (e.g., the construction in AliasGenerator.ExtractNamedArguments and any references in AliasModelExtractor.Extract) to use the new two-parameter (Options, MethodImpl) shape, and remove DefaultConstraintOptions if it becomes unused; alternatively, if you intend it as a placeholder, add a clear TODO comment above ExtractedOptions referencing planned use in AliasGenerator.ExtractNamedArguments and AliasModelExtractor.Extract instead of leaving it silently unused.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@NewType.Generator/AliasModel.cs`:
- Around line 19-21: AliasModel and ConstraintModel currently store Roslyn
Location objects (Location? Location) which breaks incremental generator
caching; replace those Location fields with plain primitives (e.g., string?
SourceFilePath, int? StartLine, int? StartColumn or a small struct of file path
+ line/column) so the models contain only strings/bools/enums/EquatableArrays,
and remove any nested Roslyn types from ConstraintModel; when you need to report
diagnostics reconstruct a Roslyn Location via Location.Create(...) from the
stored primitives in the diagnostic/emission path rather than keeping Location
in the model.
In `@NewType.Generator/AliasModelExtractor.cs`:
- Around line 370-372: The loop in AliasModelExtractor.cs that iterates
method.GetAttributes() dereferences attributeData.AttributeClass with a
null-forgiving operator; add a null check (e.g., skip/continue when
attributeData.AttributeClass is null) before comparing its Name to
ConstraintAttributeSource.AttributeName so you don't risk a
NullReferenceException; update the comparison to only access AttributeClass.Name
when non-null (or use attributeData.AttributeClass?.Name and compare safely) in
the method that contains this foreach.
In `@NewType.Generator/AliasValidationAttributeSource.cs`:
- Around line 9-10: AttributeNamespace is set to "newtypeValidation" which makes
FullyQualifiedName inconsistent with the actual generated attribute namespace in
Source; update AttributeNamespace to match the namespace used in the generated
attribute (the "newtype" namespace used when emitting the validation attribute)
so FullyQualifiedName (which uses AttributeName) resolves to the real type name,
and double-check AttributeName and FullyQualifiedName still compose correctly
with the generated attribute symbol.
- Around line 16-27: Fix the XML documentation typos on the
newtypeValidationAttribute: change "valdiation" to "validation" in the summary
that describes methods, and update the second summary (currently "Creates a new
alias attribute") to correctly describe that the constructor creates a new
validation attribute for aliased types (or similar wording) so the docs
accurately reflect newtypeValidationAttribute's purpose.
---
Nitpick comments:
In `@NewType.Generator/AliasGenerator.cs`:
- Around line 140-158: The diagnostics ValidatorInvalidDiagnostic and
ValidatorMultipleDiagnostic currently use a placeholder category ("Unknown");
update their category argument to a descriptive string such as
"NewtypeGenerator" or "Validation" to improve filtering and organization —
locate the two static fields ValidatorInvalidDiagnostic and
ValidatorMultipleDiagnostic in AliasGenerator.cs and replace the category value
for each DiagnosticDescriptor construction with the chosen specific category.
In `@NewType.Generator/AliasModel.cs`:
- Around line 58-59: The ConstraintOptions field on the ExtractedOptions record
struct is unused; remove it to avoid confusion: delete the ConstraintOptions
parameter from the ExtractedOptions declaration and update all call sites (e.g.,
the construction in AliasGenerator.ExtractNamedArguments and any references in
AliasModelExtractor.Extract) to use the new two-parameter (Options, MethodImpl)
shape, and remove DefaultConstraintOptions if it becomes unused; alternatively,
if you intend it as a placeholder, add a clear TODO comment above
ExtractedOptions referencing planned use in AliasGenerator.ExtractNamedArguments
and AliasModelExtractor.Extract instead of leaving it silently unused.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
NewType.Generator/AliasAttributeSource.csNewType.Generator/AliasCodeGenerator.csNewType.Generator/AliasGenerator.csNewType.Generator/AliasModel.csNewType.Generator/AliasModelExtractor.csNewType.Generator/AliasValidationAttributeSource.csNewType.Tests/Types.cs
Comment on lines
370
to
372
| foreach (var attributeData in method.GetAttributes()) | ||
| { | ||
| if (attributeData.AttributeClass!.Name == ConstraintAttributeSource.AttributeName) |
There was a problem hiding this comment.
Potential null reference on AttributeClass.
Line 372 uses attributeData.AttributeClass!.Name with a null-forgiving operator. While AttributeClass being null is rare, it can occur for malformed attributes or during error recovery. Consider adding a null check to avoid potential runtime exceptions during compilation.
🛡️ Proposed fix
foreach (var attributeData in method.GetAttributes())
{
- if (attributeData.AttributeClass!.Name == ConstraintAttributeSource.AttributeName)
+ if (attributeData.AttributeClass?.Name == ConstraintAttributeSource.AttributeName)
{📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| foreach (var attributeData in method.GetAttributes()) | |
| { | |
| if (attributeData.AttributeClass!.Name == ConstraintAttributeSource.AttributeName) | |
| foreach (var attributeData in method.GetAttributes()) | |
| { | |
| if (attributeData.AttributeClass?.Name == ConstraintAttributeSource.AttributeName) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@NewType.Generator/AliasModelExtractor.cs` around lines 370 - 372, The loop in
AliasModelExtractor.cs that iterates method.GetAttributes() dereferences
attributeData.AttributeClass with a null-forgiving operator; add a null check
(e.g., skip/continue when attributeData.AttributeClass is null) before comparing
its Name to ConstraintAttributeSource.AttributeName so you don't risk a
NullReferenceException; update the comparison to only access AttributeClass.Name
when non-null (or use attributeData.AttributeClass?.Name and compare safely) in
the method that contains this foreach.
0c886cb to
38b359e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
NewType.Generator/AliasModelExtractor.cs (1)
371-373:⚠️ Potential issue | 🟡 MinorPotential null reference on
AttributeClassnot fully addressed.While the
Where(x => x is not null)filter on line 371 ensuresattributeDatais non-null,attributeData.AttributeClasscan still be null for malformed attributes. The null-forgiving operator on line 373 could cause aNullReferenceException.🛡️ Proposed fix
foreach (var attributeData in method.GetAttributes().Where(x => x is not null)) { - if (attributeData.AttributeClass!.Name == ConstraintAttributeSource.AttributeName) + if (attributeData.AttributeClass?.Name == ConstraintAttributeSource.AttributeName) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@NewType.Generator/AliasModelExtractor.cs` around lines 371 - 373, The code assumes attributeData.AttributeClass is non-null when comparing its Name to ConstraintAttributeSource.AttributeName inside AliasModelExtractor (loop over method.GetAttributes()); update the check to guard against a null AttributeClass before accessing Name (for example, use a null-conditional check or an explicit null check on attributeData.AttributeClass) so the if condition only compares Name when AttributeClass is non-null and avoids the null-forgiving operator.NewType.Generator/AliasCodeGenerator.cs (3)
760-777:⚠️ Potential issue | 🟡 Minor
AppendConstraintCheckeruses incorrect indentation approach.The method receives an
indentparameter but then triples it via.Append(indent).Append(indent).Append(indent)(lines 767-769), which doesn't adapt to actual nesting level. The caller should pass the correct body indent, and this method should use it directly.♻️ Proposed fix
- private void AppendConstraintChecker(string indent, string valueName) + private void AppendConstraintChecker(string bodyIndent, string valueName) { - if (!_model.ConstraintModel.Valid) return; + if (!_model.ConstraintModel.UseConstraints) return; if (!_model.ConstraintModel.InRelease) _sb.AppendLine("#if DEBUG"); - _sb.Append(indent).Append(indent).Append(indent) - .AppendLine($"if (!{_model.ConstraintModel.ValidationSymbolName}({valueName}))"); - _sb.Append(indent).Append(indent).Append(indent).Append(indent) - .AppendLine( - $"throw new InvalidOperationException($\"Failed validation check when trying to create '{_model.TypeName}' with '{_model.AliasedTypeMinimalName}' value: {{{valueName}}}\");"); + _sb.AppendLine($"{bodyIndent}if (!{_model.ConstraintModel.ValidationSymbolName}({valueName}))"); + _sb.AppendLine($"{bodyIndent}{SingleIndent}throw new InvalidOperationException($\"Failed validation check when trying to create '{_model.TypeName}' with '{_model.AliasedTypeMinimalName}' value: {{{valueName}}}\");"); if (!_model.ConstraintModel.InRelease) _sb.AppendLine("#endif"); else _sb.AppendLine(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@NewType.Generator/AliasCodeGenerator.cs` around lines 760 - 777, The method AppendConstraintChecker currently multiplies the passed indent (using .Append(indent) three times) instead of using the caller-provided body indent directly; change AppendConstraintChecker to use the given indent once for the if-line (use _sb.Append(indent).AppendLine(...)) and use two levels for the inner throw-line (use _sb.Append(indent).Append(indent).AppendLine(...)) so the if and its body are indented relative to the body indent the caller passes; keep all other logic and symbols (_sb, _model.ConstraintModel, ValidationSymbolName, TypeName, AliasedTypeMinimalName, the `#if` DEBUG guards) unchanged.
150-164:⚠️ Potential issue | 🟡 MinorIndentation is hardcoded and ignores namespace depth.
The constructor body uses
SingleIndentrepeated 3 times (line 155-156), which always produces 12 spaces. However, the correct indentation depends on whether the type has a namespace:
- With namespace:
memberIndent(8 spaces) +SingleIndent(4 spaces) = 12 spaces ✓- Without namespace:
memberIndent(4 spaces) +SingleIndent(4 spaces) = 8 spaces ✗ (code emits 12)The generated code will be visually misaligned for types without a namespace.
♻️ Proposed fix
if (_model.ConstraintModel.UseConstraints) { + var bodyIndent = memberIndent + SingleIndent; _sb.AppendLine($"{memberIndent}public {_model.TypeName}({_model.AliasedTypeFullName} value)"); - _sb.Append(memberIndent).Append('{').AppendLine(); + _sb.AppendLine($"{memberIndent}{{"); - AppendConstraintChecker(SingleIndent, "value"); - _sb.Append(SingleIndent).Append(SingleIndent).Append(SingleIndent).AppendLine("_value = value;"); + AppendConstraintChecker(bodyIndent, "value"); + _sb.AppendLine($"{bodyIndent}_value = value;"); - _sb.Append(memberIndent).Append('}').AppendLine(); + _sb.AppendLine($"{memberIndent}}}"); }And update
AppendConstraintCheckerto use the passed indent directly:private void AppendConstraintChecker(string bodyIndent, string valueName) { if (!_model.ConstraintModel.Valid) return; if (!_model.ConstraintModel.InRelease) _sb.AppendLine("#if DEBUG"); - _sb.Append(indent).Append(indent).Append(indent) - .AppendLine($"if (!{_model.ConstraintModel.ValidationSymbolName}({valueName}))"); - _sb.Append(indent).Append(indent).Append(indent).Append(indent) - .AppendLine($"throw new InvalidOperationException(...);"); + _sb.AppendLine($"{bodyIndent}if (!{_model.ConstraintModel.ValidationSymbolName}({valueName}))"); + _sb.AppendLine($"{bodyIndent}{SingleIndent}throw new InvalidOperationException(...);"); if (!_model.ConstraintModel.InRelease) _sb.AppendLine("#endif"); else _sb.AppendLine(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@NewType.Generator/AliasCodeGenerator.cs` around lines 150 - 164, The constructor body indentation is hardcoded by repeating SingleIndent three times which misaligns code when memberIndent varies (types without namespaces); modify the constructor generation in AliasCodeGenerator (the block that builds the public {_model.TypeName}({_model.AliasedTypeFullName} value) constructor) to compute indentation by concatenating memberIndent with a single SingleIndent (instead of using SingleIndent repeated 3 times) for the lines inside the braces, and update AppendConstraintChecker to accept and use the caller-provided indent string (pass memberIndent + SingleIndent when calling AppendConstraintChecker) so the constraint checks and the subsequent "_value = value;" line align correctly regardless of namespace depth.
719-736:⚠️ Potential issue | 🟡 MinorSame indentation issue in forwarded constructor.
Lines 727-728 use the same hardcoded triple-indent pattern that doesn't adapt to namespace presence.
♻️ Proposed fix
if (_model.ConstraintModel.UseConstraints) { const string valueName = "newValue"; + var bodyIndent = indent + SingleIndent; _sb.AppendLine($"{indent}public {_model.TypeName}({parameters})"); - _sb.Append(indent).Append('{').AppendLine(); - _sb.Append(indent).Append(SingleIndent) - .AppendLine($"var {valueName} = new {_model.AliasedTypeFullName}({arguments});"); + _sb.AppendLine($"{indent}{{"); + _sb.AppendLine($"{bodyIndent}var {valueName} = new {_model.AliasedTypeFullName}({arguments});"); - AppendConstraintChecker(SingleIndent, valueName); - _sb.Append(SingleIndent).Append(SingleIndent).Append(SingleIndent).AppendLine($"_value = {valueName};"); + AppendConstraintChecker(bodyIndent, valueName); + _sb.AppendLine($"{bodyIndent}_value = {valueName};"); - _sb.Append(indent).Append('}').AppendLine(); + _sb.AppendLine($"{indent}}}"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@NewType.Generator/AliasCodeGenerator.cs` around lines 719 - 736, The constructor generation uses a hardcoded triple SingleIndent when writing the "_value = ..." assignment (see the block generating public {_model.TypeName}(...) with constraints and the call to AppendConstraintChecker), which fails to adapt to the surrounding "indent" (namespace/class nesting); replace the chained .Append(SingleIndent).Append(SingleIndent).Append(SingleIndent) with a composition that begins with the existing indent (e.g., .Append(indent).Append(SingleIndent).Append(SingleIndent) or equivalent) so the assignment line aligns relative to the current indent, and apply the same change where the forwarded constructor line is emitted so both paths use the indent + SingleIndent pattern rather than fixed triple SingleIndent.
🧹 Nitpick comments (2)
NewType.Generator/AliasGenerator.cs (1)
138-156: Consider using a more descriptive diagnostic category.Both diagnostic descriptors use
category: "Unknown". Roslyn conventions suggest using a meaningful category like"Usage","Design", or a project-specific category like"Newtype"for better tooling integration and filtering.♻️ Suggested improvement
private static readonly DiagnosticDescriptor ValidatorInvalidDiagnostic = new( id: "NEWTYPE001", title: "Malformed validation method", messageFormat: "Incorrectly formed validation method for type '{0}'. Expected signature: 'bool {1}({2})'.", - category: "Unknown", + category: "Newtype", DiagnosticSeverity.Error, isEnabledByDefault: true ); private static readonly DiagnosticDescriptor ValidatorMultipleDiagnostic = new( id: "NEWTYPE002", title: "Multiple validators", messageFormat: "Only a single validation method should be used", - category: "Unknown", + category: "Newtype", DiagnosticSeverity.Error, isEnabledByDefault: true );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@NewType.Generator/AliasGenerator.cs` around lines 138 - 156, The diagnostics ValidatorInvalidDiagnostic and ValidatorMultipleDiagnostic currently use category "Unknown"; change their category strings to a meaningful value (e.g. "Usage", "Design", or a project-specific category like "Newtype") to improve tooling/filtering; update the category parameter in the constructors for ValidatorInvalidDiagnostic and ValidatorMultipleDiagnostic to the chosen descriptive category and ensure both descriptors use the same consistent category name across AliasGenerator.cs.NewType.Generator/AliasModelExtractor.cs (1)
470-478: Minor: Formatting inconsistency in ternary expression.Missing space before
nullon line 478.♻️ Suggested fix
private static LocationInfo? ToLocationStruct(Location? location) => location is not null && location.IsInSource ? new LocationInfo( location.SourceTree.FilePath, location.SourceSpan, new LinePositionSpan( location.GetLineSpan().StartLinePosition, location.GetLineSpan().EndLinePosition)) - :null; + : null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@NewType.Generator/AliasModelExtractor.cs` around lines 470 - 478, The ternary expression in ToLocationStruct has a formatting inconsistency: there's no space before the trailing null; update the expression inside the ToLocationStruct method so the conditional's false branch is formatted with a space before null (i.e., "... : null;") preserving current semantics and keeping the use of Location, LocationInfo, SourceTree/FilePath, SourceSpan, and LinePositionSpan unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@NewType.Generator/AliasCodeGenerator.cs`:
- Line 762: The guard currently checks _model.ConstraintModel.Valid but should
check _model.ConstraintModel.UseConstraints so code generation only emits
constraint checks when a real validation method exists; change the early return
in the method that contains the line "if (!_model.ConstraintModel.Valid)
return;" to "if (!_model.ConstraintModel.UseConstraints) return;" (ensure any
subsequent logic that references ValidationSymbolName or valueName remains
unchanged so you don't emit "if (!({valueName}))" when no validator exists).
In `@NewType.Generator/AliasValidationAttributeSource.cs`:
- Around line 3-5: The XML summary on AliasValidationAttributeSource.cs
incorrectly calls this the "[Alias] attribute"; update the XML doc comment to
accurately describe the file as the source text for the Alias validation
attribute (e.g., "Source text for the alias validation attribute that gets
injected into user compilations") so it reflects the actual purpose of the
AliasValidationAttributeSource class/file and remove the copy-paste wording from
NewtypeAttributeSource.
---
Duplicate comments:
In `@NewType.Generator/AliasCodeGenerator.cs`:
- Around line 760-777: The method AppendConstraintChecker currently multiplies
the passed indent (using .Append(indent) three times) instead of using the
caller-provided body indent directly; change AppendConstraintChecker to use the
given indent once for the if-line (use _sb.Append(indent).AppendLine(...)) and
use two levels for the inner throw-line (use
_sb.Append(indent).Append(indent).AppendLine(...)) so the if and its body are
indented relative to the body indent the caller passes; keep all other logic and
symbols (_sb, _model.ConstraintModel, ValidationSymbolName, TypeName,
AliasedTypeMinimalName, the `#if` DEBUG guards) unchanged.
- Around line 150-164: The constructor body indentation is hardcoded by
repeating SingleIndent three times which misaligns code when memberIndent varies
(types without namespaces); modify the constructor generation in
AliasCodeGenerator (the block that builds the public
{_model.TypeName}({_model.AliasedTypeFullName} value) constructor) to compute
indentation by concatenating memberIndent with a single SingleIndent (instead of
using SingleIndent repeated 3 times) for the lines inside the braces, and update
AppendConstraintChecker to accept and use the caller-provided indent string
(pass memberIndent + SingleIndent when calling AppendConstraintChecker) so the
constraint checks and the subsequent "_value = value;" line align correctly
regardless of namespace depth.
- Around line 719-736: The constructor generation uses a hardcoded triple
SingleIndent when writing the "_value = ..." assignment (see the block
generating public {_model.TypeName}(...) with constraints and the call to
AppendConstraintChecker), which fails to adapt to the surrounding "indent"
(namespace/class nesting); replace the chained
.Append(SingleIndent).Append(SingleIndent).Append(SingleIndent) with a
composition that begins with the existing indent (e.g.,
.Append(indent).Append(SingleIndent).Append(SingleIndent) or equivalent) so the
assignment line aligns relative to the current indent, and apply the same change
where the forwarded constructor line is emitted so both paths use the indent +
SingleIndent pattern rather than fixed triple SingleIndent.
In `@NewType.Generator/AliasModelExtractor.cs`:
- Around line 371-373: The code assumes attributeData.AttributeClass is non-null
when comparing its Name to ConstraintAttributeSource.AttributeName inside
AliasModelExtractor (loop over method.GetAttributes()); update the check to
guard against a null AttributeClass before accessing Name (for example, use a
null-conditional check or an explicit null check on
attributeData.AttributeClass) so the if condition only compares Name when
AttributeClass is non-null and avoids the null-forgiving operator.
---
Nitpick comments:
In `@NewType.Generator/AliasGenerator.cs`:
- Around line 138-156: The diagnostics ValidatorInvalidDiagnostic and
ValidatorMultipleDiagnostic currently use category "Unknown"; change their
category strings to a meaningful value (e.g. "Usage", "Design", or a
project-specific category like "Newtype") to improve tooling/filtering; update
the category parameter in the constructors for ValidatorInvalidDiagnostic and
ValidatorMultipleDiagnostic to the chosen descriptive category and ensure both
descriptors use the same consistent category name across AliasGenerator.cs.
In `@NewType.Generator/AliasModelExtractor.cs`:
- Around line 470-478: The ternary expression in ToLocationStruct has a
formatting inconsistency: there's no space before the trailing null; update the
expression inside the ToLocationStruct method so the conditional's false branch
is formatted with a space before null (i.e., "... : null;") preserving current
semantics and keeping the use of Location, LocationInfo, SourceTree/FilePath,
SourceSpan, and LinePositionSpan unchanged.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
NewType.Generator/AliasAttributeSource.csNewType.Generator/AliasCodeGenerator.csNewType.Generator/AliasGenerator.csNewType.Generator/AliasModel.csNewType.Generator/AliasModelExtractor.csNewType.Generator/AliasValidationAttributeSource.csNewType.Generator/AnalyzerReleases.Unshipped.mdNewType.Tests/Types.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- NewType.Tests/Types.cs
|
|
||
| private void AppendConstraintChecker(string indent, string valueName) | ||
| { | ||
| if (!_model.ConstraintModel.Valid) return; |
There was a problem hiding this comment.
Guard condition checks Valid but should check UseConstraints.
Line 762 checks _model.ConstraintModel.Valid which is true when no validation method exists OR when the method is correctly formed. This means the method will still try to emit constraint code when no validation method exists (ValidationSymbolName is null), causing line 768 to emit if (!({valueName})).
The guard should use UseConstraints which is only true when a valid validation method actually exists.
🐛 Proposed fix
private void AppendConstraintChecker(string indent, string valueName)
{
- if (!_model.ConstraintModel.Valid) return;
+ if (!_model.ConstraintModel.UseConstraints) return;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (!_model.ConstraintModel.Valid) return; | |
| if (!_model.ConstraintModel.UseConstraints) return; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@NewType.Generator/AliasCodeGenerator.cs` at line 762, The guard currently
checks _model.ConstraintModel.Valid but should check
_model.ConstraintModel.UseConstraints so code generation only emits constraint
checks when a real validation method exists; change the early return in the
method that contains the line "if (!_model.ConstraintModel.Valid) return;" to
"if (!_model.ConstraintModel.UseConstraints) return;" (ensure any subsequent
logic that references ValidationSymbolName or valueName remains unchanged so you
don't emit "if (!({valueName}))" when no validator exists).
Comment on lines
+3
to
+5
| /// <summary> | ||
| /// Source text for the [Alias] attribute that gets injected into user compilations. | ||
| /// </summary> |
There was a problem hiding this comment.
Incorrect summary describes this as an "[Alias] attribute".
The XML doc says "Source text for the [Alias] attribute" but this file provides the validation attribute source. This appears to be a copy-paste error from NewtypeAttributeSource.
📝 Proposed fix
/// <summary>
-/// Source text for the [Alias] attribute that gets injected into user compilations.
+/// Source text for the [newtypeValidation] attribute that gets injected into user compilations.
/// </summary>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /// <summary> | |
| /// Source text for the [Alias] attribute that gets injected into user compilations. | |
| /// </summary> | |
| /// <summary> | |
| /// Source text for the [newtypeValidation] attribute that gets injected into user compilations. | |
| /// </summary> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@NewType.Generator/AliasValidationAttributeSource.cs` around lines 3 - 5, The
XML summary on AliasValidationAttributeSource.cs incorrectly calls this the
"[Alias] attribute"; update the XML doc comment to accurately describe the file
as the source text for the Alias validation attribute (e.g., "Source text for
the alias validation attribute that gets injected into user compilations") so it
reflects the actual purpose of the AliasValidationAttributeSource class/file and
remove the copy-paste wording from NewtypeAttributeSource.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I've noticed in my own projects that often when I create a "type alias", it is a constrained version of the wrapped type, e.g:
Usually this involves writing repeat boilerplate involving
Debug.Assertwithin the type constructors/setters, as a simple verification layer during design and testing. In relatively high-performance code, I don't want to be checking for this excessivelyThis PR adds a "valid" constraint to newtype, injecting a user-defined validation method into the constructor, helping to avoid invariants. Optionally enabled, optionally enabled in release.
Example:
Generated code:
Includes an analyzer warning if the constraint is enabled but a method is not provided
Feedback on API and naming appreciated.
(I was actually originally going the route of making my own source generator for this, but as I tried to implement it I realized the design was growing closer and closer to what newtype already was, so I decided it makes more sense to add on to it instead xd)
Summary by CodeRabbit
New Features
Tests
Diagnostics / Docs