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

API diff between .NET 8.0 Preview 3 and .NET 8 Preview 4 #8469

Merged
merged 9 commits into from
Jun 15, 2023

Conversation

carlossanlop
Copy link
Member

  • ASP.NET - @dotnet/aspnet-api-review

  • WinForms - @merriemcgaw @dreddy-work @JeremyKuhne

  • WPF - @singhashish-wpf and @pchaurasia14

  • Libraries - @jeffhandley @karelz @ericstj @stephentoub

  • Microsoft.CSharp: @cston @333fred

  • Microsoft.VisualBasic: @cston @333fred

  • System.CodeDom: @dotnet/area-system-codedom

  • System.Collections: @dotnet/area-system-collections

  • System.ComponentModel.DataAnnotations: @dotnet/area-system-componentmodel-dataannotations

  • System.ComponentModel: @dotnet/area-system-componentmodel

  • System.Data: @ajcvickers @DavoudEshtehari @David-Engel @roji

  • System.Diagnostics.Tracing: @noahfalk @tommcdon @tarekgh

  • System.Formats.Asn1: @dotnet/area-system-formats-asn1

  • System.Globalization: @dotnet/area-system-globalization

  • @System.IO: @dotnet/area-system-io

  • System.Net: @dotnet/ncl

  • System.Numerics: @dotnet/area-system-numerics

  • System.Reflection: @dotnet/area-system-reflection

  • System.Reflection.Metadata: @dotnet/area-system-reflection-metadata

  • System.Resources: @dotnet/area-system-resources

  • System.Runtime: @dotnet/area-system-runtime

  • System.Runtime.CompilerServices: @dotnet/area-system-runtime-compilerservices

  • System.Runtime.InteropServices: @dotnet/interop-contrib

  • System.Runtime.Intrinsics: @dotnet/area-system-runtime-intrinsics

  • System.Security: @dotnet/area-system-security

  • System.Text.Json: @dotnet/area-system-text-json

  • System.Text: @dotnet/area-system-text-encoding

  • System.Text.RegularExpressions: @dotnet/area-system-text-regularexpressions

  • System.Threading: @mangod9 @kouvel

  • System.Threading.Channels: @dotnet/area-system-threading-channels

  • System.Threading.Tasks: @dotnet/area-system-threading-tasks

  • System.Transactions: @GrabYourPitchforks (not owner but made the changes)

  • System.Xml: @dotnet/area-system-xml

  • System: @dotnet/area-system-runtime

@carlossanlop carlossanlop self-assigned this May 18, 2023
@@ -0,0 +1,13 @@
# System.Numerics
Copy link
Member

Choose a reason for hiding this comment

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

System.Numerics changes LGTM

CC. @dotnet/area-system-numerics

@@ -0,0 +1,35 @@
# System.Runtime.Intrinsics.Arm
Copy link
Member

Choose a reason for hiding this comment

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

System.Runtime.Intrinsics.Arm changes LGTM

CC. @dotnet/area-system-runtime-intrinsics, @kunalspathak

@@ -0,0 +1,166 @@
# System.Runtime.Intrinsics.Wasm
Copy link
Member

Choose a reason for hiding this comment

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

System.Runtime.Intrinsics.Wasm changes LGTM

CC. @dotnet/area-system-runtime-intrinsics

Copy link
Member

Choose a reason for hiding this comment

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

@radekdoulik, are these "on by default" now or is there still work before that happens?

@@ -0,0 +1,582 @@
# System.Runtime.Intrinsics.X86
Copy link
Member

Choose a reason for hiding this comment

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

System.Runtime.Intrinsics.X86 LGTM

CC. @dotnet/area-system-runtime-intrinsics, @dotnet/avx512-contrib

@@ -0,0 +1,28 @@
# System.Threading.Tasks
Copy link
Member

Choose a reason for hiding this comment

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

System.Threading.Tasks LGTM.

cc. @dotnet/area-system-threading-tasks, @stephentoub

Copy link
Member

@tannergooding tannergooding May 18, 2023

Choose a reason for hiding this comment

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

The serialization diffs on the last two APIs have the same question as https://github.com/dotnet/core/pull/8469/files#r1198216476 on if anything changed in the tooling here.

@@ -0,0 +1,331 @@
# System
Copy link
Member

Choose a reason for hiding this comment

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

System.Runtime changes LGTM.

CC. @dotnet/area-system-runtime

Copy link
Member

Choose a reason for hiding this comment

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

The serialization diffs on the various APIs have the same question as https://github.com/dotnet/core/pull/8469/files#r1198216476 on if anything changed in the tooling here.

Copy link
Member Author

Choose a reason for hiding this comment

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

The main issue was the inability of the tool to generate the diff for new attributes, particularly the obsolete attribute. It should be fixed now.

+ public DateTimeOffset GetLocalNow();
+ public virtual long GetTimestamp();
+ public virtual DateTimeOffset GetUtcNow();
+ }
Copy link
Member

Choose a reason for hiding this comment

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

LGTM

public class Task<TResult> : Task {
+ public new Task<TResult> WaitAsync(TimeSpan timeout, TimeProvider timeProvider);
+ public new Task<TResult> WaitAsync(TimeSpan timeout, TimeProvider timeProvider, CancellationToken cancellationToken);
}
Copy link
Member

Choose a reason for hiding this comment

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

LGTM

public enum NumberStyles {
+ AllowBinarySpecifier = 1024,
+ BinaryNumber = 1027,
}
Copy link
Member

Choose a reason for hiding this comment

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

LGTM

@tarekgh
Copy link
Member

tarekgh commented May 18, 2023

Globalization and the parts in System and Tasks related to TimeProvider LGTM.

Copy link
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

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

System.IO* and the new Ascii APIs LGTM, but the exceptions ctors got obsoleted (not added) and this requires a change.


``` diff
namespace System.IO {
public class DirectoryNotFoundException : IOException {
Copy link
Member

Choose a reason for hiding this comment

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

these ctors got obsoleted, not added: dotnet/runtime#84383

Copy link
Member Author

Choose a reason for hiding this comment

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

The ObsoleteAttribute is now showing up. Please ignore the slightly weird formatting. At least it makes sense now.

Copy link
Member

Choose a reason for hiding this comment

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

It looks better now, but I wonder if it could be improved: no + or - for the ctor itself, just mention the ctor with no prefx (no diff), and two new lines (+) for the attributes?

+        [EditorBrowsableAttribute(EditorBrowsableState.Never)]
+        [ObsoleteAttribute("This API supports obsolete formatter-based serialization. It should not be called or extended by application code.", DiagnosticId="SYSLIB0051", UrlFormat="https://aka.ms/dotnet-warnings/{0}")]
         protected DirectoryNotFoundException(SerializationInfo info, StreamingContext context);

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree it would be nice to show such diffs as you describe. However, adding changes to the AsmDiff tool is time consuming and I currently cannot address it. If you don't mind, I'll merge this PR with the addressed fixes and will open an issue in the arcade repo to consider your suggestion.

Copy link
Member Author

Choose a reason for hiding this comment

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

@carlossanlop
Copy link
Member Author

@BrennanConroy @tarekgh @tannergooding @adamsitnik @tommcdon @cston I fixed the weird diffs related to attributes. Would you mind taking a look again?

- protected MalformedLineException(SerializationInfo info, StreamingContext context);
+ [EditorBrowsableAttribute(EditorBrowsableState.Never)]
+ [ObsoleteAttribute("This API supports obsolete formatter-based serialization. It should not be called or extended by application code.", DiagnosticId="SYSLIB0051", UrlFormat="https://aka.ms/dotnet-warnings/{0}")]
+ protected MalformedLineException(SerializationInfo info, StreamingContext context);
Copy link
Member

Choose a reason for hiding this comment

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

In the future, could we render these differently? Right now it looks like we deleted an API and added an API, when this could instead just show an addition for the two new attributes.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree the output is ugly. Unfortunately, fixing AsmDiff is very low priority right now. It takes a lot of my time to fix it since the code is difficult to maintain and the project has no tests. I am hoping we can eventually move to using GenAPI instead of Microsoft.Cci for generating these API diffs, that would help a lot. I can't promise anything in the short term, but I'll keep this in mind.

Copy link
Member Author

@carlossanlop carlossanlop May 22, 2023

Choose a reason for hiding this comment

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

For reference, I have a PR out here to fix the attribute problem reported in other comments: dotnet/arcade#13641

- public abstract class RegexRunnerFactory
+ [EditorBrowsableAttribute(EditorBrowsableState.Never)]
+ public abstract class RegexRunnerFactory
}
Copy link
Member

Choose a reason for hiding this comment

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

Regex LGTM

+ public bool AppendFormatted<T>(T value, string? format);
+ public bool AppendLiteral(string value);
+ }
}
Copy link
Member

Choose a reason for hiding this comment

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

System.Text.Unicode LGTM

+ DesktopPC = 94,
+ DeviceAudioPlayer = 102,
+ DeviceCamera = 100,
+ DeviceCellPhone = 99,
Copy link
Member

Choose a reason for hiding this comment

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

System.Drawing* and WinForms LGTM.

+ [ObsoleteAttribute("This API supports obsolete formatter-based serialization. It should not be called or extended by application code.", DiagnosticId="SYSLIB0051", UrlFormat="https://aka.ms/dotnet-warnings/{0}")]
+ public override void GetObjectData(SerializationInfo info, StreamingContext context);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

}

Microsoft.CSharp and Microsoft.VisualBasic LGTM.

Copy link
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

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

The System.IO LGTM now, I left a comment with an idea for further simplification.


``` diff
namespace System.IO {
public class DirectoryNotFoundException : IOException {
Copy link
Member

Choose a reason for hiding this comment

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

It looks better now, but I wonder if it could be improved: no + or - for the ctor itself, just mention the ctor with no prefx (no diff), and two new lines (+) for the attributes?

+        [EditorBrowsableAttribute(EditorBrowsableState.Never)]
+        [ObsoleteAttribute("This API supports obsolete formatter-based serialization. It should not be called or extended by application code.", DiagnosticId="SYSLIB0051", UrlFormat="https://aka.ms/dotnet-warnings/{0}")]
         protected DirectoryNotFoundException(SerializationInfo info, StreamingContext context);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.