feat(analyzers): add AL0018 to warn when Version.props not imported - #27
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughA new Roslyn analyzer (AL0018) is introduced to enforce importing Version.props in Directory.Build.props files. It scans XML files for the required import element, reports diagnostics when missing, and includes localized resource strings and comprehensive test coverage. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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. Comment |
Summary of ChangesHello @ANcpLua, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new C# diagnostic analyzer, AL0018, aimed at promoting best practices for centralized version management in .NET projects. The analyzer identifies 'Directory.Build.props' files that do not explicitly import 'Version.props', a common pattern for defining package versions across a solution. By flagging these omissions, it helps ensure consistency and simplifies version updates, aligning with existing version management patterns. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new analyzer, AL0018, to ensure Version.props is imported in Directory.Build.props for centralized version management. The implementation is solid, and the accompanying tests cover the main scenarios. I've provided feedback to enhance the analyzer's correctness, improve maintainability, and address minor performance considerations. My suggestions include refining the import detection logic to prevent false negatives, adding a test case for this refinement, and improving code clarity and organization.
| .Any(static import => | ||
| import.Attribute("Project") is { Value: { } projectValue } && | ||
| projectValue.Contains(VersionPropsFileName, StringComparison.OrdinalIgnoreCase)); |
There was a problem hiding this comment.
The current logic using string.Contains is too broad and can lead to false negatives. For example, it would incorrectly match an import for a file named MyVersion.props. To make the check more precise, you should verify that the filename part of the Project attribute's value is exactly Version.props.
You can achieve this using System.IO.Path.GetFileName(). This change will also require making the lambda non-static to access VersionPropsFileName.
.Any(import =>
import.Attribute("Project") is { Value: { } projectValue } &&
System.IO.Path.GetFileName(projectValue).Equals(VersionPropsFileName, StringComparison.OrdinalIgnoreCase));| .Where(static f => f.Path.EndsWith(DirectoryBuildPropsFileName, StringComparison.OrdinalIgnoreCase)) | ||
| .ToList(); |
There was a problem hiding this comment.
The .ToList() call materializes the collection into a list, which causes an unnecessary memory allocation. The foreach loop can iterate directly over the IEnumerable<T> returned by Where. Removing this call will make the code slightly more efficient.
.Where(static f => f.Path.EndsWith(DirectoryBuildPropsFileName, StringComparison.OrdinalIgnoreCase));| var location = Location.Create(propsFile.Path, sourceText.Lines[0].Span, | ||
| new Microsoft.CodeAnalysis.Text.LinePositionSpan( | ||
| new Microsoft.CodeAnalysis.Text.LinePosition(0, 0), | ||
| new Microsoft.CodeAnalysis.Text.LinePosition(0, 0))); |
There was a problem hiding this comment.
The creation of the Location object can be simplified. The current implementation uses the TextSpan of the entire first line (sourceText.Lines[0].Span) but a zero-length LinePositionSpan. This is slightly inconsistent.
For clarity, it's better to use a zero-length TextSpan at the start of the file to match the zero-length LinePositionSpan.
var location = Location.Create(propsFile.Path,
new Microsoft.CodeAnalysis.Text.TextSpan(0, 0),
new Microsoft.CodeAnalysis.Text.LinePositionSpan(
new Microsoft.CodeAnalysis.Text.LinePosition(0, 0),
new Microsoft.CodeAnalysis.Text.LinePosition(0, 0)));| var diagnostic = Diagnostic.Create(Rule, location); | ||
| context.ReportDiagnostic(diagnostic); | ||
| } | ||
| } catch (Exception) { |
There was a problem hiding this comment.
Catching the generic System.Exception can hide other unexpected issues. It's better to catch the more specific System.Xml.XmlException that XDocument.Parse throws on failure. This ensures that you are only handling expected parsing errors and not masking other potential bugs.
} catch (System.Xml.XmlException) {| <!-- AL0018: Version.props not imported --> | ||
| <data name="AL0018AnalyzerTitle" xml:space="preserve"> | ||
| <value>Version.props not imported</value> | ||
| </data> | ||
| <data name="AL0018AnalyzerMessageFormat" xml:space="preserve"> | ||
| <value>Directory.Build.props should import Version.props for centralized version management</value> | ||
| </data> | ||
| <data name="AL0018AnalyzerDescription" xml:space="preserve"> | ||
| <value>Version.props should be imported in Directory.Build.props to enable centralized package version management using $(VariableName) syntax.</value> | ||
| </data> |
There was a problem hiding this comment.
For better maintainability and consistency, the resource entries for AL0018 should be grouped with other version management-related analyzers. Please move this block to be after the entries for AL0017 (around line 206). After reordering and saving, the Resources.Designer.cs file will be regenerated with the correct order as well.
| // No diagnostics expected - only Directory.Build.props is checked | ||
| await test.RunAsync(TestContext.Current.CancellationToken); | ||
| } | ||
| } |
There was a problem hiding this comment.
To ensure the analyzer's logic is robust against false negatives, it would be beneficial to add a test case that checks for imports of files with names similar to Version.props (e.g., MyVersion.props). This test should verify that a diagnostic is correctly reported in such a scenario.
[Fact]
public async Task ShouldReportWhenSimilarButIncorrectPropsImported() {
var directoryBuildProps = """
<Project>
<Import Project="MyVersion.props" />
</Project>
"""
var test = new CSharpAnalyzerTest<Al0018VersionPropsNotImportedAnalyzer, DefaultVerifier> {
TestCode = EmptyCode,
TestState = { AdditionalFiles = { ("Directory.Build.props", directoryBuildProps) } }
};
test.ExpectedDiagnostics.Add(
new DiagnosticResult(Al0018VersionPropsNotImportedAnalyzer.DiagnosticId, DiagnosticSeverity.Warning)
.WithLocation("Directory.Build.props", 1, 1));
await test.RunAsync(TestContext.Current.CancellationToken);
}
}There was a problem hiding this comment.
Pull request overview
This PR adds a new analyzer AL0018 that enforces centralized version management by warning when Directory.Build.props files don't import Version.props. This works in conjunction with AL0017 to ensure version properties are properly defined and imported.
Changes:
- Adds AL0018VersionPropsNotImportedAnalyzer to detect missing Version.props imports
- Adds comprehensive test coverage with 4 test scenarios
- Adds resource strings for diagnostic messages
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/ANcpLua.Analyzers/Analyzers/AL0018VersionPropsNotImportedAnalyzer.cs | New analyzer implementation that checks Directory.Build.props files for Version.props import statements |
| tests/ANcpLua.Analyzers.Tests/AL0018AnalyzerTests.cs | Comprehensive test coverage including positive/negative cases and edge cases |
| src/ANcpLua.Analyzers/Resources.resx | Adds AL0018 resource strings (also includes unrelated AL0026/AL0027 entries) |
| src/ANcpLua.Analyzers/Resources.Designer.cs | Generated resource accessors for AL0018 (also includes unrelated AL0026/AL0027 entries) |
Files not reviewed (1)
- src/ANcpLua.Analyzers/Resources.Designer.cs: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| internal static string AL0026AnalyzerMessageFormat { | ||
| get { | ||
| return ResourceManager.GetString("AL0026AnalyzerMessageFormat", resourceCulture); | ||
| } | ||
| } | ||
|
|
||
| internal static string AL0026AnalyzerDescription { | ||
| get { | ||
| return ResourceManager.GetString("AL0026AnalyzerDescription", resourceCulture); | ||
| } | ||
| } | ||
|
|
||
| internal static string AL0027AnalyzerTitle { | ||
| get { | ||
| return ResourceManager.GetString("AL0027AnalyzerTitle", resourceCulture); | ||
| } | ||
| } | ||
|
|
||
| internal static string AL0027AnalyzerMessageFormat { | ||
| get { | ||
| return ResourceManager.GetString("AL0027AnalyzerMessageFormat", resourceCulture); | ||
| } | ||
| } | ||
|
|
||
| internal static string AL0027AnalyzerDescription { | ||
| get { | ||
| return ResourceManager.GetString("AL0027AnalyzerDescription", resourceCulture); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
The Resources.Designer.cs file includes resource property accessors for AL0026 and AL0027 analyzers (lines 408-442), which are not part of this pull request. These entries should be removed to keep the PR scope focused on AL0018 only.
| internal static string AL0026AnalyzerMessageFormat { | |
| get { | |
| return ResourceManager.GetString("AL0026AnalyzerMessageFormat", resourceCulture); | |
| } | |
| } | |
| internal static string AL0026AnalyzerDescription { | |
| get { | |
| return ResourceManager.GetString("AL0026AnalyzerDescription", resourceCulture); | |
| } | |
| } | |
| internal static string AL0027AnalyzerTitle { | |
| get { | |
| return ResourceManager.GetString("AL0027AnalyzerTitle", resourceCulture); | |
| } | |
| } | |
| internal static string AL0027AnalyzerMessageFormat { | |
| get { | |
| return ResourceManager.GetString("AL0027AnalyzerMessageFormat", resourceCulture); | |
| } | |
| } | |
| internal static string AL0027AnalyzerDescription { | |
| get { | |
| return ResourceManager.GetString("AL0027AnalyzerDescription", resourceCulture); | |
| } | |
| } |
| <!-- AL0018: Version.props not imported --> | ||
| <data name="AL0018AnalyzerTitle" xml:space="preserve"> | ||
| <value>Version.props not imported</value> | ||
| </data> | ||
| <data name="AL0018AnalyzerMessageFormat" xml:space="preserve"> | ||
| <value>Directory.Build.props should import Version.props for centralized version management</value> | ||
| </data> | ||
| <data name="AL0018AnalyzerDescription" xml:space="preserve"> | ||
| <value>Version.props should be imported in Directory.Build.props to enable centralized package version management using $(VariableName) syntax.</value> | ||
| </data> |
There was a problem hiding this comment.
The AL0018 resource entries are placed at the end of the file (after AL0027), but should be placed in numerical order after AL0017 resources. This would improve consistency with the project's organizational structure where analyzer resources are ordered by their numeric IDs.
| <!-- AL0026: Avoid DateTime time accessors --> | ||
| <data name="AL0026AnalyzerTitle" xml:space="preserve"> | ||
| <value>Avoid DateTime time accessors</value> | ||
| </data> | ||
| <data name="AL0026AnalyzerMessageFormat" xml:space="preserve"> | ||
| <value>'{0}' should be replaced with TimeProvider.System.GetUtcNow() for better testability</value> | ||
| </data> | ||
| <data name="AL0026AnalyzerDescription" xml:space="preserve"> | ||
| <value>DateTime time accessors make code difficult to test. Use TimeProvider.System.GetUtcNow() instead, which can be mocked in tests.</value> | ||
| </data> | ||
| <!-- AL0027: Avoid legacy JSON library --> | ||
| <data name="AL0027AnalyzerTitle" xml:space="preserve"> | ||
| <value>Avoid legacy JSON library</value> | ||
| </data> | ||
| <data name="AL0027AnalyzerMessageFormat" xml:space="preserve"> | ||
| <value>'{0}' is from a legacy JSON library. Use System.Text.Json instead.</value> | ||
| </data> | ||
| <data name="AL0027AnalyzerDescription" xml:space="preserve"> | ||
| <value>The legacy JSON library should be replaced with System.Text.Json for better performance and native .NET support.</value> | ||
| </data> |
There was a problem hiding this comment.
The Resources.resx file includes resource entries for AL0026 and AL0027 analyzers (lines 267-286), which are not part of this pull request. These entries should be removed to keep the PR scope focused on AL0018 only.
| <!-- AL0026: Avoid DateTime time accessors --> | |
| <data name="AL0026AnalyzerTitle" xml:space="preserve"> | |
| <value>Avoid DateTime time accessors</value> | |
| </data> | |
| <data name="AL0026AnalyzerMessageFormat" xml:space="preserve"> | |
| <value>'{0}' should be replaced with TimeProvider.System.GetUtcNow() for better testability</value> | |
| </data> | |
| <data name="AL0026AnalyzerDescription" xml:space="preserve"> | |
| <value>DateTime time accessors make code difficult to test. Use TimeProvider.System.GetUtcNow() instead, which can be mocked in tests.</value> | |
| </data> | |
| <!-- AL0027: Avoid legacy JSON library --> | |
| <data name="AL0027AnalyzerTitle" xml:space="preserve"> | |
| <value>Avoid legacy JSON library</value> | |
| </data> | |
| <data name="AL0027AnalyzerMessageFormat" xml:space="preserve"> | |
| <value>'{0}' is from a legacy JSON library. Use System.Text.Json instead.</value> | |
| </data> | |
| <data name="AL0027AnalyzerDescription" xml:space="preserve"> | |
| <value>The legacy JSON library should be replaced with System.Text.Json for better performance and native .NET support.</value> | |
| </data> |
Add new analyzer that reports when Directory.Build.props doesn't import Version.props for centralized version management. - Checks for Import elements referencing Version.props - Supports various path formats (Version.props, ../build/Version.props) - Only checks Directory.Build.props files in AdditionalFiles - Includes 4 tests covering various scenarios Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
9245eb3 to
c8596f8
Compare
…e strings (#178) Closes task #27 ("AL wave: rewrite analyzer docs generator (post-renumber)"). What landed (no AL package version bump — pure doc + internal tools change): New under tools/ANcpLua.Analyzers.DocsGenerator/ (docs-only, never shipped in the analyzer DLL because the 2.0.1 analyzer no longer emits AL0xxx; nothing at runtime needs the rename map): - AlIdMigrationCatalog.cs — 89 AL0xxx → AL1xxx rename rows hand-transcribed from eng/analyzer-renumber-plan.md §2. Validate() asserts structural invariants (no duplicate OldId/NewId, every NewId matches ^AL1[0-8]\d{2}$, every OldId matches ^AL\d{4}$). No hardcoded ExpectedCount — count is a consequence of the invariants, not a property worth asserting on its own. Public class (not internal) so the tests project can call Validate() via ProjectReference without InternalsVisibleTo — IVT would expose the tools Exe's top-level Program and collide with the tests project's own Program (CS0433). Public is safe because the tools assembly is never packed. - MigrationCatalogRenderer.cs — mirrors QYL's CatalogStatistics-driven section-array pattern (qyl repo's tools/Qyl.OpenTelemetry.SemanticConventions.Analyzers.DocsGenerator/ MigrationCatalogRenderer.cs:21-186). 6 sections: header, summary, completion audit, band breakdown table, Old→New mapping table sorted by NewId, regenerate footer. Orchestrator wiring (DocsGenerator.cs): - Compute MigrationCatalogStats once at Run() and thread to Audit/Check/ Generate. Mirrors the qyl repo's CatalogStatistics threading. - Generate step 3 writes docs/migration-catalog.md. - Check step 3 enforces drift detection on the same file. - Audit prints catalog stats (89 renames, 9 bands). RepoLayout.cs: new MigrationCatalogPath(repoRoot) accessor. Cosmetic fix in IndexDocsRenderer.cs (4 occurrences): - Replace post-renumber-stale "AL00xx–AL18xx" with "AL10xx–AL18xx". The AL00xx range no longer hosts this analyzer's IDs — sibling packages (AotReflection, ExtensibleEnumMirror, DiscriminatedUnion) own AL0xxx per eng/analyzer-renumber-plan.md §0. Each replacement has a "// renumber: bump if AL bands shift again" signpost above it. - Added a "See also" link to the new docs/migration-catalog.md. EnforceIdsRewriter.cs comment: "/// AL00XX:" placeholder → "/// AL####:" (the actual regex was already AL-prefix-agnostic; only the comment was stale). Mandatory unit test (the highest-leverage piece of the whole change): - tests/ANcpLua.Analyzers.Tests/AnalyzerConventionTests.cs gains AlIdMigrationCatalog_StructuralInvariants_Hold which runs Validate() on every CI build. Catches hand-transcription drift in the 89-row Entries array immediately, instead of only when someone runs --check on a dev machine. Generated docs/migration-catalog.md committed for --check drift gating. Verification: - dotnet build tools/ANcpLua.Analyzers.DocsGenerator/ --nologo -warnaserror -> 0 warnings, 0 errors - dotnet build tests/ANcpLua.Analyzers.Tests/ --nologo -warnaserror -> 0 warnings, 0 errors - dotnet run --project tests/ANcpLua.Analyzers.Tests/ -> 758 pass, 0 fail - dotnet run --project tools/ANcpLua.Analyzers.DocsGenerator/ -> emits 7 artifacts including the new docs/migration-catalog.md - dotnet run --project tools/ANcpLua.Analyzers.DocsGenerator/ -- --check -> all artifacts up to date (clean drift gate) - dotnet run --project tools/ANcpLua.Analyzers.DocsGenerator/ -- --audit -> 89 renames across 9 bands; counts match renumber-plan §1 exactly - grep -c AL00xx docs/ANcpLua.Analyzers.md -> 0 - grep -cE '^\| `AL0' docs/migration-catalog.md -> 89
Summary
Test plan
ShouldReportWhenVersionPropsNotImported- warns when no import existsShouldNotReportWhenVersionPropsImported- no warning with proper importShouldNotReportWhenVersionPropsImportedWithPath- handles relative pathsShouldNotReportForOtherPropsFiles- only checks Directory.Build.props🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.