New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mark applicable structs as readonly #24997

Merged
merged 2 commits into from Nov 3, 2017

Conversation

Projects
None yet
5 participants
@stephentoub
Member

stephentoub commented Nov 1, 2017

Fixes #24900
Related to dotnet/coreclr#14789
Related to dotnet/corert#4855

cc: @jkotas, @jaredpar, @VSadov, @tmat for System.Reflection.Metadata (which has the bulk of the changes)

@marek-safar

This comment has been minimized.

Show comment
Hide comment
@marek-safar

marek-safar Nov 1, 2017

Contributor

I am wondering if this is potentially a breaking change. The runtimes don't ensure that the struct is read-only but C# compiler assumes that. Therefore in a scenario where read-only struct is used in pinvoke and actually modified are we sure the side effects are always visible?

Contributor

marek-safar commented Nov 1, 2017

I am wondering if this is potentially a breaking change. The runtimes don't ensure that the struct is read-only but C# compiler assumes that. Therefore in a scenario where read-only struct is used in pinvoke and actually modified are we sure the side effects are always visible?

@stephentoub

This comment has been minimized.

Show comment
Hide comment
@stephentoub

stephentoub Nov 1, 2017

Member

I am wondering if this is potentially a breaking change. The runtimes don't ensure that the struct is read-only but C# compiler assumes that. Therefore in a scenario where read-only struct is used in pinvoke and actually modified are we sure the side effects are always visible?

Can you provide an example?
cc: @jaredpar, @VSadov

Member

stephentoub commented Nov 1, 2017

I am wondering if this is potentially a breaking change. The runtimes don't ensure that the struct is read-only but C# compiler assumes that. Therefore in a scenario where read-only struct is used in pinvoke and actually modified are we sure the side effects are always visible?

Can you provide an example?
cc: @jaredpar, @VSadov

@marek-safar

This comment has been minimized.

Show comment
Hide comment
@marek-safar

marek-safar Nov 2, 2017

Contributor

I don't right now but possibly something like this where you pass in readonly version instead of non-readonly version and runtime decides to optimize/flow it differently.

readonly struct Foo
{
	public readonly int foo;
}

class X
{
	[DllImport ("foo.dll")]
	static extern void Foo (ref Foo foo);
}
Contributor

marek-safar commented Nov 2, 2017

I don't right now but possibly something like this where you pass in readonly version instead of non-readonly version and runtime decides to optimize/flow it differently.

readonly struct Foo
{
	public readonly int foo;
}

class X
{
	[DllImport ("foo.dll")]
	static extern void Foo (ref Foo foo);
}
@stephentoub

This comment has been minimized.

Show comment
Hide comment
@stephentoub

stephentoub Nov 2, 2017

Member

possibly something like this

@jaredpar or @VSadov could likely answer better, but from my perspective, in your example you can already do that with readonly fields (just remove the 'readonly' from the struct itself), and it's not really any different from that. Interop is inherently unsafe and allows you to break out of whatever language/runtime enforcement exists.

Member

stephentoub commented Nov 2, 2017

possibly something like this

@jaredpar or @VSadov could likely answer better, but from my perspective, in your example you can already do that with readonly fields (just remove the 'readonly' from the struct itself), and it's not really any different from that. Interop is inherently unsafe and allows you to break out of whatever language/runtime enforcement exists.

@stephentoub

This comment has been minimized.

Show comment
Hide comment
@stephentoub

stephentoub Nov 2, 2017

Member

@dotnet-bot test Linux x64 Release Build please (#25004)

Member

stephentoub commented Nov 2, 2017

@dotnet-bot test Linux x64 Release Build please (#25004)

@jkotas

This comment has been minimized.

Show comment
Hide comment
@jkotas

jkotas Nov 2, 2017

Member

Also, you can change readonly fields via reflection today. Runtime is not enforcing readonliness for reflection access either.

C# readonly is pretty similar to C++ const in this sense. There are number of ways how one can step around it.

Member

jkotas commented Nov 2, 2017

Also, you can change readonly fields via reflection today. Runtime is not enforcing readonliness for reflection access either.

C# readonly is pretty similar to C++ const in this sense. There are number of ways how one can step around it.

@karelz karelz added the area-Meta label Nov 2, 2017

@stephentoub stephentoub merged commit 45b724f into dotnet:master Nov 3, 2017

13 checks passed

CROSS Check Build finished.
Details
Linux arm Release Build Build finished.
Details
Linux x64 Release Build Build finished.
Details
NETFX x86 Release Build Build finished.
Details
OSX x64 Debug Build Build finished.
Details
Packaging All Configurations x64 Debug Build Build finished.
Details
Tizen armel Debug Build Build finished.
Details
UWP CoreCLR x64 Debug Build Build finished.
Details
UWP NETNative x86 Release Build Build finished.
Details
WIP ready for review
Details
Windows x64 Debug Build Build finished.
Details
Windows x86 Release Build Build finished.
Details
license/cla All CLA requirements met.
Details

@stephentoub stephentoub deleted the stephentoub:readonly_structs branch Nov 3, 2017

@karelz karelz added this to the 2.1.0 milestone Nov 18, 2017

@VladimirReshetnikov

This comment has been minimized.

Show comment
Hide comment
@VladimirReshetnikov

VladimirReshetnikov Nov 23, 2017

Making System.Nullable<T> a readonly struct would be a breaking change. Consider the following program:

using System;

struct S
{
    int x;

    public override string ToString()
    {
        x = 5;
        return "";
    }

    static void Main()
    {
        S? s = new S();
        Console.WriteLine(s.Value.x); // 0
        s.ToString();
        Console.WriteLine(s.Value.x); // 5
    }
}

If System.Nullable<T> were a readonly struct, both lines would print 0.

VladimirReshetnikov commented Nov 23, 2017

Making System.Nullable<T> a readonly struct would be a breaking change. Consider the following program:

using System;

struct S
{
    int x;

    public override string ToString()
    {
        x = 5;
        return "";
    }

    static void Main()
    {
        S? s = new S();
        Console.WriteLine(s.Value.x); // 0
        s.ToString();
        Console.WriteLine(s.Value.x); // 5
    }
}

If System.Nullable<T> were a readonly struct, both lines would print 0.

@stephentoub

This comment has been minimized.

Show comment
Hide comment
@stephentoub

stephentoub Nov 23, 2017

Member

Thanks for calling this out, @VladimirReshetnikov. It's not so much making Nullable<T> readonly that's the problem, but rather changing its value field to be readonly (and then the type can't be readonly). I've put up a PR to revert it.

Member

stephentoub commented Nov 23, 2017

Thanks for calling this out, @VladimirReshetnikov. It's not so much making Nullable<T> readonly that's the problem, but rather changing its value field to be readonly (and then the type can't be readonly). I've put up a PR to revert it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment