Conversation
|
Wondering if this whould go under a feature flag, since technically it may change the codegen. |
|
I consider it more of a fix. Basically the current way does not fully understand what Roslyn can do with respect to the "readonly" keyword and possible placement of [ReadOnly] attribute. See example here - even placement of readonly keyword at the method level (which existing code is supposed to support) gets overriden by readonly on struct level, and therefore method does not get the [ReadOnly] by Roslyn. |
…y-on-all-structs-to-remove-unnecessary-copies
…y-on-all-structs-to-remove-unnecessary-copies
|
Wouldn't IL tests be more reliable? The warning could theoretically be broken (or its behavior changed), independent of whether or not the defense copy is emitted. |
|
This was the only part of the test suite that failed when adding this feature, that is why considered it. |
|
Just something simple like https://github.com/dotnet/fsharp/blob/main/tests/FSharp.Compiler.ComponentTests/EmittedIL/EmptyArray.fs, to show the instruction is present when expected, and missing when dealing with readonly structs. |
Thanks, this is the way. |
…y-on-all-structs-to-remove-unnecessary-copies
|
I do agree this can count as a bug fix Could you add test cases for extension methods on readonly structs please? |
Works. |

In C#, [Readonly]can be applied both on method level as well as struct level.
When [Readonly] is on entire struct, typically via "readonly struct" definition, semantically it is equivalent to having [Readonly] on every method. However, F# only reads [Readonly] at method level at the moment.
This PR changes the behaviour of IlMethodyInfo.IsReadOnly to also detect [Readonly] on struct level.
That way, the MethodCall will be understood by F# compiler as 'NeverMutates' and therefore enable performance optimization by avoiding copies.