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

Fixing API compat issues between ref/src for attributes on properties #68432

Merged
merged 6 commits into from
Apr 30, 2022

Conversation

ericstj
Copy link
Member

@ericstj ericstj commented Apr 23, 2022

API compat had a regression about a year ago where it stopped checking attributes on members other than methods. dotnet/arcade#9168 fixes that. This PR brings us back up to date.

Marking this breaking because one member was missing obsolete in the ref assembly.

@ericstj ericstj added the breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. label Apr 23, 2022
@ericstj ericstj requested review from stephentoub, terrajobst, eerhardt and a team April 23, 2022 02:48
@ghost ghost added the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Apr 23, 2022
@ghost
Copy link

ghost commented Apr 23, 2022

Added needs-breaking-change-doc-created label because this PR has the breaking-change label.

When you commit this breaking change:

  1. Create and link to this PR and the issue a matching issue in the dotnet/docs repo using the breaking change documentation template, then remove this needs-breaking-change-doc-created label.
  2. Ask a committer to mail the .NET Breaking Change Notification DL.

Tagging @dotnet/compat for awareness of the breaking change.

@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@@ -1577,6 +1577,7 @@ public enum SchemaType
public enum SerializationFormat
{
Xml = 0,
[System.ObsoleteAttribute("SerializationFormat.Binary is obsolete and should not be used. See https://aka.ms/serializationformat-binary-obsolete for more information.", DiagnosticId = "SYSLIB0038")]
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a breaking change: it was already made in the src but never shipped in references.

Copy link
Member

Choose a reason for hiding this comment

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

This should be fine. We already have obsolete attributes regarding binary formatter in various places; it seems we simply missed this occurrence. However, given this is in System.Data, it might be a very different scenario. /cc @roji @ajcvickers

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, we did this recently in #39289 as part of obsoleting (and eventually removing) all uses of BinaryFormatter, for 7.0 (so hasn't shipped yet). It's indeed being treated and documented as a breaking change.

Copy link
Member

@rzikm rzikm left a comment

Choose a reason for hiding this comment

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

System.Net.* changes LGTM.

Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

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

Other than the mono build break, LGTM.

@terrajobst
Copy link
Member

terrajobst commented Apr 25, 2022

I've used the patch file to get to a quick summary of all new attributes:

All attribute additions
[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicProperties | DynamicallyAccessedMemberTypes.PublicFields)]
[EditorBrowsable(EditorBrowsableState.Advanced)]
[EditorBrowsable(EditorBrowsableState.Never)]
[Obsolete("Add(String parameterName, Object value) has been deprecated. Use AddWithValue(String parameterName, Object value) instead.")]
[Obsolete("Assembly.CodeBase and Assembly.EscapedCodeBase are only included for .NET Framework compatibility. Use Assembly.Location instead.", DiagnosticId = "SYSLIB0012", UrlFormat = "https://aka.ms/dotnet-warnings/{0}")]
[Obsolete("Assembly.CodeBase and Assembly.EscapedCodeBase are only included for .NET Framework compatibility. Use Assembly.Location.", DiagnosticId = "SYSLIB0012", UrlFormat = "https://aka.ms/dotnet-warnings/{0}")]
[Obsolete("IPAddress.Address is address family dependent and has been deprecated. Use IPAddress.Equals to perform comparisons instead.")]
[Obsolete("IsolatedStorageFile.CurrentSize has been deprecated because it is not CLS Compliant. To get the current size use IsolatedStorageFile.UsedSize instead.")]
[Obsolete("IsolatedStorageFile.MaximumSize has been deprecated because it is not CLS Compliant. To get the maximum size use IsolatedStorageFile.Quota instead.")]
[Obsolete("Marshalling arbitrary types may be unavailable in future releases. Specify the type you wish to marshal as.")]
[Obsolete("Marshalling as AnsiBStr may be unavailable in future releases.")]
[Obsolete("Marshalling as Currency may be unavailable in future releases.")]
[Obsolete("Marshalling as TBstr may be unavailable in future releases.")]
[Obsolete("Marshalling as VBByRefString may be unavailable in future releases.")]
[Obsolete("OffsetToStringData has been deprecated. Use string.GetPinnableReference() instead.")]
[Obsolete("PerformanceCounter.DefaultFileMappingSize has been deprecated and is not used. Use machine.config or an application configuration file to set the size of the PerformanceCounter file mapping instead.")]
[Obsolete("PerformanceCounterManager has been deprecated. Use the PerformanceCounters through the System.Diagnostics.PerformanceCounter class instead.")]
[Obsolete("Process.NonpagedSystemMemorySize has been deprecated because the type of the property can't represent all valid results. Use System.Diagnostics.Process.NonpagedSystemMemorySize64 instead.")]
[Obsolete("Process.PagedMemorySize has been deprecated because the type of the property can't represent all valid results. Use System.Diagnostics.Process.PagedMemorySize64 instead.")]
[Obsolete("Process.PagedSystemMemorySize has been deprecated because the type of the property can't represent all valid results. Use System.Diagnostics.Process.PagedSystemMemorySize64 instead.")]
[Obsolete("Process.PeakPagedMemorySize has been deprecated because the type of the property can't represent all valid results. Use System.Diagnostics.Process.PeakPagedMemorySize64 instead.")]
[Obsolete("Process.PeakVirtualMemorySize has been deprecated because the type of the property can't represent all valid results. Use System.Diagnostics.Process.PeakVirtualMemorySize64 instead.")]
[Obsolete("Process.PeakWorkingSet has been deprecated because the type of the property can't represent all valid results. Use System.Diagnostics.Process.PeakWorkingSet64 instead.")]
[Obsolete("Process.PrivateMemorySize has been deprecated because the type of the property can't represent all valid results. Use System.Diagnostics.Process.PrivateMemorySize64 instead.")]
[Obsolete("Process.VirtualMemorySize has been deprecated because the type of the property can't represent all valid results. Use System.Diagnostics.Process.VirtualMemorySize64 instead.")]
[Obsolete("Process.WorkingSet has been deprecated because the type of the property can't represent all valid results. Use System.Diagnostics.Process.WorkingSet64 instead.")]
[Obsolete("ReflectionPermissionAttribute.ReflectionEmit has been deprecated and is not supported.")]
[Obsolete("ReflectionPermissionAttribute.TypeInformation has been deprecated and is not supported.")]
[Obsolete("ReflectionPermissionFlag.AllFlags has been deprecated. Use PermissionState.Unrestricted to get full access.")]
[Obsolete("ReflectionPermissionFlag.ReflectionEmit  has been deprecated and is not supported.")]
[Obsolete("ReflectionPermissionFlag.TypeInformation has been deprecated and is not supported.")]
[Obsolete("RegistryPermissionAttribute.Add has been deprecated. Use ViewAndModify instead.")]
[Obsolete("SecurityProtocolType.Ssl3 has been deprecated and is not supported.")]
[Obsolete("SerializationFormat.Binary is obsolete and should not be used. See https://aka.ms/serializationformat-binary-obsolete for more information.", DiagnosticId = "SYSLIB0038")]
[Obsolete("StandardEventKeywords.CorrelationHint has an incorrect value and has been deprecated. Use CorrelationHint2 instead.")]
[Obsolete("The ApartmentState property has been deprecated. Use GetApartmentState, SetApartmentState or TrySetApartmentState instead.")]
[Obsolete("vbNewLine has been deprecated. For a carriage return and line feed, use vbCrLf. For the current platform's newline, use System.Environment.NewLine."]
[Obsolete("WaitHandle.Handle has been deprecated. Use the SafeWaitHandle property instead.")]
[RequiresAssemblyFiles("Returns <Unknown> for modules with no file path")]
[RequiresAssemblyFiles("This member throws an exception for assemblies embedded in a single-file app")]
[SupportedOSPlatform("maccatalyst")]
[UnsupportedOSPlatform("freebsd")]
[UnsupportedOSPlatform("illumos")]
[UnsupportedOSPlatform("ios")]
[UnsupportedOSPlatform("osx")]
[UnsupportedOSPlatform("solaris")]
[UnsupportedOSPlatform("tvos")]

These are the types:

  • DynamicallyAccessedMembers, RequiresAssemblyFiles. Linker related. I wouldn't consider them source breaking as they make it easier to troubleshoot runtime issues when trimming your app.
  • EditorBrowsable. Not source breaking.
  • Obsolete. Source breaking, but we have said in the past we're OK with that.
  • SupportedOSPlatform, UnsupportedOSPlatform. Platform compat annotations. I wouldn't consider them source breaking as they make it easier to troubleshoot runtime issues when running on a different operating system.

This will allow us to ensure the codebase is ready to ingest the arcade
fix to ApiCompat.
@ghost
Copy link

ghost commented Apr 28, 2022

Tagging subscribers to this area: @dotnet/area-infrastructure-libraries
See info in area-owners.md if you want to be subscribed.

Issue Details

API compat had a regression about a year ago where it stopped checking attributes on members other than methods. dotnet/arcade#9168 fixes that. This PR brings us back up to date.

Marking this breaking because one member was missing obsolete in the ref assembly.

Author: ericstj
Assignees: ericstj
Labels:

area-Infrastructure-libraries, breaking-change, new-api-needs-documentation, needs-breaking-change-doc-created

Milestone: -

Copy link
Member

@bartonjs bartonjs left a comment

Choose a reason for hiding this comment

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

LGTM, except for the System.Data.Common.Tests build failures.

@ericstj ericstj merged commit 0e84780 into dotnet:main Apr 30, 2022
DrewScoggins added a commit to DrewScoggins/performance-2 that referenced this pull request May 3, 2022
This was introduced by dotnet/runtime#68432. We need to mark the
test as not runnable on the platforms where we are missing this property
DrewScoggins added a commit to dotnet/performance that referenced this pull request May 3, 2022
This was introduced by dotnet/runtime#68432. We need to mark the
test as not runnable on the platforms where we are missing this property
@ghost ghost locked as resolved and limited conversation to collaborators Jun 3, 2022
@ericstj ericstj added this to the 7.0.0 milestone Sep 15, 2022
@ericstj
Copy link
Member Author

ericstj commented Sep 15, 2022

@ericstj ericstj removed the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Sep 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Infrastructure-libraries breaking-change Issue or PR that represents a breaking API or functional change over a prerelease.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants