Don't emit IsReadOnlyAttribute if not available.

[nojaf]

Fixes #14275

I'm not sure how to add a test for this fix.
I believe IsReadOnlyAttribute is available for both net472 and net6.0. Only when targeting netstandard2.0 or lower can you come across the situation where it is not present.
I'm not sure how to add a test for that exact condition.

I was able to compile my [repro]
(https://github.com/nojaf/serialize-struct) with the local compiler and the test now passes.

[nojaf]

Some proof this worked on my machine:

Signed and sealed in blood,

Florian

[NinoFloris]

@nojaf I don't think this is the right way to go.

Roslyn instead synthesizes an internal attribute if the tfm does not have it. This allows compilers to still apply the same optimizations for apps that have dependencies compiled for high compatibility tfms like ns2.0

[vzarytovskii]

@nojaf I don't think this is the right way to go.

Roslyn instead synthesizes an internal attribute if the tfm does not have it. This allows compilers to still apply the same optimizations for apps that have dependencies compiled for high compatibility tfms like ns2.0

I don't think we do that. We usually check for runtime presence instead.

[NinoFloris]

I don't think we do that. We usually check for runtime presence instead.

Sure you'd only add the internal attribute if the presence check fails.

I don't see why a ns2.0 library should have its consumers lose out on perf benefits just because its tfm lacks the attributes to convey the actual applicable semantics.

In the end it's just my opinion, do with the knowledge what you wish, but I at least wanted to mention the approach roslyn takes here.

[vzarytovskii]

@dsyme @KevinRansom wdyt about the codegen missing attributes here?

[NinoFloris]

To add a bit of extra context, Roslyn does this only in specific cases. It doesn't do so for features that are explicitly not supported on older tfms like non nullable reference types. There you have to manually add the attributes if you turn on the feature and use it, as it will produce a compiler error otherwise.

C# 11 SetsRequiredMembersAttribute and UnscopedRefAttribute are good examples, as they fail to compile when you target net6.0 tfms as well. On the flip side C# 11 did start to add RefSafetyRulesAttribute by default. See dotnet/runtime#76032 (comment) for a bit of rationale when C# emits by default or not.

[github-actions]

Failed to run : https://github.com/dotnet/fsharp/actions/runs/3429398593

[vzarytovskii]

Ok, I vote we merge this one as is, since it's breaking a bunch of things, and have a follow-up item for generating a stub for it.

@NinoFloris @dotnet/fsharp-team-msft what do you say?

[NinoFloris]

Sounds good to me!

[vzarytovskii]

/backport to release/dev17.4

[github-actions]

Started backporting to release/dev17.4: https://github.com/dotnet/fsharp/actions/runs/3430274253

[SamuelBerger]

We cannot upgrade to .NET 7 because of emitted IsReadOnlyAttribute in netstandard2.0 projects, tried with latest VS preview containing SDK 7.0.200-preview.22628.1 and 7.0.300-preview.23106.2

The funny thing is: the attribute is only emitted when the package System.Collections.Immutable is referenced inside the project.

An updated version of @nojaf repro having the System.Collections.Immutable package referenced https://github.com/SamuelBerger/serialize-struct

@vzarytovskii is this fixed included in 7.0.200-preview.22628.1 and/or 7.0.300-preview.23106.2?

[vzarytovskii]

We cannot upgrade to .NET 7 because of emitted IsReadOnlyAttribute in netstandard2.0 projects, tried with latest VS preview containing SDK 7.0.200-preview.22628.1 and 7.0.300-preview.23106.2

The funny thing is: the attribute is only emitted when the package System.Collections.Immutable is referenced inside the project.

An updated version of @nojaf repro having the System.Collections.Immutable package referenced SamuelBerger/serialize-struct

@vzarytovskii Vlad Zarytovskii FTE is this fixed included in 7.0.200-preview.22628.1 and/or 7.0.300-preview.23106.2?

Hm, this should be included in the 7.0.200 release and all previews release after Nov 14, 2022.
I am not sure when those preview were released.

[SamuelBerger]

But can you reproduce the issue? (System.TypeLoadException : Could not load type 'System.Runtime.CompilerServices.IsReadOnlyAttribute' from assembly 'System.Collections.Immutable, Version=7.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a')

Or is it a weirdness on my side?

[vzarytovskii]

Ok, it seems is is a separate bug, since System.Collections.Immutable inludes its own IsReadOnlyAttribute:

And we are checking it only by namespace, we decide to emit it but not generate attribute.

I will revert generating it altogether, until proper fix is in place (with codegen).

[vzarytovskii]

A workaround would be to have it in your project (create a namespace S.R.CS with IsReadOnlyAttribute inside).

[vzarytovskii]

@0101 fyi, I will be reverting it alltogether

[SamuelBerger]

When adding:

namespace System.Runtime.CompilerServices 

[<System.AttributeUsage(System.AttributeTargets.All, Inherited=false)>]
[<Sealed>]
type IsReadOnlyAttribute() =
    inherit System.Attribute()

to the project containing the struct, it still picks up the one from System.Collections.Immutable:

.method public hidebysig specialname instance int32
  get_StartLine() cil managed
{
  .custom instance void [System.Collections.Immutable]System.Runtime.CompilerServices.IsReadOnlyAttribute::.ctor()
    = (01 00 00 00 )
  .custom instance void [netstandard]System.Runtime.CompilerServices.CompilerGeneratedAttribute::.ctor()
    = (01 00 00 00 )
  .custom instance void [netstandard]System.Diagnostics.DebuggerNonUserCodeAttribute::.ctor()
    = (01 00 00 00 )
  .maxstack 8

  IL_0000: ldarg.0      // this
  IL_0001: ldfld        int32 FSharpLibrary.FRange::StartLine@
  IL_0006: ret

} // end of method FRange::get_StartLine

But never mind, we will simply wait with the .NET 7 upgrade.

[vzarytovskii]

When adding:

namespace System.Runtime.CompilerServices 

[<System.AttributeUsage(System.AttributeTargets.All, Inherited=false)>]
[<Sealed>]
type IsReadOnlyAttribute() =
    inherit System.Attribute()

to the project containing the struct, it still picks up the one from System.Collections.Immutable:

.method public hidebysig specialname instance int32
  get_StartLine() cil managed
{
  .custom instance void [System.Collections.Immutable]System.Runtime.CompilerServices.IsReadOnlyAttribute::.ctor()
    = (01 00 00 00 )
  .custom instance void [netstandard]System.Runtime.CompilerServices.CompilerGeneratedAttribute::.ctor()
    = (01 00 00 00 )
  .custom instance void [netstandard]System.Diagnostics.DebuggerNonUserCodeAttribute::.ctor()
    = (01 00 00 00 )
  .maxstack 8

  IL_0000: ldarg.0      // this
  IL_0001: ldfld        int32 FSharpLibrary.FRange::StartLine@
  IL_0006: ret

} // end of method FRange::get_StartLine

But never mind, we will simply wait with the .NET 7 upgrade.

Ugh, that's unexpected, it probably starts searching from the "top" and references are going first.
That's annoying,