Added inref immutability assumption removal

[TIHan]

This is to resolve: #7406

Gets rid of the immutability assumption for .NET structs when it's an inref.

Added awareness of IsReadOnlyAttribute on method calls from structs, only aware of IL method calls, not F# method calls. This is not a feature, but merely an optimization and will help mitigate extra warnings as a result of the immutability assumption change.

• Find out if we can optimize the attribute lookup? Simple query, will only check on structs and when we must take an address.
• Tests

[TIHan]

Almost done. Just need a few additional checks for extensions.

[TIHan]

If tests pass, then this is finished.

[dsyme]

Hi @TIHan could you review these before proceeding with this?

• Optimisation failure with structs in referenced projects #3923
• [F# 5.x] better warnings on mutable struct #3968 #4576 (old pull request, may have bit-rotted)

I believe it's the same root issue, where .NET structs are assumed to be immutable. And you're fixing this for the inref case, which seems reasonable as it is essentially new, and also fixing the other issues around ReadOnly attributes etc.

The code and tests look good, I really just want to double check that this is not impacting the performance of existing code that doesn't use inref, and that we're thinking carefully about the impact of performance of code that does use inref on .NET struct types

[TIHan]

Yea, I can do another review pass that them. I actually looked at both of those on Friday and it did help me. It might be the same root issue.

The core of both the non-inref and inref rules are here (as of current changes):

let isRecdOrStructTyconRefReadOnlyAux g m isInref (tcref: TyconRef) =
    if isInref && tcref.IsILStructOrEnumTycon then
        isTyconRefReadOnly g m tcref
    else
        isTyconRefReadOnly g m tcref || isTyconRefAssumedReadOnly g tcref

I agree that we really need to be careful on existing code that does not use inref. Just in case, I'll add tests that will execute both the non-inref and inref versions to show expected behavior; probably in fsfromfsviacs.

Concerning performance, existing inref code will see an impact if they are using .NET structs that are not marked as read-only, or if the method/property they are calling is not marked read-only. NETCore 2.1+ marked a lot of types as read-only, such as DateTime, Int, etc. In NETCore 3.0, methods are starting to be marked as read-only, such as the System.Numerics Vector types' methods (thanks @tannergooding). With the C# 8 compiler, it will start automatically marking properties as read-only if it only has an automatic getter, no setter. It will even mark them in lower C# lang versions as it is really considered a performance optimization. Based on this, I'm not worried about the performance implications of inref moving forward.

Another thing that we should be doing, is disallowing [<IsReadOnly>] attribute to be put on methods. I'm not going to make that part of this PR though

[TIHan]

Ok, I added tests. I was still able to reproduce @saul 's issue, but it didn't happen on the inref version. I'll look into it since I'm already doing this work.

[TIHan]

Ok @dsyme , I believe these changes do not affect non-inref behavior. I added a test to verify the current behavior that matches @saul 's behavior.

Whenever we resolve #3923 then the test will fail.

[TIHan]

Thank you all for reviewing. Glad to see this get in.