Could you please elaborate what exactly is wrong with copying the structure? Is copy or original value going to misbehave in some way?

I am fine with the changes made inside the MetadataTypeName type. However, I don't want to be prevented from making copies of this structure or having to deal with build breaks caused by an analyzer complaining about it.

Done with review pass (iteration 1)

Could you please elaborate what exactly is wrong with copying the structure?

We recently did a performance investigation which revealed a large number of allocations (10+GB) in calls to MetadataTypeName.NamespaceSegments. This is a lazily-computed value which is persisted to a mutable value type, so part of the investigation time involved determining if the code was operating on the "original" instance of this type or a copy (where the latter would fail to persist the lazily-computed value such that subsequent calls return the cached copy).

Usage of the type in the current code base suggests that copies of this value type have been carefully avoided, likely for this reason. If copies are being carefully avoided, analyzer assistance may help developers use this type in the expected manner over time.

What effect is this going to have?

An analyzer will report a warning if a copy of an instance of this type is made (as opposed to moving the instance, which is safe). The attribute eliminates the need to perform the investigation described previously (in fact, the evaluation was actually performed by applying this attribute and seeing what happened).

Current usage of this type produced one true positive report from this analyzer, and zero false positives. The analyzer is good, but not perfect, so it is possible that some future use would report a false positive that needs to be suppressed or fixed.

Is my observation correct that the changes made didn't actually eliminate any allocations?

@AlekseyTs That is correct. The changes only provide compile-time verification of a condition which was already occurring.

This change is completely optional. The attribute helped with the investigation I was doing, but that was true even when the attribute wasn't already present (I added it, and then reviewed the results). If you feel it would be valuable for maintaining a condition we can merge it, otherwise it remains something developers are responsible for.

If you think it might be valuable but are concerned about the maintainability over time, we can also merge it now and it can be reverted if it's ever a problem.

@dotnet/roslyn-compiler if you ever run into problems with this analyzer, please feel free to reach out for me to fix the analyzer and/or revert this change (or rather, the use of the attribute since the readonly modifiers were valid either way 😄 ).

as opposed to moving the instance, which is safe

What does a move look like? Basically what code do we type to perform a move that will be recognized by the analyzer?

The most common moves are assigning a return value to a local, or returning a local from a method. These are already recognized. Flow analysis for other cases is high on my list of things I want for this analyzer. In the meantime, the following works if you end up needing it:

internal static class NonCopyableExtensions
{
    public static T Move<T>(this ref T value)
            where T : struct
    {
        T result = value;
#if DEBUG
        value = default;
#endif
        return result;
    }
}

The other case that's a bit weird is move to box. Boxing a value is treated as an allowed move, but once the value is in the box you can't move it back out since other code might be referencing the same location. For this case, you can use Unsafe.Unbox<T> to get a typed reference to the boxed value.