Skip to content

Feat/bindnodesignal generator#152

Merged
GeWuYou merged 5 commits into
mainfrom
feat/bindnodesignal-generator
Mar 31, 2026
Merged

Feat/bindnodesignal generator#152
GeWuYou merged 5 commits into
mainfrom
feat/bindnodesignal-generator

Conversation

@GeWuYou

@GeWuYou GeWuYou commented Mar 31, 2026

Copy link
Copy Markdown
Owner

Summary by Sourcery

添加了一个新的 Roslyn 增量生成器,用于基于一个新的特性自动绑定和解绑 Godot 节点的 CLR 事件,并包含相应的诊断、测试和文档。

New Features:

  • 引入 BindNodeSignalAttribute,以声明式方式描述 Godot 节点字段与其 CLR 事件之间的绑定关系。
  • 为带有 BindNodeSignalAttribute 标记的、作为 partial 的 Godot 节点类生成 __BindNodeSignals_Generated__UnbindNodeSignals_Generated 辅助方法,用于集中管理事件订阅。

Enhancements:

  • 添加 BindNodeSignal 专用的诊断描述符和规则注册,用于验证其用法以及为生成的绑定配置生命周期钩子。

Documentation:

  • 在源生成器的 README 中记录 BindNodeSignal 的使用方式以及生成的辅助方法,并包含示例代码。

Tests:

  • 添加 BindNodeSignalGenerator 测试套件,覆盖代码生成、与 GetNode 的共存、无效的信号或处理程序配置,以及生命周期钩子相关警告诊断等场景。
Original summary in English

Summary by Sourcery

Add a new Roslyn incremental generator to automatically bind and unbind Godot node CLR events based on a new attribute, including diagnostics, tests, and documentation.

New Features:

  • Introduce the BindNodeSignalAttribute to declaratively describe bindings between Godot node fields and their CLR events.
  • Generate __BindNodeSignals_Generated and __UnbindNodeSignals_Generated helper methods for partial Godot node classes marked with BindNodeSignalAttribute to centralize event subscription management.

Enhancements:

  • Add BindNodeSignal-specific diagnostic descriptors and rule registrations to validate usage and lifecycle hook wiring for generated bindings.

Documentation:

  • Document BindNodeSignal usage and generated helper methods in the source generators README, including example code.

Tests:

  • Add BindNodeSignalGenerator test suite covering code generation, coexistence with GetNode, invalid signal or handler configurations, and lifecycle-hook warning diagnostics.

GeWuYou added 2 commits March 31, 2026 09:39
- 实现 BindNodeSignalGenerator 源生成器,用于自动生成 Godot 节点事件绑定与解绑逻辑
- 添加 BindNodeSignalAttribute 特性,标记需要生成绑定逻辑的事件处理方法
- 实现完整的诊断系统,包括嵌套类型、静态方法、字段类型等错误检查
- 添加生命周期方法调用检查,在 _Ready 和 _ExitTree 中验证生成方法的调用
- 支持方法签名与事件委托的兼容性验证
- 实现单元测试覆盖各种使用场景和错误情况
- 新增 AnalyzerReleases.Unshipped.md 文件记录代码分析规则
- 添加 GF_Godot_GetNode 系列规则定义(001-006)
- 添加 GF_Godot_BindNodeSignal 系列规则定义(001-009)
- 创建 README.md 文件详述源码生成器使用方法
- 文档化 GetNode 和 BindNodeSignal 特性用法示例
- 说明 Godot 场景相关的编译期生成能力
@sourcery-ai

sourcery-ai Bot commented Mar 31, 2026

Copy link
Copy Markdown

Reviewer's Guide

引入了一个新的 [BindNodeSignal] 特性以及对应的 Roslyn 增量生成器,用于自动生成 Godot 节点信号订阅/取消订阅方法,并包含诊断信息、测试以及文档更新。

BindNodeSignal 增量生成与运行时使用的时序图

sequenceDiagram
    participant Dev as Developer
    participant CSharpCompiler as CSharpCompiler
    participant Roslyn as RoslynIncrementalPipeline
    participant Gen as BindNodeSignalGenerator
    participant Hud as TestAppHud
    participant Button as GodotButton

    Dev->>CSharpCompiler: Build project with [BindNodeSignal] methods
    CSharpCompiler->>Roslyn: Provide syntax trees and symbols
    Roslyn->>Gen: Initialize(context)
    Roslyn->>Gen: Execute(compilation, candidates)
    Gen->>Gen: Find methods with BindNodeSignalAttribute
    Gen->>Gen: Validate node field, event, and handler signature
    Gen-->>Roslyn: Generated partial Hud with __BindNodeSignals_Generated and __UnbindNodeSignals_Generated
    Roslyn-->>CSharpCompiler: Add generated source to compilation
    CSharpCompiler-->>Dev: Emit assembly

    Dev->>Hud: Instantiate and add to scene
    Hud->>Hud: _Ready()
    Hud->>Hud: __BindNodeSignals_Generated()
    Hud->>Button: subscribe Pressed += OnStartButtonPressed

    Button-->>Hud: Pressed event
    Hud->>Hud: OnStartButtonPressed()

    Hud->>Hud: _ExitTree()
    Hud->>Hud: __UnbindNodeSignals_Generated()
    Hud->>Button: unsubscribe Pressed -= OnStartButtonPressed
Loading

File-Level Changes

Change Details Files
Add BindNodeSignal incremental source generator to emit bind/unbind helper methods for annotated handlers.
  • 通过语法提供器和语义模型扫描带有 [BindNodeSignal] 的方法,收集候选项并按其包含的类型进行分组。
  • 根据诊断规则验证候选方法及其关联的节点字段(非静态、字段存在、仅允许实例成员、继承自 Godot.Node、事件存在、委托签名兼容)。
  • 生成定义 __BindNodeSignals_Generated 和 __UnbindNodeSignals_Generated 方法的部分类实现,将 CLR 事件与处理方法连接起来。
  • 确保仅支持部分类且不支持嵌套类型,并在生成过程中使用类型/命名空间辅助方法(GetNamespace、ResolveGenerics、AreAllDeclarationsPartial)。
GFramework.Godot.SourceGenerators/BindNodeSignalGenerator.cs
Add diagnostics set for BindNodeSignal misuse and lifecycle hook guidance.
  • 定义错误诊断,用于检测不支持的嵌套类、静态方法、缺失或无效的节点字段、非 Node 字段类型、缺失事件以及不兼容的处理方法签名。
  • 定义警告诊断:当类重写 _Ready 或 _ExitTree 却未调用生成的绑定/解绑辅助方法时发出警告。
  • 将新的诊断 ID 及其严重级别集成到分析器发布清单中。
GFramework.Godot.SourceGenerators/Diagnostics/BindNodeSignalDiagnostics.cs
GFramework.Godot.SourceGenerators/AnalyzerReleases.Unshipped.md
Introduce BindNodeSignal attribute abstraction for consumers.
  • 定义针对方法的密封 BindNodeSignalAttribute,允许在同一方法上使用多个特性实例。
  • 在构造函数中存储节点字段名和信号(事件)名,并进行空值检查,同时提供 XML 文档说明其使用意图。
GFramework.Godot.SourceGenerators.Abstractions/BindNodeSignalAttribute.cs
Add unit tests covering generation output and diagnostics for BindNodeSignalGenerator.
  • 验证在典型用法下生成的绑定/解绑方法,以及与 GetNode 生成成员共存时的行为。
  • 测试单个处理方法的多次订阅以及多个节点字段的正确连线。
  • 使用 Roslyn 测试框架断言覆盖缺失事件、不兼容处理方法签名以及未在生命周期钩子中调用生成辅助方法等诊断场景。
GFramework.Godot.SourceGenerators.Tests/BindNodeSignal/BindNodeSignalGeneratorTests.cs
Update documentation to describe BindNodeSignal usage.
  • 扩展 README,记录 BindNodeSignal 的使用模式以及与 GetNode 的配合使用,包括示例 Hud 类及生命周期调用。
  • 说明仅支持基于 CLR 事件的 Godot 信号,不会自动调用 Connect/Disconnect。
  • 文档化用于绑定和解绑的生成辅助方法名称。
GFramework.Godot.SourceGenerators/README.md

Tips and commands

Interacting with Sourcery

  • 触发新评审: 在 pull request 中评论 @sourcery-ai review
  • 继续讨论: 直接回复 Sourcery 的评审评论。
  • 从评审评论生成 GitHub issue: 在某条评审评论下回复,要求 Sourcery 从该评论创建 issue。你也可以直接在该评论下回复 @sourcery-ai issue 来创建 issue。
  • 生成 pull request 标题: 在 pull request 标题中任意位置写入 @sourcery-ai 可随时生成标题。你也可以在 pull request 中评论 @sourcery-ai title 来(重新)生成标题。
  • 生成 pull request 摘要: 在 pull request 正文任意位置写入 @sourcery-ai summary,即可在该位置生成 PR 摘要。你也可以在 pull request 中评论 @sourcery-ai summary 来在任意时间(重新)生成摘要。
  • 生成评审者指南: 在 pull request 中评论 @sourcery-ai guide,可在任意时间(重新)生成评审者指南。
  • 解决所有 Sourcery 评论: 在 pull request 中评论 @sourcery-ai resolve 来解决所有 Sourcery 评论。如果你已经处理完所有评论且不想再看到它们,这会非常有用。
  • 忽略所有 Sourcery 评审: 在 pull request 中评论 @sourcery-ai dismiss 来忽略所有现有的 Sourcery 评审。特别适用于你想从头开始新一轮评审的情况——别忘了评论 @sourcery-ai review 以触发新的评审!

Customizing Your Experience

访问你的 dashboard 以:

  • 启用或禁用评审特性,例如 Sourcery 自动生成的 pull request 摘要、评审者指南等。
  • 更改评审语言。
  • 添加、移除或编辑自定义评审说明。
  • 调整其他评审设置。

Getting Help

Original review guide in English

Reviewer's Guide

Introduces a new [BindNodeSignal] attribute and corresponding Roslyn incremental generator to auto-generate Godot node signal subscription/unsubscription methods, along with diagnostics, tests, and documentation updates.

Sequence diagram for BindNodeSignal incremental generation and runtime usage

sequenceDiagram
    participant Dev as Developer
    participant CSharpCompiler as CSharpCompiler
    participant Roslyn as RoslynIncrementalPipeline
    participant Gen as BindNodeSignalGenerator
    participant Hud as TestAppHud
    participant Button as GodotButton

    Dev->>CSharpCompiler: Build project with [BindNodeSignal] methods
    CSharpCompiler->>Roslyn: Provide syntax trees and symbols
    Roslyn->>Gen: Initialize(context)
    Roslyn->>Gen: Execute(compilation, candidates)
    Gen->>Gen: Find methods with BindNodeSignalAttribute
    Gen->>Gen: Validate node field, event, and handler signature
    Gen-->>Roslyn: Generated partial Hud with __BindNodeSignals_Generated and __UnbindNodeSignals_Generated
    Roslyn-->>CSharpCompiler: Add generated source to compilation
    CSharpCompiler-->>Dev: Emit assembly

    Dev->>Hud: Instantiate and add to scene
    Hud->>Hud: _Ready()
    Hud->>Hud: __BindNodeSignals_Generated()
    Hud->>Button: subscribe Pressed += OnStartButtonPressed

    Button-->>Hud: Pressed event
    Hud->>Hud: OnStartButtonPressed()

    Hud->>Hud: _ExitTree()
    Hud->>Hud: __UnbindNodeSignals_Generated()
    Hud->>Button: unsubscribe Pressed -= OnStartButtonPressed
Loading

File-Level Changes

Change Details Files
Add BindNodeSignal incremental source generator to emit bind/unbind helper methods for annotated handlers.
  • Scans methods with [BindNodeSignal] via syntax provider and semantic model to collect candidates grouped by containing type.
  • Validates candidate methods and associated node fields against diagnostics (non-static, field existence, instance-only, derives from Godot.Node, event existence, compatible delegate signature).
  • Generates partial class implementations that define __BindNodeSignals_Generated and __UnbindNodeSignals_Generated methods wiring CLR events to handler methods.
  • Ensures only partial, non-nested types are supported and uses type/namespace helpers (GetNamespace, ResolveGenerics, AreAllDeclarationsPartial) for generation.
GFramework.Godot.SourceGenerators/BindNodeSignalGenerator.cs
Add diagnostics set for BindNodeSignal misuse and lifecycle hook guidance.
  • Defines error diagnostics for unsupported nested classes, static methods, missing or invalid node fields, non-Node field types, missing events, and incompatible handler signatures.
  • Defines warning diagnostics when classes override _Ready or _ExitTree but do not call the generated bind/unbind helper methods.
  • Integrates new diagnostic IDs into the analyzer releases manifest with severities.
GFramework.Godot.SourceGenerators/Diagnostics/BindNodeSignalDiagnostics.cs
GFramework.Godot.SourceGenerators/AnalyzerReleases.Unshipped.md
Introduce BindNodeSignal attribute abstraction for consumers.
  • Defines sealed BindNodeSignalAttribute targeting methods, allowing multiple attributes per method.
  • Stores node field name and signal (event) name with null-checking in the constructor and XML documentation for usage intent.
GFramework.Godot.SourceGenerators.Abstractions/BindNodeSignalAttribute.cs
Add unit tests covering generation output and diagnostics for BindNodeSignalGenerator.
  • Verifies generated bind/unbind methods for typical usage and coexistence with GetNode-generated members.
  • Tests multiple subscriptions for a single handler and accurate wiring for multiple node fields.
  • Covers diagnostics for missing events, incompatible handler signatures, and missing lifecycle hook calls to generated helper methods using Roslyn test harness expectations.
GFramework.Godot.SourceGenerators.Tests/BindNodeSignal/BindNodeSignalGeneratorTests.cs
Update documentation to describe BindNodeSignal usage.
  • Extends README to document BindNodeSignal usage pattern alongside GetNode, including example Hud class and lifecycle calls.
  • Clarifies that only CLR event-based Godot signals are supported and Connect/Disconnect are not invoked automatically.
  • Documents generated helper method names for binding and unbinding.
GFramework.Godot.SourceGenerators/README.md

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@deepsource-io

deepsource-io Bot commented Mar 31, 2026

Copy link
Copy Markdown

DeepSource Code Review

We reviewed changes in a628ade...693cad2 on this pull request. Below is the summary for the review, and you can see the individual issues we found as inline review comments.

See full review on DeepSource ↗

PR Report Card

Overall Grade   Security  

Reliability  

Complexity  

Hygiene  

Code Review Summary

Analyzer Status Updated (UTC) Details
C# Mar 31, 2026 4:11a.m. Review ↗
Secrets Mar 31, 2026 4:11a.m. Review ↗

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - 我发现了两个问题,并给出了一些总体反馈:

  • 建议防范用户自定义名为 __BindNodeSignals_Generated__UnbindNodeSignals_Generated 的成员(例如,通过检查是否已存在这些名称的方法并报告诊断),以避免当使用方意外定义了相同签名时出现静默的编译冲突。
  • 当前的 IsCandidate 过滤器使用的是 attribute.Name.ToString().Contains("BindNodeSignal", StringComparison.Ordinal),这可能会匹配到无关的特性,并带来字符串分配开销;你可以改为匹配 IdentifierNameSyntax/QualifiedNameSyntax,并将简单名称精确地与 BindNodeSignal 比较,以同时提升正确性和性能。
  • AnalyzerReleases.Unshipped.md 中声明的规则目前到 GF_Godot_BindNodeSignal_008,但 BindNodeSignalDiagnostics 还定义了 GF_Godot_BindNodeSignal_009;建议将该规则加入表格,使分析器元数据与已实现的诊断保持同步。
面向 AI Agent 的提示词
Please address the comments from this code review:

## Overall Comments
- Consider guarding against user-defined members named `__BindNodeSignals_Generated` or `__UnbindNodeSignals_Generated` (e.g., by checking for existing methods with these names and reporting a diagnostic) to avoid silent compilation conflicts when a consumer accidentally defines the same signatures.
- The `IsCandidate` filter currently uses `attribute.Name.ToString().Contains("BindNodeSignal", StringComparison.Ordinal)`, which may match unrelated attributes and adds string-allocation overhead; you could instead match on `IdentifierNameSyntax`/`QualifiedNameSyntax` and compare the simple name exactly to `BindNodeSignal` for both correctness and performance.
- AnalyzerReleases.Unshipped.md declares rules up to `GF_Godot_BindNodeSignal_008`, but `BindNodeSignalDiagnostics` also defines `GF_Godot_BindNodeSignal_009`; consider adding this rule to the table so analyzer metadata stays in sync with the implemented diagnostics.

## Individual Comments

### Comment 1
<location path="GFramework.Godot.SourceGenerators/BindNodeSignalGenerator.cs" line_range="71-74" />
<code_context>
+        if (bindNodeSignalAttribute is null || godotNodeSymbol is null)
+            return;
+
+        var methodCandidates = candidates
+            .Where(static candidate => candidate is not null)
+            .Select(static candidate => candidate!)
+            .Where(candidate => ResolveAttributes(candidate.MethodSymbol, bindNodeSignalAttribute).Count > 0)
+            .ToList();
+
</code_context>
<issue_to_address>
**suggestion (performance):** ResolveAttributes 每个方法被调用两次,这是可以避免的开销。

在 `Execute` 中,`ResolveAttributes` 既在 LINQ 过滤器里计算,又在 `group.Methods` 循环内部再次调用。由于特性解析依赖语义分析且相对昂贵,建议针对每个 `MethodCandidate` 只解析一次特性(例如,在 `MethodCandidate` 上存一个 `IReadOnlyList<AttributeData>`,或者使用一个中间投影),然后复用该结果,以避免重复工作,这对大型项目会有帮助。

建议实现方式:

```csharp
        if (bindNodeSignalAttribute is null || godotNodeSymbol is null)
            return;

        // Cache resolved attributes per method candidate to avoid repeated semantic work.
        var methodAttributes = candidates
            .Where(static candidate => candidate is not null)
            .Select(static candidate => candidate!)
            .ToDictionary(
                static candidate => candidate,
                candidate => ResolveAttributes(candidate.MethodSymbol, bindNodeSignalAttribute));

        var methodCandidates = methodAttributes
            .Where(static kvp => kvp.Value.Count > 0)
            .Select(static kvp => kvp.Key)
            .ToList();


```

为了在 `Execute` 中彻底避免对每个方法调用两次 `ResolveAttributes`,你还需要:

1. 找到后面 `Execute``group.Methods` 的循环,在那里又调用了 `ResolveAttributes`(大致类似 `ResolveAttributes(methodCandidate.MethodSymbol, bindNodeSignalAttribute)`)。
2. 将这些调用替换为对上面创建的 `methodAttributes` 字典的查找。例如:
   - 如果当前有 `var attributes = ResolveAttributes(methodCandidate.MethodSymbol, bindNodeSignalAttribute);`,请改为 `var attributes = methodAttributes[methodCandidate];`3. 确保在定义 `group.Methods` 循环的作用域内可以访问 `methodAttributes` 字典。如果该循环在一个嵌套的本地函数或单独的辅助方法里,请将 `methodAttributes` 作为参数传入,而不是在那儿重新计算特性。

这些调整能确保每个方法的特性只解析一次,并在过滤和生成逻辑中复用。
</issue_to_address>

### Comment 2
<location path="GFramework.Godot.SourceGenerators/BindNodeSignalGenerator.cs" line_range="247-254" />
<code_context>
+        context.ReportDiagnostic(Diagnostic.Create(descriptor, location, messageArgs));
+    }
+
+    private static string ResolveCtorString(
+        AttributeData attribute,
+        int index)
+    {
+        if (attribute.ConstructorArguments.Length <= index)
+            return string.Empty;
+
+        return attribute.ConstructorArguments[index].Value as string ?? string.Empty;
+    }
+
</code_context>
<issue_to_address>
**suggestion:** 缺失的构造函数参数会退化为空字符串,这会导致诊断信息令人困惑。

由于 `ResolveCtorString` 在构造函数参数缺失或非字符串时会返回 `string.Empty`,这个空名称会继续流入 `FindField` / `FindEvent`,从而产生类似 “field '' not found” 这种不清晰的诊断。建议显式检测“缺失或类型错误”的情况并报告专门的诊断,或者至少在解析出的名称为空时尽早短路,让用户能得到关于错误特性用法的更清晰提示。

建议实现方式:

```csharp
    private static bool TryResolveCtorString(
        AttributeData attribute,
        int index,
        out string value)
    {
        value = string.Empty;

        // Missing ctor argument at the requested index.
        if (attribute.ConstructorArguments.Length <= index)
            return false;

        var ctorArgument = attribute.ConstructorArguments[index];

        // We only support primitive string literals here; anything else is considered invalid.
        if (ctorArgument.Kind != TypedConstantKind.Primitive || ctorArgument.Value is not string s)
            return false;

        // Treat empty/whitespace-only names as invalid to avoid confusing diagnostics like "field '' not found".
        if (string.IsNullOrWhiteSpace(s))
            return false;

        value = s;
        return true;
    }

    /// <summary>
    /// Legacy helper kept for backward compatibility. Prefer <see cref="TryResolveCtorString"/>.
    /// This method returns <see cref="string.Empty"/> when the constructor argument is missing or invalid.
    /// </summary>
    private static string ResolveCtorString(
        AttributeData attribute,
        int index)
    {
        return TryResolveCtorString(attribute, index, out var value)
            ? value
            : string.Empty;
    }

```

为了完整实现你在诊断行为上的改进建议,还需要在本文件其他位置做以下修改:

1. **更新所有调用点**,将目前类似以下形式:
   - `var fieldName = ResolveCtorString(attribute, 0);`
   - `var eventName = ResolveCtorString(attribute, 0);`
   然后把这些值传入 `FindField``FindEvent` 或类似方法的调用,改为:
   ```csharp
   if (!TryResolveCtorString(attribute, 0, out var fieldName))
   {
       // Report a dedicated "invalid attribute usage" diagnostic and return.
       ReportDiagnostic(
           context,
           InvalidBindNodeSignalAttributeDescriptor, // define this descriptor if it doesn't exist yet
           candidate,
           attribute,
           /* any message arguments you need, e.g. candidate.Method.Identifier.Text */
       );
       return;
   }

   var field = FindField(fieldName, ...);
   ```
2. **引入一个专门的诊断描述符**(如果你还没有的话),用于 `BindNodeSignalAttribute` 上无效/缺失的构造函数参数,例如:
   ```csharp
   private static readonly DiagnosticDescriptor InvalidBindNodeSignalAttributeDescriptor =
       new DiagnosticDescriptor(
           "GODOTSG001",
           "Invalid BindNodeSignal attribute usage",
           "The {0} attribute requires a non-empty string literal for its '{1}' constructor argument.",
           "Usage",
           DiagnosticSeverity.Error,
           isEnabledByDefault: true);
   ```
3.`TryResolveCtorString` 返回 `false` 时,在提前返回的路径上使用这个新的描述符,这样用户能看到类似以下清晰的诊断信息:
   - “The BindNodeSignal attribute requires a non-empty string literal for its 'signalName' constructor argument.”
   而不是后续产生的诸如 “field '' not found” 之类的诊断。

这些更改可以确保缺失或类型不正确的构造函数参数不再悄然退化为空字符串,并让用户获得关于错误特性用法的明确诊断提示。
</issue_to_address>

Sourcery 对开源项目免费——如果你觉得我们的代码审查有帮助,请考虑分享 ✨
帮我变得更有用!请在每条评论上点 👍 或 👎,我会根据反馈改进后续的审查。
Original comment in English

Hey - I've found 2 issues, and left some high level feedback:

  • Consider guarding against user-defined members named __BindNodeSignals_Generated or __UnbindNodeSignals_Generated (e.g., by checking for existing methods with these names and reporting a diagnostic) to avoid silent compilation conflicts when a consumer accidentally defines the same signatures.
  • The IsCandidate filter currently uses attribute.Name.ToString().Contains("BindNodeSignal", StringComparison.Ordinal), which may match unrelated attributes and adds string-allocation overhead; you could instead match on IdentifierNameSyntax/QualifiedNameSyntax and compare the simple name exactly to BindNodeSignal for both correctness and performance.
  • AnalyzerReleases.Unshipped.md declares rules up to GF_Godot_BindNodeSignal_008, but BindNodeSignalDiagnostics also defines GF_Godot_BindNodeSignal_009; consider adding this rule to the table so analyzer metadata stays in sync with the implemented diagnostics.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- Consider guarding against user-defined members named `__BindNodeSignals_Generated` or `__UnbindNodeSignals_Generated` (e.g., by checking for existing methods with these names and reporting a diagnostic) to avoid silent compilation conflicts when a consumer accidentally defines the same signatures.
- The `IsCandidate` filter currently uses `attribute.Name.ToString().Contains("BindNodeSignal", StringComparison.Ordinal)`, which may match unrelated attributes and adds string-allocation overhead; you could instead match on `IdentifierNameSyntax`/`QualifiedNameSyntax` and compare the simple name exactly to `BindNodeSignal` for both correctness and performance.
- AnalyzerReleases.Unshipped.md declares rules up to `GF_Godot_BindNodeSignal_008`, but `BindNodeSignalDiagnostics` also defines `GF_Godot_BindNodeSignal_009`; consider adding this rule to the table so analyzer metadata stays in sync with the implemented diagnostics.

## Individual Comments

### Comment 1
<location path="GFramework.Godot.SourceGenerators/BindNodeSignalGenerator.cs" line_range="71-74" />
<code_context>
+        if (bindNodeSignalAttribute is null || godotNodeSymbol is null)
+            return;
+
+        var methodCandidates = candidates
+            .Where(static candidate => candidate is not null)
+            .Select(static candidate => candidate!)
+            .Where(candidate => ResolveAttributes(candidate.MethodSymbol, bindNodeSignalAttribute).Count > 0)
+            .ToList();
+
</code_context>
<issue_to_address>
**suggestion (performance):** ResolveAttributes is invoked twice per method, which is avoidable work.

In `Execute`, `ResolveAttributes` is evaluated in the LINQ filter and again inside the `group.Methods` loop. Since attribute resolution is semantic and relatively expensive, consider resolving attributes once per `MethodCandidate` (e.g., store an `IReadOnlyList<AttributeData>` on `MethodCandidate` or use an intermediate projection) and reuse that result to avoid duplicate work, which will help on large projects.

Suggested implementation:

```csharp
        if (bindNodeSignalAttribute is null || godotNodeSymbol is null)
            return;

        // Cache resolved attributes per method candidate to avoid repeated semantic work.
        var methodAttributes = candidates
            .Where(static candidate => candidate is not null)
            .Select(static candidate => candidate!)
            .ToDictionary(
                static candidate => candidate,
                candidate => ResolveAttributes(candidate.MethodSymbol, bindNodeSignalAttribute));

        var methodCandidates = methodAttributes
            .Where(static kvp => kvp.Value.Count > 0)
            .Select(static kvp => kvp.Key)
            .ToList();


```

To fully avoid invoking `ResolveAttributes` twice per method in `Execute`, you should also:

1. Locate the `group.Methods` loop later in `Execute` where `ResolveAttributes` is called again (likely something like `ResolveAttributes(methodCandidate.MethodSymbol, bindNodeSignalAttribute)`).
2. Replace those calls with lookups into the `methodAttributes` dictionary introduced above. For example:
   - If you have `var attributes = ResolveAttributes(methodCandidate.MethodSymbol, bindNodeSignalAttribute);`, change it to `var attributes = methodAttributes[methodCandidate];`.
3. Ensure the `methodAttributes` dictionary is in scope where the `group.Methods` loop is defined. If the loop is in a nested local function or separate helper, pass `methodAttributes` as a parameter to that function instead of recomputing attributes there.

These adjustments will make sure each method's attributes are resolved exactly once and reused across both the filtering and the generation logic.
</issue_to_address>

### Comment 2
<location path="GFramework.Godot.SourceGenerators/BindNodeSignalGenerator.cs" line_range="247-254" />
<code_context>
+        context.ReportDiagnostic(Diagnostic.Create(descriptor, location, messageArgs));
+    }
+
+    private static string ResolveCtorString(
+        AttributeData attribute,
+        int index)
+    {
+        if (attribute.ConstructorArguments.Length <= index)
+            return string.Empty;
+
+        return attribute.ConstructorArguments[index].Value as string ?? string.Empty;
+    }
+
</code_context>
<issue_to_address>
**suggestion:** Missing constructor arguments degrade into empty strings, which can lead to confusing diagnostics.

Because `ResolveCtorString` returns `string.Empty` for missing or non-string ctor args, that empty name flows into `FindField` / `FindEvent` and yields diagnostics like “field '' not found,” which are unclear. Consider detecting the "missing or wrong type" case explicitly and reporting a dedicated diagnostic, or at least short-circuiting when the resolved name is empty so the user gets a clearer message about incorrect attribute usage.

Suggested implementation:

```csharp
    private static bool TryResolveCtorString(
        AttributeData attribute,
        int index,
        out string value)
    {
        value = string.Empty;

        // Missing ctor argument at the requested index.
        if (attribute.ConstructorArguments.Length <= index)
            return false;

        var ctorArgument = attribute.ConstructorArguments[index];

        // We only support primitive string literals here; anything else is considered invalid.
        if (ctorArgument.Kind != TypedConstantKind.Primitive || ctorArgument.Value is not string s)
            return false;

        // Treat empty/whitespace-only names as invalid to avoid confusing diagnostics like "field '' not found".
        if (string.IsNullOrWhiteSpace(s))
            return false;

        value = s;
        return true;
    }

    /// <summary>
    /// Legacy helper kept for backward compatibility. Prefer <see cref="TryResolveCtorString"/>.
    /// This method returns <see cref="string.Empty"/> when the constructor argument is missing or invalid.
    /// </summary>
    private static string ResolveCtorString(
        AttributeData attribute,
        int index)
    {
        return TryResolveCtorString(attribute, index, out var value)
            ? value
            : string.Empty;
    }

```

To fully implement your diagnostic-behavior suggestion, the following additional changes are required elsewhere in this file:

1. **Update all call sites** that currently do something like:
   - `var fieldName = ResolveCtorString(attribute, 0);`
   - `var eventName = ResolveCtorString(attribute, 0);`
   and then pass those values into `FindField`, `FindEvent`, or similar, to instead use:
   ```csharp
   if (!TryResolveCtorString(attribute, 0, out var fieldName))
   {
       // Report a dedicated "invalid attribute usage" diagnostic and return.
       ReportDiagnostic(
           context,
           InvalidBindNodeSignalAttributeDescriptor, // define this descriptor if it doesn't exist yet
           candidate,
           attribute,
           /* any message arguments you need, e.g. candidate.Method.Identifier.Text */
       );
       return;
   }

   var field = FindField(fieldName, ...);
   ```
2. **Introduce a dedicated diagnostic descriptor** (if you don't already have one) for invalid/missing constructor arguments on `BindNodeSignalAttribute`, for example:
   ```csharp
   private static readonly DiagnosticDescriptor InvalidBindNodeSignalAttributeDescriptor =
       new DiagnosticDescriptor(
           "GODOTSG001",
           "Invalid BindNodeSignal attribute usage",
           "The {0} attribute requires a non-empty string literal for its '{1}' constructor argument.",
           "Usage",
           DiagnosticSeverity.Error,
           isEnabledByDefault: true);
   ```
3. Use this new descriptor in the early-return path when `TryResolveCtorString` returns `false`, so that users see a clear diagnostic like:
   - “The BindNodeSignal attribute requires a non-empty string literal for its 'signalName' constructor argument.”
   instead of downstream diagnostics like “field '' not found”.

These changes ensure that missing or incorrectly-typed constructor arguments no longer degrade into empty strings, and that users get a targeted, understandable diagnostic about incorrect attribute usage.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment thread GFramework.Godot.SourceGenerators/BindNodeSignalGenerator.cs Outdated
Comment thread GFramework.Godot.SourceGenerators/BindNodeSignalGenerator.cs Outdated
GeWuYou added 3 commits March 31, 2026 10:26
- 实现 BindNodeSignalGenerator 源代码生成器,用于自动化节点信号绑定
- 添加完整的诊断系统,包含 11 种不同的错误和警告场景检测
- 生成对称的绑定和解绑方法,确保资源正确释放
- 支持一个处理方法通过多个特性绑定到多个节点事件
- 实现生命周期钩子调用检查,确保在 _Ready 和 _ExitTree 中正确调用生成的方法
- 提供详细的单元测试覆盖各种使用场景和边界条件
- 生成器与现有的 GetNode 声明完全兼容并可共存
- 包含命名冲突检测和构造参数验证等安全检查机制
- 实现 BindNodeSignalGenerator 用于生成节点信号绑定与解绑逻辑
- 实现 GetNodeGenerator 用于生成 Godot 节点获取注入逻辑
- 添加 BindNodeSignalDiagnostics 提供详细的诊断错误信息
- 集成到 AnalyzerReleases.Unshipped.md 追踪新的分析规则
- 支持 [BindNodeSignal] 属性的方法自动生成事件绑定代码
- 支持 [GetNode] 属性的字段自动生成节点获取代码
- 提供生命周期方法集成的智能提示和验证功能
- 在 BindNodeSignalDiagnostics.cs 中添加 Microsoft.CodeAnalysis 引用
- 在 BindNodeSignalGenerator.cs 中添加 Roslyn 相关命名空间引用
- 在 GetNodeGenerator.cs 中添加 Roslyn 相关命名空间引用
- 在 GlobalUsings.cs 中集中管理全局命名空间引用
- 在 ContextGetGenerator.cs 中添加字符串和扩展方法引用
- 在 CommonDiagnostics.cs 中添加诊断相关命名空间引用
- 在 Common 全局引用文件中统一管理 Microsoft.CodeAnalysis 引用
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant