Fix CodeQL and SonarCloud issues with refactoring and improvements - #38
Conversation
Addresses all 14 CodeQL findings, 73 SonarCloud code smells, and the 1 security hotspot reported by the "Build Documents" CI job's quality reports (CodeQL Quality Report / SonarCloud Quality Report). Security hotspot: - DotNetGenerator.cs: added an explicit timeout to the Regex call to resolve the new_security_hotspots_reviewed quality-gate failure. CodeQL (14/14): - Extracted a complex boolean condition into named local variables (cs/complex-condition). - Converted 5 imperative loops that only projected values into `.Select()` LINQ expressions (cs/linq/missed-select). - Bundled the ref parameters of HandleBlockCommandComment into a state object (cs/too-many-ref-parameters). - Replaced 7 ambiguous `Path.Combine` calls in test files with `Path.Join` to remove root-drive ambiguity (cs/path-combine). SonarCloud (73 smells): - Refactored ~28 methods flagged for cognitive complexity (S3776) by extracting helper methods. - Replaced repeated string literals with named constants (S1192). - Narrowed several interface-typed locals to their concrete types where safe (CA1859). - Grouped clusters of related parameters into parameter objects to resolve S107 (too many parameters). - Fixed misc CA1847, CA1816, xUnit2032 findings. - Removed redundant null-forgiving operators (S8969), keeping two documented as false positives where the compiler cannot prove non-null. - Documented the clang PATH lookup (S4036) as an intentional, overridable design (APIMARK_CLANG_PATH) rather than a vulnerability. No test-framework packages (xunit.v3, coverlet.collector) were touched. Build is clean with 0 warnings/0 errors; test pass counts are unchanged from baseline (pre-existing environment-only failures in ApiMark.Cpp.Tests and ApiMark.MSBuild.PackageTests, caused by clang not being installed in this local environment, are unaffected by these changes and reproduce identically on main). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Fixes CI lint failure: cspell flagged 'NOSONAR' (standard SonarQube suppression directive comment) and 'linkifying' (verb form of existing dictionary entries linkify/linkifies/linkified) as unknown words. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This pull request refactors several ApiMark emitters and supporting utilities to address static-analysis findings (CodeQL/SonarCloud) by reducing duplication, tightening type usage, and clarifying the structure of emitted documentation across .NET, C++, and VHDL generators.
Changes:
- Refactored C++ AST parsing and emission to extract repeated literals/constants and split large emission blocks into smaller helpers for clearer flow.
- Refactored .NET/VHDL emitters and parsers to reduce parameter lists, centralize common rendering logic (examples, package sections), and improve internal readability.
- Refactored glob pattern collection and CLI/MSBuild argument construction into smaller, single-responsibility helpers; updated tests and spell-check dictionary accordingly.
Reviewed changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/ApiMark.Vhdl.Tests/VhdlGeneratorTests.cs | Uses Path.Join in temp path creation for consistency. |
| test/ApiMark.Vhdl.Tests/VhdlEmitterTests.cs | Adjusts runtime-type assertion style for IApiEmitter. |
| test/ApiMark.Vhdl.Tests/VhdlAstParserTests.cs | Uses Path.Join for temp file path creation. |
| test/ApiMark.DotNet.Tests/TypeLinkResolverTests.cs | Adds GC.SuppressFinalize in Dispose pattern. |
| test/ApiMark.DotNet.Tests/DotNetGeneratorTests.cs | Simplifies heading predicate by using Contains('('). |
| test/ApiMark.DotNet.Tests/DotNetAstModelTests.cs | Updates assertions to Assert.IsType(..., exactMatch: false) for read-only interface checks. |
| test/ApiMark.DotNet.Tests/DocumentationCoverageCheckerTests.cs | Updates helper return type to List<string> where mutation is required. |
| test/ApiMark.Cpp.Tests/CppGeneratorTests.cs | Simplifies heading predicate and uses Path.Join for header path. |
| test/ApiMark.Cpp.Tests/CppEmitterTests.cs | Uses Path.Join for header path construction. |
| src/ApiMark.Vhdl/VhdlEmitterSingleFile.cs | Splits package emission into dedicated helpers for summary/types/constants/components/subprograms. |
| src/ApiMark.Vhdl/VhdlEmitterGradualDisclosure.cs | Mirrors package emission refactor and extracts subprogram list emission. |
| src/ApiMark.Vhdl/VhdlAst/VhdlAstParser.cs | Refactors port parsing and package item parsing into smaller helpers; streamlines parameter extraction. |
| src/ApiMark.Tool/Program.cs | Extracts language IDs/constants; centralizes required-option validation and --enforce-docs parsing. |
| src/ApiMark.MSBuild/ApiMarkTask.cs | Deduplicates CLI arg building with helper methods; removes unnecessary null-forgiving on ToolDllPath. |
| src/ApiMark.DotNet/XmlDocReader.cs | Refactors example parsing and inheritdoc resolution into smaller helpers; improves list rendering loop. |
| src/ApiMark.DotNet/TypeLinkResolver.cs | Extracts linkification branches into small helpers for readability and reuse. |
| src/ApiMark.DotNet/DotNetGenerator.cs | Refactors inheritance-target collection into helpers; adds regex timeout for exclude patterns. |
| src/ApiMark.DotNet/DotNetEmitterSingleFile.cs | Introduces a context record and example rendering helper to reduce parameter counts and duplication. |
| src/ApiMark.DotNet/DotNetEmitterGradualDisclosure.cs | Adds buckets class/constants and extracts type-header/member processing helpers to simplify large methods. |
| src/ApiMark.DotNet/DotNetEmitter.cs | Extracts method visibility logic and removes redundant accessor null checks. |
| src/ApiMark.DotNet/DotNetAstModel.cs | Introduces DotNetAstModelArgs to bundle constructor parameters. |
| src/ApiMark.Cpp/CppTypeLinkResolver.cs | Extracts intra-library link building and external-type tracking; refactors qualifier stripping. |
| src/ApiMark.Cpp/CppGenerator.cs | Splits namespace collection into dedicated helpers per declaration kind. |
| src/ApiMark.Cpp/CppEmitterSingleFile.cs | Splits class emission into signature/doc/member/type-alias helpers; extracts member-body writers. |
| src/ApiMark.Cpp/CppEmitterGradualDisclosure.cs | Extracts repeated labels/prefixes into constants and adjusts operator/returns labeling. |
| src/ApiMark.Cpp/CppAst/ClangAstParser.cs | Extracts repeated JSON field/kind strings into constants and modularizes class/function/enum parsing. |
| src/ApiMark.Core/GlobFileCollector.cs | Refactors pattern processing into ProcessPattern/ProcessLiteralPattern/ProcessGlobPattern. |
| .cspell.yaml | Adds words (linkifying, NOSONAR) to reduce false positives. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- VhdlAstParser.cs: replace 'Where(x => x != null)' with 'OfType<...>()' so the foreach variable is non-nullable, avoiding nullable-reference warnings on the downstream interface_signal_declaration() etc. calls. - DotNetGenerator.cs: change BuildInheritanceChain's return type from Dictionary<,> to IReadOnlyDictionary<,> to match its XML doc comment and its read-only usage in XmlDocReader. - DotNetEmitterSingleFile.cs: correct the SharedExternalTypes XML doc comment - LinkifyExternalType tracks non-System external types regardless of generateLinks, so the set is populated even though the single-file emitter never reads it back. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 28 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
src/ApiMark.DotNet/DotNetEmitterGradualDisclosure.cs:14
- The PR description focuses on C++ emitter/glob collector changes, but this PR also contains substantial refactoring in the .NET/VHDL emitters and tool/MSBuild logic. Please update the PR description (or split the PR) so reviewers know the full scope being changed.
/// <summary>
/// Mutable accumulators for the per-kind member table rows and external type references
/// collected while writing a single type page. Used to reduce the parameter count on the
/// member-processing helpers in <see cref="DotNetEmitterGradualDisclosure"/>.
/// </summary>
src/ApiMark.DotNet/DotNetGenerator.cs:562
- This AddRange call builds an IEnumerable<string?> (because MapAccessorReferenceToEventId returns string?) and passes it to List.AddRange, which will not compile under nullable reference types. Filter/cast to a non-null string sequence before adding.
targets.AddRange(accessor.Overrides
.Select(overrideRef => MapAccessorReferenceToEventId(overrideRef, prefix))
.Where(evId => evId != null && !targets.Contains(evId, StringComparer.Ordinal))!);
}
DotNetGenerator.cs: AddPropertyAccessorOverrides and AddEventAccessorOverrides filtered null property/event IDs with 'Where(x => x != null ...)!', but the trailing '!' only suppresses the top-level null-state and does not change the LINQ pipeline's element type from string? to string. Passing that IEnumerable<string?> to List<string>.AddRange triggers CS8620 under nullable reference types. Replaced with '.OfType<string>()' before the Where filter so the sequence is genuinely non-null. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This pull request introduces several improvements and refactorings to the C++ emitter and glob file collector logic, focusing on code maintainability, clarity, and consistency in emitted documentation. The main changes include extracting repeated string literals into constants, modularizing class documentation emission, and refactoring the glob pattern processing for better readability and separation of concerns.
C++ Emitter improvements:
"operator","Returns", and"Operators") into private constants (OperatorNamePrefix,ReturnsLabel,OperatorsLabel) and replaced all usages with these constants for consistency and easier future changes. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11]CppEmitterSingleFile.csby splitting the logic into three dedicated methods:WriteSingleFileClassSignature,WriteSingleFileClassDocBlock, andWriteSingleFileClassMembers, improving readability and separation of concerns. [1] [2] [3]IReadOnlyDictionary/IReadOnlyListto mutable types (Dictionary/List) where mutation is required, improving type correctness. [1] [2] [3] [4]Glob pattern collector refactor:
Collectmethod inGlobFileCollector.csby extracting logic for processing patterns into dedicated methods:ProcessPattern,ProcessLiteralPattern, andProcessGlobPattern, clarifying the flow and responsibilities of each part of the pattern processing. [1] [2]Spell checker configuration:
.cspell.yamlto avoid false positives in spell checking, including"linkifying"and"NOSONAR". [1] [2]