We should compare a performance type.IsPrimitive and type.IsEnum vs IsDefine() before remove old code.

The custom attribute targets old runtimes.
It is for edge scenarios to target old runtimes with modern compiler.
It will lose value over time.
So the addition could only have value for Windows PowerShell but not for PowerShell Core and we can remove it.

Binary modules targeting netstandard2.0 will still generate the custom attribute as the compiler can't be sure the attribute will be present when the DLL is loaded.

At what point are we considered to have completely dropped support for netstandard2.0 modules? Doing so effectively means we no longer support cross-compatibility with WinPS modules at all, and module authors have to pick either 5.1 or the new version to ship modules for.

Doing so effectively means we no longer support cross-compatibility with WinPS modules at all

No :-) In this case, we do not need to look back. What is current status of the code?

• no security issues
• no performance issues
• no functional issues (do you know real broken scenarios? I think they don’t exist at all because it’s all for performance)

So only our intention is to benefit from modern runtime to reduce false positives.
I think "world wide plan" is to move community to .Net 6.0 LTS. That means Netstandard2.1. In any case, the old code will decrease over time, and if we did not address (the performance) it earlier, then it does not make sense to address it to the future paying with performance due to new code without an urgent need.

Not sure I follow you through to the conclusion there. Are you saying we should consciously break the compatibility with netstandard2.0 here and now, or are you saying we should leave behind the old, ineffective code as a fallback?

If you're saying it's not worth changing at all... I don't really agree. PS has to handle mutable from immutable structs a bit differently in a few cases that I see in the code. Being able to yield less false positives from this method should (in theory) allow the engine to take the more efficient code path more often -- though, certainly, the cases where we need to be working with structs in PS directly aren't that common.

Are you saying we should consciously break the compatibility with netstandard2.0 here and now, or are you saying we should leave behind the old, ineffective code as a fallback?

We can not break "compatibility with netstandard2.0" because the method is only for performance.
Yes, I ask you do not see back because it makes no sense - we have not real issues and real scenario to address and measure - measure it mandatory for performance changes. False positives on the old way will be reduced over time by themselves.

What does the method return for the type before and after the change?

If the method was returning true in this case before this change, then the method is actually wrong. That's part of why I made this PR, because the assumptions this method was using are very flawed. We can pretty easily verify that datetime is immutable:

None of its fields can be modified; it's not mutable. Any methods on the type simply return a new instance, and don't modify the original. The fact that it's decorated with the appropriate compiler attribute also confirms this.

If we have code that is relying on these false assumptions somehow, I'm happy to fix that too. I intend to investigate the CI failures tonight.

By your own comments, though, this method should only be used for optimizations. I doubt the CI failure is actually caused by relying on the logic, but I will verify.

CIs fail on this. To save your time - it is Datetime.

The only thing that is going to change with datetime is that when you assign one to a variable it will be done faster.

The current behavior is that it will be detected as mutable and a pointless copy will be done to ensure changes aren't persisted. But it's immutable, there will be no changes, so no copy is required.

@vexx32 Please revert the questionable change. The PR will be merged for non-LTS version and will affecte most of users only after next PowerShell LTS - in the time this will be not actual I believe.

What is a private field is not readonly but a property (used the field) is?
I think we should revert the code and enhance a code for methods below to take into account new readonly methods.

The private field can still mutate, e.g., if the type provides a method that modifies the private field. Without checking all the fields, we cannot be sure the struct is immutable.

Also, even if we could check it more heuristically like you suggest, it's likely that doing more complex logic would be more expensive, since we'd need to make several different checks and verify a wider scope of things than just checking all the fields.

Please re-read my counterexample:

What is a private field is not readonly but a property (used the field) is?

using System;
struct S
{
    private int a;
    S(int p)
    {
        a = 1;
    }
    public int GetInt => a;
    public readonly int GetIntMethod()
    {
        // we could a = 2 if not readonly
        return a;
    }
}

You code return true but must false. See then compare with private readonly int a;.

Also the method is all for performance so we shouldn't address all rare cases. We should only reduce false positives in original code.

even if we could check it more heuristically like you suggest

I mean new C# 8.0 feature https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/builtin-types/struct#readonly-instance-members

Right, but that's a limited case, and doesn't account for (as an example) a method being provided that mutates the private field. We don't have a way to ensure that a settable field is not being set, so it will reduce the false positives more effectively to simply check for settable fields. Relying on the attribute is good but will still result in false negatives (the method returning false when the value type is in fact mutable) which I think we should definitely avoid.

I'm not sure I necessarily agree with your reasoning that it's only worth improving in certain ways. If we choose to compare lists of properties and methods as well as fields, in many cases we will end up needing to make more reflection calls than only iterating the fields.

but that's a limited case

Whole the method is limited case because it is only for value types and most of calls will fast return on first condition type.IsPrimitive.
But it is BINDER code that is very-very sensitive and we need strong motivation to change it. Currently no use case exists we should address and no reliable tests and measures too.

Right, that's why the current flawed assumptions are concerning.

But if it helps, I can add a case for primitives/enums back in, it most likely would short-circuit things a bit quicker.

Added the short-circuiting for enums and primitives back in and tidied things up a little bit.