Skip to content
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

Allow taking unmanaged/pinned addresses of readonly lvalues. #22400

Merged
merged 4 commits into from
Sep 29, 2017

Conversation

VSadov
Copy link
Member

@VSadov VSadov commented Sep 28, 2017

Allow taking unmanaged/pinned addresses of readonly variables.

Fixes:#22306

Customer scenario

Historically we require that lvalue is writable before allowing taking a pointer.
While there are ways to "cast away" readonliness, why is that even required before "casting away" most of the type system safeguards anyways?

After introducing more readonly variables in the language, the limitation becomes more apparent. - In anticipation of unsafe use, API creators avoid readonly even for safe scenarios. - Not a desirable outcome.

See for example
https://github.com/dotnet/corefx/blob/fa27201badbbce3cdb752d52593493a60c2f7d34/src/System.Memory/src/System/ReadOnlySpan.cs#L309

public ref T DangerousGetPinnableReference() on ReadOnlySpan<T> returns an ordinary reference to the first element (on an otherwise readonly data type).
It could instead return a readonly ref, but that would inconvenience the users since the main purpose is to use this method in “fixed” statement.

@tannergooding
Copy link
Member

@VSadov, does this also allow:

public class MyClass
{
    private static readonly Guid MyGuid = new Guid("");

    public void MyMethod()
    {
        fixed (Guid* pMyGuid = &MyGuid)
        {
        }
    }
}

@VSadov
Copy link
Member Author

VSadov commented Sep 28, 2017

@tannergooding - yes. That is the point.

You do not need to jump through hoops like:

public class MyClass
{
    private static readonly Guid MyGuid = new Guid("");

    public void MyMethod()
    {
        fixed (Guid* pMyGuid = &System.Runtime.CompilerServices.Unsafe.AsRef(in MyGuid))
        {
        }
    }
}

@tannergooding
Copy link
Member

Awesome! (I assumed it would, I just wanted to make sure since the title indicated variables, which I assumed to mean locals, rather than also including fields).

@VSadov
Copy link
Member Author

VSadov commented Sep 28, 2017

"readonly variables" - to mean generally an lvalue that is readonly.
That includes fields, locals, ref readonly methods/indexers, etc...

@VSadov VSadov changed the title Allow taking unmanaged/pinned addresses of readonly variables. Allow taking unmanaged/pinned addresses of readonly lvalues. Sep 28, 2017
@VSadov
Copy link
Member Author

VSadov commented Sep 28, 2017

@dotnet/roslyn-compiler - please review

@jcouv
Copy link
Member

jcouv commented Sep 28, 2017

        int **j = &i;  // CS0459

The error code comments can be removed. #Pending


Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/SemanticErrorTests.cs:10270 in 7cfa15e. [](commit_id = 7cfa15e04778b2abc200a5383891b110e528d06a, deletion_comment = False)

@VSadov
Copy link
Member Author

VSadov commented Sep 28, 2017

CC: @jkotas - FYI

@VSadov
Copy link
Member Author

VSadov commented Sep 28, 2017

        int **j = &i;  // CS0459

Perhaps the error itself can be removed too.


In reply to: 332912171 [](ancestors = 332912171)


Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/SemanticErrorTests.cs:10270 in 7cfa15e. [](commit_id = 7cfa15e04778b2abc200a5383891b110e528d06a, deletion_comment = False)

Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM and approved for 15.5 pending another review.

Copy link
Member

@agocke agocke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

}");
}";

var comp = CompileAndVerify(source, parseOptions: TestOptions.Regular.WithPEVerifyCompatFeature());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this something we're just keeping around until the verifier is updated?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. A last-resort workaround if compatibility with PEVerify is needed. - It does not always work with new features,as some features by definition do not conform with PEVerify, but at least for old features it can be used.


In reply to: 141783389 [](ancestors = 141783389)

IL_0007: ldloca.s V_0
IL_0009: pop
IL_000a: ret
IL_0005: pop
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice

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

Successfully merging this pull request may close these issues.

Allow taking an unmanaged pointer off readonly lvalues
5 participants