Ensure metadata location when created from xml element with known location

[JanKrivanek]

Fixes #8579

Context

MetadataElement of an ItemElement doesn't have line&col of Location set despite those are created from known xml.
This way errors referencing the Location from metadata would end up pointing to location (0, 0) in the file.

Reason

This happens only to XmlElementWithLocation (as opposed to XmlAttributeWithLocation) during creating XmlElementWithLocation from scratch (during parsing phase that doesn't have the reader with locations already available) from an existing XmlAttributeWithLocation (that has the proper location intitialized from Load phase). Due to the need to keep the constructors contract (we override existing XmlElement), we cannot pass the additional info in the constructor.

Changes Made

Making the Location property of XmlElementWithLocation writable - so that it can be properly rewritten if constructed from an existing node with available location info.

Testing

Hand testing plus applying the changfe to the other PR: #8581, solved the issue with (0, 0) warnings location

Note

This or #8581 PR (whichever is merged later) should remove the workaround https://github.com/dotnet/msbuild/pull/8581/files#diff-e289ba4ce7fa0e72cf63049cce595eafcad1e7b2034ccb3305cd0f06c2f822b8R196-R197

[Forgind]

This looks solid to me 🙂

My only real concern is over incorrect locations. I'm mostly thinking of the XmlUtilities change—if its children have locations, then I imagine their locations would shift slightly (longer name --> higher column number; possible reformatting --> different line number; etc.). If those stack up, we might be noticeably wrong, which could be very confusing. Do we appropriately account for those sorts of changes?

[JanKrivanek]

@Forgind - very good point about precautions with locations shifts!
The location is set in two places:

• RenameXmlElement - there location is preserved from the element being renamed (not the attributes) - location of element should not change with rename
• ProjectMetadataElement.CreateDisconnected - there the location is argument is populated only for code paths executed on initial project parsing (ProjectParser.ParseProjectItemElement) - so the location is not altered.

As for subsequent edits of the document (and hence possible subsequent locations shift) - this remains unchanged - the location is not guaranteed after edits:

https://github.com/dotnet/msbuild/blob/main/src/Build/ElementLocation/XmlElementWithLocation.cs#L83

[Forgind]

@Forgind - very good point about precautions with locations shifts! The location is set in two places:

• RenameXmlElement - there location is preserved from the element being renamed (not the attributes) - location of element should not change with rename
• ProjectMetadataElement.CreateDisconnected - there the location is argument is populated only for code paths executed on initial project parsing (ProjectParser.ParseProjectItemElement) - so the location is not altered.

As for subsequent edits of the document (and hence possible subsequent locations shift) - this remains unchanged - the location is not guaranteed after edits:

https://github.com/dotnet/msbuild/blob/main/src/Build/ElementLocation/XmlElementWithLocation.cs#L83

For RenameXmlElement, I think row should be constant, but column could change? Am I wrong about that? Like if I have:
<Foo><Bar /></Foo>, then change it to <FooFooFoo><Bar /></FooFooFoo>, the column of Bar would change.

[JanKrivanek]

For RenameXmlElement, I think row should be constant, but column could change? Am I wrong about that? Like if I have:
<Foo><Bar /></Foo>, then change it to <FooFooFoo><Bar /></FooFooFoo>, the column of Bar would change.

Correct.
This is something that's unchanged by this PR (it's regard the location of other xml elements after editing some elements)

[Forgind]

If it's unchanged, I certainly can't complain too much 🙂

I was thinking you were adding locations in places there previously weren't locations, in which case if they're edited --> wrong, users might expect them to be right anyway, whereas they'd have no expectations without a location. But it sounds like you've looked into this fairly carefully, so I'm satisfied with it.

[ladipro]

nit: I wonder if there's a slightly more elegant way of setting location for newly created elements than adding a Location setter. If you passed the location to ProjectRootElement.CreateElement which in turn would pass it to a new overload of XmlDocumentWithLocation.CreateElement, you could set its _reader field for the duration of the base call, similar to what Load does. Note that _reader is of type IXmlLineInfo, so not necessarily an XML reader. Or you could introduce a new field (_currentLocation?) , which would then even avoid boxing.

Overall, I would find it nicer to leave the *WithLocation classes concerned with setting location without leaking the setter like this. What do you think?

[JanKrivanek]

Thanks for looking into this! Great suggestion!

I was as well trying to see if I can just extend the creation and avoid subsequent mutation.
The problem that stopped me and let me to current code is that the XmlDocumentWithLocation extends XmlDocument and our overriden CreateElement is not being called directy, but via a XmlDocument function that inside calls internal helper function:

So we'd need to duplicate some of that internal XmlDocument functionality (and maintain it going forward).

Not sure which is the lesser evil (allowing mutations vs duplicating XmlDocument code) - I arbitrarily decided for openng for mutation.

What do you think about this?

[JanKrivanek]

Great thanks for offline discussion and tips! - I ended up passing the info via AsyncLocal filed.

[ladipro]

Looks great! I'm curious why you chose AsyncLocal<> over ThreadLocal<> when the relevant code is not async.

[rainersigwald]

Is there a way to add some test coverage for this?

[JanKrivanek]

Looks great! I'm curious why you chose AsyncLocal<> over ThreadLocal<> when the relevant code is not async.

Good question. It was meant to be future-proofing. AsyncLocal should be full superset of ThreadLocal in regards of corectness guarantee, and I wouldn't expect much observable perf difference.
But those are just my personal feelings - so let's rather test this:

        public class AsyncLocalTest
        {
            private readonly AsyncLocal<int> _al = new AsyncLocal<int>();
            private readonly ThreadLocal<int> _tl = new ThreadLocal<int>();
            private readonly TimeSpan[] _spans;

            private AsyncLocalTest(int threadsCount)
            {
                _spans = new TimeSpan[threadsCount];
            }

            public static void Test(int threadsCount, int repetitions, bool useAsyncLocal)
            {
                AsyncLocalTest test = new(threadsCount);

                Action<int> valueSetter = useAsyncLocal ? (i => test._al.Value = i) : (i => test._tl.Value = i);
                Func<int> valueGetter = useAsyncLocal ? (() => test._al.Value) : (() => test._tl.Value);

                //
                // Warmup
                //
                test.TestInternal(0, 2, 0, valueSetter, valueGetter);

                //
                // Nullify timestamps
                //
                for (int i = 0; i < threadsCount; i++)
                {
                    test._spans[i] = TimeSpan.Zero;
                }

                //
                // Run test in parallel
                //
                Random r = new Random();
                Thread[] ts = Enumerable.Range(0, 16).Select(i =>
                    new Thread(() => test.TestInternal(r.Next(), repetitions, i, valueSetter, valueGetter))).ToArray();

                foreach (Thread thread in ts)
                {
                    thread.Start();
                }

                foreach (Thread thread in ts)
                {
                    thread.Join();
                }

                //
                // Results
                //
                Console.WriteLine($"====== {(useAsyncLocal ? "Async" : "Thread")} local: =========");
                foreach (TimeSpan span in test._spans)
                {
                    Console.WriteLine(span);
                }
            }

            private void TestInternal(int start, int repetitions, int idx, Action<int> valueSetter, Func<int> valueGetter)
            {
                if (start > int.MaxValue - repetitions)
                {
                    start = int.MaxValue - repetitions;
                }

                valueSetter(start);

                Stopwatch sw = Stopwatch.StartNew();
                for (int i = start; i < start + repetitions; i++)
                {
                    // Test correctness
                    if (valueGetter() != i)
                    {
                        throw new Exception();
                    }

                    valueSetter(i + 1);
                }
                sw.Stop();
                _spans[idx] = sw.Elapsed;
            }
        }

When run as (on 20 logical cores):

    AsyncLocalTest.Test(16, 1000000, useAsyncLocal:true);
    AsyncLocalTest.Test(16, 1000000, useAsyncLocal:false);

it doesn't crash (so both are giving needed guarantee in simple threading scenario) and gives following output:

====== Async local: =========
00:00:00.2879965
00:00:00.3978832
00:00:00.4252494
00:00:00.3571488
00:00:00.2553354
00:00:00.4321759
00:00:00.3594426
00:00:00.1722112
00:00:00.3180074
00:00:00.1307697
00:00:00.2985377
00:00:00.2280112
00:00:00.2669700
00:00:00.2654464
00:00:00.3281217
00:00:00.3372726
====== Thread local: =========
00:00:00.0285052
00:00:00.3106148
00:00:00.2867749
00:00:00.4214141
00:00:00.0598388
00:00:00.2942306
00:00:00.2251675
00:00:00.4182183
00:00:00.3176023
00:00:00.2683520
00:00:00.2399414
00:00:00.0855781
00:00:00.2897219
00:00:00.0877046
00:00:00.0506610
00:00:00.1883100

I'd conclude that for our purposes we can call them to be equally performant (well below any level of being able to impact overall perf numbers)

[JanKrivanek]

Is there a way to add some test coverage for this?

Tests added