-
-
Notifications
You must be signed in to change notification settings - Fork 385
feat(csharp): unify partial-class parts for member and base resolution #732
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
9410e35
feat(csharp): unify partial-class parts for member and base resolution
vitali87 5f3ee0b
fix(csharp): resolve fields declared on another partial-class part
vitali87 aee1d76
fix(csharp): scope partial-class grouping to the declaring directory …
vitali87 0754c79
Merge branch 'main' into feat/csharp-partial-classes
vitali87 09e9b4f
Merge remote-tracking branch 'origin/main' into feat/csharp-partial-c…
vitali87 10cfe35
Merge remote-tracking branch 'origin/feat/csharp-partial-classes' int…
vitali87 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,144 @@ | ||
| # (H) C# Phase 3 tail: partial-class member unification. A class split across | ||
| # (H) files with `partial` is one logical type; a typed receiver must resolve to | ||
| # (H) members and bases declared on ANY part, not just the receiver's own part. | ||
| # (H) (Unique-name calls already resolve via the generic fallback; these tests | ||
| # (H) use decoys so only cross-part typed resolution can bind them.) | ||
| from pathlib import Path | ||
| from unittest.mock import MagicMock | ||
|
|
||
| import pytest | ||
|
|
||
| from codebase_rag.tests.conftest import get_relationships, run_updater | ||
|
|
||
| SKIP = "c_sharp" | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def csharp_project(temp_repo: Path) -> Path: | ||
| project = temp_repo / "csharp_partial" | ||
| project.mkdir() | ||
| return project | ||
|
|
||
|
|
||
| def _call_targets(mock_ingestor: MagicMock) -> set[str]: | ||
| return {c.args[2][2] for c in get_relationships(mock_ingestor, "CALLS")} | ||
|
|
||
|
|
||
| def test_typed_receiver_binds_member_on_other_part( | ||
| csharp_project: Path, mock_ingestor: MagicMock | ||
| ) -> None: | ||
| (csharp_project / "A.cs").write_text( | ||
| """ | ||
| namespace N; | ||
| public partial class Widget { public void Alpha() { } } | ||
| public class Other { public void Beta() { } } | ||
| """, | ||
| encoding="utf-8", | ||
| ) | ||
| (csharp_project / "B.cs").write_text( | ||
| "namespace N;\npublic partial class Widget { public void Beta() { } }\n", | ||
| encoding="utf-8", | ||
| ) | ||
| (csharp_project / "App.cs").write_text( | ||
| "namespace N;\npublic class App { public void Run() { var w = new Widget(); w.Beta(); } }\n", | ||
| encoding="utf-8", | ||
| ) | ||
| run_updater(csharp_project, mock_ingestor, skip_if_missing=SKIP) | ||
|
|
||
| targets = _call_targets(mock_ingestor) | ||
| # (H) `Beta` exists on both Widget (part B) and Other, so the generic | ||
| # (H) name-only resolver is ambiguous and drops it. Only typing `w` to the | ||
| # (H) Widget partial group and finding Beta on part B binds it correctly -- | ||
| # (H) and it must be Widget's Beta, never Other's. | ||
| assert any(t.endswith(".Widget.Beta") for t in targets), targets | ||
| assert not any(t.endswith(".Other.Beta") for t in targets), targets | ||
|
|
||
|
|
||
| def test_typed_receiver_binds_inherited_via_other_part( | ||
| csharp_project: Path, mock_ingestor: MagicMock | ||
| ) -> None: | ||
| (csharp_project / "Base.cs").write_text( | ||
| """ | ||
| namespace N; | ||
| public class Base { public void Ping() { } } | ||
| public class Decoy { public void Ping() { } } | ||
| """, | ||
| encoding="utf-8", | ||
| ) | ||
| (csharp_project / "P1.cs").write_text( | ||
| "namespace N;\npublic partial class Widget : Base { }\n", | ||
| encoding="utf-8", | ||
| ) | ||
| (csharp_project / "P2.cs").write_text( | ||
| "namespace N;\npublic partial class Widget { public void Own() { } }\n", | ||
| encoding="utf-8", | ||
| ) | ||
| (csharp_project / "App.cs").write_text( | ||
| "namespace N;\npublic class App { public void Run() { var w = new Widget(); w.Ping(); } }\n", | ||
| encoding="utf-8", | ||
| ) | ||
| run_updater(csharp_project, mock_ingestor, skip_if_missing=SKIP) | ||
|
|
||
| targets = _call_targets(mock_ingestor) | ||
| # (H) `Ping` is inherited through the base declared on part 1, but the | ||
| # (H) receiver may resolve to part 2; spanning the partial group reaches the | ||
| # (H) base. The Decoy.Ping makes the generic fallback ambiguous. | ||
| assert any(t.endswith(".Base.Ping") for t in targets), targets | ||
| assert not any(t.endswith(".Decoy.Ping") for t in targets), targets | ||
|
|
||
|
|
||
| def test_field_typed_receiver_sees_field_on_other_part( | ||
| csharp_project: Path, mock_ingestor: MagicMock | ||
| ) -> None: | ||
| (csharp_project / "F1.cs").write_text( | ||
| """ | ||
| namespace N; | ||
| public class Helper { public void Run() { } } | ||
| public class Decoy { public void Run() { } } | ||
| public partial class Widget { private Helper helper; } | ||
| """, | ||
| encoding="utf-8", | ||
| ) | ||
| (csharp_project / "F2.cs").write_text( | ||
| "namespace N;\npublic partial class Widget { public void Use() { helper.Run(); } }\n", | ||
| encoding="utf-8", | ||
| ) | ||
| run_updater(csharp_project, mock_ingestor, skip_if_missing=SKIP) | ||
|
|
||
| targets = _call_targets(mock_ingestor) | ||
| # (H) `helper` is a field declared on the OTHER part; typing it requires the | ||
| # (H) field lookup to span the partial group. The Decoy.Run makes the generic | ||
| # (H) fallback ambiguous, so only the field-typed receiver can bind Helper.Run. | ||
| assert any(t.endswith(".Helper.Run") for t in targets), targets | ||
| assert not any(t.endswith(".Decoy.Run") for t in targets), targets | ||
|
|
||
|
|
||
| def test_same_name_partial_in_different_projects_not_merged( | ||
| csharp_project: Path, mock_ingestor: MagicMock | ||
| ) -> None: | ||
| # (H) Two independent projects (directories) both declare `partial class | ||
| # (H) Widget` in namespace N. They are DIFFERENT types; grouping them would | ||
| # (H) resolve a call in one project to the other's member across the assembly | ||
| # (H) boundary. The group key is directory-scoped so they stay separate. | ||
| (csharp_project / "proj1").mkdir() | ||
| (csharp_project / "proj2").mkdir() | ||
| (csharp_project / "proj1" / "A.cs").write_text( | ||
| """ | ||
| namespace N; | ||
| public partial class Widget { public void Alpha() { } } | ||
| public class Decoy { public void Beta() { } } | ||
| public class App { public void Run() { var w = new Widget(); w.Beta(); } } | ||
| """, | ||
| encoding="utf-8", | ||
| ) | ||
| (csharp_project / "proj2" / "B.cs").write_text( | ||
| "namespace N;\npublic partial class Widget { public void Beta() { } }\n", | ||
| encoding="utf-8", | ||
| ) | ||
| run_updater(csharp_project, mock_ingestor, skip_if_missing=SKIP) | ||
|
|
||
| targets = _call_targets(mock_ingestor) | ||
| # (H) proj1's Widget has no Beta, so any `Widget.Beta` edge would be proj2's, | ||
| # (H) reached across the project boundary. (Decoy.Beta blocks the generic | ||
| # (H) fallback, so only the partial-group path could produce it.) | ||
| assert not any(t.endswith(".Widget.Beta") for t in targets), targets |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If a C# class is defined in the global namespace (i.e., outside of any namespace declaration),
module_qnwill be empty (""). In this case,len(module_qn) + 1evaluates to1, causingclass_qn[1:]to slice off the first character of the class name. This can lead to incorrect grouping of unrelated classes (for example,WidgetandFidgetwould both resolve to the key"idget"and be grouped together as parts of the same partial class). Checking ifmodule_qnis non-empty before slicing prevents this issue.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in the latest push. The partial-group key is now the declaring DIRECTORY (module_qn minus the file stem) plus the namespace-qualified name, so two independent projects that both declare
N.Widgetlive in different directories and are not merged across the assembly boundary. Parts in different directories of one project fall back to generic resolution (safe under-merge) rather than risk a cross-project wrong edge. Addedtest_same_name_partial_in_different_projects_not_merged(two project dirs + a decoy so the generic fallback can't mask it; verified RED before, GREEN after).