Tagging subscribers to this area: @steveisok, @dotnet/area-system-diagnostics
See info in area-owners.md if you want to be subscribed.

@copilot please invoke the code-review skill and post the analysis/comments as a comment on this PR

@copilot please invoke the code-review skill and post the analysis/comments as a comment on this PR

🤖 Copilot Code Review — PR #124680

Holistic Assessment

Motivation: The PR backports XML documentation from dotnet-api-docs into the source implementation of FileVersionInfo. This is part of a broader effort to move API docs into the runtime source. The motivation is clear and the scope is appropriate.

Approach: The net change (after the ref assembly and csproj reverts based on reviewer feedback) is purely additive <value>, <param>, and <returns> XML doc tags on the single shared source file. No ref assembly or csproj changes remain. The UseCompilerGeneratedDocXmlFile stays false because this library uses GeneratePlatformNotSupportedAssemblyMessage and a scheme for handling that hasn't been designed yet (per @jkotas's comment).

Summary: ⚠️ Needs Human Review. The docs-only change is low-risk and the content is mostly accurate. However, I identified two issues that need human judgment: (1) the PR intentionally deviates from the upstream dotnet-api-docs for int property <value> tags (saying 0 instead of null), which is arguably a correction but creates divergence, and (2) since UseCompilerGeneratedDocXmlFile remains false, these <value> tags won't actually appear in any generated XML doc file or on learn.microsoft.com — they only serve as inline developer documentation in the source code. Maintainers should decide whether that's acceptable or whether this work should wait until the scheme for assemblies with platform-not-supported stubs is resolved.

Detailed Findings

⚠️ UseCompilerGeneratedDocXmlFile remains false — docs have no external effect

Because <UseCompilerGeneratedDocXmlFile>false</UseCompilerGeneratedDocXmlFile> is still set in the csproj (line 6), the compiler will not produce a .xml documentation file. This means the <value>, <param>, and <returns> tags added in this PR will only be visible to developers reading the source code — they won't flow to learn.microsoft.com or IntelliSense consumers via NuGet. The PR description acknowledges this, but it raises the question of whether the work should wait until the scheme @jkotas described for assemblies with GeneratePlatformNotSupportedAssemblyMessage is in place. The value of the PR is reduced to "developer documentation in source" rather than "seeds API docs."

⚠️ Intentional divergence from upstream dotnet-api-docs for int properties

The upstream XML (FileBuildPart) says:

A value representing the build number of the file or <see langword="null" /> if the file did not contain version information.

The PR changes this to 0 (zero) instead of null for all six int properties (FileBuildPart, FilePrivatePart, FileMajorPart, FileMinorPart, ProductBuildPart, ProductMajorPart, ProductMinorPart, ProductPrivatePart). This is arguably more accurate for .NET Core (int cannot be null), but:

• It creates intentional divergence from the canonical docs source.
• The upstream docs likely say null because this historically applied to .NET Framework where the behavior may have differed.
• Note: FileMajorPart and FileMinorPart already say 0 (zero) in the upstream, so they match. The divergence is on FileBuildPart, FilePrivatePart, ProductBuildPart, ProductMajorPart, ProductMinorPart, and ProductPrivatePart.

Maintainers should decide: fix the upstream docs instead, or accept the divergence here?

✅ Content accuracy verified against upstream

I verified the <value> tags for all 28 properties, <param> for GetVersionInfo, and <returns> for both GetVersionInfo and ToString against the upstream dotnet-api-docs XML. Other than the intentional null → 0 change for int properties, the content matches the upstream faithfully.

✅ Summary tags — minor rewording is correct

A few existing <summary> tags were reformatted from multi-line to single-line (e.g., IsDebug, IsPatched, IsPreRelease, ToString). The wording matches the upstream docs and no semantic changes were introduced.

💡 ToString returns tag is very long

The <returns> tag for ToString() (line 311) lists all 13 properties inline. This matches the upstream docs exactly, so it's faithful — but it's a very long single-line XML doc comment. This is a minor readability observation, not actionable.

✅ No ref assembly or csproj changes

The ref assembly (ref/System.Diagnostics.FileVersionInfo.cs) and project file (System.Diagnostics.FileVersionInfo.csproj) are confirmed unchanged from the base branch, correctly addressing @gewarren's earlier feedback.

🤖 Copilot Code Review — PR #124405

Holistic Assessment

Motivation: Backporting XML documentation from dotnet-api-docs into the FileVersionInfo source is part of a well-established effort to move API docs into the runtime source. The motivation is clear and justified.

Approach: The PR adds <value>, <param>, <returns>, and <exception> XML doc tags to all public members, removes UseCompilerGeneratedDocXmlFile=false from the csproj to enable compiler-generated docs, and extends the test csproj with an Apple mobile workaround. The approach is sound.

Summary: ⚠️ Needs Human Review. The documentation content is largely accurate and the infrastructure change is well-supported by eng/intellisense.targets. However, there are two items needing human judgment: (1) the InternalName value docs are inconsistent with all other string? properties by not mentioning the null case, and (2) @stephentoub's approval was on commit a640b63 which predates the csproj change and test csproj changes.

Detailed Findings

✅ Removing UseCompilerGeneratedDocXmlFile=false is correct

I verified that eng/intellisense.targets (lines 90–153) has full infrastructure for PNSE assemblies. When UseCompilerGeneratedDocXmlFile is true (the default) and the current TF is a PNSE build (GeneratePlatformNotSupportedAssemblyMessage is set), the build system:

1. Identifies a non-PNSE sibling TF via IsCandidateCompilerGeneratedDocFile
2. Adds a ProjectReference to that sibling to retrieve its compiler-generated doc XML
3. Uses that doc XML for the PNSE assembly

For FileVersionInfo, the net11.0-windows and net11.0-unix TFs will produce the XML docs; the plain net11.0 PNSE build will consume them. CS1591 warnings are also suppressed for PNSE builds (line 25–28). This contradicts the earlier concern that "a scheme for assemblies with platform-not-supported stubs" doesn't exist — it does, and the change is correct.

⚠️ InternalName value docs don't mention the null case

All other string? properties consistently document the null case with "or null if the file did not contain version information." But InternalName (line 127) says:

The internal name of the file. If none exists, this property will contain the original name of the file without the extension.

This text matches the upstream dotnet-api-docs verbatim, but InternalName is typed string? and can be null when no version info exists (the backing field _internalName defaults to null and is only set in the platform-specific constructors when version data is found). The Copilot PR reviewer flagged this at #discussion_r3048609728 but it wasn't addressed.

Recommendation: Either adopt the reviewer's suggestion or at minimum add "or null if the file did not contain version information" to be consistent with all sibling properties.

✅ XML doc content is faithful to upstream with correct fixes

I verified <value> tags for all 28 properties, <param> for GetVersionInfo, <returns> for both GetVersionInfo and ToString, and the <exception> tag against the upstream dotnet-api-docs XML. The intentional change from null to 0 (zero) for int properties (FileBuildPart, FilePrivatePart, ProductBuildPart, etc.) is correct — int cannot be null. The ToString returns text was also updated to better match actual behavior ("empty" instead of "null" for string values in output).

✅ Reviewer feedback was addressed

The latest commits addressed 3 of 4 suggestions from the Copilot PR reviewer:

• ✅ LegalCopyright — added null case to value docs
• ✅ GetVersionInfo param — removed "fully qualified" since method accepts relative paths
• ✅ Added <exception cref="FileNotFoundException"> to GetVersionInfo
• ❌ InternalName — not addressed (noted above)

💡 Test csproj changes are mixed scope

The test csproj changes (extending the Apple mobile workaround with '$(TargetsAppleMobile)' != 'true' and adding <ReferenceOutputAssembly>false</ReferenceOutputAssembly>) are unrelated to XML doc backporting. The ReferenceOutputAssembly=false is correct — tests only load the TestAssembly by file path via FileVersionInfo.GetVersionInfo(), not by referencing types from it. These are reasonable fixes but could have been a separate PR for cleaner scope.

⚠️ Prior approval may need re-confirmation

@stephentoub's approval (review 3889615641, state: APPROVED) was submitted on commit a640b63d0 ("Remove XML docs from ref assembly per feedback"). The current HEAD (e0d06f7db) includes additional changes:

• Removal of UseCompilerGeneratedDocXmlFile=false from csproj (commit 854b40d5)
• Test csproj Apple mobile fix
• Multiple doc text refinements

These are non-trivial additions relative to what was approved.

Generated by Code Review for issue #124405 · ◷

/ba-g all failing tests are unrelated.