Skip to content
This repository was archived by the owner on Sep 13, 2022. It is now read-only.

Add private fields for structs#1012

Merged
terrajobst merged 1 commit intodotnet:masterfrom
terrajobst:struct-fields
Dec 12, 2018
Merged

Add private fields for structs#1012
terrajobst merged 1 commit intodotnet:masterfrom
terrajobst:struct-fields

Conversation

@terrajobst
Copy link
Copy Markdown

@terrajobst terrajobst commented Dec 12, 2018

No public API changes, but this fixes #678.

In a nutshell, the compiler needs to know whether a struct has any fields in order to apply definitive assignment rules. While stripping all private fields from types is generally OK, we can't do this for structs. Fortunately, for private fields the compiler doesn't really care what they are, but what their characteristics are. For example:

  1. Does the struct have any fields?

  2. Does the struct contain any reference types (to validate generic instantiations that have the unmanaged constraint)?

  3. Does the struct use the generic parameter in a field declaration (to validate cyclic layout problems)?

This adds dummy fields to structs to conform to these rules. These aren't computed separately but are instead taken from .NET Core.

For more details, see this issue in CoreFX:

https://github.com/dotnet/corefx/issues/6185

@terrajobst terrajobst requested review from a team as code owners December 12, 2018 18:33
@terrajobst terrajobst added the * NO MERGE * Applied to pull-requests that aren't ready to be merged yet. label Dec 12, 2018
@terrajobst
Copy link
Copy Markdown
Author

@ericstj @jaredpar please make sure this makes sense to you.

@terrajobst terrajobst added this to the .NET Standard 2.1 milestone Dec 12, 2018
@ericstj
Copy link
Copy Markdown
Member

ericstj commented Dec 12, 2018

Do you have tooling to generate these files? GenAPI should do the right thing now and I trust that more than my eyes to review the sources (though I do trust @jkotas's 😄 )

@terrajobst
Copy link
Copy Markdown
Author

terrajobst commented Dec 12, 2018

Do you have tooling to generate these files?

You mean the reference assembly? I've used API Reviewer to export the .NET Core 3.0 reference assemblies into a one-file-per-namespace and that's what I diff against .NET Standard using Beyond Compare. So the private field information is taken from the .NET Core.

@terrajobst terrajobst changed the base branch from reflection-update to master December 12, 2018 18:55
@terrajobst terrajobst removed the * NO MERGE * Applied to pull-requests that aren't ready to be merged yet. label Dec 12, 2018
@ericstj
Copy link
Copy Markdown
Member

ericstj commented Dec 12, 2018

I see, so you're relying on us getting it right in the .NETCore references. I was chatting with @ahsonkhan the other day that we were missing an APICompat rule to keep those _private fields up to date. As a result we'll probably want to do another pass on this before shipping. I've opened dotnet/arcade#1560 to track fixing API Compat.

@terrajobst
Copy link
Copy Markdown
Author

terrajobst commented Dec 12, 2018

We were missing an APICompat rule to keep those _private fields up to date. As a result we'll probably want to do another pass on this before shipping.

Sounds good to me. Was just thinking about how we enforce that a given struct conforms to the characteristic and I was doubting that APICompat did this already. Glad to hear it will be taken care of.

@ericstj, any reason we shouldn't merge this iteration as a down payment?

This fixes dotnet#678. In a nutshell, the compiler needs to know whether a struct
has any fields in order to apply definitive assignment rules. While stripping
all private fields from types is generally OK, we can't do this for structs.
Fortunately, for private fields the compiler doesn't really care what they
are, but what their characteristics are. For example:

1. Does the struct have any fields?

2. Does the struct contain any reference types (to validate generic
   instantiations that have the unmanaged constraint)?

3. Does the struct use the generic parameter in a field declaration (to
   validate cyclic layout problems)?

This adds dummy fields to structs to conform to these rules. These aren't
computed separately but are instead taken from .NET Core.

For more details, see this issue in CoreFX:

https://github.com/dotnet/corefx/issues/6185
@ericstj
Copy link
Copy Markdown
Member

ericstj commented Dec 12, 2018

So long as you can repeat it reasonably well in the future I'm fine with that.

@jaredpar
Copy link
Copy Markdown
Member

Such a _dummy change 😉

@terrajobst
Copy link
Copy Markdown
Author

OK, given that this isn't publicly facing I'm not going to wait for @dotnet/nsboard-unity and @dotnet/nsboard-xamarin. If you have concerns, let me know :-)

@terrajobst terrajobst merged commit 310aef2 into dotnet:master Dec 12, 2018
@terrajobst terrajobst deleted the struct-fields branch December 12, 2018 23:16
@sharwell
Copy link
Copy Markdown

sharwell commented Jun 9, 2019

@terrajobst I'm having trouble finding the netstandard 2.0 release that contains these fields. The lack of the fields is causing the Make Field Readonly refactoring in Visual Studio to silently break code in Roslyn.

@terrajobst
Copy link
Copy Markdown
Author

We didn't do it in time for .NET Standard 2.0. This change is only in 2.1.

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.

Add private fields information to netstandard ref

6 participants