Make System.Formats.Nrbf public by adamsitnik · Pull Request #103232 · dotnet/runtime

adamsitnik · 2024-06-10T15:53:23Z

Changes:

fixes #102014

I am going to try to add ArrayRecord.TotalElementsCount in a separate PR, as I need to verify how to implement it for jagged arrays and get the name approved

- rename the folders, project and solution files - rename the namespace - set IsPackable to true and reference the project, not the source files

- PayloadReader -> NrbfDecoder - ArrayRecord<T> -> SZArrayRecord<T> - Read -> Decode

… the code

…rt for custom offset arrays

dotnet-policy-service · 2024-06-10T15:54:07Z

Tagging subscribers to this area: @dotnet/area-system-security, @bartonjs, @vcsjones
See info in area-owners.md if you want to be subscribed.

@jkotas

- introduce SerializationRecord.TypeName, remove ArrayRecord.ElementType - rename IsTypeNameMatching to TypeNameMatches, make it non-virtual and ignore assembly names - remove type-forwarding logic, use it only for testing cc @jkotas

…nstraint, re-add all values present in the NRBF spec

adamsitnik · 2024-06-11T16:59:07Z

src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/SerializationRecordId.cs

+    {
+        int id = reader.ReadInt32();
+
+        if (id == 0)


0 is the only illegal value. It allowed me to avoid the need of introducing Null property, as all IsNull checks are now Equals(default)

I know it's less pretty, but this is the API that got approved and TBH 99.99% of users should not use it at all.

adamsitnik · 2024-06-11T17:04:10Z

...ensions/src/System/Resources/Extensions/BinaryFormat/Deserializer/ArrayRecordDeserializer.cs

    [RequiresUnreferencedCode("Calls System.Windows.Forms.BinaryFormat.BinaryFormattedObject.TypeResolver.GetType(TypeName)")]
    internal static Array? GetSimpleBinaryArray(ArrayRecord arrayRecord, BinaryFormattedObject.ITypeResolver typeResolver)
    {
-        if (arrayRecord.ArrayType is not (BinaryArrayType.Single or BinaryArrayType.Jagged or BinaryArrayType.Rectangular))


these types are now simply rejected by the decoder itself

src/libraries/System.Formats.Nrbf/ref/System.Formats.Nrbf.csproj

JeremyKuhne · 2024-06-11T17:46:57Z

...sources.Extensions/src/System/Resources/Extensions/BinaryFormat/Deserializer/Deserializer.cs


    // Keeping a separate stack for ids for fast infinite loop checks.
-    private readonly Stack<int> _parseStack = [];
+    private readonly Stack<SerializationRecordId> _parseStack = [];


This will significantly hurt performance for deep graphs if SerializationRecordId is not RuntimeHelpers.IsBitwiseEquatable<T>(). If it isn't, or can't be made so, we should allow getting the raw value.

PTAL at daa7431, I think I was able to remove the need for having this separate stack

I think you can just throw there.

JeremyKuhne · 2024-06-11T17:48:59Z

src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/SerializationRecordId.cs

+/// <summary>
+/// The ID of <see cref="SerializationRecord" />.
+/// </summary>
+public readonly struct SerializationRecordId : IEquatable<SerializationRecordId>


See my comment about bitwise-equality: https://github.com/dotnet/runtime/pull/103232/files/a171648cfff83059d12b0021fc162bea9d67de5b#r1635270940

@adamsitnik, @GrabYourPitchforks overriding GetHashCode or Equals makes this id not bitwise equatable from the runtime's perspective. This makes searching for existing ids in arrays suboptimal. Is there a reason we can't expose the underlying value?

This makes searching for existing ids in arrays suboptimal.

But with my latest changes (daa7431) we don't need to do that anymore?

cc @bartonjs

overriding GetHashCode or Equals makes this id not bitwise equatable from the runtime's perspective

That just because we have not implemented general bitwise equatable optimization in the runtime. I would be best to avoid introducing workarounds for that in public API designs. Instead, add ref-counts to the general bitwise equatable.

@adamsitnik we don't for that specific usage, but other usages would hurt.

@jkotas I'm not sure what you mean? Can you point out the general bitwise equatable?

The issue that I have linked in my comment has the design discussion about it.

@jkotas, sorry I misunderstood your comment. I thought there was some sort of internal back door, not that we needed to add links to the existing issue. :)

src/libraries/System.Formats.Nrbf/ref/System.Formats.Nrbf.csproj

src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ArraySinglePrimitiveRecord.cs

src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ArraySingleObjectRecord.cs

src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ArraySingleStringRecord.cs

jkotas · 2024-06-13T14:25:03Z

I do not have any more comments, but I have not done thorough review and I am not intimately familiar with NRBF details to sign-off.

JeremyKuhne

I have no more blocking comments. The deserializer code looks good.

adamsitnik · 2024-06-13T17:40:40Z

@ericstj @carlossanlop one of the CI legs keeps failing with the following error (and it's the last error so it's blocking me from merging this PR):

❌artifacts\bin\testPackages\projects\System.Resources.Extensions\project.csproj(0,0): error NU1101: Unable to find package System.Formats.Nrbf. No packages exist with this id in source(s): D:\a\_work\1\s\artifacts\packages\Debug\, dotnet-eng, dotnet-libraries, dotnet-libraries-transport, dotnet-public, dotnet-tools, dotnet9, dotnet9-transport

System.Resources.Extensions is an existing package, so far it was referencing this code via links to source files, now it's going to just reference the new package: System.Formats.Nrbf

Is it some kind of a chicken and egg problem? How do I fix it? My current guess is to add sth similar to https://github.com/dotnet/runtime/blob/main/src/libraries/testPackages/packageSettings/System.Windows.Extensions/settings.targets

but it's just a guess.

FWIW the magic repro command:

.\build.cmd -c Release -test -s tools+libs+libs.tests /p:TestAssemblies=false /p:TestPackages=true

ericstj · 2024-06-13T18:59:32Z

For brand new packages you need to tell APICompat that a previous version doesn't exist. Do that by adding

runtime/src/libraries/System.Net.ServerSentEvents/src/System.Net.ServerSentEvents.csproj

Lines 13 to 16 in d2cada8

    
               <!-- Disabling baseline validation since this is a brand new package. 
        
                    Once this package has shipped a stable version, the following line 
        
                    should be removed in order to re-enable validation. --> 
        
               <DisablePackageBaselineValidation>true</DisablePackageBaselineValidation>

I'll make a suggestion that fixes it.

Also - you can reproduce this by running dotnet pack on your source project. That will build all configurations of it and test the production of a package. It's a good idea when adding a new package that you try this out and even experiment consuming your package in projects as a user would - just to double check that you're getting the behavior you expect. This is something that's possible with packages, but not with our big product.

src/libraries/System.Formats.Nrbf/src/System.Formats.Nrbf.csproj

Co-authored-by: Eric StJohn <ericstj@microsoft.com>

adamsitnik · 2024-06-14T04:53:33Z

@ericstj thanks a lot!

src/libraries/System.Formats.Nrbf/src/System.Formats.Nrbf.csproj

adamsitnik · 2024-06-14T15:26:25Z

The build analysis is red, despite the only failing test being known:

I'll try the bypass feature..

adamsitnik · 2024-06-14T15:28:19Z

/ba-g There is only one test failure and it's known, so the Build Anslysis should be green.

ericstj · 2024-06-14T16:05:07Z

@ericstj thanks a lot!

No problem - I see this didn't add the Package.md - please make sure to do that in a follow up PR. I filed #103481 to track it.

miloush · 2024-06-27T11:13:53Z

System.Formats.Nrbf isn't a great ".NET like" name, it's not clear what the acronym stands for and it wouldn't be easy to find. Couldn't it just be BinaryFormat, RemotingBinaryFormat or something else that is not a 4 letter acronym?

Citing from General Naming Conventions
✔️ DO favor readability over brevity.
❌ DO NOT use any acronyms that are not widely accepted, and even if they are, only when necessary.

bartonjs · 2024-06-27T16:55:01Z

The name "Nrbf" comes from the name of the spec: [MS-NRBF]. Just like we picked "Cbor" for the spec that called itself "CBOR" and "Cose" for the one calling itself "COSE".

adamsitnik added 6 commits June 10, 2024 14:00

apply the new library name: System.Formats.Nrbf

8a396bb

- rename the folders, project and solution files - rename the namespace - set IsPackable to true and reference the project, not the source files

rename the types and methods, make them public:

b49c824

- PayloadReader -> NrbfDecoder - ArrayRecord<T> -> SZArrayRecord<T> - Read -> Decode

introduce non-generic PrimitiveTypeRecord, use it where it simplifies…

8ed896f

… the code

remove ClassRecord.GetArrayOfPrimitiveType, add GetArrayRecord

380b82e

introduce SerializationRecordId, use it

0edfa3c

make BinaryArrayType internal, add ArrayRecord.IsJagged, remove suppo…

f205fa9

…rt for custom offset arrays

adamsitnik added the binaryformatter-migration Issues related to the removal of BinaryFormatter and migrations away from it label Jun 10, 2024

ghost added the area-System.Security label Jun 10, 2024

dotnet-policy-service bot assigned adamsitnik Jun 10, 2024

This was referenced Jun 10, 2024

Wasm test failure: System addon update list error SyntaxError: XMLHttpRequest.open #102955

Closed

[wasm] ystem addon update list error SyntaxError: XMLHttpRequest.open: 'http://%(server)s/dummy-system-addons.xml' is not a valid URL. #103060

Closed

adamsitnik added 4 commits June 11, 2024 10:42

fix the build

43336ed

TypeName matching changes:

e703c6b

- introduce SerializationRecord.TypeName, remove ArrayRecord.ElementType - rename IsTypeNameMatching to TypeNameMatches, make it non-virtual and ignore assembly names - remove type-forwarding logic, use it only for testing cc @jkotas

rename RecordType to SerializationRecordType, remove the byte size co…

7bd4b27

…nstraint, re-add all values present in the NRBF spec

add ref project

a171648

adamsitnik commented Jun 11, 2024

View reviewed changes

adamsitnik marked this pull request as ready for review June 11, 2024 17:07

adamsitnik requested review from GrabYourPitchforks, JeremyKuhne, bartonjs and jkotas June 11, 2024 17:08

adamsitnik commented Jun 11, 2024

View reviewed changes

src/libraries/System.Formats.Nrbf/ref/System.Formats.Nrbf.csproj Show resolved Hide resolved

JeremyKuhne reviewed Jun 11, 2024

View reviewed changes

carlossanlop reviewed Jun 11, 2024

View reviewed changes

src/libraries/System.Formats.Nrbf/ref/System.Formats.Nrbf.csproj Outdated Show resolved Hide resolved

jkotas reviewed Jun 11, 2024

View reviewed changes

src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ArraySinglePrimitiveRecord.cs Outdated Show resolved Hide resolved

jkotas reviewed Jun 11, 2024

View reviewed changes

src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ArraySingleObjectRecord.cs Outdated Show resolved Hide resolved

jkotas reviewed Jun 11, 2024

View reviewed changes

src/libraries/System.Formats.Nrbf/src/System/Formats/Nrbf/ArraySingleStringRecord.cs Outdated Show resolved Hide resolved

jkotas removed their request for review June 13, 2024 14:26

build-analysis bot mentioned this pull request Jun 13, 2024

Ahead-of-time compilation is not supported for the target runtime identifier 'win-x86' #103417

Closed

JeremyKuhne approved these changes Jun 13, 2024

View reviewed changes

build-analysis bot mentioned this pull request Jun 13, 2024

System.Net.Security.Tests.SslStreamTlsResumeTests.ClientDisableTlsResume_Succeeds failing on windows #103409

Closed

ericstj reviewed Jun 13, 2024

View reviewed changes

src/libraries/System.Formats.Nrbf/src/System.Formats.Nrbf.csproj Show resolved Hide resolved

Apply suggestions from code review

a151e6e

Co-authored-by: Eric StJohn <ericstj@microsoft.com>

adamsitnik commented Jun 14, 2024

View reviewed changes

src/libraries/System.Formats.Nrbf/src/System.Formats.Nrbf.csproj Outdated Show resolved Hide resolved

Apply suggestions from code review

729a61f

This was referenced Jun 14, 2024

NoWorkload-NoWebcil-ST-Wasm.Build.Tests.WorkItemExecution #103114

Closed

Intermittent build failure in AfterSourceBuild: "Could not write state file" #76488

Open

add missing xml docs

770761d

build-analysis bot mentioned this pull request Jun 14, 2024

SslStreamTlsResumeTests.ClientDisableTlsResume_Succeeds failed in CI #103449

Open

adamsitnik merged commit 121230d into dotnet:main Jun 14, 2024

ericstj mentioned this pull request Jun 14, 2024

System.Formats.Nrbf lacks a package readme #103481

Closed

bartonjs added area-System.Runtime area-System.Formats.Nrbf and removed area-System.Runtime area-System.Security labels Jun 17, 2024

adamsitnik mentioned this pull request Jun 18, 2024

[API Proposal]: Add API to detect Binary Format (MS-NRBF) header #84090

Closed

ellahathaway mentioned this pull request Jun 21, 2024

Update Source-Build SDK Diff Tests Baselines and Exclusions dotnet/sdk#41716

Merged

github-actions bot locked and limited conversation to collaborators Jul 28, 2024

Conversation

adamsitnik commented Jun 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dotnet-policy-service bot commented Jun 10, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jkotas commented Jun 13, 2024

Uh oh!

JeremyKuhne left a comment

Choose a reason for hiding this comment

Uh oh!

adamsitnik commented Jun 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ericstj commented Jun 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

adamsitnik commented Jun 14, 2024

Uh oh!

Uh oh!

adamsitnik commented Jun 14, 2024

Uh oh!

adamsitnik commented Jun 14, 2024

Uh oh!

ericstj commented Jun 14, 2024

Uh oh!

miloush commented Jun 27, 2024

Uh oh!

bartonjs commented Jun 27, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

adamsitnik commented Jun 10, 2024 •

edited

Loading

adamsitnik commented Jun 13, 2024 •

edited

Loading

ericstj commented Jun 13, 2024 •

edited

Loading