Add nullable annotations #101
Conversation
|
I'll have to merge this into a branch for testing. It might not work for those who aren't using nullable notations. Or we might just have to add some additional #pragmas. PolySharp looks interesting. Just need to make sure it doesn't create a downstream dependency (hence the private all for the other packages). I see you've fallen for the CodeAnalysis.CSharp upgrade trap (as I have many times!). |
Huh, that's interesting. I assume you mean netstandard2.0. It seems like it supports netstandard2.0 to me: https://www.nuget.org/packages/Microsoft.CodeAnalysis.CSharp The official Godot NuGet package uses nullable annotations so there should be no issue here. |
Yep, 2.0 :) But I remember now, CSharp lib is tied to .NET version and we're limited for backwards compatibility with Godot 3. I'll be dropping support for Godot 3 in the next major version. No plans for that as yet, but it's on the cards.
Ok, great :) |
|
Oh, and thanks for the FluentAssertions/pin version tip. Very handy :) |
WalkthroughThis update introduces nullable reference type annotations across the codebase, updating method signatures, constructor parameters, properties, and local variables to explicitly allow or disallow null values as appropriate. The project file is updated to enable nullable reference types and adjust package dependencies. No core logic or control flow is altered. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Method
Note over Method: Method now accepts/returns nullable types
Caller->>Method: Call with nullable argument
Method-->>Caller: Return nullable value
Note over Caller,Method: Nullability is explicitly handled
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (5)
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (1)
SourceGenerators/InputMapExtensions/InputMapAttribute.cs (1)
8-8: Consider the semantic implications of usingnull!as default.While
[CallerFilePath]should provide a non-null value at compile time, usingnull!as a default for a non-nullable parameter creates semantic confusion. The parameter is declared as non-nullable but explicitly defaults to null with warning suppression.Consider either:
- Keeping the parameter nullable (
string? classPath = null) if null is a valid scenario- Using an empty string default (
string classPath = "") if non-null is required
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (42)
SourceGenerators/AutoloadExtensions/AutoloadAttribute.cs(1 hunks)SourceGenerators/AutoloadExtensions/AutoloadDataModel.cs(1 hunks)SourceGenerators/AutoloadExtensions/AutoloadScraper.cs(14 hunks)SourceGenerators/AutoloadExtensions/AutoloadSourceGenerator.cs(4 hunks)SourceGenerators/CodeCommentsExtensions/CodeCommentsSourceGenerator.cs(3 hunks)SourceGenerators/DiagnosticDetail.cs(1 hunks)SourceGenerators/GodotNotifyExtensions/GodotNotifyDataModel.cs(2 hunks)SourceGenerators/GodotNotifyExtensions/GodotNotifySourceGenerator.cs(2 hunks)SourceGenerators/GodotOverrideExtensions/GodotOverrideSourceGenerator.cs(3 hunks)SourceGenerators/InputMapExtensions/InputMapAttribute.cs(1 hunks)SourceGenerators/InputMapExtensions/InputMapDataModel.cs(1 hunks)SourceGenerators/InputMapExtensions/InputMapScraper.cs(3 hunks)SourceGenerators/InputMapExtensions/InputMapSourceGenerator.cs(3 hunks)SourceGenerators/LayerNamesExtensions/LayerNamesAttribute.cs(1 hunks)SourceGenerators/LayerNamesExtensions/LayerNamesDataModel.cs(1 hunks)SourceGenerators/LayerNamesExtensions/LayerNamesScraper.cs(1 hunks)SourceGenerators/LayerNamesExtensions/LayerNamesSourceGenerator.cs(3 hunks)SourceGenerators/OnImportExtensions/OnImportAttribute.cs(2 hunks)SourceGenerators/OnImportExtensions/OnImportDataModel.cs(4 hunks)SourceGenerators/OnImportExtensions/OnImportSourceGenerator.cs(4 hunks)SourceGenerators/OnInstantiateExtensions/OnInstantiateAttribute.cs(1 hunks)SourceGenerators/OnInstantiateExtensions/OnInstantiateDataModel.cs(2 hunks)SourceGenerators/OnInstantiateExtensions/OnInstantiateSourceGenerator.cs(3 hunks)SourceGenerators/SceneTreeExtensions/SceneTreeAttribute.cs(1 hunks)SourceGenerators/SceneTreeExtensions/SceneTreeDataModel.cs(1 hunks)SourceGenerators/SceneTreeExtensions/SceneTreeNode.cs(1 hunks)SourceGenerators/SceneTreeExtensions/SceneTreeScraper.cs(13 hunks)SourceGenerators/SceneTreeExtensions/SceneTreeSourceGenerator.cs(3 hunks)SourceGenerators/SourceGeneratorForDeclaredFieldWithAttribute.cs(1 hunks)SourceGenerators/SourceGeneratorForDeclaredMemberWithAttribute.cs(3 hunks)SourceGenerators/SourceGeneratorForDeclaredMethodWithAttribute.cs(1 hunks)SourceGenerators/SourceGeneratorForDeclaredPropertyWithAttribute.cs(1 hunks)SourceGenerators/SourceGeneratorForDeclaredTypeWithAttribute.cs(1 hunks)SourceGenerators/SourceGenerators.csproj(3 hunks)SourceGenerators/Utilities/DataModel/BaseDataModel.cs(1 hunks)SourceGenerators/Utilities/Extensions/CompilationExtensions.cs(2 hunks)SourceGenerators/Utilities/Extensions/GeneratorExtensions.cs(1 hunks)SourceGenerators/Utilities/Extensions/GodotExtensions.cs(2 hunks)SourceGenerators/Utilities/Extensions/LinqExtensions.cs(1 hunks)SourceGenerators/Utilities/Extensions/SymbolExtensions.cs(1 hunks)SourceGenerators/Utilities/Log.cs(1 hunks)SourceGenerators/Utilities/TreeNode.cs(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (8)
SourceGenerators/AutoloadExtensions/AutoloadAttribute.cs (1)
SourceGenerators/Utilities/Extensions/SymbolExtensions.cs (1)
ClassPath(31-32)
SourceGenerators/InputMapExtensions/InputMapDataModel.cs (1)
SourceGenerators/Utilities/Extensions/SymbolExtensions.cs (1)
INamedTypeSymbol(25-26)
SourceGenerators/LayerNamesExtensions/LayerNamesDataModel.cs (1)
SourceGenerators/Utilities/Extensions/SymbolExtensions.cs (1)
INamedTypeSymbol(25-26)
SourceGenerators/LayerNamesExtensions/LayerNamesSourceGenerator.cs (6)
SourceGenerators/LayerNamesExtensions/Resources.cs (1)
Resources(5-9)SourceGenerators/CodeCommentsExtensions/CodeCommentsSourceGenerator.cs (1)
GeneratedCode(14-26)SourceGenerators/GodotNotifyExtensions/GodotNotifySourceGenerator.cs (1)
GeneratedCode(14-23)SourceGenerators/InputMapExtensions/InputMapSourceGenerator.cs (1)
GeneratedCode(14-26)SourceGenerators/OnInstantiateExtensions/OnInstantiateSourceGenerator.cs (1)
GeneratedCode(14-26)SourceGenerators/SourceGeneratorForDeclaredMemberWithAttribute.cs (3)
GeneratedCode(89-89)GeneratedCode(91-105)SyntaxNode(119-120)
SourceGenerators/LayerNamesExtensions/LayerNamesScraper.cs (2)
SourceGenerators/InputMapExtensions/InputMapScraper.cs (1)
IEnumerable(11-54)SourceGenerators/AutoloadExtensions/AutoloadScraper.cs (1)
IEnumerable(15-73)
SourceGenerators/SceneTreeExtensions/SceneTreeScraper.cs (5)
SourceGenerators/SceneTreeExtensions/SceneTreeNode.cs (2)
SceneTreeNode(3-20)SceneTreeNode(10-16)SourceGenerators/Utilities/Tree.cs (1)
Tree(5-43)SourceGenerators/Utilities/Extensions/RegexExtensions.cs (2)
Dictionary(8-8)Dictionary(9-14)SourceGenerators/Utilities/Extensions/CompilationExtensions.cs (1)
ValidateTypeCase(32-50)SourceGenerators/Utilities/TreeNode.cs (2)
TreeNode(3-17)TreeNode(11-16)
SourceGenerators/SourceGeneratorForDeclaredTypeWithAttribute.cs (7)
SourceGenerators/GodotNotifyExtensions/GodotNotifySourceGenerator.cs (1)
GeneratedCode(14-23)SourceGenerators/SourceGeneratorForDeclaredFieldWithAttribute.cs (3)
GeneratedCode(10-10)GeneratedCode(11-12)SyntaxNode(14-15)SourceGenerators/SceneTreeExtensions/SceneTreeSourceGenerator.cs (1)
GeneratedCode(14-37)SourceGenerators/SourceGeneratorForDeclaredPropertyWithAttribute.cs (2)
GeneratedCode(10-10)GeneratedCode(11-12)SourceGenerators/SourceGeneratorForDeclaredMethodWithAttribute.cs (2)
GeneratedCode(10-10)GeneratedCode(11-12)SourceGenerators/SourceGeneratorForDeclaredMemberWithAttribute.cs (3)
GeneratedCode(89-89)GeneratedCode(91-105)SyntaxNode(119-120)CustomGeneratorTests/MyClassAttributeGenerator.cs (1)
GeneratedCode(11-27)
SourceGenerators/OnImportExtensions/OnImportDataModel.cs (1)
SourceGenerators/OnImportExtensions/Resources.cs (1)
Resources(5-63)
🔇 Additional comments (137)
SourceGenerators/LayerNamesExtensions/LayerNamesScraper.cs (1)
11-11: LGTM! Nullable parameter correctly represents optionality.The change from
string gdRoottostring? gdRootproperly reflects that the Godot project root parameter is optional. This aligns with similar changes in other scraper classes and improves null-safety.SourceGenerators/LayerNamesExtensions/LayerNamesDataModel.cs (1)
10-10: LGTM! Constructor parameter aligns with scraper interface.The nullable annotation on the
gdRootparameter correctly matches the signature change inLayerNamesScraper.GetLayerNames(), maintaining consistency throughout the call chain.SourceGenerators/AutoloadExtensions/AutoloadAttribute.cs (1)
6-6: LGTM! Correct usage of null-forgiving operator for CallerFilePath.The change from
nulltonull!properly suppresses nullable warnings for the[CallerFilePath]parameter while maintaining non-nullable semantics. This is the standard pattern for attribute parameters with CallerFilePath defaults.SourceGenerators/InputMapExtensions/InputMapDataModel.cs (1)
12-12: LGTM! Constructor parameter follows consistent nullable pattern.The change to
string? gdRootmaintains consistency with theInputMapScraper.GetInputActions()method signature and follows the same nullable annotation pattern established across other DataModel classes in the codebase.SourceGenerators/Utilities/Extensions/GeneratorExtensions.cs (3)
7-7: Excellent improvement to null safety.Changing the return type to
string?properly expresses that this method may not find a project directory and return null. This forces callers to handle the null case appropriately and aligns well with the "Try" naming pattern.
7-7: LGTM! Return type correctly reflects nullable behavior.Changing the return type to
string?properly expresses that this method can return null when the Godot project directory is not found in the analyzer configuration options. The "Try" prefix already suggests this optional behavior.
7-7: Excellent nullable annotation improvement.The return type change from
stringtostring?correctly reflects that this method can returnnullwhen the Godot project directory is not found in the analyzer configuration options. This enhances null safety and makes the API contract explicit.SourceGenerators/SceneTreeExtensions/SceneTreeNode.cs (3)
6-6: Good improvement to API expressiveness.Making the
Typeproperty and constructor parameter nullable appropriately expresses that the node type might not always be determinable in scene tree contexts. The existingToStringimplementation already handles null values gracefully.Also applies to: 10-10
6-6:✅ Verification successful
LGTM! Type property correctly made nullable.
The change to make the
Typeproperty and constructor parameter nullable is consistent and appears appropriate for scenarios where type information might not be available.Run the following script to verify that existing code properly handles nullable Type values:
Also applies to: 10-10
🏁 Script executed:
#!/bin/bash # Description: Check how Type property is used throughout the codebase to ensure null handling is adequate # Search for usage of SceneTreeNode.Type property rg -A 3 -B 1 "\.Type\b" --type cs # Search for Type property comparisons or string operations that might not handle null rg -A 2 -B 1 "(Type\s*==|Type\s*!=|Type\.)" --type csLength of output: 12659
LGTM – Nullable
Typeproperty is safeI searched all references to
SceneTreeNode.Typeand found:
- In
SceneTreeScraper, any initialnullis immediately overwritten byGetFullNameand guarded byDebug.Assert.- No code invokes methods or properties on
Typewithout a null check.ToString()already handlesnullgracefully.This change is safe to merge.
6-6: Appropriate nullable annotations for optional type information.Making the
Typeproperty and constructor parameter nullable correctly expresses that type information may be optional or unknown in certain scenarios. The consistency between the property declaration and constructor parameter is well-maintained.Also applies to: 10-10
SourceGenerators/SourceGenerators.csproj (2)
5-5: Good addition of nullable reference types support.Enabling nullable reference types aligns with the PR objectives and provides better null-safety throughout the codebase.
132-132:✅ Verification successful
Verify PolySharp package version and compatibility.
The addition of PolySharp 1.15.0 to replace IndexRange and IsExternalInit packages aligns with the PR objectives. However, ensure this version is current and compatible with the project's requirements.
🌐 Web query:
What is the latest version of PolySharp and are there any known compatibility issues with netstandard2.0?💡 Result:
Latest Version of PolySharp
The latest version of PolySharp as of June 2025 is 1.15.0. This release includes support for C# 13 and new .NET 9 polyfills, as well as various updates and improvements[2][3].
Compatibility with netstandard2.0
PolySharp is specifically designed to enable modern C# language features on projects that target older frameworks, including netstandard2.0[2][4][8]. It works by generating source-only polyfills for missing types and attributes at compile time, based on the target framework and language version.
Known Compatibility Considerations
- General Compatibility: PolySharp supports netstandard2.0 as a target and is commonly used in libraries that target this framework to enable newer C# features downlevel[2][8].
- Project Setup Requirements: PolySharp is a Roslyn source generator and requires your project to use the SDK-style .csproj format with
<PackageReference>. It does not work with olderpackages.config-based projects[8].- InternalsVisibleTo Issues: There are known edge cases where using PolySharp in projects with
InternalsVisibleTo(such as when both the main library and a test project reference PolySharp and share internal APIs) can lead to issues with duplicated generated types (likeIsExternalInit). In such scenarios, you may need to adjust your project structure or use workarounds to avoid conflicts[9].- Multi-targeting: If your solution includes projects that multi-target netstandard2.0 along with newer frameworks, be careful with visibility and type duplication across assemblies, especially with unit test projects[9].
No Major Blocking Issues
There are no general blocking compatibility issues reported for using PolySharp with netstandard2.0. The main caveats relate to complex solutions with multiple projects and internal API sharing. Otherwise, you should be able to use the latest PolySharp in netstandard2.0 projects to access many new C# language features seamlessly[2][3][8].
In summary:
- The latest version of PolySharp is 1.15.0.
- It is compatible with netstandard2.0, provided you use SDK-style projects.
- Watch for potential issues with
InternalsVisibleToand test projects, but for most scenarios, PolySharp works as intended for netstandard2.0 projects[2][3][8][9].Citations:
- 1: https://github.com/Sergio0694/PolySharp
- 2: https://www.nuget.org/packages/PolySharp/
- 3: https://github.com/Sergio0694/PolySharp/releases
- 4: https://www.nuget.org/packages/PolySharp/1.7.1
- 5: https://blog.postsharp.net/metalama-2025-0-ga
- 6: https://stu.dev/dotnet-framework-support-for-dotnetnet-standard-2-0/
- 7: open-telemetry/opentelemetry-dotnet#3448
- 8: https://blog.genezini.com/p/how-to-use-c-sharp-11-features-in-dotnet-6-or-older-versions/
- 9: Sergio0694/PolySharp#50
- 10: https://learn.microsoft.com/en-us/dotnet/standard/net-standard
PolySharp 1.15.0 is up to date and compatible with netstandard2.0
- File: SourceGenerators/SourceGenerators.csproj (line 132)
<PackageReference Include="PolySharp" Version="1.15.0" PrivateAssets="all" />- 1.15.0 is the latest release as of June 2025.
- PolySharp’s source generators fully support SDK-style netstandard2.0 projects.
- No blocking compatibility issues; only watch for potential
InternalsVisibleToor multi-targeting edge cases in multi-project solutions.SourceGenerators/Utilities/TreeNode.cs (1)
3-6: Excellent use of nullable annotations for tree structure semantics.Making the
Parentproperty nullable correctly models that root nodes don't have parents. This change improves type safety and aligns well with the existingIsRootproperty logic.SourceGenerators/SceneTreeExtensions/SceneTreeDataModel.cs (1)
12-12:❓ Verification inconclusive
Verify null handling in downstream method.
Making
godotProjectDirnullable is appropriate as the project directory might not always be available. Ensure thatGD.GetResourcePathon line 15 properly handles null values for this parameter.
🏁 Script executed:
#!/bin/bash # Description: Check if GD.GetResourcePath method handles nullable godotProjectDir parameter # Search for the GD.GetResourcePath method implementation ast-grep --pattern $'GetResourcePath($_, $_) { $$$ }'Length of output: 56
Verify null handling in GD.GetResourcePath
I wasn’t able to find the
GD.GetResourcePathimplementation in this repository (it’s part of the external Godot API). Please manually confirm that passing anullvalue forgodotProjectDirinto this method is safe and won’t cause runtime errors.• File: SourceGenerators/SceneTreeExtensions/SceneTreeDataModel.cs
• Constructor call at line 12:GD.GetResourcePath(godotProjectDir, …)SourceGenerators/Utilities/Extensions/LinqExtensions.cs (1)
5-6: Correct nullable annotation for dictionary lookup method.Making the return type nullable accurately reflects that the method can return
default(TValue), which could be null for reference types. This improves type safety and caller awareness of potential null values.SourceGenerators/InputMapExtensions/InputMapScraper.cs (3)
1-1: LGTM! Necessary import for nullable annotations.The
System.Diagnostics.CodeAnalysisnamespace is correctly added to support the[NotNullWhen]attribute used later in the file.
11-11: LGTM! Appropriate nullable annotation for optional parameter.Making
gdRootnullable correctly reflects that this parameter can be null, which is already handled by the null-coalescing operator inGD.GetProjectFile(csFile, gdRoot).
40-40: LGTM! Correct implementation of nullable out parameter pattern.The
[NotNullWhen(true)]attribute correctly indicates that when the method returnstrue, theactionout parameter is guaranteed to be non-null. This is a best practice for nullable reference types with out parameters.SourceGenerators/Utilities/Extensions/GodotExtensions.cs (2)
7-7: LGTM! Correct nullable annotation for field initialized to null.Making
_resPathnullable correctly reflects that it's initialized tonulland can remain null until first assignment.
29-29: LGTM! Appropriate nullable annotations for optional parameters.Making the
projectDirparameters nullable correctly reflects their optional nature. The existing null-coalescing operators (??) already handle null values appropriately.Also applies to: 32-32
SourceGenerators/OnInstantiateExtensions/OnInstantiateAttribute.cs (1)
6-9: LGTM! Nullable annotations correctly reflect existing behavior.The constructor already handles null values with the pattern
ctor is null or "" or "none", so making both the parameter and property nullable correctly expresses the intended API design.SourceGenerators/Utilities/DataModel/BaseDataModel.cs (1)
7-9:✅ Verification successful
Verify that
GetNamespaceDeclaration()returns nullable values.The properties are changed to nullable, but please confirm that the
symbol.GetNamespaceDeclaration()extension method actually returns nullable values for these tuple elements. If not, these annotations may be incorrect.
🏁 Script executed:
#!/bin/bash # Description: Verify the return type of GetNamespaceDeclaration method # Expected: The method should return a tuple with nullable string elements # Search for the GetNamespaceDeclaration method implementation ast-grep --pattern 'GetNamespaceDeclaration($_) { $$$ }' # Also search for any return statements in this method rg -A 3 -B 3 "GetNamespaceDeclaration"Length of output: 2262
No changes needed:
GetNamespaceDeclarationalready returns nullable stringsThe extension method is declared as:
public static (string? NamespaceDeclaration, string? NamespaceClosure, string? NamespaceIndent) GetNamespaceDeclaration(this ISymbol symbol, string indent = " ")Each tuple element is
string?, so annotatingNSOpen,NSClose, andNSIndentas nullable is correct.SourceGenerators/Utilities/Extensions/CompilationExtensions.cs (2)
7-7: Correct nullable annotation for method return type.The change to
string?is appropriate sinceResolveDuplicatescan return null (viaFirstOrDefault), making the overall return value nullable.
39-40: Appropriate nullable cast with proper null handling.The cast to
IAssemblySymbol?correctly acknowledges thatGetAssemblyOrModuleSymbolcan return null, and the subsequent null check on line 40 handles this case properly.SourceGenerators/DiagnosticDetail.cs (1)
5-8: Well-designed nullable contracts for diagnostic properties.The changes create a logical separation where optional metadata (
Id,Category) can be null, while essential diagnostic information (Title,Message) is required and non-nullable. This ensures proper initialization while allowing flexibility for optional fields.SourceGenerators/OnImportExtensions/OnImportAttribute.cs (1)
6-6: Consistent nullable annotations for optional import name.The changes correctly make both the
importAsparameter andDisplayNameproperty nullable, which is appropriate sinceimportAshas a default value ofnullandDisplayNameis directly assigned from it.Also applies to: 18-18
SourceGenerators/AutoloadExtensions/AutoloadDataModel.cs (2)
7-7: Appropriate nullable annotations for optional autoload properties.Making
GodotNameandDisplayNamenullable is correct as these represent optional display information that may not always be available.
11-11:✅ Verification successful
Verify consistent nullable handling in related autoload components.
The nullable
gdRootparameter is appropriate for optional Godot root specification. Ensure thatAutoloadScraper.GetAutoloadsand other related methods properly handle nullgdRootvalues.
🏁 Script executed:
#!/bin/bash # Description: Verify that AutoloadScraper.GetAutoloads handles nullable gdRoot parameter # Expected: Method signature should accept string? gdRoot and handle null appropriately ast-grep --pattern 'GetAutoloads($_, $_, $_)' | head -10 rg -A 5 "GetAutoloads.*gdRoot"Length of output: 1319
gdRoot nullability is handled correctly
AutoloadScraper.GetAutoloads(compilation, csPath, gdRoot)accepts astring? gdRootand defaults it to the project file’s directory when null (gdRoot ??= Path.GetDirectoryName(gdFile)). No further changes required.SourceGenerators/SceneTreeExtensions/SceneTreeAttribute.cs (1)
9-12: LGTM! Correct nullable annotation implementation.The nullable annotations are properly applied:
tscnRelativeToClassPathis correctly made nullable since it acceptsnulland is checked for null in the constructorclassPathwith[CallerFilePath]is correctly made non-nullable withnull!default, as the compiler guarantees this parameter will never be null at runtimeSourceGenerators/OnInstantiateExtensions/OnInstantiateSourceGenerator.cs (4)
1-1: LGTM! Necessary import for nullable annotations.Adding
System.Diagnostics.CodeAnalysisis required for the[MaybeNull]attribute used in the static property.
11-11: LGTM! Correct use of MaybeNull attribute.The
[field: MaybeNull]attribute correctly indicates that the backing field for this property may be null before the lazy initialization occurs.
14-14: LGTM! Appropriate nullable return type.Changing the return type to
(string? GeneratedCode, DiagnosticDetail? Error)allows for proper null handling in error scenarios where either the generated code or error details may be null.
25-25: LGTM! Safer nullable cast.Casting to
(string?)instead of(string)is more defensive, as attribute constructor arguments could potentially be null.SourceGenerators/OnInstantiateExtensions/OnInstantiateDataModel.cs (2)
13-13: LGTM! Consistent nullable parameter and property design.Making
ConstructorScope,ctor, andgodotProjectDirnullable is appropriate since these parameters can legitimately be null in the source generator context.Also applies to: 15-15
44-44:✅ Verification successful
Verify the null-forgiving operator is safe.
The null-forgiving operator
x.Type!suppresses nullable warnings, but ensure thatx.Typeis guaranteed to be non-null in this context. If there's any possibility it could be null, proper null checking should be added instead.
🏁 Script executed:
#!/bin/bash # Search for similar usage patterns of ParameterSyntax.Type to understand if it can be null ast-grep --pattern 'x.Type' | head -20 # Search for null checks on ParameterSyntax.Type in the codebase rg -A 3 -B 3 "\.Type.*null|null.*\.Type" --type csLength of output: 2704
🏁 Script executed:
#!/bin/bash # Display context around the usage of x.Type in OnInstantiateDataModel.cs sed -n '30,80p' SourceGenerators/OnInstantiateExtensions/OnInstantiateDataModel.csLength of output: 2042
Ensure
x.Typeis never null before using!In
OnInstantiateDataModel.cstheArgParts(ParameterSyntax x)helper is only called on method-declaration parameters, andParameterSyntax.Typeis non-null for all declared parameters. Therefore the null-forgiving operator here is safe.No changes required.
SourceGenerators/GodotNotifyExtensions/GodotNotifySourceGenerator.cs (1)
1-1: LGTM! Consistent nullable annotations pattern.The changes follow the same correct nullable reference type pattern applied across other source generators:
- Added necessary import for
MaybeNullattribute- Properly annotated the static template property backing field
- Updated return type to nullable tuple for proper error handling
Also applies to: 11-11, 14-14
SourceGenerators/GodotNotifyExtensions/GodotNotifyDataModel.cs (2)
13-14: LGTM! Nullable properties improve semantic clarity.The change from
stringtostring?forGetAccessandSetAccessproperties correctly represents the semantic difference between "no accessor" (null) and "accessor with default visibility" (empty string).
39-45: LGTM! Null handling logic is correct.The updated
GetAccessibilitymethod properly handles null cases and maintains the existing logic while improving type safety with nullable annotations.SourceGenerators/InputMapExtensions/InputMapSourceGenerator.cs (4)
1-1: LGTM! Appropriate namespace import for nullable attributes.Adding
System.Diagnostics.CodeAnalysisenables the use ofMaybeNullattribute for better nullable static analysis.
11-12: LGTM! Correct use of MaybeNull attribute.The
[field: MaybeNull]attribute properly indicates that the backing field for the lazy-initialized static property may be null before first access.
14-14: LGTM! Nullable return types align with base class.The nullable tuple return type
(string? GeneratedCode, DiagnosticDetail? Error)correctly allows for null values and matches the pattern established in the base class.
25-25: LGTM! Appropriate use of null-forgiving operator.The null-forgiving operator is correctly used here since attribute constructor arguments are guaranteed to be non-null by the source generator infrastructure.
SourceGenerators/SceneTreeExtensions/SceneTreeSourceGenerator.cs (4)
1-1: LGTM! Consistent namespace import pattern.Adding
System.Diagnostics.CodeAnalysisfollows the same pattern as other source generators for nullable attribute support.
11-12: LGTM! Consistent MaybeNull attribute usage.The
[field: MaybeNull]attribute correctly indicates the lazy-initialized template property backing field may be null.
14-14: LGTM! Nullable return types maintain consistency.The nullable tuple return type maintains consistency with other source generators and the base class signature.
32-35: LGTM! Appropriate nullability handling in attribute reconstruction.The mixed approach using
(string?)for the first argument and null-forgiving operators for others correctly reflects the different nullability expectations of theSceneTreeAttributeconstructor parameters.SourceGenerators/LayerNamesExtensions/LayerNamesSourceGenerator.cs (4)
1-1: LGTM! Consistent nullable annotation support.Adding the
System.Diagnostics.CodeAnalysisnamespace maintains consistency with other source generator files for nullable attribute support.
11-12: LGTM! Proper MaybeNull attribute usage.The
[field: MaybeNull]attribute correctly indicates the lazy-initialized template property backing field may be null before first access.
14-14: LGTM! Consistent nullable return type.The nullable tuple return type
(string? GeneratedCode, DiagnosticDetail? Error)maintains consistency across all source generators and properly allows for null values.
25-25: LGTM! Safe use of null-forgiving operator.The null-forgiving operator is appropriately used since attribute constructor arguments are guaranteed to be non-null by the source generator framework.
SourceGenerators/SceneTreeExtensions/SceneTreeScraper.cs (6)
19-19: LGTM: Appropriate nullable annotation for optional field.The change to make
_resPathnullable correctly reflects that this field may not be initialized and can legitimately be null.
28-29: LGTM: Consistent nullable annotations for optional variables.Making
curNodeandsceneTreenullable appropriately reflects their usage patterns where they start as null and are assigned later.
101-102: LGTM: Appropriate nullable return type for optional cache lookup.The change to return
string?correctly reflects that the cache lookup may not find a matching entry.
222-222: LGTM: Consistent nullable parameter and appropriate null assertion.The nullable parameter correctly reflects optional parent nodes, and the null-forgiving operator at line 232 is safely used within a context where parent is verified to be non-null.
Also applies to: 232-232
155-155:❌ Incorrect review comment
Potentially unsafe tree traversal assumptions.
Using
x.Parent!assumes the Parent property is never null during tree traversal, but this could fail if the tree structure is unexpected.Also applies to: 172-172, 177-177
🏁 Script executed:
#!/bin/bash # Check TreeNode implementation to verify Parent property nullability guarantees ast-grep --pattern 'class TreeNode<$_> { $$$ public $_ Parent { get; } = $_; $$$ }'Length of output: 91
Continuing verification of
Parentnullability by inspecting the surrounding code:
🏁 Script executed:
#!/bin/bash # Locate and show context around lines with `Parent!` in SceneTreeScraper file="SourceGenerators/SceneTreeExtensions/SceneTreeScraper.cs" if [ -f "$file" ]; then echo "Using file: $file" else file=$(rg -l "SceneTreeScraper.cs") echo "Located file: $file" fi echo "Lines 130–200:" sed -n '130,200p' "$file"Length of output: 3723
Continuing verification by inspecting the file header and any
Traversedefinitions:
🏁 Script executed:
#!/bin/bash file="SourceGenerators/SceneTreeExtensions/SceneTreeScraper.cs" echo "=== Lines 1–130 of $file ===" sed -n '1,130p' "$file"Length of output: 6288
Inspecting
Tree<T>andTreeNode<T>definitions for nullability:
🏁 Script executed:
#!/bin/bash # Search for Tree and TreeNode class definitions echo "Searching for Tree<T> definition:" rg -n "class Tree<" . echo "Searching for TreeNode<T> definition:" rg -n "class TreeNode<" . echo "Searching for Traverse extension or method:" rg -n "Traverse" -n SourceGeneratorsLength of output: 1975
Verifying
TreeNode<T>’sParentnullability andIsRootlogic:
🏁 Script executed:
#!/bin/bash # Show TreeNode<T> implementation to confirm Parent property and IsRoot sed -n '1,200p' SourceGenerators/Utilities/TreeNode.csLength of output: 484
Parent nullability is safe after IsRoot check
The
Traversecallback only usesx.Parent!inside theelsebranch ofif (x.IsRoot), so at that pointParentis guaranteed non-null. No changes are needed here.Likely an incorrect or invalid review comment.
320-321:✅ Verification successful
Verify scene tree initialization guarantees.
Using null-forgiving operators assumes
sceneTreeis always initialized by this point, but this should be verified.
🏁 Script executed:
#!/bin/bash # Check if there are code paths where sceneTree might remain null ast-grep --pattern 'sceneTree = new($_)'Length of output: 179
I’d like to verify all code paths for
sceneTreeassignment before concluding it’s always non-null. Let’s dump where it’s declared/used and view the surrounding logic:
🏁 Script executed:
#!/bin/bash # List every occurrence of `sceneTree` rg -n "sceneTree" SourceGenerators/SceneTreeExtensions/SceneTreeScraper.cs # Show lines 150–350 to see the full initialization/return logic sed -n '150,350p' SourceGenerators/SceneTreeExtensions/SceneTreeScraper.csLength of output: 8502
Let’s inspect the
NodeMatchhandler to confirm that it always invokesAddRootNode(and thusAddNode) at least once:
🏁 Script executed:
#!/bin/bash # Locate and dump the NodeMatch function to see root-node handling rg -n "void NodeMatch" SourceGenerators/SceneTreeExtensions/SceneTreeScraper.cs sed -n '80,140p' SourceGenerators/SceneTreeExtensions/SceneTreeScraper.csLength of output: 2739
Let’s confirm where and how
NodeMatch(which drives the first call toAddNode) is invoked withinGetNodes:
🏁 Script executed:
#!/bin/bash # Locate the GetNodes method signature rg -n "GetNodes" SourceGenerators/SceneTreeExtensions/SceneTreeScraper.cs # Show the start of GetNodes through its main line-processing loop sed -n '1,200p' SourceGenerators/SceneTreeExtensions/SceneTreeScraper.csLength of output: 9715
sceneTree is always initialized by the first NodeMatch
The root
[node]section is guaranteed to be processed byNodeMatch, which calls eitherAddRootNodeorAddInheritedScene—both of which invoke the innerAddNode(SceneTreeNode…)and assignsceneTreebefore it’s used at lines 320–321. No code paths can reach the cache assignment with a nullsceneTree.SourceGenerators/CodeCommentsExtensions/CodeCommentsSourceGenerator.cs (3)
1-1: LGTM: Required import for nullable annotations.Adding the
System.Diagnostics.CodeAnalysisnamespace is necessary for the[MaybeNull]attribute used in the file.
11-12: LGTM: Appropriate use of MaybeNull attribute for lazy initialization.The
[field: MaybeNull]attribute correctly indicates that the backing field for this lazy-initialized property may be null before first access.
14-14: LGTM: Consistent nullable return type for optional code generation.Changing the return type to nullable tuple elements allows the generator to indicate when no code should be generated or when errors occur.
SourceGenerators/GodotOverrideExtensions/GodotOverrideSourceGenerator.cs (1)
1-1: LGTM: Consistent nullable annotation pattern.These changes follow the same pattern as other source generators in the PR, appropriately adding nullable support without changing logic.
Also applies to: 11-12, 14-14
SourceGenerators/AutoloadExtensions/AutoloadSourceGenerator.cs (1)
1-1: LGTM: Consistent nullable annotation pattern.These changes follow the established pattern for source generators in this PR.
Also applies to: 11-12, 22-22
SourceGenerators/AutoloadExtensions/AutoloadScraper.cs (7)
2-2: LGTM: Added required using directive for nullable attributes.The
System.Diagnostics.CodeAnalysisnamespace import is necessary for the[NotNullWhen(true)]attributes used later in the file.
15-15: LGTM: Nullable parameter allows flexibility for gdRoot.Making
gdRootnullable is appropriate since it has a fallback assignment on line 19.
46-46: LGTM: Correct use of NotNullWhen attribute.The
[NotNullWhen(true)]attributes correctly indicate that the out parameters will be non-null when the method returns true, which matches the implementation logic.
62-62: LGTM: Nullable return type reflects uncertainty.The nullable return type appropriately indicates that type determination may fail for unknown file extensions.
88-88: LGTM: Consistent nullable variable declarations.The local nullable variables appropriately reflect that type determination may fail during parsing.
Also applies to: 104-104, 128-128
168-168: LGTM: Consistent nullable parameter and return types.The nullable annotations consistently reflect the uncertainty in type determination across all scrapers and helper methods.
Also applies to: 205-205, 258-258, 302-302, 320-320, 342-342, 354-354
66-68:✅ Verification successful
Verify null-forgiving operators are safe.
The null-forgiving operators (
!) are used when passing nullablepathto methods expecting non-null strings. While the logic suggestspathshould be non-null at this point (coming from successful regex match), consider adding explicit null checks for robustness.
🏁 Script executed:
#!/bin/bash # Check if there are any scenarios where path could be null after successful regex match ast-grep --pattern $'if (TryMatchAutoload($_, out var name, out var path)) $$$ TryGetType($$$)'Length of output: 96
I’d like to see the surrounding definitions to be sure that
pathis non-nullable before the switch. Let’s grab theTryMatchAutoloadsignature and the switch block inAutoloadScraper.cs:
🏁 Script executed:
#!/bin/bash # Locate the AutoloadScraper file and dump the TryMatchAutoload signature + switch context set -e # Find the file containing the AutoloadScraper class file=$(rg -l "class AutoloadScraper" -g '*.cs') echo "Inspecting: $file" # Show the TryMatchAutoload method signature and a few lines around it echo -e "\n--- TryMatchAutoload definition ---" rg -n "TryMatchAutoload" -C5 "$file" # Show the switch on extension where path! is used and a few lines around echo -e "\n--- Switch on extension calling TryGetType ---" rg -n "=> Mini" -C5 "$file"Length of output: 2384
Null-forgiving usage is safe.
TryMatchAutoloadis annotated with[NotNullWhen(true)]on itspathout parameter, andTryGetType(path)is only ever called when that method returnstrue. At that point,pathis guaranteed non-null, so thepath!operators inside the switch are safe. No further null checks are required.SourceGenerators/OnImportExtensions/OnImportSourceGenerator.cs (4)
1-1: LGTM: Added required using directive.The
System.Diagnostics.CodeAnalysisnamespace is needed for the[field: MaybeNull]attribute.
11-12: LGTM: Appropriate use of MaybeNull attribute.The
[field: MaybeNull]attribute correctly indicates that the backing field may be null before lazy initialization, while the property itself will never return null due to the null-coalescing assignment.
23-23: LGTM: Nullable return type allows for flexible error handling.The nullable tuple return type is consistent with the pattern across other source generators and allows for null values in either generated code or error details.
35-41:✅ Verification successful
Review null handling in attribute reconstruction.
Most constructor arguments use null-forgiving operators, but the second argument (line 36) is explicitly cast to nullable string. This suggests the second parameter of
OnImportAttributeallows null values while others don't.
🏁 Script executed:
#!/bin/bash # Verify the constructor signature of OnImportAttribute to confirm which parameters are nullable ast-grep --pattern $'class OnImportAttribute { $$$ OnImportAttribute($$$) { $$$ } $$$ }'Length of output: 101
🏁 Script executed:
#!/bin/bash # Locate the definition of OnImportAttribute rg "class OnImportAttribute" -n # For each file defining it, display its constructor signature for file in $(rg -l "class OnImportAttribute" -g '*.cs'); do echo "---- $file ----" rg -n "OnImportAttribute\s*\(" "$file" -A2 doneLength of output: 746
Null handling is correct
TheOnImportAttributeconstructor defines only the second parameter (importAs) as nullable (string?), and the code’s casts align with this:
- Arguments 0, 2–6 are non-nullable and use
!- Argument 1 is nullable and cast to
string?No changes required.
SourceGenerators/SourceGeneratorForDeclaredFieldWithAttribute.cs (1)
10-11: LGTM: Consistent nullable return type pattern.The change from non-nullable to nullable tuples allows for more flexible error handling and aligns with the same pattern applied across other source generator base classes.
SourceGenerators/SourceGeneratorForDeclaredMethodWithAttribute.cs (1)
10-11: LGTM: Consistent nullable return type pattern.The nullable tuple return types are consistent with the changes made across other source generator base classes and provide flexibility for null values in generated code or diagnostic details.
SourceGenerators/SourceGeneratorForDeclaredTypeWithAttribute.cs (2)
10-11: LGTM: Consistent nullable return type implementation.The method signature changes correctly implement nullable return types
(string? GeneratedCode, DiagnosticDetail? Error), which aligns with the broader nullable annotation effort across all source generator classes. This allows generators to properly express when no code is generated or when no error occurs.
10-12: LGTM! Consistent nullable reference type adoption.The method signatures are correctly updated to use nullable tuple elements
(string? GeneratedCode, DiagnosticDetail? Error), which allows generators to optionally return no generated code or diagnostic details. This change aligns with the broader pattern across all source generator classes in the codebase.SourceGenerators/SourceGeneratorForDeclaredPropertyWithAttribute.cs (2)
10-11: LGTM: Maintains consistency with nullable return types.The nullable return type implementation is consistent with the pattern established across other source generator classes, properly expressing optional generated code and error details.
10-12: LGTM! Nullable annotations correctly applied.The method signatures are properly updated to return nullable tuple elements, maintaining consistency with the other source generator base classes. The delegation pattern and core functionality remain unchanged.
SourceGenerators/SourceGeneratorForDeclaredMemberWithAttribute.cs (10)
64-64: LGTM: Essential null check for symbol safety.Adding the null check for
symbolprevents potential null reference exceptions whenGetDeclaredSymbolreturns null, which can happen in certain compilation scenarios.
65-65: LGTM: Safe attribute class access.Using the null-conditional operator
?.forAttributeClassproperly handles cases where the attribute might not have a resolved class type, improving robustness.
72-73: LGTM: Appropriate use of null-forgiving operators.The null-forgiving operators (
!) are correctly used here since the code path ensureserroris not null whengeneratedCodeis null, and the diagnostic creation can handle null values for optional properties likeIdandCategory.
89-89: LGTM: Consistent abstract method signature.The abstract method signature update to nullable return types maintains consistency with the implementation pattern across all derived source generator classes.
91-91: LGTM: Matching private method signature.The private
_GenerateCodemethod signature correctly matches the abstract method signature with nullable return types.
122-129: LGTM: Modern array initialization syntax.The update to collection expression syntax
[...]for array initialization is a good modernization that improves readability while maintaining the same functionality.
64-65: LGTM! Excellent defensive programming.The null checks for
symboland the null-conditional operator forAttributeClass?.Nameimprove robustness by preventing potential NullReferenceExceptions during semantic analysis.
72-73: LGTM! Appropriate nullable handling.The null-forgiving operator
error!.Idis safe here since the error is already checked for null on line 70. The null-conditional operator forApplicationSyntaxReference?.GetSyntax()correctly handles cases where syntax reference might be unavailable.
89-91: LGTM! Consistent nullable tuple signatures.The method signatures are correctly updated to return nullable tuple elements, maintaining consistency with the inheritance hierarchy and allowing optional return values.
122-129: LGTM! Modern collection expression syntax.The array initialization is updated to use C# collection expressions
[...], which is more readable and concise than the traditional array initializer syntax.SourceGenerators/Utilities/Log.cs (5)
15-18: LGTM: Proper nullable handling for logging methods.The changes correctly:
- Allow nullable
object?messages for flexible logging- Use null-forgiving operators (
null!) forCallerFilePathandCallerMemberNamedefaults, which is appropriate since these compiler-provided attributes are never actually null at runtime
20-20: LGTM: Consistent nullable parameter.The
Formatmethod parameter change toobject?correctly matches the calling method signatures and handles null messages appropriately.
23-27: LGTM: Accurate nullable return types.The helper methods correctly return
string?since they can returnnullwhen their respective enable flags arefalse. This accurately represents the actual behavior and improves static analysis.
15-18: LGTM! Correct nullable annotations and caller attribute handling.The
object? msg = nullparameters correctly allow nullable messages, and thenull!for caller info parameters is the appropriate pattern since the compiler guarantees non-null values forCallerFilePathandCallerMemberNameattributes.
20-27: LGTM! Consistent nullable return types.The helper methods correctly return
string?to reflect that they may return null when their respective flags are disabled. TheFormatmethod parameter matches the public method signatures for consistency.SourceGenerators/Utilities/Extensions/SymbolExtensions.cs (15)
14-14: LGTM: Nullable annotation correctly reflects method behavior.The change from
stringtostring?is accurate since the method returnsnullwhen the symbol is in the global namespace.
17-17: LGTM: Nullable tuple components correctly reflect method behavior.The nullable annotations on the tuple components are accurate since the method returns
(null, null, null)when the namespace is null.
25-25: LGTM: Nullable annotation correctly reflects method behavior.The change to
INamedTypeSymbol?is accurate since the method can return null from either the null-conditional operation or theascast.
31-31: LGTM: Nullable annotation correctly reflects method behavior.The change to
string?is accurate sinceFirstOrDefault()can return null and the?.FilePathaccess can also be null.
34-34: LGTM: Nullable parameter annotation correctly reflects method behavior.The change to
IEnumerable<string>?is accurate since the method explicitly handles the null case withusings is null ? "" : string.Join("\n", usings).
14-15: LGTM! Correct nullable annotation.The method can return
nullwhen the containing namespace is global, so marking the return type asstring?accurately reflects the method's behavior.
17-23: LGTM! Correct nullable tuple components.The tuple components are correctly marked as nullable since the method returns
(null, null, null)when no namespace is present.
25-26: LGTM! Correct nullable return type.The method can return
nullwhen there's no containing type and the symbol is not anINamedTypeSymbol, so the nullable return type is appropriate.
31-32: LGTM! Correct nullable return type.The method uses
FirstOrDefault()which can returnnull, and the chained nullable operations correctly propagate nullability, makingstring?the appropriate return type.
34-48: LGTM! Appropriate nullable parameter.Making the
usingsparameter nullable allows callers to explicitly passnull, which the method already handles correctly with the null check on line 39.
14-14: LGTM! Correct nullable annotation.The method correctly returns
nullwhen the symbol is in the global namespace, making the nullable return type appropriate.
17-17: LGTM! Correct tuple nullable annotations.The tuple components can indeed be
nullwhen the symbol is in the global namespace, making these nullable annotations accurate.
31-32: LGTM! Correct nullable annotation.The
FilePathproperty can indeed benull, making the nullable return type appropriate for this method.
34-34: LGTM! Reasonable nullable parameter.Making the
usingsparameter nullable allows for optional usage, which is consistent with the method's logic that handlesnullusings appropriately.
25-26:✅ Verification successful
Verify the nullable logic is correct.
The method uses null-conditional chaining and null-coalescing, which suggests it can return
null. The nullable return type annotation appears correct, but please verify the logic handles all cases properly.
🏁 Script executed:
#!/bin/bash # Description: Verify OuterType method usage patterns to ensure nullable annotation is appropriate. # Expected: Find usage patterns that handle potential null returns. rg -A 3 -B 1 "\.OuterType\(\)" --type csLength of output: 622
Nullable logic in
OuterTypeis correct.The method either recurses up through
ContainingTypeor casts the original symbol toINamedTypeSymbol, returningnullonly if neither path yields a named type. TheINamedTypeSymbol?annotation accurately reflects this behavior. No changes required.SourceGenerators/OnImportExtensions/OnImportDataModel.cs (27)
2-2: LGTM: Required using for nullable attributes.The addition of
System.Diagnostics.CodeAnalysisis necessary to support the[NotNullWhen(true)]attribute used later in the file.
49-49: LGTM: Nullable out parameter correctly reflects method behavior.The change to
out object? valueis accurate since the method can assign null values fromExplicitDefaultValueor the attribute retrieval.
55-55: LGTM: Proper use of NotNullWhen attribute.The
[NotNullWhen(true)]attribute is correctly applied - when the method returnstrue, theoutparameter is guaranteed to be non-null from the attribute's constructor argument.
62-62: LGTM: Nullable annotations correctly reflect method behavior.Both the parameter and return type nullable annotations are accurate since the method can handle and return null values.
66-66: LGTM: Nullable out parameters correctly reflect method behavior.The nullable annotations are accurate since the parameters can be assigned null values when the corresponding methods don't exist or don't match the expected signature.
73-73: LGTM: Nullable return type correctly reflects method behavior.The change to
string?is accurate since the method returns null in several conditional branches when the method doesn't exist or doesn't match the expected signature.
88-88: LGTM: Nullable cast properly handles potential null values.The cast to
(long?)correctly handles the case where the attribute is null or the constructor argument value might be null.
89-89: Verify safety of null-forgiving operator usage.The null-forgiving operator
!assumes the constructor argument value is never null. While the attribute nullability is handled by the null-conditional operator, the constructor argument value itself could potentially be null, which would cause a runtime exception.Consider verifying if the
HintAttributeconstructor always provides non-null values for the second parameter, or use a safer approach:-hintString = (string)attribute?.ConstructorArguments[1].Value!; +hintString = (string?)attribute?.ConstructorArguments[1].Value ?? "";
92-92: LGTM: Nullable return type correctly reflects method behavior.The change to
string?is accurate since the method explicitly returns null when the underlying enum type is null.
114-114: LGTM: Nullable properties correctly reflect data model.The nullable annotations for
DefaultValueandUnderlyingEnumTypeare accurate since these properties can legitimately be null based on the parameter analysis logic.
116-116: LGTM: Nullable properties correctly reflect data model.The nullable annotations for
HasParams,ShowParams, andDefaultParamsare accurate since these properties can be null when the corresponding methods don't exist in the containing type.
2-2: LGTM! Required import for nullable annotations.The
System.Diagnostics.CodeAnalysisusing is necessary for the[NotNullWhen]attribute used in the nullable reference type annotations.
49-49: LGTM! Correct nullable out parameter.The
object?annotation is appropriate since the method can assignnullto thevalueparameter.
55-60: LGTM! Proper nullable annotations with NotNullWhen.The
[NotNullWhen(true)]attribute correctly indicates that when the method returnstrue, thevalueparameter is guaranteed to be non-null. The nullable member access onAttributeClass?.Nameis also appropriate.
62-63: LGTM! Correct nullable parameter and return type.Both the parameter and return type are appropriately marked as nullable since the method can accept and return
nullvalues.
66-82: LGTM! Correct nullable out parameters and return type.The nullable annotations are appropriate since the methods return
nullwhen the specified method names don't exist or don't match the expected signature.
85-90: Verify the null-forgiving operator usage.The nullable member access on line 87 is correct, but the null-forgiving operator on line 89 could be risky. If
attributeisnull, thenattribute?.ConstructorArguments[1].Value!would evaluate tonull, and casting it tostringcould cause issues.Consider using a more explicit null check or providing a default value:
-hintString = (string)attribute?.ConstructorArguments[1].Value!; +hintString = (string?)attribute?.ConstructorArguments[1].Value ?? string.Empty;
92-96: LGTM! Correct nullable return type.The method can return
nullwhen the type is not an enum or when the underlying type is not available, sostring?is the appropriate return type.
112-116: LGTM! Correct nullable record parameters.The nullable annotations on the record parameters correctly reflect that these values can be
nullbased on the logic in theCreateImportOptionmethod.
2-2: LGTM! Appropriate using directive.Adding
System.Diagnostics.CodeAnalysisis necessary for the nullable analysis attributes used in this file.
49-49: LGTM! Correct nullable out parameter.The default value can indeed be
null, making the nullable annotation appropriate.
55-60: LGTM! Proper use of NotNullWhen attribute.The
[NotNullWhen(true)]attribute correctly indicates that when the method returnstrue, thevalueout parameter will not benull. This is appropriate for the try-pattern method.
62-64: LGTM! Correct nullable return and parameter types.Both the input and output can be
null, making these nullable annotations accurate.
66-66: LGTM! Correct nullable out parameters.These parameters can indeed be
nullwhen the corresponding methods don't exist, making the nullable annotations appropriate.
73-82: LGTM! Correct nullable return type.The method can return
nullin multiple scenarios (method not found, wrong signature, etc.), making the nullable return type accurate.
92-96: LGTM! Correct nullable return type.The method correctly returns
nullwhen the type is not an enum, making the nullable return type appropriate.
114-116: LGTM! Appropriate nullable record parameters.The nullable annotations correctly reflect that
DefaultValue,UnderlyingEnumType, and the various parameter strings can all benullbased on the logic in the constructor.SourceGenerators/InputMapExtensions/InputMapAttribute.cs (1)
8-8: LGTM! Proper use of null-forgiving operator with CallerFilePath.The change from nullable to non-nullable parameter with
null!default is appropriate for[CallerFilePath]parameters, as the compiler will provide a non-null value at runtime when called normally.SourceGenerators/LayerNamesExtensions/LayerNamesAttribute.cs (1)
8-8: LGTM! Consistent nullable annotation pattern with CallerFilePath.The change from nullable to non-nullable parameter with
null!default follows the same correct pattern as other attribute classes in this PR. The null-forgiving operator is appropriate here since[CallerFilePath]will provide a non-null value at runtime.
| @@ -5,7 +5,7 @@ namespace Godot; | |||
| [AttributeUsage(AttributeTargets.Class)] | |||
| public sealed class InputMapAttribute : Attribute | |||
| { | |||
| public InputMapAttribute([CallerFilePath] string classPath = null) | |||
| public InputMapAttribute([CallerFilePath] string classPath = null!) | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reconsider using null-forgiving operator as default value.
Using null! as a default value for a non-nullable parameter is an anti-pattern that undermines the benefits of nullable reference types. While [CallerFilePath] typically provides a value, this approach declares the parameter as non-nullable while providing a null default.
Consider reverting to the nullable approach:
-public InputMapAttribute([CallerFilePath] string classPath = null!)
+public InputMapAttribute([CallerFilePath] string? classPath = null)Or if you want to enforce non-null, use a proper default:
-public InputMapAttribute([CallerFilePath] string classPath = null!)
+public InputMapAttribute([CallerFilePath] string classPath = "")📝 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.
| public InputMapAttribute([CallerFilePath] string classPath = null!) | |
| public InputMapAttribute([CallerFilePath] string? classPath = null) |
🤖 Prompt for AI Agents
In SourceGenerators/InputMapExtensions/InputMapAttribute.cs at line 8, avoid
using the null-forgiving operator 'null!' as the default value for the
non-nullable parameter classPath. Instead, change the parameter type to a
nullable string and set the default to null, or provide a meaningful non-null
default value. This aligns with nullable reference type best practices and
ensures the parameter's nullability is correctly represented.
| @@ -5,7 +5,7 @@ namespace Godot; | |||
| [AttributeUsage(AttributeTargets.Class)] | |||
| public sealed class LayerNamesAttribute : Attribute | |||
| { | |||
| public LayerNamesAttribute([CallerFilePath] string classPath = null) | |||
| public LayerNamesAttribute([CallerFilePath] string classPath = null!) | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Same issue as InputMapAttribute - avoid null-forgiving operator as default.
This has the same problematic pattern as InputMapAttribute where a non-nullable parameter uses null! as the default value, which is an anti-pattern for nullable reference types.
Apply the same fix as suggested for InputMapAttribute:
-public LayerNamesAttribute([CallerFilePath] string classPath = null!)
+public LayerNamesAttribute([CallerFilePath] string? classPath = null)Or use a proper non-null default if you want to enforce non-nullability.
📝 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.
| public LayerNamesAttribute([CallerFilePath] string classPath = null!) | |
| public LayerNamesAttribute([CallerFilePath] string? classPath = null) |
🤖 Prompt for AI Agents
In SourceGenerators/LayerNamesExtensions/LayerNamesAttribute.cs at line 8, the
constructor parameter classPath is declared as non-nullable but uses the
null-forgiving operator with a default null value, which is an anti-pattern. To
fix this, change the parameter to be nullable by adding a question mark (string?
classPath) and handle the null case inside the constructor, or provide a proper
non-null default value instead of null!. This will align with nullable reference
type best practices and avoid misleading nullability annotations.
💡 Verification agent
🧩 Analysis chain
Consistent but semantically confusing pattern with null! default.
This follows the same pattern as other attribute classes in the PR, but maintains the semantic inconsistency of declaring a non-nullable parameter while defaulting to null!. While [CallerFilePath] should provide a non-null value at compile time, the pattern creates confusion.
Consider establishing a consistent approach across all attribute classes - either maintain nullable parameters or use non-null defaults like empty strings.
Let me verify this pattern across other attribute classes:
🏁 Script executed:
#!/bin/bash
# Description: Check the pattern usage across all attribute classes in the codebase
# Expected: Find similar patterns in other attribute constructors
rg -A 2 -B 2 "\[CallerFilePath\].*classPath.*null!" --type csLength of output: 1458
Standardize CallerFilePath Parameter Defaults Across Attribute Constructors
All three attribute constructors in this PR use the pattern:
public XAttribute([CallerFilePath] string classPath = null!)– declaring a non‐nullable parameter with a null! default. While the compiler will supply a non‐null file path at call sites, defaulting to null! is semantically confusing and bypasses null safety checks.
Consider one of these two consistent approaches for all attribute classes:
- Default to an empty string and drop the null‐forgiving operator:
- public XAttribute([CallerFilePath] string classPath = null!)
- public XAttribute([CallerFilePath] string classPath = "")
- Or mark the parameter as nullable and keep
nullas the default: - public XAttribute([CallerFilePath] string classPath = null!)
- public XAttribute([CallerFilePath] string? classPath = null)
Affected files:
- SourceGenerators/InputMapExtensions/InputMapAttribute.cs
- SourceGenerators/LayerNamesExtensions/LayerNamesAttribute.cs
- SourceGenerators/SceneTreeExtensions/SceneTreeAttribute.cs
🤖 Prompt for AI Agents
In SourceGenerators/LayerNamesExtensions/LayerNamesAttribute.cs at line 8, the
constructor parameter classPath is declared as non-nullable but defaults to
null! which is semantically confusing and bypasses null safety. To fix this,
standardize the parameter by either making classPath nullable and defaulting to
null, or keep it non-nullable and default it to an empty string without the
null-forgiving operator. Apply the same consistent approach across all attribute
classes in the PR.
| @@ -63,12 +63,12 @@ void SectionMatch() | |||
|
|
|||
| void NodeMatch() | |||
| { | |||
| var nodeName = values.Get("name"); | |||
| var nodeName = values.Get("name")!; | |||
There was a problem hiding this comment.
Potentially unsafe null-forgiving operators on dictionary lookups.
Using ! on values.Get("name") and values.Get("type") assumes these keys will always exist, but this could cause runtime exceptions if the TSCN file format is unexpected or malformed.
Consider adding explicit null checks instead of using null-forgiving operators:
-var nodeName = values.Get("name")!;
+var nodeName = values.Get("name") ?? throw new InvalidOperationException("Node name is required");
-else if (nodeType is not null) nodeType = compilation.ValidateTypeCase("GodotSharp", "Godot", values.Get("type")!);
+else if (nodeType is not null) nodeType = compilation.ValidateTypeCase("GodotSharp", "Godot", values.Get("type") ?? throw new InvalidOperationException("Node type is required"));Also applies to: 71-71
🤖 Prompt for AI Agents
In SourceGenerators/SceneTreeExtensions/SceneTreeScraper.cs at lines 66 and 71,
the code uses the null-forgiving operator (!) on dictionary lookups which can
cause runtime exceptions if the keys "name" or "type" are missing. Replace these
with explicit null checks by retrieving the values safely and handling the case
where the keys do not exist, such as by returning early, throwing a descriptive
exception, or providing a default value to ensure robustness against malformed
input.
| @@ -256,7 +256,7 @@ Tree<SceneTreeNode> GetCachedScene(string resource) | |||
|
|
|||
| void EditableMatch() | |||
| { | |||
| var path = values.Get("path"); | |||
| var path = values.Get("path")!; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potentially unsafe dictionary access for required values.
Using null-forgiving operators on values.Get("path")! and values.Get("id")! assumes these required values are always present.
Consider explicit validation:
-var path = values.Get("path")!;
+var path = values.Get("path") ?? throw new InvalidOperationException("Path is required for editable match");
-var id = values.Get("id")!;
+var id = values.Get("id") ?? throw new InvalidOperationException("ID is required for external resource");Also applies to: 270-270
🤖 Prompt for AI Agents
In SourceGenerators/SceneTreeExtensions/SceneTreeScraper.cs at lines 259 and
270, the code uses null-forgiving operators on values.Get("path")! and
values.Get("id")!, assuming these keys always exist. To fix this, add explicit
checks to verify that these keys are present in the dictionary before accessing
them. If a key is missing, handle the situation gracefully, such as by throwing
a clear exception or returning an error, to avoid potential runtime null
reference exceptions.
| @@ -90,15 +90,15 @@ bool HasResource() | |||
|
|
|||
| string GetResource() | |||
| { | |||
| var resource = resources[resourceId]; | |||
| var resource = resources[resourceId!]!; | |||
There was a problem hiding this comment.
Potentially unsafe dictionary access with null-forgiving operators.
Using resources[resourceId!]! assumes both that resourceId is not null and that the dictionary contains the key. This could cause KeyNotFoundException at runtime.
Consider safer dictionary access:
-var resource = resources[resourceId!]!;
+if (resourceId is null || !resources.TryGetValue(resourceId, out var resource) || resource is null)
+ throw new InvalidOperationException($"Resource {resourceId} not found or invalid");
-var resource = resources[resourceId]!;
+if (!resources.TryGetValue(resourceId, out var resource) || resource is null)
+ throw new InvalidOperationException($"Resource {resourceId} not found or invalid");Also applies to: 300-300
🤖 Prompt for AI Agents
In SourceGenerators/SceneTreeExtensions/SceneTreeScraper.cs at line 93, the code
uses null-forgiving operators to access the dictionary with
resources[resourceId!]!, which can throw a KeyNotFoundException if resourceId is
null or the key is missing. Replace this with a safer approach by checking if
resourceId is not null and using TryGetValue on the dictionary to safely
retrieve the resource, handling the case where the key is absent. Apply the same
fix at line 300.
| List<SceneTreeNode> uniqueNodes = []; | ||
| var resources = new Dictionary<string, string>(); | ||
| var resources = new Dictionary<string, string?>(); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify resource value handling with nullable dictionary values.
Changing the dictionary to store nullable strings could impact downstream code that expects non-null values.
🏁 Script executed:
#!/bin/bash
# Check how resources dictionary values are used to ensure null handling is appropriate
ast-grep --pattern 'resources[$_]'Length of output: 280
🏁 Script executed:
#!/bin/bash
# Find where the resources dictionary is populated to determine if null values could be inserted
ast-grep --pattern 'resources.Add($_, $_)'
ast-grep --pattern 'resources[$_] = $_'Length of output: 196
Ensure non-null resource values or handle nulls explicitly
By changing the dictionary to Dictionary<string, string?>, we’ve introduced the possibility of null values, yet downstream lookups use the null-forgiving operator (!), which will throw if a null ever makes its way in. You should either revert the value type to non-nullable or add explicit null checks at insertion or lookup.
Please address the following spots:
-
SceneTreeScraper.cs:273
resources.Add(id, path);
▸ Validate
pathis notnullbefore adding, or throw an informative exception. -
SceneTreeScraper.cs:93 & 300
var resource = resources[resourceId!]!;
▸ Replace
!with a null‐check orTryGetValueto handle missing/nullentries safely.
🤖 Prompt for AI Agents
In SourceGenerators/SceneTreeExtensions/SceneTreeScraper.cs at lines 31, 93,
273, and 300, the dictionary resources is declared with nullable string values,
but downstream code uses the null-forgiving operator which can cause runtime
exceptions if null values are present. To fix this, either revert the dictionary
value type to non-nullable string or add explicit null checks: at line 273,
validate that path is not null before adding to resources and throw an
informative exception if it is; at lines 93 and 300, replace the null-forgiving
operator with safe access patterns such as TryGetValue and handle cases where
the resource is missing or null gracefully.
| @@ -20,6 +22,6 @@ protected override (string GeneratedCode, DiagnosticDetail Error) GenerateCode(C | |||
| return (output, null); | |||
|
|
|||
| Godot.CodeCommentsAttribute ReconstructAttribute() | |||
| => new((string)attribute.ConstructorArguments[0].Value); | |||
| => new((string)attribute.ConstructorArguments[0].Value!); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potentially unsafe cast with null-forgiving operator.
Using (string)attribute.ConstructorArguments[0].Value! assumes the constructor argument value is never null, but this could fail if the attribute is malformed.
Consider safer casting:
-=> new((string)attribute.ConstructorArguments[0].Value!);
+=> new(attribute.ConstructorArguments[0].Value as string ?? throw new InvalidOperationException("CodeComments attribute requires a string argument"));📝 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.
| => new((string)attribute.ConstructorArguments[0].Value!); | |
| => new(attribute.ConstructorArguments[0].Value as string | |
| ?? throw new InvalidOperationException("CodeComments attribute requires a string argument")); |
🤖 Prompt for AI Agents
In SourceGenerators/CodeCommentsExtensions/CodeCommentsSourceGenerator.cs at
line 25, the code uses a potentially unsafe cast with a null-forgiving operator
on attribute.ConstructorArguments[0].Value. To fix this, replace the direct cast
with a safer approach that checks for null before casting, such as using a
conditional cast or null-coalescing operator to handle possible null values
gracefully and avoid runtime exceptions.
| @@ -20,6 +22,6 @@ protected override (string GeneratedCode, DiagnosticDetail Error) GenerateCode(C | |||
| return (output, null); | |||
|
|
|||
| Godot.GodotOverrideAttribute ReconstructAttribute() | |||
| => new((bool)attribute.ConstructorArguments[0].Value); | |||
| => new((bool)attribute.ConstructorArguments[0].Value!); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potentially unsafe cast with null-forgiving operator.
Similar to the previous file, using (bool)attribute.ConstructorArguments[0].Value! assumes the constructor argument is never null.
Consider safer casting:
-=> new((bool)attribute.ConstructorArguments[0].Value!);
+=> new(attribute.ConstructorArguments[0].Value as bool? ?? throw new InvalidOperationException("GodotOverride attribute requires a boolean argument"));📝 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.
| => new((bool)attribute.ConstructorArguments[0].Value!); | |
| => new(attribute.ConstructorArguments[0].Value as bool? | |
| ?? throw new InvalidOperationException("GodotOverride attribute requires a boolean argument")); |
🤖 Prompt for AI Agents
In SourceGenerators/GodotOverrideExtensions/GodotOverrideSourceGenerator.cs at
line 25, the cast (bool)attribute.ConstructorArguments[0].Value! uses the
null-forgiving operator which assumes the value is never null and can cause
runtime exceptions. To fix this, replace the cast with a safer approach such as
checking if the value is null before casting or using a nullable bool and
handling the null case appropriately to avoid invalid casts.
| @@ -28,15 +30,15 @@ protected override (string GeneratedCode, DiagnosticDetail Error) GenerateCode(C | |||
| return (output, null); | |||
|
|
|||
| Godot.AutoloadAttribute ReconstructAttribute() | |||
| => new((string)attribute.ConstructorArguments[0].Value); | |||
| => new((string)attribute.ConstructorArguments[0].Value!); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potentially unsafe attribute value casting.
Multiple uses of null-forgiving operators when casting attribute constructor arguments assume these values are never null, which could cause runtime exceptions if attributes are malformed.
Consider safer attribute value extraction:
-=> new((string)attribute.ConstructorArguments[0].Value!);
+=> new(attribute.ConstructorArguments[0].Value as string ?? throw new InvalidOperationException("Autoload attribute requires a string argument"));
-.Where(x => x.AttributeClass?.Name == RenameAttribute)
+.Where(x => x.AttributeClass?.Name == RenameAttribute)
.ToDictionary(
- x => (string)x.ConstructorArguments[1].Value!, // GodotName
- x => (string)x.ConstructorArguments[0].Value!); // DisplayName
+ x => x.ConstructorArguments[1].Value as string ?? throw new InvalidOperationException("Rename attribute GodotName must be a string"),
+ x => x.ConstructorArguments[0].Value as string ?? throw new InvalidOperationException("Rename attribute DisplayName must be a string"));Also applies to: 38-42
🤖 Prompt for AI Agents
In SourceGenerators/AutoloadExtensions/AutoloadSourceGenerator.cs at lines 33
and 38-42, the code uses null-forgiving operators when casting attribute
constructor arguments, which can cause runtime exceptions if the attribute
values are null. To fix this, add null checks before casting or use safe casting
methods like 'as' with null checks, and handle the null case appropriately to
avoid exceptions from malformed attributes.
| propertyHint = (long?)attribute?.ConstructorArguments[0].Value; | ||
| hintString = (string)attribute?.ConstructorArguments[1].Value!; | ||
| } |
There was a problem hiding this comment.
Potential null reference issue with null-forgiving operator.
The null-forgiving operator on line 89 could be problematic. According to the HintAttribute constructor in the relevant code snippet, both parameters have default values, meaning hintString defaults to null. If someone uses [Hint()] without parameters, this would result in a null value being cast, potentially causing issues.
Consider removing the null-forgiving operator and handling the potential null case:
-hintString = (string)attribute?.ConstructorArguments[1].Value!;
+hintString = (string?)attribute?.ConstructorArguments[1].Value ?? string.Empty;🤖 Prompt for AI Agents
In SourceGenerators/OnImportExtensions/OnImportDataModel.cs around lines 88 to
90, the null-forgiving operator is used when casting the second constructor
argument to string, but since this argument can be null by default, this may
cause a null reference issue. Remove the null-forgiving operator and add a null
check or a fallback default value for hintString to safely handle cases where
the attribute is used without parameters.
I've added nullable annotations to the main project. There was a lot to change so I might have missed something, but I've tried my best to track what should be nullable and what should be suppressed.
I also switched from IsExternalInit to PolySharp because it has a lot more features, including attributes helpful here (MaybeNull).
Summary by CodeRabbit