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

Mark applicable structs as readonly #14789

Merged
merged 1 commit into from Nov 3, 2017

Conversation

Projects
None yet
8 participants
@stephentoub
Copy link
Member

stephentoub commented Nov 1, 2017

In a few known "why wasn't this annotated" cases (e.g. nullable), I added readonly to fields in order to allow readonly on the type.

Contributes to dotnet/corefx#24900

cc: @jaredpar, @VSadov, @jkotas

@jkotas
Copy link
Member

jkotas left a comment

We should make sure that the corefx contract consistency check start verifying the readonly annotations (if they are not doing it already). The contracts have to stay in sync with the implementation.

@@ -11,7 +11,7 @@ namespace System.Diagnostics.Tracing
/// <summary>
/// TraceLogging: Empty struct indicating no payload data.
/// </summary>
internal struct EmptyStruct
internal readonly struct EmptyStruct

This comment has been minimized.

Copy link
@jkotas

jkotas Nov 1, 2017

Member

@brianrob Are you ok with marking these as readonly - will it cause issues with source sharing?

This comment has been minimized.

Copy link
@stephentoub

stephentoub Nov 1, 2017

Author Member

This is also in the same boat as the other "empty" structs below. I'm fine keeping the changes for these and fine getting rid of them.

@@ -10,7 +10,7 @@
namespace System
{
// This class represents the void return type
public struct Void
public readonly struct Void

This comment has been minimized.

Copy link
@jkotas

jkotas Nov 1, 2017

Member

You can mark this readonly, but it has zero value here - just adds extra bytes to the binary. The marking on this type looks a bit odd to me.

I am wondering what the rule should be: mark structs as read only because of it makes senses (it would be my preference), or just because of we can?

This comment has been minimized.

Copy link
@stephentoub

stephentoub Nov 1, 2017

Author Member

Yeah, I went back and forth on that. There are a few types like this in a PR I'm about to put up in corefx as well. I could go either way.

This comment has been minimized.

Copy link
@danmosemsft

danmosemsft Nov 1, 2017

Member

Perhaps mark like public /* readonly */ struct Void so it's clear it was intentionally skipped.

This comment has been minimized.

Copy link
@stephentoub

stephentoub Nov 2, 2017

Author Member

Perhaps mark like public /* readonly */ struct Void so it's clear it was intentionally skipped.

Ok, I'll do that for internal types and leave public types annotated. Seems like a reasonable place to land.

EDIT: Actually, I'll just remove them from these no-member types. We can always add them later if desired.

@@ -19,7 +19,7 @@ internal static class PaddingHelpers

/// <summary>Padding structure used to minimize false sharing</summary>
[StructLayout(LayoutKind.Explicit, Size = PaddingHelpers.CACHE_LINE_SIZE - sizeof(int))]
internal struct PaddingFor32
internal readonly struct PaddingFor32

This comment has been minimized.

Copy link
@jkotas

jkotas Nov 1, 2017

Member

Same here.

@@ -53,7 +53,7 @@ namespace System
[StructLayout(LayoutKind.Auto)]
[Serializable]
[System.Runtime.CompilerServices.TypeForwardedFrom("mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089")]
public partial struct DateTime : IComparable, IFormattable, IConvertible, IComparable<DateTime>, IEquatable<DateTime>, ISerializable
public readonly partial struct DateTime : IComparable, IFormattable, IConvertible, IComparable<DateTime>, IEquatable<DateTime>, ISerializable

This comment has been minimized.

Copy link
@jkotas

jkotas Nov 1, 2017

Member

I see that you were not able to mark DateTimeOffset because of its internal implementation detail around binary serialization. It is pretty unfortunate. I am wondering whether the DateTimeOffset case would warrant using the Unsafe helper to cast away the readonly-liness, so that DateTimeOffset can be marked as well.

This comment has been minimized.

Copy link
@stephentoub

stephentoub Nov 1, 2017

Author Member

Exactly. I started looking at fixing it, but decided to keep this PR just focused on the easy cases where implementation changes weren't necessary. There are probably also additional structs that could be made readonly if their fields were appropriately annotated; I only looked at / fixed a few in that category (e.g. DateTime, Nullable), but didn't do a full sweep looking for others; that can be done subsequently, either piecemeal or with an analyzer.

This comment has been minimized.

Copy link
@benaadams

benaadams Aug 19, 2018

Collaborator

DateTimeOffset #19552

@jkotas

This comment has been minimized.

Copy link
Member

jkotas commented Nov 1, 2017

KeyValuePair is a good candidate to annotate.

@jkotas

This comment has been minimized.

Copy link
Member

jkotas commented Nov 1, 2017

@dotnet-bot test Windows_NT x64 corefx_baseline

@stephentoub

This comment has been minimized.

Copy link
Member Author

stephentoub commented Nov 1, 2017

We should make sure that the corefx contract consistency check start verifying the readonly annotations (if they are not doing it already). The contracts have to stay in sync with the implementation.

I don't believe they do yet. And in my corefx change, I went through and manually propagated the visible changes to the refs.
cc: @weshaggard, @ericstj

KeyValuePair is a good candidate to annotate.

Another case where the fields aren't currently annotated as readonly, so I didn't pick it up. I'll fix that one here, but others can be done subsequently.

Mark applicable structs as readonly
In a few cases (e.g. nullable), I added readonly to fields in order to allow readonly on the type.

@stephentoub stephentoub force-pushed the stephentoub:readonly_structs branch from 356e306 to 834d439 Nov 2, 2017

@stephentoub

This comment has been minimized.

Copy link
Member Author

stephentoub commented Nov 2, 2017

@dotnet-bot test Windows_NT x64 full_opt ryujit CoreCLR Perf Tests Correctness please
@dotnet-bot test Windows_NT x86 full_opt ryujit CoreCLR Perf Tests Correctness please

@stephentoub

This comment has been minimized.

Copy link
Member Author

stephentoub commented Nov 2, 2017

@jkotas, should these CoreCLR Perf Tests Correctness legs be working? The details links just come back as 404s.

@weshaggard

This comment has been minimized.

Copy link
Member

weshaggard commented Nov 2, 2017

I don't believe they do yet. And in my corefx change, I went through and manually propagated the visible changes to the refs.

I don't think we are currently. It should be easy enough to add for anyone that wants to verify it. The code lives in Microsoft.Cci.Extensions in Buildtools.

@jkotas

This comment has been minimized.

Copy link
Member

jkotas commented Nov 2, 2017

@adiaaida I see you have pending PR to change perf related legs. Are the perf test correctness failures known issue?

@jkotas

This comment has been minimized.

Copy link
Member

jkotas commented Nov 2, 2017

It should be easy enough to add for anyone that wants to verify it. The code lives in Microsoft.Cci.Extensions in Buildtools.

We need tracking issue for this at least. It needs to be done before we ship 2.1 to ensure consistency.

@jkotas

jkotas approved these changes Nov 2, 2017

Copy link
Member

jkotas left a comment

LGTM

@adiaaida

This comment has been minimized.

Copy link
Member

adiaaida commented Nov 2, 2017

I can't see the failure because I removed the jobs with my last change. I'll re-run the Perf Build and Test runs, which should tell us if this change will break the perf testing.

@dotnet-bot test Perf Build and Test please

@VSadov

VSadov approved these changes Nov 2, 2017

Copy link
Member

VSadov left a comment

LGTM
Nice change!!!

@stephentoub

This comment has been minimized.

Copy link
Member Author

stephentoub commented Nov 3, 2017

@adiaaida, how long is that perf build and test leg expected to take? It's been running for almost 5 hours. Similarly in the PR at #14836.

@jkotas

This comment has been minimized.

Copy link
Member

jkotas commented Nov 3, 2017

We should not block on the Perf Build and Test

@jkotas jkotas merged commit 278c8f3 into dotnet:master Nov 3, 2017

15 of 18 checks passed

Windows_NT x64 full_opt ryujit CoreCLR Perf Tests Correctness Build finished.
Details
Windows_NT x86 full_opt ryujit CoreCLR Perf Tests Correctness Build finished.
Details
Perf Build and Test Started.
Details
CROSS Check Build finished.
Details
CentOS7.1 x64 Checked Innerloop Build and Test Build finished.
Details
CentOS7.1 x64 Debug Innerloop Build Build finished.
Details
OSX10.12 x64 Checked Innerloop Build and Test Build finished.
Details
Tizen armel Cross Checked Innerloop Build and Test Build finished.
Details
Ubuntu arm64 Cross Debug Innerloop Build Build finished.
Details
Ubuntu armlb Cross Debug Innerloop Build Build finished.
Details
Ubuntu x64 Checked Innerloop Build and Test Build finished.
Details
Ubuntu x64 Innerloop Formatting Build finished.
Details
Ubuntu16.04 armlb Cross Debug Innerloop Build Build finished.
Details
WIP ready for review
Details
Windows_NT x64 Checked Innerloop Build and Test Build finished.
Details
Windows_NT x64 Innerloop Formatting Build finished.
Details
Windows_NT x86 Checked Innerloop Build and Test Build finished.
Details
license/cla All CLA requirements met.
Details

dotnet-bot added a commit to dotnet/corefx that referenced this pull request Nov 3, 2017

Mark applicable structs as readonly (dotnet/coreclr#14789)
In a few cases (e.g. nullable), I added readonly to fields in order to allow readonly on the type.

Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
@jkotas

This comment has been minimized.

Copy link
Member

jkotas commented Nov 3, 2017

It should be easy enough to add for anyone that wants to verify it. The code lives in Microsoft.Cci.Extensions in Buildtools.

We need tracking issue for this at least. It needs to be done before we ship 2.1 to ensure consistency.

Opened dotnet/corefx#25039 on this

stephentoub added a commit to dotnet/corefx that referenced this pull request Nov 3, 2017

Mark applicable structs as readonly (dotnet/coreclr#14789)
In a few cases (e.g. nullable), I added readonly to fields in order to allow readonly on the type.

Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>

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

nategraf pushed a commit to nategraf/coreclr that referenced this pull request Nov 7, 2017

Mark applicable structs as readonly (dotnet#14789)
In a few cases (e.g. nullable), I added readonly to fields in order to allow readonly on the type.

dotnet-bot added a commit to dotnet/corert that referenced this pull request Nov 8, 2017

Mark applicable structs as readonly (dotnet/coreclr#14789)
In a few cases (e.g. nullable), I added readonly to fields in order to allow readonly on the type.

Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>

jkotas added a commit to dotnet/corert that referenced this pull request Nov 8, 2017

Mark applicable structs as readonly (dotnet/coreclr#14789)
In a few cases (e.g. nullable), I added readonly to fields in order to allow readonly on the type.

Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>

jkotas added a commit to dotnet/corert that referenced this pull request Nov 9, 2017

Mark applicable structs as readonly (dotnet/coreclr#14789)
In a few cases (e.g. nullable), I added readonly to fields in order to allow readonly on the type.

Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
@@ -47,10 +47,10 @@ internal static string PairToString(object key, object value)
// and IReadOnlyDictionary<TKey, TValue>.
[Serializable]
[System.Runtime.CompilerServices.TypeForwardedFrom("mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089")]
public struct KeyValuePair<TKey, TValue>
public readonly struct KeyValuePair<TKey, TValue>

This comment has been minimized.

Copy link
@weshaggard

weshaggard Jan 5, 2018

Member

I'm working on the APICompat checks work but I'm not sure if this a compatible change. @jkotas @stephentoub is this an API compatible change?

This comment has been minimized.

Copy link
@jkotas

jkotas Jan 5, 2018

Member

Marking struct as readonly should be compatible change. The other direction (removing readonly on a struct) is not a compatible change.

cc @VSadov

This comment has been minimized.

Copy link
@jaredpar

jaredpar Jan 5, 2018

Member

Yes this is a compatible change.

dotnet-bot added a commit to dotnet/corefx that referenced this pull request Jan 13, 2018

Mark applicable structs as readonly (dotnet/coreclr#14789)
In a few cases (e.g. nullable), I added readonly to fields in order to allow readonly on the type.

Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>

dotnet-bot added a commit to dotnet/corefx that referenced this pull request Jan 13, 2018

Mark applicable structs as readonly (dotnet/coreclr#14789)
In a few cases (e.g. nullable), I added readonly to fields in order to allow readonly on the type.

Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>

safern added a commit to dotnet/corefx that referenced this pull request Jan 16, 2018

Mark applicable structs as readonly (dotnet/coreclr#14789)
In a few cases (e.g. nullable), I added readonly to fields in order to allow readonly on the type.

Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>

safern added a commit to dotnet/corefx that referenced this pull request Jan 16, 2018

Mark applicable structs as readonly (dotnet/coreclr#14789)
In a few cases (e.g. nullable), I added readonly to fields in order to allow readonly on the type.

Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.