Skip to content

Commit 07139c0

Browse files
antonsyndantonsynd
committed
emitter: Fix some shadowing
1 parent 81d2f20 commit 07139c0

2 files changed

Lines changed: 304 additions & 31 deletions

File tree

‎docs/implementation_planning/task_list_0_1_10_codegen_fixes.md‎

Lines changed: 251 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,251 @@
1+
# Task List: Phase 0.1.10 Code Generation Fixes
2+
3+
**Goal:** Fix the remaining 27 test failures in the Sharpy compiler related to code generation for modules, imports, and variable redefinitions.
4+
5+
---
6+
7+
## Phase 0.1.10: Code Generation Fixes
8+
9+
### Overview
10+
11+
After fixing the local variable shadowing issues, there remain three categories of test failures:
12+
13+
| Category | Test Count | Root Cause |
14+
|----------|------------|------------|
15+
| Module Import Alias Generation | 3 | Using wrong class name for Sharpy modules (uses last segment instead of `Exports`) |
16+
| Variable Redefinition at Module Level | 6 | Generates duplicate field declarations instead of versioned names |
17+
| Nested Package Namespace Generation | 18 | Namespace path duplication for `__init__.spy` files in packages |
18+
19+
---
20+
21+
### Task 0.1.10.CG1: Fix Module Import Alias to Use `Exports` Class
22+
23+
**Files:**
24+
- `src/Sharpy.Compiler/CodeGen/RoslynEmitter.cs` (lines 261-347 `GenerateImportUsings`)
25+
26+
**Problem:**
27+
When importing a Sharpy module (not a .NET namespace), the generated using alias points to the wrong class:
28+
- Current: `using utils_helpers = Utils.Helpers.Helpers;`
29+
- Expected: `using utils_helpers = Utils.Helpers.Exports;`
30+
31+
The current code extracts the last segment of the namespace (`Helpers`) as the class name, but Sharpy modules expose their members via a class named `Exports`.
32+
33+
**Fix Approach:**
34+
1. In `GenerateImportUsings()`, when generating the alias for non-.NET Sharpy modules:
35+
- Change `moduleClassName` from the last namespace segment to the literal `"Exports"`
36+
- This applies to both `import module` (line 320-340) and `import module as alias` (line 284-304)
37+
38+
2. Similarly in `GenerateFromImportUsings()` (line 350-398):
39+
- The `using static` should point to `Exports` class, not the module name
40+
41+
**Test Commands:**
42+
```bash
43+
dotnet test --filter "ConvertModuleNameToNamespace_SnakeCase|GenerateCompilationUnit_WithImportModule"
44+
```
45+
46+
**Expected Generated Code:**
47+
```csharp
48+
// For: import utils.helpers
49+
using utils_helpers = Utils.Helpers.Exports;
50+
51+
// For: import utils.helpers as h
52+
using h = Utils.Helpers.Exports;
53+
54+
// For: from config import MAX_SIZE
55+
using static TestProject.Config.Exports;
56+
```
57+
58+
---
59+
60+
### Task 0.1.10.CG2: Track Module-Level Variable Redefinitions
61+
62+
**Files:**
63+
- `src/Sharpy.Compiler/CodeGen/RoslynEmitter.cs` (lines 495-622 `GenerateModuleClass`, lines 2164-2259 `GenerateModuleLevelField`)
64+
65+
**Problem:**
66+
Sharpy allows variable redefinition at module level that changes the type:
67+
```python
68+
x: int = 1
69+
x: auto = "hello" # Redefines x as string
70+
```
71+
72+
Currently generates invalid C# with duplicate field declarations:
73+
```csharp
74+
public static int X = 1;
75+
public static string X = "hello"; // CS0102: Duplicate definition
76+
```
77+
78+
**Fix Approach:**
79+
1. Add `_moduleVariableVersions` dictionary (similar to `_variableVersions` for locals):
80+
```csharp
81+
private readonly Dictionary<string, int> _moduleVariableVersions = new();
82+
```
83+
84+
2. In `GenerateModuleLevelField()`:
85+
- Check if the variable name already exists in `_moduleVariableVersions`
86+
- If so, generate a versioned name: `X_1`, `X_2`, etc.
87+
- Track the current version so later code references resolve correctly
88+
89+
3. In `GenerateModuleClass()`:
90+
- Pre-scan all variable declarations to build the version map BEFORE generating fields
91+
- This ensures the Main() method can reference the correct versioned variable
92+
93+
4. Update `GetMangledVariableName()`:
94+
- When resolving module-level variable references, consult `_moduleVariableVersions`
95+
- Return the highest version for the current reference point
96+
97+
**Expected Generated Code:**
98+
```csharp
99+
public static int X = 1;
100+
public static string X_1 = "hello";
101+
102+
public static void Main()
103+
{
104+
// References before redefinition use X
105+
// References after redefinition use X_1
106+
}
107+
```
108+
109+
**Test Commands:**
110+
```bash
111+
dotnet test --filter "VariableRedefinition|Variable_FirstAssignment|Assignment_Reference"
112+
```
113+
114+
**Complexity Notes:**
115+
- This requires tracking statement order to determine which version to reference
116+
- Consider adding a "current statement index" context to resolve references correctly
117+
- The Main() method generation happens after field generation, so version tracking must persist
118+
119+
---
120+
121+
### Task 0.1.10.CG3: Fix Nested Package Namespace Generation
122+
123+
**Files:**
124+
- `src/Sharpy.Compiler/CodeGen/RoslynEmitter.cs` (lines 193-220 `GenerateProjectNamespace`)
125+
- `src/Sharpy.Compiler/ProjectCompiler.cs` (namespace generation logic)
126+
127+
**Problem:**
128+
For deeply nested packages like `level1/level2/level3/__init__.spy`, the namespace is generated incorrectly:
129+
- Current: `TestProject.Level1.Level2.Level3.Level3` (duplicated segment)
130+
- Expected: `TestProject.Level1.Level2.Level3.Init`
131+
132+
The import alias also has issues:
133+
- Current: `using level1_level2_level3 = TestProject.Level1.Level2.Level3.Level3;`
134+
- Expected: `using level1_level2_level3 = TestProject.Level1.Level2.Level3.Exports;`
135+
136+
**Root Cause Analysis:**
137+
1. `GenerateProjectNamespace()` adds directory parts AND the filename
138+
2. For `__init__.spy` files, the filename becomes `Init`
139+
3. The import generates using the last directory segment as the class name
140+
141+
**Fix Approach:**
142+
1. In `GenerateProjectNamespace()`:
143+
- When the file is `__init__.spy`, use `Init` as the class name (already correct)
144+
- Verify the directory path doesn't include the file's containing directory twice
145+
146+
2. In `GenerateImportUsings()`:
147+
- For package imports (paths ending in `__init__.spy`), the class name should be `Exports` (same as Task CG1)
148+
- The namespace path should NOT duplicate the last segment
149+
150+
3. Debug by logging the intermediate values:
151+
- `relativePath`, `relativeDir`, `fileName`
152+
- Compare expected vs actual for multi-level paths
153+
154+
**Example Structure:**
155+
```
156+
src/
157+
main.spy → TestProject.Main
158+
level1/
159+
__init__.spy → TestProject.Level1.Init
160+
level2/
161+
__init__.spy → TestProject.Level1.Level2.Init
162+
level3/
163+
__init__.spy → TestProject.Level1.Level2.Level3.Init
164+
```
165+
166+
**Test Commands:**
167+
```bash
168+
dotnet test --filter "EdgeCase_DeepNesting|PackageInit_NestedPackages|ComplexScenario_Nested"
169+
```
170+
171+
---
172+
173+
### Task 0.1.10.CG4: Fix `__init__.spy` Class Name for Packages
174+
175+
**Files:**
176+
- `src/Sharpy.Compiler/CodeGen/RoslynEmitter.cs` (lines 625-655 `GetModuleClassName`)
177+
178+
**Problem:**
179+
Package `__init__.spy` files generate a class named `Init`, but the import system expects `Exports`:
180+
- Generated: `namespace TestProject.Mypackage { public static class Init { ... } }`
181+
- Import expects: `using mypackage = TestProject.Mypackage.Exports;`
182+
183+
**Fix Approach:**
184+
1. In `GetModuleClassName()`:
185+
- If the filename is `__init__`, return `"Exports"` instead of `"Init"`
186+
- This makes package exports consistent with the import alias generation
187+
188+
2. Alternative: Change import generation to use `Init` for packages
189+
- Less preferred because `Exports` is a clearer semantic name
190+
191+
**Test Commands:**
192+
```bash
193+
dotnet test --filter "PackageInit"
194+
```
195+
196+
---
197+
198+
### Task 0.1.10.CG5: Handle Re-export Syntax in `__init__.spy`
199+
200+
**Files:**
201+
- `src/Sharpy.Compiler/Parser/Parser.cs` (import/export parsing)
202+
- `src/Sharpy.Compiler/CodeGen/RoslynEmitter.cs` (re-export code generation)
203+
204+
**Problem:**
205+
The test `PackageInit_WithReExports_ExportsModuleMembers` fails with parser errors:
206+
```
207+
Parser error at line 3, column 6: Expected identifier, got Dot
208+
```
209+
210+
This suggests the re-export syntax in `__init__.spy` is not being parsed correctly. The syntax likely involves:
211+
```python
212+
from .basic import BasicOperation
213+
from .advanced import AdvancedOperation
214+
```
215+
216+
**Fix Approach:**
217+
1. Check if relative imports (starting with `.`) are supported in the parser
218+
2. Ensure the lexer/parser handles dotted imports correctly
219+
3. Generate appropriate `using static` or re-export statements
220+
221+
**Test Commands:**
222+
```bash
223+
dotnet test --filter "PackageInit_WithReExports|ComplexScenario_PackageWithMultipleModules"
224+
```
225+
226+
---
227+
228+
## Summary
229+
230+
| Task ID | Title | Estimated Complexity |
231+
|---------|-------|---------------------|
232+
| 0.1.10.CG1 | Fix Module Import Alias to Use `Exports` Class | Low |
233+
| 0.1.10.CG2 | Track Module-Level Variable Redefinitions | High |
234+
| 0.1.10.CG3 | Fix Nested Package Namespace Generation | Medium |
235+
| 0.1.10.CG4 | Fix `__init__.spy` Class Name for Packages | Low |
236+
| 0.1.10.CG5 | Handle Re-export Syntax in `__init__.spy` | Medium |
237+
238+
**Recommended Order:**
239+
1. CG1 (quick win, unblocks many tests)
240+
2. CG4 (quick win, related to CG1)
241+
3. CG3 (after CG1/CG4 are fixed, easier to debug)
242+
4. CG5 (depends on CG3 for package namespace fixes)
243+
5. CG2 (most complex, can be done in parallel)
244+
245+
**Full Test Command:**
246+
```bash
247+
dotnet test
248+
```
249+
250+
Current status: 27 failing, 2994 passing, 82 skipped
251+
Target: 0 failing (excluding legitimately skipped tests)

‎src/Sharpy.Compiler/CodeGen/RoslynEmitter.cs‎

Lines changed: 53 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -49,9 +49,37 @@ public RoslynEmitter(CodeGenContext context)
4949
/// <returns>The C# variable name with version suffix (e.g., "x", "x_1", "x_2")</returns>
5050
private string GetMangledVariableName(string name, bool isNewDeclaration)
5151
{
52-
// Check if this is a reference to a const variable - use constant case
53-
// Check both local and module-level consts
54-
if (_constVariables.Contains(name) || _moduleConstVariables.Contains(name))
52+
var baseName = NameMangler.ToCamelCase(name);
53+
54+
// FIRST: Check if this is a local variable that shadows a module-level one
55+
// Local variables take precedence over module-level variables
56+
if (_variableVersions.ContainsKey(baseName))
57+
{
58+
// There's a local variable with this name - use local resolution
59+
if (isNewDeclaration)
60+
{
61+
// This is a redefinition of an existing local variable
62+
var currentVersion = _variableVersions[baseName];
63+
var newVersion = currentVersion + 1;
64+
_variableVersions[baseName] = newVersion;
65+
return $"{baseName}_{newVersion}";
66+
}
67+
else
68+
{
69+
// This is a reference to the local variable
70+
var currentVersion = _variableVersions[baseName];
71+
return currentVersion == 0 ? baseName : $"{baseName}_{currentVersion}";
72+
}
73+
}
74+
75+
// Check if this is a reference to a local const variable - use constant case
76+
if (_constVariables.Contains(name))
77+
{
78+
return NameMangler.ToConstantCase(name);
79+
}
80+
81+
// Check if this is a reference to a module-level const - use constant case
82+
if (_moduleConstVariables.Contains(name))
5583
{
5684
return NameMangler.ToConstantCase(name);
5785
}
@@ -77,37 +105,18 @@ private string GetMangledVariableName(string name, bool isNewDeclaration)
77105
return name.Replace(".", "_");
78106
}
79107

80-
var baseName = NameMangler.ToCamelCase(name);
81-
108+
// If we reach here, this is a new local variable that doesn't shadow any module-level var
82109
if (isNewDeclaration)
83110
{
84-
// This is a new declaration or redefinition
85-
if (_variableVersions.TryGetValue(baseName, out var currentVersion))
86-
{
87-
// Variable already exists - increment version for redefinition
88-
var newVersion = currentVersion + 1;
89-
_variableVersions[baseName] = newVersion;
90-
return $"{baseName}_{newVersion}";
91-
}
92-
else
93-
{
94-
// First declaration of this variable
95-
_variableVersions[baseName] = 0;
96-
return baseName;
97-
}
111+
// First declaration of this local variable
112+
_variableVersions[baseName] = 0;
113+
return baseName;
98114
}
99115
else
100116
{
101-
// This is a reference - use the current version
102-
if (_variableVersions.TryGetValue(baseName, out var currentVersion))
103-
{
104-
return currentVersion == 0 ? baseName : $"{baseName}_{currentVersion}";
105-
}
106-
else
107-
{
108-
// Variable not yet declared (shouldn't happen in valid code)
109-
return baseName;
110-
}
117+
// Reference to a variable not yet declared (shouldn't happen in valid code)
118+
// Fall back to just returning the base name
119+
return baseName;
111120
}
112121
}
113122

@@ -597,8 +606,14 @@ private ClassDeclarationSyntax GenerateModuleClass(List<Statement> statements)
597606
// Note: Main is generated for ALL entry point files (even empty ones), per line 557
598607
bool willHaveMainMethod = hasMainFunction || (!hasMainFunction && _context.IsEntryPoint);
599608

609+
// Collect all function names to check for class name collisions
610+
var functionNames = statements
611+
.OfType<FunctionDef>()
612+
.Select(f => NameMangler.Transform(f.Name, NameContext.Method))
613+
.ToHashSet();
614+
600615
// Generate module class name from source file name
601-
var moduleClassName = GetModuleClassName(willHaveMainMethod);
616+
var moduleClassName = GetModuleClassName(willHaveMainMethod, functionNames);
602617

603618
return ClassDeclaration(moduleClassName)
604619
.WithModifiers(TokenList(
@@ -607,7 +622,7 @@ private ClassDeclarationSyntax GenerateModuleClass(List<Statement> statements)
607622
.WithMembers(List(declarations));
608623
}
609624

610-
private string GetModuleClassName(bool willGenerateMainMethod = false)
625+
private string GetModuleClassName(bool willGenerateMainMethod = false, HashSet<string>? functionNames = null)
611626
{
612627
// Get the file name without extension and convert to PascalCase
613628
if (!string.IsNullOrEmpty(_context.SourceFilePath))
@@ -624,6 +639,13 @@ private string GetModuleClassName(bool willGenerateMainMethod = false)
624639
return "Program";
625640
}
626641

642+
// Avoid name collision: if the class name matches any function name in the module,
643+
// append "Module" suffix to the class name (C# doesn't allow method name == enclosing type name)
644+
if (functionNames != null && functionNames.Contains(className))
645+
{
646+
return $"{className}Module";
647+
}
648+
627649
return className;
628650
}
629651
}

0 commit comments

Comments
 (0)