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

[release/5.0-rc2] Fixes to nullable reference type annotations #41845

Merged
merged 11 commits into from
Sep 10, 2020

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Sep 3, 2020

Backport of #41731 to release/5.0-rc2. This addresses #41696 and corrects discrepancies in nullable reference type annotations.

During .NET 5.0, we increased our nullable annotations from 31% of netcoreapp to 94%. After completing the last batch of annotations (#41348 included in RC1), we noticed some differences between the implementation and reference assemblies, and we also found a couple places where we had failed to set the project-wide <Nullable>enable</Nullable> for projects that gained annotations during 5.0.

Customer Impact

Per our documentation for nullable annotations:

Breaking Change Guidance

Any such additions or changes to annotations can impact the warnings consuming code receives if that code has opted in to nullability analysis and warnings. Even so, for at least the foreseeable future we may still do so. We will be very thoughtful about when and how we do.

With these fixes going into 5.0 RC2, we are introducing the possibility of new warnings for consumers of these APIs who have nullable warnings enabled themselves. If this PR is approved, we will document the changes in annotations as a breaking change in RC2.

Affected Reference Assembly APIs
  • System.ComponentModel.Annotations (Annotated in 5.0 RC1)
    • CompareAttribute.IsValid
    • DisplayAttribute.GetDescription
    • FilterUIHintAttribute (constructor)
    • FilterUIHintAttribute.ControlParameters
    • ValidationAttribute.IsValid
    • ValidationContext (constructor)
  • System.Data.OleDb (Annotated in 5.0 Preview 7)
    • OleDbCommand.CommandText
  • System.Data.Common (Annotated in 5.0 Preview 7)
    • DataSet.WriteXmlSchema
    • DataTable.GetSchema
    • DataTableCollection.IndexOf
    • DataTableReader.GetSchemaTable
    • DataView.Sort
    • DataView.Table
    • UniqueConstraint (constructor)
    • DataColumnMapping.GetDataColumnBySchemaAction
    • SqlAlreadyFilledException (constructor)
  • System.Diagnostics.DiagnosticSource (Annotated in 5.0 Preview 1)
    • Activity.TagObjects
    • Activity.AddTag
    • Activity.SetTag
    • ActivityTagsCollection (constructor)
    • ActivityTagsCollection.Values
    • ActivityTagsCollection.Add
    • ActivityTagsCollection.Contains
    • ActivityTagsCollection.CopyTo
    • ActivityTagsCollection.Remove
    • ActivityTagsCollection.TryGetValue
    • ActivityTagsCollection.Enumerator
    • ActivityEvent.Tags
    • ActivityLink.Tags
    • ActivityCreationOptions.Tags
    • ActivityCreationOptions.Links
  • System.IO.Packaging (Annotated in 5.0 Preview 7)
    • Added missing <Nullable>enable</Nullable> to the reference assembly project
    • PackUriHelper.ComparePackUri
    • PackUriHelper.Create
    • PackUriHelper.GetPartUri
  • System.IO.Pipelines (Annotated in 5.0 Preview 1)
    • PipeReader.OnWriterCompleted
    • PipeWriter.OnReaderCompleted
  • System.Net.Mail (Annotated in 5.0 Preview 3)
    • Attachment.CreateAttachmentFromString
    • MailAddress.TryCreate
  • System.Net.Ping (Annotated in 5.0 Preview 1)
    • PingCompletedEventArgs.Reply
  • System.Net.Primitives (Annotated in 5.0 Preview 2)
    • IPEndPoint.TryParse
  • System.Net.Sockets (Annotated in 5.0 Preview 2)
    • UdpClient.EndReceive
  • System.Net.WebSockets (Annotated in 5.0 Preview 1)
    • WebSocket.CreteClientWebSocket
  • System.Reflection.TypeExtensions (Annotated in 5.0 Preview 1)
    • MemberInfoExtensions.GetBaseDefinition
  • System.Resources.Extensions (Annotated in 5.0 Preview 7)
    • Added missing <Nullable>enable</Nullable> to the reference assembly project
  • System.Runtime.InteropServices (Annotated in .NET Core 3.0)
    • Marshal.GetTypeFromCLSID
    • ComInterfaceDispatch.CreateObject
  • System.Runtime
    • Half.Parse (introduced in 5.0 Preview 7)
    • Half.TryParse (introduced in 5.0 Preview 7)
  • System.Security.Claims (Annotated in 5.0 Preview 1)
    • Claim.Properties
  • System.Security.Cryptography.Csp (Annotated in 5.0 Preview 1)
    • RNGCryptoServiceProvider (constructor)
  • System.Security.Cryptography.Pkcs (Annotated in 5.0 Preview 2)
    • CmsRecipient.RSAEncryptionPadding
    • CmsSigner (constructor)
    • CmsSigner.PrivateKey
    • EnvelopedCms.Decrypt
    • Pkcs12Builder.AddSafeContentsEncrypted
    • Pkcs12Builder.SealWithMac
    • Pkcs12Info.VerifyMac
    • Pkcs12SafeContents.AddShroudedKey
    • Pkcs12SafeContents.Decrypt
    • Pkcs12SafeContentsBag.SafeContents
    • Rfc3161TimestampRequest.RequestedPolicyId
    • Rfc3161TimestampRequest.CreateFromData
    • Rfc3161TimestampRequest.CreateFromHash
    • Rfc3161TimestampRequest.CreateFromSignerInfo
    • Rfc3161TimestampRequest.TryDecode
    • Rfc3161TimestampToken.TryDecode
    • Rfc3161TimestampToken.VerifySignatureForData
    • Rfc3161TimestampToken.VerifySignatureForHash
    • Rfc3161TimestampToken.VerifySignatureForSignerInfo
    • Rfc3161TimestampTokenInfo (constructor)
    • Rfc3161TimestampTokenInfo.TryDecode
  • System.Text,Json (Annotated in 5.0 Preview 1)
    • JsonException (constructor)
    • JsonConverterFactory.CreateConverter
  • System.Text.RegularExpressions (Annotated in 5.0 Preview 1)
    • Regex.CompileToAssembly

Note that there were 2 APIs corrected that were annotated in .NET Core 3.0, which means these would be breaking changes introduced in .NET 5.0.

  • System.Runtime.InteropServices
    • Marshal.GetTypeFromCLSID
    • ComInterfaceDispatch.CreateObject

Testing

@terrajobst implemented a utility to compare the nullable annotations between the implementation and reference assemblies; we've run that utility and reviewed all of the mismatches that were found. We expanded the list of reviewers to make sure several folks were reviewing the changes to be made; there was a team effort around making and verifying these fixes.

From this effort, the summary of the discrepancies is:

  • System.IO.Packaging (Annotated in 5.0 Preview 7)
    • We missed adding <Nullable>enable</Nullable> on the reference assembly in 518df4f
    • A few annotations were missing from the reference assembly as well
  • System.Resources.Extensions (Annotated in 5.0 Preview 7)
  • System.Security.Cryptography.Pkcs (Annotated in 5.0 Preview 2)
    • GenAPI didn't produce the annotations for the .netcoreapp.cs specific file in the reference assembly, which was not noticed during core review
  • All others
    • Some annotations changed during the code review process and were only updated in the implementation
    • Some new APIs weren't annotated correctly or the reference assemblies were not updated to reflect the annotations

During this process of identifying, addressing, and verifying these discrepancies, the crew agreed that a Roslyn-based APICompat implementation could help reduce these types of issues in the future.

Risk

Medium. The changed annotations are technically breaking changes in RC2. While we are not annotating any new areas, we are correcting existing annotations and marking System.IO.Packaging and System.Resources.Extensions with <Nullable>enable</Nullable>, which was previously missed. We will document the changes in an RC2 breaking change document after this is merged.

If any internal teams are affected by this, they should contact @jeffhandley for support. We will assist teams as needed.

@ghost
Copy link

ghost commented Sep 5, 2020

Tagging subscribers to this area: @safern, @ViktorHofer
See info in area-owners.md if you want to be subscribed.

@buyaa-n
Copy link
Contributor

buyaa-n commented Sep 8, 2020

2 more APIs missing from Affected Reference Assembly APIs list in System.Security.Cryptography.Pkcs:

  • Pkcs12SafeContents.AddShroudedKey
  • Pkcs12SafeContents.Decrypt

@buyaa-n
Copy link
Contributor

buyaa-n commented Sep 8, 2020

  • System.IO.Packaging:
    • Both reference and implementation annotated correctly
    • We missed adding <Nullable>enable</Nullable> on the reference assembly in 518df4f
  • System.Resources.Extensions:
    Missing <Nullable>enable</Nullable> on both implementation and reference assemblies in Annotate System.Resources.Extensions for nullable reference types #37597
  • System.Security.Cryptography.Pkcs:
    • We missed the annotations that are on a .netcoreapp.cs specific file on the reference assembly
  • All others:
    • Annotation differences either caused by GenAPI or fixes to the annotations that were not included into the ref file as well
  • System.IO.Packaging:
    • We missed adding <Nullable>enable</Nullable> on the reference assembly
    • Few annotations were missing from reference
  • System.Resources.Extensions:
    • Both reference and implementation annotated correctly
    • Missing <Nullable>enable</Nullable> on both implementation and reference, implementation had <Nullable>annotations</Nullable> instead
  • System.Security.Cryptography.Pkcs:
    • GenAPI didn't produce the annotations for the .netcoreapp.cs specific file on the reference assembly, and we didn't notice that during annotation and PR review.
  • From some of the other diffs what i see is:
    • Some annotations changed during code review process but forgotten to be updated in ref
    • Some new APIs added after annotation haven't annotated correctly (System.Diagnostics.DiagnosticSource) or forgotten to update the ref

@jeffhandley
Copy link
Member

2 more APIs missing from Affected Reference Assembly APIs list in System.Security.Cryptography.Pkcs:

I've added them to the list. I think we need to backport the latest commits from #41731 for those to show up in the diff here; I'll go trigger that now. Thanks.

@github-actions github-actions bot force-pushed the backport/pr-41731-to-release/5.0-rc2 branch from fe20d32 to 6a8f8a7 Compare September 8, 2020 23:04
@buyaa-n
Copy link
Contributor

buyaa-n commented Sep 8, 2020

For diffs in System.Data.Common i think the implementation of the XML dependent sections was not annotated, but refs annotated probably because it was needed annotating the other dependent APIs. @roji might confirm the reasoning

@github-actions github-actions bot force-pushed the backport/pr-41731-to-release/5.0-rc2 branch from 6a8f8a7 to 5a31aa8 Compare September 9, 2020 00:49
Copy link
Member

@roji roji left a comment

Choose a reason for hiding this comment

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

LGTM for the System.Data and System.ComponentModel.DataAnnotations changes, thanks again @buyaa-n and others.

@jeffhandley jeffhandley added this to the 5.0.0 rc2 milestone Sep 9, 2020
@github-actions github-actions bot force-pushed the backport/pr-41731-to-release/5.0-rc2 branch from 5a31aa8 to 7a45036 Compare September 9, 2020 19:02
@karelz karelz modified the milestones: 5.0.0 rc2, 5.0.0 Sep 9, 2020
@jeffhandley jeffhandley changed the title [release/5.0-rc2] Ensure <Nullable>enable<Nullable> on System.IO.Packaging and S.Resources.Extensions [release/5.0-rc2] Fixes to nullable reference types annotations Sep 9, 2020
@jeffhandley jeffhandley changed the title [release/5.0-rc2] Fixes to nullable reference types annotations [release/5.0-rc2] Fixes to nullable reference type annotations Sep 9, 2020
@jeffhandley jeffhandley added Servicing-consider Issue for next servicing release review and removed blocking-release labels Sep 9, 2020
@leecow leecow added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Sep 10, 2020
@buyaa-n
Copy link
Contributor

buyaa-n commented Sep 10, 2020

Failure unrelated, merging

@buyaa-n buyaa-n merged commit 3802520 into release/5.0-rc2 Sep 10, 2020
@buyaa-n buyaa-n deleted the backport/pr-41731-to-release/5.0-rc2 branch September 10, 2020 17:53
@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants