Make System.Formats.Nrbf public

[adamsitnik]

Changes:

• apply the new library name: System.Formats.Nrbf
  • rename the folders, project and solution files
  • rename the namespace
  • set IsPackable to true and reference the project, not the source files
• rename PayloadReader to NrbfDecoder, ArrayRecord<T> to SZArrayRecord<T> and Read to Decode
• remove ClassRecord.GetArrayOfPrimitiveType, add GetArrayRecord
• introduce non-generic PrimitiveTypeRecord, use it where it simplifies the code
• introduce SerializationRecordId, use it
• make BinaryArrayType internal, remove support for custom offset arrays
• expose ClassRecord.TypeName
• rename IsTypeNameMatching to TypeNameMatches, make it non-virtual and ignore assembly names
• rename RecordType to SerializationRecordType
• remove all the SerializationRecordType values that can not be used by the user
• make SerializationRecordType : int, not SerializationRecordType : byte
• add ref project

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

[dotnet-policy-service]

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

[adamsitnik]

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]

these types are now simply rejected by the decoder itself

[JeremyKuhne]

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.

[adamsitnik]

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

[JeremyKuhne]

I think you can just throw there.

[JeremyKuhne]

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

[JeremyKuhne]

@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?

[adamsitnik]

This makes searching for existing ids in arrays suboptimal.

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

[adamsitnik]

cc @bartonjs

[jkotas]

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.

[JeremyKuhne]

@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?

[jkotas]

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

[JeremyKuhne]

@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. :)

[jkotas]

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]

@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]

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.

[adamsitnik]

@ericstj thanks a lot!

[adamsitnik]

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

I'll try the bypass feature..

[adamsitnik]

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

[ericstj]

@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]

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]

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]

Changes:

• apply the new library name: System.Formats.Nrbf
  • rename the folders, project and solution files
  • rename the namespace
  • set IsPackable to true and reference the project, not the source files
• rename PayloadReader to NrbfDecoder, ArrayRecord<T> to SZArrayRecord<T> and Read to Decode
• remove ClassRecord.GetArrayOfPrimitiveType, add GetArrayRecord
• introduce non-generic PrimitiveTypeRecord, use it where it simplifies the code
• introduce SerializationRecordId, use it
• make BinaryArrayType internal, remove support for custom offset arrays
• expose ClassRecord.TypeName
• rename IsTypeNameMatching to TypeNameMatches, make it non-virtual and ignore assembly names
• rename RecordType to SerializationRecordType
• remove all the SerializationRecordType values that can not be used by the user
• make SerializationRecordType : int, not SerializationRecordType : byte
• add ref project

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

[dotnet-policy-service]

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

[adamsitnik]

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]

these types are now simply rejected by the decoder itself

[JeremyKuhne]

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.

[adamsitnik]

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

[JeremyKuhne]

I think you can just throw there.

[JeremyKuhne]

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

[JeremyKuhne]

@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?

[adamsitnik]

This makes searching for existing ids in arrays suboptimal.

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

[adamsitnik]

cc @bartonjs

[jkotas]

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.

[JeremyKuhne]

@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?

[jkotas]

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

[JeremyKuhne]

@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. :)

[jkotas]

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]

@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]

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.

[adamsitnik]

@ericstj thanks a lot!

[adamsitnik]

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

I'll try the bypass feature..

[adamsitnik]

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

[ericstj]

@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]

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]

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]

Changes:

• apply the new library name: System.Formats.Nrbf
  • rename the folders, project and solution files
  • rename the namespace
  • set IsPackable to true and reference the project, not the source files
• rename PayloadReader to NrbfDecoder, ArrayRecord<T> to SZArrayRecord<T> and Read to Decode
• remove ClassRecord.GetArrayOfPrimitiveType, add GetArrayRecord
• introduce non-generic PrimitiveTypeRecord, use it where it simplifies the code
• introduce SerializationRecordId, use it
• make BinaryArrayType internal, remove support for custom offset arrays
• expose ClassRecord.TypeName
• rename IsTypeNameMatching to TypeNameMatches, make it non-virtual and ignore assembly names
• rename RecordType to SerializationRecordType
• remove all the SerializationRecordType values that can not be used by the user
• make SerializationRecordType : int, not SerializationRecordType : byte
• add ref project

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

[dotnet-policy-service]

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

[adamsitnik]

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]

these types are now simply rejected by the decoder itself

[JeremyKuhne]

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.

[adamsitnik]

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

[JeremyKuhne]

I think you can just throw there.

[JeremyKuhne]

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

[JeremyKuhne]

@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?

[adamsitnik]

This makes searching for existing ids in arrays suboptimal.

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

[adamsitnik]

cc @bartonjs

[jkotas]

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.

[JeremyKuhne]

@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?

[jkotas]

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

[JeremyKuhne]

@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. :)

[jkotas]

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]

@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]

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.

[adamsitnik]

@ericstj thanks a lot!

[adamsitnik]

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

I'll try the bypass feature..

[adamsitnik]

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

[ericstj]

@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]

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]

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]

Changes:

• apply the new library name: System.Formats.Nrbf
  • rename the folders, project and solution files
  • rename the namespace
  • set IsPackable to true and reference the project, not the source files
• rename PayloadReader to NrbfDecoder, ArrayRecord<T> to SZArrayRecord<T> and Read to Decode
• remove ClassRecord.GetArrayOfPrimitiveType, add GetArrayRecord
• introduce non-generic PrimitiveTypeRecord, use it where it simplifies the code
• introduce SerializationRecordId, use it
• make BinaryArrayType internal, remove support for custom offset arrays
• expose ClassRecord.TypeName
• rename IsTypeNameMatching to TypeNameMatches, make it non-virtual and ignore assembly names
• rename RecordType to SerializationRecordType
• remove all the SerializationRecordType values that can not be used by the user
• make SerializationRecordType : int, not SerializationRecordType : byte
• add ref project

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

[dotnet-policy-service]

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

[adamsitnik]

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]

these types are now simply rejected by the decoder itself

[JeremyKuhne]

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.

[adamsitnik]

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

[JeremyKuhne]

I think you can just throw there.

[JeremyKuhne]

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

[JeremyKuhne]

@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?

[adamsitnik]

This makes searching for existing ids in arrays suboptimal.

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

[adamsitnik]

cc @bartonjs

[jkotas]

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.

[JeremyKuhne]

@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?

[jkotas]

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

[JeremyKuhne]

@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. :)

[jkotas]

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]

@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]

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.

[adamsitnik]

@ericstj thanks a lot!

[adamsitnik]

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

I'll try the bypass feature..

[adamsitnik]

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

[ericstj]

@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]

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]

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]

Changes:

• apply the new library name: System.Formats.Nrbf
  • rename the folders, project and solution files
  • rename the namespace
  • set IsPackable to true and reference the project, not the source files
• rename PayloadReader to NrbfDecoder, ArrayRecord<T> to SZArrayRecord<T> and Read to Decode
• remove ClassRecord.GetArrayOfPrimitiveType, add GetArrayRecord
• introduce non-generic PrimitiveTypeRecord, use it where it simplifies the code
• introduce SerializationRecordId, use it
• make BinaryArrayType internal, remove support for custom offset arrays
• expose ClassRecord.TypeName
• rename IsTypeNameMatching to TypeNameMatches, make it non-virtual and ignore assembly names
• rename RecordType to SerializationRecordType
• remove all the SerializationRecordType values that can not be used by the user
• make SerializationRecordType : int, not SerializationRecordType : byte
• add ref project

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

[dotnet-policy-service]

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

[adamsitnik]

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]

these types are now simply rejected by the decoder itself

[JeremyKuhne]

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.

[adamsitnik]

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

[JeremyKuhne]

I think you can just throw there.

[JeremyKuhne]

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

[JeremyKuhne]

@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?

[adamsitnik]

This makes searching for existing ids in arrays suboptimal.

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

[adamsitnik]

cc @bartonjs

[jkotas]

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.

[JeremyKuhne]

@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?

[jkotas]

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

[JeremyKuhne]

@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. :)

[jkotas]

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]

@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]

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.

[adamsitnik]

@ericstj thanks a lot!

[adamsitnik]

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

I'll try the bypass feature..

[adamsitnik]

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

[ericstj]

@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]

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]

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]

Changes:

• apply the new library name: System.Formats.Nrbf
  • rename the folders, project and solution files
  • rename the namespace
  • set IsPackable to true and reference the project, not the source files
• rename PayloadReader to NrbfDecoder, ArrayRecord<T> to SZArrayRecord<T> and Read to Decode
• remove ClassRecord.GetArrayOfPrimitiveType, add GetArrayRecord
• introduce non-generic PrimitiveTypeRecord, use it where it simplifies the code
• introduce SerializationRecordId, use it
• make BinaryArrayType internal, remove support for custom offset arrays
• expose ClassRecord.TypeName
• rename IsTypeNameMatching to TypeNameMatches, make it non-virtual and ignore assembly names
• rename RecordType to SerializationRecordType
• remove all the SerializationRecordType values that can not be used by the user
• make SerializationRecordType : int, not SerializationRecordType : byte
• add ref project

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

[dotnet-policy-service]

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

[adamsitnik]

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]

these types are now simply rejected by the decoder itself

[JeremyKuhne]

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.

[adamsitnik]

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

[JeremyKuhne]

I think you can just throw there.

[JeremyKuhne]

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

[JeremyKuhne]

@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?

[adamsitnik]

This makes searching for existing ids in arrays suboptimal.

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

[adamsitnik]

cc @bartonjs

[jkotas]

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.

[JeremyKuhne]

@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?

[jkotas]

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

[JeremyKuhne]

@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. :)

[jkotas]

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]

@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]

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.

[adamsitnik]

@ericstj thanks a lot!

[adamsitnik]

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

I'll try the bypass feature..

[adamsitnik]

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

[ericstj]

@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]

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]

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]

Changes:

• apply the new library name: System.Formats.Nrbf
  • rename the folders, project and solution files
  • rename the namespace
  • set IsPackable to true and reference the project, not the source files
• rename PayloadReader to NrbfDecoder, ArrayRecord<T> to SZArrayRecord<T> and Read to Decode
• remove ClassRecord.GetArrayOfPrimitiveType, add GetArrayRecord
• introduce non-generic PrimitiveTypeRecord, use it where it simplifies the code
• introduce SerializationRecordId, use it
• make BinaryArrayType internal, remove support for custom offset arrays
• expose ClassRecord.TypeName
• rename IsTypeNameMatching to TypeNameMatches, make it non-virtual and ignore assembly names
• rename RecordType to SerializationRecordType
• remove all the SerializationRecordType values that can not be used by the user
• make SerializationRecordType : int, not SerializationRecordType : byte
• add ref project

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

[dotnet-policy-service]

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

[adamsitnik]

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]

these types are now simply rejected by the decoder itself

[JeremyKuhne]

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.

[adamsitnik]

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

[JeremyKuhne]

I think you can just throw there.

[JeremyKuhne]

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

[JeremyKuhne]

@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?

[adamsitnik]

This makes searching for existing ids in arrays suboptimal.

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

[adamsitnik]

cc @bartonjs

[jkotas]

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.

[JeremyKuhne]

@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?

[jkotas]

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

[JeremyKuhne]

@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. :)

[jkotas]

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]

@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]

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.

[adamsitnik]

@ericstj thanks a lot!

[adamsitnik]

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

I'll try the bypass feature..

[adamsitnik]

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

[ericstj]

@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]

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]

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]

Changes:

• apply the new library name: System.Formats.Nrbf
  • rename the folders, project and solution files
  • rename the namespace
  • set IsPackable to true and reference the project, not the source files
• rename PayloadReader to NrbfDecoder, ArrayRecord<T> to SZArrayRecord<T> and Read to Decode
• remove ClassRecord.GetArrayOfPrimitiveType, add GetArrayRecord
• introduce non-generic PrimitiveTypeRecord, use it where it simplifies the code
• introduce SerializationRecordId, use it
• make BinaryArrayType internal, remove support for custom offset arrays
• expose ClassRecord.TypeName
• rename IsTypeNameMatching to TypeNameMatches, make it non-virtual and ignore assembly names
• rename RecordType to SerializationRecordType
• remove all the SerializationRecordType values that can not be used by the user
• make SerializationRecordType : int, not SerializationRecordType : byte
• add ref project

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

[dotnet-policy-service]

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

[adamsitnik]

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]

these types are now simply rejected by the decoder itself

[JeremyKuhne]

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.

[adamsitnik]

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

[JeremyKuhne]

I think you can just throw there.

[JeremyKuhne]

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

[JeremyKuhne]

@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?

[adamsitnik]

This makes searching for existing ids in arrays suboptimal.

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

[adamsitnik]

cc @bartonjs

[jkotas]

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.

[JeremyKuhne]

@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?

[jkotas]

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

[JeremyKuhne]

@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. :)

[jkotas]

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]

@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]

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.

[adamsitnik]

@ericstj thanks a lot!

[adamsitnik]

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

I'll try the bypass feature..

[adamsitnik]

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

[ericstj]

@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]

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]

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".