Skip to content

Commit 38d5a74

Browse files
author
Danielle Jenkins
committed
Add GD0001 signal connection leak analyzer
1 parent ed85930 commit 38d5a74

26 files changed

+2265
-425
lines changed

‎.github/CODEOWNERS‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
* @megacrit

‎.github/dependabot.yml‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
version: 2
2+
updates:
3+
- package-ecosystem: "nuget"
4+
directory: "/"
5+
schedule:
6+
interval: "monthly"
7+
open-pull-requests-limit: 10
8+
commit-message:
9+
prefix: "deps"
10+
include: "scope"
11+
12+
- package-ecosystem: "github-actions"
13+
directory: "/"
14+
schedule:
15+
interval: "monthly"
16+
open-pull-requests-limit: 5
17+
commit-message:
18+
prefix: "ci"

‎.github/workflows/ci.yml‎

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@ jobs:
4343
run: dotnet build --no-restore --configuration Release
4444

4545
- name: Test
46-
run: dotnet test --no-build --configuration Release --verbosity normal --logger "trx;LogFileName=test-results.trx"
46+
run: dotnet test --no-build --configuration Release --verbosity normal --logger "trx;LogFileName=test-results.trx" --collect:"XPlat Code Coverage" --results-directory ./coverage
4747

4848
- name: Upload test results
4949
uses: actions/upload-artifact@v4
@@ -52,6 +52,13 @@ jobs:
5252
name: test-results-${{ matrix.os }}-${{ matrix.dotnet }}
5353
path: '**/test-results.trx'
5454

55+
- name: Upload coverage reports
56+
if: matrix.os == 'ubuntu-latest' && matrix.dotnet == '8.0.x'
57+
uses: actions/upload-artifact@v4
58+
with:
59+
name: coverage-report
60+
path: ./coverage/**/*.xml
61+
5562
- name: Pack
5663
if: matrix.os == 'ubuntu-latest' && matrix.dotnet == '8.0.x'
5764
run: dotnet pack --no-build --configuration Release --output ./artifacts
@@ -80,6 +87,13 @@ jobs:
8087
with:
8188
dotnet-version: '8.0.x'
8289

90+
- name: Validate NuGet API Key
91+
run: |
92+
if [ -z "${{ secrets.NUGET_API_KEY }}" ]; then
93+
echo "::error::NUGET_API_KEY secret is not configured. Please add it to the repository secrets."
94+
exit 1
95+
fi
96+
8397
- name: Publish to NuGet
8498
run: |
8599
dotnet nuget push "./artifacts/*.nupkg" \

‎GitVersion.yml‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
next-version: 0.1.0
2+
mode: Mainline

‎README.md‎

Lines changed: 88 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -20,21 +20,72 @@ dotnet add package GodotSharpAnalyzers
2020
### Via PackageReference
2121

2222
```xml
23-
<PackageReference Include="GodotSharpAnalyzers" Version="1.0.0" />
23+
<PackageReference Include="GodotSharpAnalyzers" Version="0.1.0">
24+
<PrivateAssets>all</PrivateAssets>
25+
<IncludeAssets>analyzers; build</IncludeAssets>
26+
</PackageReference>
2427
```
2528

2629
## Analyzer Rules
2730

28-
| Rule ID | Category | Description |
29-
|---------|----------|-------------|
30-
| GSA0001 | Usage | Signal connections should use nameof |
31-
| GSA0002 | Usage | Node paths should be validated |
32-
| GSA0003 | Usage | Export attributes on incompatible types |
33-
| GSA0004 | Performance | Prefer cached node references |
34-
| GSA0005 | Memory | Dispose nodes properly |
35-
| GSA0006 | Threading | Godot API calls from background threads |
36-
| GSA0007 | Usage | Use GD.PushError instead of GD.Print for errors |
37-
| GSA0008 | Usage | Resource paths should use res:// or user:// |
31+
- [x] **GD0001** (Memory): Signal connections should be disconnected to prevent memory leaks
32+
- [x] **GD0001a** (Memory): Signal connected to lambda expression (informational)
33+
34+
## Configuration
35+
36+
You can customize the analyzer behavior by setting MSBuild properties in your project file:
37+
38+
### Disable All Analyzers
39+
```xml
40+
<PropertyGroup>
41+
<EnableGodotSharpAnalyzers>false</EnableGodotSharpAnalyzers>
42+
</PropertyGroup>
43+
```
44+
45+
### Disable Specific Rules
46+
```xml
47+
<PropertyGroup>
48+
<GodotSharpAnalyzersDisableSignalConnectionLeak>true</GodotSharpAnalyzersDisableSignalConnectionLeak>
49+
<GodotSharpAnalyzersDisableSignalLambdaConnectionLeak>true</GodotSharpAnalyzersDisableSignalLambdaConnectionLeak>
50+
</PropertyGroup>
51+
```
52+
53+
### Treat Signal Connection Leak Rule as Error
54+
```xml
55+
<PropertyGroup>
56+
<GodotSharpAnalyzersSignalConnectionLeakAsError>true</GodotSharpAnalyzersSignalConnectionLeakAsError>
57+
</PropertyGroup>
58+
```
59+
60+
### Suppress Singleton Signal Warnings
61+
62+
Option 1: Using MSBuild properties (recommended)
63+
```xml
64+
<PropertyGroup>
65+
<GodotSharpAnalyzersSuppressSingletonSignals>true</GodotSharpAnalyzersSuppressSingletonSignals>
66+
</PropertyGroup>
67+
```
68+
69+
Option 2: Using .editorconfig
70+
```xml
71+
[*.cs]
72+
godot_analyzer.suppress_singleton_signals = true
73+
```
74+
75+
### Show Information Messages
76+
```xml
77+
<PropertyGroup>
78+
<GodotSharpAnalyzersShowInfo>true</GodotSharpAnalyzersShowInfo>
79+
</PropertyGroup>
80+
```
81+
82+
## Known Limitations
83+
84+
**GD0001 - Signal Connection Leak Detection:**
85+
- Only analyzes within single class scope
86+
- May produce false positives for temporary objects or valid persistent connections
87+
- Doesn't track cross-method signal flows or complex object lifetimes
88+
- Conditional connections (e.g., in loops or if statements) may not be fully analyzed
3889

3990
## Building from Source
4091

@@ -63,11 +114,36 @@ dotnet test
63114
4. Push to the branch (`git push origin feature/amazing-analyzer`)
64115
5. Open a Pull Request
65116

117+
### Setting up CI/CD for Releases
118+
119+
To enable automatic publishing of NuGet packages when tags are created:
120+
121+
1. **Generate a NuGet API Key:**
122+
- Go to [nuget.org](https://www.nuget.org/) and sign in
123+
- Navigate to your account settings → API Keys
124+
- Create a new API key with "Push new packages and package versions" permissions
125+
- Optionally scope it to specific packages or use global permissions
126+
127+
2. **Add the API Key to GitHub Secrets:**
128+
- Go to your repository on GitHub
129+
- Navigate to Settings → Secrets and variables → Actions
130+
- Click "New repository secret"
131+
- Name: `NUGET_API_KEY`
132+
- Value: Paste your NuGet API key
133+
- Click "Add secret"
134+
135+
3. **Create a Release:**
136+
- Tag your release with a version number: `git tag v0.1.0`
137+
- Push the tag: `git push origin v0.1.0`
138+
- The GitHub Actions workflow will automatically build, test, and publish to NuGet
139+
140+
**Note:** The `GITHUB_TOKEN` secret is automatically provided by GitHub Actions and doesn't need manual setup.
141+
66142
## License
67143

68144
This project is licensed under the MIT License - see the [LICENSE](LICENSE) file for details.
69145

70146
## Acknowledgments
71147

72148
- Built for the Godot Engine community
73-
- Inspired by common issues found in Godot C# projects
149+
- Inspired by common issues found in Godot C# projects

‎docs/rules/GD0001.md‎

Lines changed: 137 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,137 @@
1+
# GD0001: Signal connection memory leak
2+
3+
## Rule Description
4+
5+
In Godot 4, signals connected with `+=` syntax must be manually disconnected with `-=` to prevent memory leaks. Unlike Godot 3, signals in Godot 4 do not automatically disconnect when nodes are freed.
6+
7+
**Rule ID**: GD0001
8+
**Category**: Memory
9+
**Severity**: Warning
10+
11+
## Problem
12+
13+
When you connect a signal using `+=` but never disconnect it with `-=`, the connected object will not be garbage collected even after the node is freed, leading to memory leaks.
14+
15+
```csharp
16+
public partial class Player : Node
17+
{
18+
[Signal]
19+
public delegate void HealthChangedEventHandler(int newHealth);
20+
21+
public override void _Ready()
22+
{
23+
// This creates a memory leak - no disconnection
24+
HealthChanged += OnHealthChanged; // GD0001
25+
}
26+
27+
private void OnHealthChanged(int newHealth)
28+
{
29+
GD.Print($"Health: {newHealth}");
30+
}
31+
}
32+
```
33+
34+
## Solution
35+
36+
Always disconnect signals in the `_ExitTree()` method:
37+
38+
```csharp
39+
public partial class Player : Node
40+
{
41+
[Signal]
42+
public delegate void HealthChangedEventHandler(int newHealth);
43+
44+
public override void _Ready()
45+
{
46+
HealthChanged += OnHealthChanged;
47+
}
48+
49+
public override void _ExitTree()
50+
{
51+
// Properly disconnect to prevent memory leaks
52+
HealthChanged -= OnHealthChanged;
53+
}
54+
55+
private void OnHealthChanged(int newHealth)
56+
{
57+
GD.Print($"Health: {newHealth}");
58+
}
59+
}
60+
```
61+
62+
## Special Cases
63+
64+
### Lambda Expressions (GD0001a)
65+
66+
Lambda expressions cannot be easily disconnected and will generate an informational diagnostic:
67+
68+
```csharp
69+
public override void _Ready()
70+
{
71+
// GD0001a: Lambda expressions cannot be easily disconnected
72+
HealthChanged += (health) => GD.Print($"Health: {health}");
73+
}
74+
```
75+
76+
**Recommendation**: Use named methods instead of lambda expressions for signals that need to be disconnected.
77+
78+
### Static Methods
79+
80+
Static method connections typically don't cause memory leaks and are ignored by this rule:
81+
82+
```csharp
83+
public override void _Ready()
84+
{
85+
// No warning - static methods don't typically cause leaks
86+
HealthChanged += HealthLogger.LogHealth;
87+
}
88+
```
89+
90+
### Singleton Classes
91+
92+
For singleton classes (classes with a static Instance property or AutoLoad attribute), you can suppress warnings using either `.editorconfig` or MSBuild properties:
93+
94+
#### Using .editorconfig
95+
```ini
96+
[*.cs]
97+
godot_analyzer.suppress_singleton_signals = true
98+
```
99+
100+
#### Using MSBuild Property
101+
```xml
102+
<PropertyGroup>
103+
<GodotSharpAnalyzersSuppressSingletonSignals>true</GodotSharpAnalyzersSuppressSingletonSignals>
104+
</PropertyGroup>
105+
```
106+
107+
**Note**: `.editorconfig` settings take precedence over MSBuild properties when both are specified.
108+
109+
## Configuration
110+
111+
You can configure this rule's behavior in your project file:
112+
113+
### Disable the rule entirely
114+
```xml
115+
<PropertyGroup>
116+
<GodotSharpAnalyzersDisableSignalConnectionLeak>true</GodotSharpAnalyzersDisableSignalConnectionLeak>
117+
</PropertyGroup>
118+
```
119+
120+
### Treat as error instead of warning
121+
```xml
122+
<PropertyGroup>
123+
<GodotSharpAnalyzersSignalConnectionLeakAsError>true</GodotSharpAnalyzersSignalConnectionLeakAsError>
124+
</PropertyGroup>
125+
```
126+
127+
### Disable lambda expression warnings
128+
```xml
129+
<PropertyGroup>
130+
<GodotSharpAnalyzersDisableSignalLambdaConnectionLeak>true</GodotSharpAnalyzersDisableSignalLambdaConnectionLeak>
131+
</PropertyGroup>
132+
```
133+
134+
## References
135+
136+
- [Godot 4 C# Signal Documentation](https://docs.godotengine.org/en/stable/tutorials/scripting/c_sharp/c_sharp_signals.html)
137+
- [GitHub Issue #89116](https://github.com/godotengine/godot/issues/89116) - Signal memory leak discussion

‎global.json‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
{
2+
"sdk": {
3+
"version": "8.0.100",
4+
"rollForward": "latestMajor"
5+
}
6+
}

‎src/GodotSharpAnalyzers/AnalyzerReleases.Unshipped.md‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,5 @@
55

66
Rule ID | Category | Severity | Notes
77
--------|----------|----------|--------------------
8-
GSA0001 | Usage | Warning | UseNameofForSignals, [SignalNameofAnalyzer](Analyzers/SignalNameofAnalyzer.cs)
9-
GSA0002 | Usage | Warning | ValidateNodePaths
10-
GSA0003 | Usage | Error | ExportOnIncompatibleType
8+
GD0001 | Memory | Warning | SignalConnectionLeak, [SignalConnectionLeakAnalyzer](Analyzers/Memory/SignalConnectionLeakAnalyzer.cs)
9+
GD0001a | Memory | Info | SignalLambdaConnectionLeak, [SignalConnectionLeakAnalyzer](Analyzers/Memory/SignalConnectionLeakAnalyzer.cs)

0 commit comments

Comments
 (0)