Skip to content
This repository was archived by the owner on Nov 1, 2020. It is now read-only.

Fix public surface to match CoreCLR#5238

Merged
jkotas merged 2 commits into
dotnet:masterfrom
jkotas:contract-fixes
Jan 12, 2018
Merged

Fix public surface to match CoreCLR#5238
jkotas merged 2 commits into
dotnet:masterfrom
jkotas:contract-fixes

Conversation

@jkotas
Copy link
Copy Markdown
Member

@jkotas jkotas commented Jan 12, 2018

No description provided.

@weshaggard
Copy link
Copy Markdown
Member

Any idea why TypedReference is still showing up as not ref in the ProjectN we are consuming in corefx?

@jkotas
Copy link
Copy Markdown
Member Author

jkotas commented Jan 12, 2018

@dotnet-bot test Ubuntu Debug and CoreCLR tests please
@dotnet-bot test Ubuntu Release please

@jkotas
Copy link
Copy Markdown
Member Author

jkotas commented Jan 12, 2018

It is expected that that TypedReference is ref in ProjectN CoreLib.

It is logical ref in CoreCLR CoreLib as well, except that it is not marked as such in metadata. The type loader is special casing it as ref for historic reasons.

@weshaggard
Copy link
Copy Markdown
Member

So you don't expect that we will never remove this baseline https://github.com/dotnet/corefx/blob/master/src/System.Runtime/src/ApiCompatBaseline.uapaot.txt#L1 because we cannot mark it as ref in the contract?

@jkotas
Copy link
Copy Markdown
Member Author

jkotas commented Jan 12, 2018

@dotnet/dnceng Is there a problem with the Ubuntu machine pool? This PR is waiting for Ubuntu machines for 15+ hours...

@mmitche
Copy link
Copy Markdown
Member

mmitche commented Jan 12, 2018

@jkotas We ran over the deployment limit somehow. I'm looking into it.

@jkotas
Copy link
Copy Markdown
Member Author

jkotas commented Jan 12, 2018

So you don't expect that we will never remove this baseline https://github.com/dotnet/corefx/blob/master/src/System.Runtime/src/ApiCompatBaseline.uapaot.txt#L1 because we cannot mark it as ref in the contract?

RuntimeArgumentHandle (and ArgIterator - not in .NET Core today) would be in the same category. It was missing the ref marker - I am fixing this here.

This compact hack has to be somewhere. The alternative would be to have it in the runtimes.

We need this compat hack because of the C# has not reconciled handling of these 3 legacy ref-like types with the new ref-like feature. So yet another alternative would be to do something about that, e.g. accept the source-breaking change in the few corner where it is causing problems for .NET Core and mark them ref-like in the .NET Core contracts.

cc @VSadov

@VSadov
Copy link
Copy Markdown
Member

VSadov commented Jan 12, 2018

C# compiler already treats "legacy" byref structs as ref structs, regardless if they are decorated with ref or not.

That is not a problem since the restrictions on those types were stronger than on the new ref structs. And in places where the constraints are stronger they are still enforced. (I.E. I think you cannot return them from a method or pass by ref at all, regardless of escape analysis).

The only place that we had to adjust is to not poison these types with Obsolete if they do become ref structs in the source, since poisoning would be breaking for existing types.
See - dotnet/roslyn#22275
This was done specifically so that you could make them ref in source, if you want to contain ByReference<T> which makes sense to do in coreclr.

I am not aware of anything else missing regarding handling of TypedReference and friends.

@VSadov
Copy link
Copy Markdown
Member

VSadov commented Jan 12, 2018

I.E. - I think it would be optional, but convenient for you to make these structs ref structs on all runtimes and that should not be breaking, since they are already treated as such.

Copy link
Copy Markdown
Member

@VSadov VSadov left a comment

Choose a reason for hiding this comment

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

LGTM

@VSadov
Copy link
Copy Markdown
Member

VSadov commented Jan 12, 2018

CC: @jaredpar

@jkotas
Copy link
Copy Markdown
Member Author

jkotas commented Jan 12, 2018

I.E. - I think it would be optional, but convenient for you to make these structs ref structs on all runtimes and that should not be breaking, since they are already treated as such.

Cool. I did not know that. So we should just mark them as ref in CoreCLR and contracts as well.

@jkotas jkotas merged commit 5d7dd3f into dotnet:master Jan 12, 2018
@jkotas jkotas deleted the contract-fixes branch January 16, 2018 03:00
am11 pushed a commit to am11/corert that referenced this pull request Jan 29, 2018
* Fix public surface to match CoreCLR

* Mark RuntimeArgumentHandle as ref
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants