Fix for XSLT extension methods format-date & format-time

[Maximys]

This is fix for #93189 .
I create it as draft, because does not sure that everythink work fine

[dotnet-policy-service]

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

[Maximys]

@dotnet/area-system-xml , pay attention, please, that current main brunch of .NET Runtime contains some invalid test cases, which was fixed by c6003c7

[lilinus]

Generally, unrelated changes should not be mixed in the same PR (i.e. refactors and bugfixing) according to the PR guide.

[Maximys]

Generally, unrelated changes should not be mixed in the same PR (i.e. refactors and bugfixing) according to the PR guide.

Thanks @lilinus, but I think you're not quite right. Most of my refactoring linked with files, which should be touched for fix current bug. May be only tests can be untouched and XsdDateTime with XsdDuration can be left in the previous folder and namespace, but I still think, that this changes make current code a little more readable.

[Maximys]

@jeffhandley , can you review curent pr, please?

[Maximys]

@jeffhandley , @dotnet/area-system-xml do you have any feedback?

[Maximys]

@krwq , any progress of current review? As you can see, I had fix one culture bug by commit, that was introduced within this PR.

[krwq]

could we try to avoid changes in product code (refactoring in tests is ok) - this code is in maintenance it's really hard to tell this change is in any way related to regression commit d784b77

Specifically I mean moving classes out of the file, any re-orders and renames should not happen in the maintenance-only code. This should be really the minimum possible fix which fixes the issue.

[krwq]

@Maximys sorry for the delay. Note this code is in maintenance and therefore the changes should be very minimal and only fix the regression. I think test code looks good for the most part.

[krwq]

These don't seem to related to this issue - if there is some product change needed it should be the only change in this file but looking at original regression it doesn't seem to touch this file.

[Maximys]

@krwq , can you review it again? I have just removed some unnecessary changes.

[krwq]

@Maximys sorry, I just came back from vacation and only took a look right now.

I'm still a bit confused on why there are so many changes here - this seems to fix a bit more than just this regression but I could be wrong. Looking at the original regression: PR d784b77 this seems pretty contained in a single file and small. Here I'm seeing: parser, XPath changes, couple of helper types. Can those be split and more self contained? Could they use original Windows API? (presumably no because it won't work on Linux but please do explain if/how you checked how to do this with other existing APIs and why it can't be done using those and explain why is new parser needed). Separate changes should be kept separate for this library even if they're around the same feature.

Note that writing new parser means we will need extra security review for this therefore it's important to re-use existing APIs when possible.

[Maximys]

@krwg, I hope no one will argue that the code quality in System.Private.Xml is not very high; I would even say that in some places it resembles Augean stables.
In my work, I always try to follow the principles from Code Complete: A Practical Handbook of Software Construction, Second Edition and the Boy Scout Rule. Additionally, I always separate commits for refactoring, adding new features, and fixing bugs, using the prefixes refactor, feat, and fix.
The new parser you’re referring to was introduced in commit 95ff95a35916f2084978468628a7cf5f2aab473f, which also renamed Parser to XsdDateTimeParser. After that, I performed a small refactor in commit dea09ad1e8f74ea1e794373899800a47ffa8d409. Finally, I fixed the bug in commits 7a30df29b886966b06e920c3a3ba1bef4f48e9f0 and 6bdbe81e54776cbc1bb8f47f695ecebcedfb0686.

[krwq]

any reason this could not use TryParseExact? Do we need to support all of these? Is this documented? Can this logic be simplified?

[krwq]

Do we actually need to introduce extra data structures? The original code didn't seem to have them

[krwq]

@Maximys this change should be very minimal for it to get a chance to make it to .NET 10. As is the earliest is .NET 11 as we consider the amount of changes too risky especially for maintenance lib.

I understand that you've put stuff in the separate commits - those are still separate concerns from my perspective and we currently squash every single PR in the end so these won't be visible in case of regression.

Please do investigate if we can simplify the parser - can some existing code be re-used? Current logic might be correct but it's hard to reason about - can you point to pieces of spec you're implementing and write comment why existing APIs can't be used? Can we make some shortcuts to improve readability? (i.e. reduce 80% of code while making most of the scenarios from the spec still work)

I want to emphasize that this library is in maintenance mode - this means we only take low-risk or high impact fixes. This one is considered only because it was a regression from one of the previous releases but the amount of changes here is much greater than original PR which regressed it and touches on the pieces of code which don't seem immediately obvious why they're related and adds a new parser and therefore it's not considered low-risk anymore.

As for quality - take into consideration this library is very old (20+ years?), has many users and it's more than 3MB of source code - while I do agree many pieces are not well written some language constructs and design patterns didn't exist when this code was written. You cannot compare quality without taking that into consideration - every single change you make here can potentially break someone and therefore concerns in this PR. Note that I'm supportive with test refactoring changes - they are little risk from my perspective because they don't touch the product.