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

ref readonly returns should not return references to temps when used with PEVerifyCompat flag #22721

Closed
VSadov opened this issue Oct 16, 2017 · 3 comments

Comments

@VSadov
Copy link
Member

VSadov commented Oct 16, 2017

PEVerifyCompat forces readonly references of readonly fields be ftched off a copy (since direct references are not allowed by PEVerify).

The whole point of a ref return, however, is to return a direct reference.

We should not be returning references to temps in cases which specifically demand references such as ref returns and ref assignments, even if that causes PEverify failure, since returning a reference to a temp completely misses the point.

See testcase ReadonlyFieldCanReturnByRefReadonly as an example.

@VSadov VSadov added this to the 15.later milestone Oct 16, 2017
@VSadov VSadov self-assigned this Oct 16, 2017
@VSadov VSadov changed the title ref readonly returns may return references to temps when used with PEVerifyCompat flag ref readonly returns should not return references to temps when used with PEVerifyCompat flag Oct 16, 2017
@jcouv
Copy link
Member

jcouv commented Oct 17, 2017

I don't understand.
I thought the purpose of the flag was to avoid a compat break on existing code. We have introduced other features that are not yet PE-verifiable, without trying to implement the compat flag for them.

@VSadov
Copy link
Member Author

VSadov commented Oct 17, 2017

The flag cannot help this particular case since the scenario cannot peverify. That would be ok since it is a new feature that cannot be found in old code.

What is not ok is that we still try try honoring the flag and as a result produce bad code.

@VSadov
Copy link
Member Author

VSadov commented Nov 9, 2017

Fixed in #22772

@VSadov VSadov closed this as completed Nov 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants