Roslyn: Cannot change initonly field outside its .ctor regression

[marek-safar]

Version Used: 2.6.0.62126 (1758d50b)

Steps to Reproduce:

1. Compile following code

struct S
{
    public enum Type
    {
        Error
    }

    readonly Type type;

    public override string ToString ()
    {
        return type.ToString ();
    }

    public static void Main ()
    {
    }
}

1. peverify output.exe

Expected Behavior:

No error (as in previous releases)

Actual Behavior:

[jcouv]

@marek-safar Which previous version produced an error? Is it VS2017/2015 (Roslyn) or older (native compiler)?

[jcouv]

Tried on native compiler, and I only got a warning for un-assigned field:

> c:\Windows\Microsoft.NET\Framework\v4.0.30319\csc.exe D:\issues\21659\ConsoleApp\ConsoleApp\Program.cs
Microsoft (R) Visual C# Compiler version 4.7.2046.0
for C# 5
Copyright (C) Microsoft Corporation. All rights reserved.

This compiler is provided as part of the Microsoft (R) .NET Framework, but only supports language versions up to C# 5, which is no longer the latest version. For compilers that support newer versions of the C# programming language, see http://go.microsoft.com/fwlink/?LinkID=533240

d:\issues\21659\ConsoleApp\ConsoleApp\Program.cs(8,19): warning CS0649: Field 'S.type' is never assigned to, and will always have its default value 0

[jcouv]

@gafter Could you comment on expected behavior? Thanks

[marek-safar]

@jcouv this is codegen regression not compiler warning regression

[jcouv]

@marek-safar sorry, I failed to notice the PEVerify step.
Still, can you clarify which version passed?

[marek-safar]

I tried with Roslyn 2.3 and older and it worked. I guess this is related to the new readonly ref support somehow

[jaredpar]

This behavior is by design. There is nothing that violates type safety here and it is more efficient to avoid the copy for non mutating operations.

[tannergooding]

@jaredpar, didn't we add a flag (peverify-compat or something) that allows the compiler to emit the old code (in case peverify success matters for users like @marek-safar).

[marek-safar]

@jaredpar I agree but this is a regression for scenarios where peverify is involved

[jaredpar]

Yes it is /features:peverify-compat

[jaredpar]

@marek-safar can u elaborate on the scenarios? The runtime team is generally skeptical that PEVerify is in significant use. Hence why it is not keeping up with changes to the language. Scenarios will help us with decisions in this area going forward.

[marek-safar]

@jaredpar I have experience with it in testing only scenarios, e.g. we use it with Cecil (manipulates the IL) to verify the output IL in various test cases where input source code is C#. I agree this is not significantly important scenarios but it would be nice to keep peverify up-to-date.

[jaredpar]

@marek-safar one option we've been discussing is moving verification to a managed library. Makes it easier to update and use in testing scenarios. The latter is very interesting to the compiler team as well. Would that work for the Cecil scenarios?

Also how do you handle the scenarios that already exist for which PEVerify will fail? Most notably ref returns and netstandard.

[marek-safar]

@jaredpar that's something we just have to workaround as it's a new feature (e.g. ref-return) and not strictly speaking a regression.

[marek-safar]

@jaredpar thinking about it the managed library approach would work just fine for us but it's a lot of work on your side

[gafter]

Just to make sure I understand the situation with this issue:

This appears to be a functional regression whereby the compiler now produces nonverifiable code for source that used to result in verifiable code. However, we have added the flag /features:peverify-compat to support users for whom verification (of older code) is a requirement. Compiling this source with that flag should restore the old (verifiable) behavior. If that is not the case, please reopen.

[benaadams]

This behavior is by design. There is nothing that violates type safety here and it is more efficient to avoid the copy for non mutating operations.

Is this new behavior? (Very happy if so) Do the methods get annotated with non-modifying/Pure or similar?

[jaredpar]

It is related to the readonly struct work. Primitives like int, enum, etc ... are implicitly readonly. Those types we understand to be non-mutating (shallowly) and hence can avoid defensive copies.

[jaredpar]

In other words ... start marking your structs as readonly 😄

[marek-safar]

@jaredpar @gafter do you have updated verification spec for this behavior we could use to implement it in our verifier ?

[gafter]

@marek-safar I believe we have not provided a new verifier spec. Please open a new issue for that.

[marek-safar]

@gafter here or do you have a different repo for that?

[gafter]

@marek-safar Here to start, since the question is about what safety rules Roslyn generates code for.

[Suchiman]

@jaredpar

one option we've been discussing is moving verification to a managed library.

AFAIK this is already in progress here: https://github.com/dotnet/corert/tree/master/src/ILVerify

[jcouv]

@Suchiman Yes, that's indeed the effort that we're counting on.

[menees]

I started using VS 2017 update 15.7.0 yesterday. I switched a SQL CLR project to using latest (for C# 7.3), and I changed several member fields to use readonly per the IDE0044 recommendation. Then I started getting the "Cannot change initonly field outside its .ctor." error when trying to use SQL's CREATE ASSEMBLY command. Thankfully, I found this issue, and the fix was just adding peverify-compat into my .csproj file.

I wanted to post this here in case anyone else runs into SQL CLR errors with this like I did. My full error message was this:

System.Data.SqlClient.SqlException: CREATE ASSEMBLY for assembly 'XXX.Sql.Common' failed because assembly 'XXX.Sql.Common' failed verification. Check if the referenced assemblies are up-to-date and trusted (for external_access or unsafe) to execute in the database. CLR Verifier error messages if any will follow this message
[ : XXX.Data.XxxCommand::PostExecute][mdToken=0x6000361][offset 0x0000006E] Cannot change initonly field outside its .ctor.

FWIW, I'm using [assembly: AllowPartiallyTrustedCallers] per Microsoft's recommendation for SQL CLR assemblies. But after going to the C# 7.3 compiler, the peverify-compat feature is required.

I also wanted to mention this here since @jaredpar said above on Oct 4, 2017, "The runtime team is generally skeptical that PEVerify is in significant use." For anyone using SQL CLR with safe or external_access assemblies, we're stuck with PEVerify. We may not count as "significant use", but we don't want to be left out of using the latest language changes. :-)

[calzakk]

I just upgraded a SQLCLR (external_access) project to Visual Studio 2019, and started getting the same error: "Cannot change initonly field outside its .ctor."

We have a lot of readonly variables, so I didn't want to go changing all this code. But then I came across the post from @bmenees about C# 7.3.

We weren't actually specifying a C# version, so I set the project to use C# 6.0 (the highest I could set other than 'default') and our stored procedures can now be deployed successfully. But until this is fixed, we're now prevented from using newer versions of C#.

[jaredpar]

@calzakk

But until this is fixed, we're now prevented from using newer versions of C#.

The particular error your getting here indicates you are using a partial trust environment on .NET Framework. That environment is not compatible with more recent versions of C# because the runtime is not updated to handle the new IL patterns that are emitted.

C# 7.0 should work here without modification. C# 7.3 can work if you pass /features:peverify-compat and avoid new features that specifically generate new IL patterns such as readonly struct.

C# 8.0 and later is only officially supported on .NET Core though.