Skip to content

Fix VB XML attribute-axis reads - #1293

Draft
CosmonautJones wants to merge 3 commits into
icsharpcode:masterfrom
CosmonautJones:fix/xml-attribute-axis-950
Draft

CosmonautJones wants to merge 3 commits into
icsharpcode:masterfrom
CosmonautJones:fix/xml-attribute-axis-950

Conversation

@CosmonautJones

Copy link
Copy Markdown
Contributor

Fixes #950.

Problem

VB's element.@name returns a string, or Nothing when the attribute is missing. The converter emits Attributes(name), a collection that cannot be assigned to a string.

Solution

For a single element, emit the XAttribute string conversion over Attribute(name), producing (string)element.Attribute("name") after simplification. For IEnumerable<XElement>, read the first element's attribute value. A missing attribute on the first element must stay null even when a later element has that attribute; an empty sequence also returns null. Explicit element/interface casts and static LINQ-to-Objects calls preserve VB's binding for derived, custom, queryable, and value-type receivers.

Unprefixed attributes ignore the default XML namespace; explicit prefixes retain their namespace. Conditional attribute chains capture the receiver once so the string conversion, null checks, and subsequent member/index access preserve VB behavior. Statement forms, including explicit Call, use an if when required.

The conditional-access lowering is the part most worth maintainer scrutiny. Tests cover null initial receivers, null intermediate values, child/index chains, With inside an unrelated conditional lambda, and void/discarded-return calls. This patch covers attribute-axis reads, not setters or the separate XML-literal entity-decoding issue.

  • Tests cover the changed code. The established self-verifying harness compiles and executes the original VB and converted C#, including 23 new behavior cases.

Validation

All results below are for exact commit ae1d57ca8b0ffa7eadedab2dc9acc83d6418ec3b on Windows x64, built with .NET SDK 10.0.402. Both Release target-framework builds completed with 0 warnings and 0 errors.

Actual test runtime Focused XML + existing Call test Bounded broader suite
Microsoft.NETCore.App 10.0.13 (net10.0) 12 passed, 0 failed, 0 skipped 515 passed, 0 failed, 3 existing skips
Microsoft.NETCore.App 8.0.32 (net8.0) 12 passed, 0 failed, 0 skipped 515 passed, 0 failed, 3 existing skips

The broader selection includes both directions' expression tests, VB-to-C# self-verifying tests, newline tests, and the existing CallStatementAsync. All 23 new XML facts passed individually in each broader run. The three existing skips are DeeplyNestedBinaryExpressionShouldNotStackOverflowAsync (too slow), LinqAggregateSumAsync, and LinqGroupJoinAsync (the latter two tracked by #29).

Build command for each target:

dotnet build Tests/Tests.csproj -c Release -f <net10.0-or-net8.0> --no-restore --disable-build-servers -m:1 -p:UseSharedCompilation=false

The net10.0 runs used dotnet test with --no-build --no-restore and these filters:

Focused: FullyQualifiedName~XmlExpressionTests|FullyQualifiedName~MethodStatementTests.CallStatementAsync
Broader: FullyQualifiedName~ICSharpCode.CodeConverter.Tests.CSharp.ExpressionTests|FullyQualifiedName~ICSharpCode.CodeConverter.Tests.VB.ExpressionTests|FullyQualifiedName~ICSharpCode.CodeConverter.Tests.CSharp.SelfVerifyingTests|FullyQualifiedName~ICSharpCode.CodeConverter.Tests.LanguageAgnostic.NewlineTests|FullyQualifiedName~MethodStatementTests.CallStatementAsync

The net8.0 assembly was executed with the existing SDK 9.0.316 VSTest 17.14.1 launcher, using /Framework:.NETCoreApp,Version=v8.0 and the same filters. That launcher ran on .NET 9.0.18; the actual testhost ran on 8.0.32, verified from its loaded coreclr.dll, host traces, and diagnostics. The compiler harness uses the repository's .NET 8 reference assemblies for both runs. This does not claim an SDK-8 build or full upstream CI, Linux, VSIX, web, or full-repository validation.

A separate internal AI QA agent reviewed the final diff, added two challenge tests whose failures were fixed, independently built and ran the final net10.0 focused suite, and checked the final broader and net8.0 evidence. No actionable findings remained. The initial focused regression reproduced the collection-to-string conversion failure before the fix.

Process

  • Attachment level: low; open to maintainer-preferred simplification of the approach.
  • LLM primarily used: OpenAI Codex (GPT-6), with separate research and QA agents. The implementation, tests, and this explanation were AI-written. This is a human-reviewed, AI-assisted submission; Travis reviewed the contribution and approved submitting this draft.
  • Relevant task prompt excerpts:
Reproduce on current upstream before assuming defect. Research correct VB semantics; cover single XElement and IEnumerable<XElement>, missing attributes, empty sequences, first-match behavior, namespaces, and compile/runtime equivalence rather than snapshots alone.

Use test-first and existing task-local .NET10 SDK10.0.402/runtime10.0.13, no new runtime install. Run focused and relevant broader checks, independent QA on exact final commit and fixes until sound.

Use the exact ae1d57ca8b0ffa7eadedab2dc9acc83d6418ec3b commit, run focused and relevant broader net8 tests, and retain separate runtime evidence. No source changes unless a real regression is reproduced; any fix needs repeat independent review. Do not expand into attribute setters.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

VB -> C#: The LINQ-to-XML attribute axis accessor should be translated as a call to XElement.Attribute rather than XElement.Attributes.

2 participants