Skip to content
This repository has been archived by the owner on Nov 22, 2023. It is now read-only.

Support .NET Standard 2.1 and .NET 5.0 #498

Closed
wants to merge 17 commits into from

Conversation

devhawk
Copy link
Contributor

@devhawk devhawk commented Oct 28, 2022

In order to support Unity (neo-project/neo#2824), we need a version of RPC Client that supports .NET Standard 2.1. Since the Neo domain model depends on Neo.VM, we will also need a .NET Standard 2.1 compatible version of Neo.VM. This PR updates NeoVM to target .NET Standard 2.1, .NET 5 and .NET 6

cc @erikzhang @shargon @roman-khimov @igormcoelho for review
cc @gsmachado @somniumwave @logikonline for input

Two of the changes were somewhat significant.

  • C# 8 doesn't support record types, so ExecutionEngineLimits was re-written as a class. In order to retain record semantics, IEquatable<ExecutionEngineLimits> was manually implemented. C# 8 also doesn't support init property setters, so a #if NET5_0_OR_GREATER preprocessor directive is used to declare ExecutionEngineLimits properties as having normal set methods instead of init on .NET Standard 2.1. On .NET 5/6, init set methods continue to be used.
  • C# 8 doesn't support relational pattern in switch expressions, as used in ScriptBuilder.EmitPush
            return bytesWritten switch
            {
                1 => Emit(OpCode.PUSHINT8, PadRight(buffer, bytesWritten, 1, value.Sign < 0)),
                2 => Emit(OpCode.PUSHINT16, PadRight(buffer, bytesWritten, 2, value.Sign < 0)),
                <= 4 => Emit(OpCode.PUSHINT32, PadRight(buffer, bytesWritten, 4, value.Sign < 0)),
                <= 8 => Emit(OpCode.PUSHINT64, PadRight(buffer, bytesWritten, 8, value.Sign < 0)),
                <= 16 => Emit(OpCode.PUSHINT128, PadRight(buffer, bytesWritten, 16, value.Sign < 0)),
                _ => Emit(OpCode.PUSHINT256, PadRight(buffer, bytesWritten, 32, value.Sign < 0)),
            };

this was replaced with code similar to what the C# compiler generates (verified via ILSpy)

            return bytesWritten <= 8
                ? bytesWritten > 4
                    ? Emit(OpCode.PUSHINT64, PadRight(buffer, bytesWritten, 8, value.Sign < 0))
                    : bytesWritten switch
                    {
                        1 => Emit(OpCode.PUSHINT8, PadRight(buffer, bytesWritten, 1, value.Sign < 0)),
                        2 => Emit(OpCode.PUSHINT16, PadRight(buffer, bytesWritten, 2, value.Sign < 0)),
                        _ => Emit(OpCode.PUSHINT32, PadRight(buffer, bytesWritten, 4, value.Sign < 0)),
                    }
                : bytesWritten > 16
                    ? Emit(OpCode.PUSHINT256, PadRight(buffer, bytesWritten, 32, value.Sign < 0))
                    : Emit(OpCode.PUSHINT128, PadRight(buffer, bytesWritten, 16, value.Sign < 0));

If folks think this is too complicated, we can replace it with a series of if statements.

Remaining changes are fairly minor:

  • Explicitly specify object type instead of using target-type object creation (SomeType value = new()) for C# 8 compat
  • Invert if statements using is not patterns for C# 8 compat
  • Replace use of or pattern in ReferenceCounter.NeedTrack with two separate if statements
  • Implement BitOperations.RotateLeft, ReferenceEqualityComparer and BigInteger.GetBitLength on .NET Standard 2.1 (.NET 5 and 6 use the BCL provided versions)
  • Use new byte[] instead of GC.AllocateUninitializedArray on .NET Standard 2.1 (.NET 5 and 6 continue to use GC.AllocateUninitializedArray)
  • Use non generic version of Enum.IsDefined for .NET standard 2.1 compat
  • Suppress warning of value parameter nullability mismatch in IDictionary<K,V>.TryGetValue implementations

@devhawk
Copy link
Contributor Author

devhawk commented Oct 28, 2022

Note, this PR should NOT be merged before 3.5 ships

@devhawk
Copy link
Contributor Author

devhawk commented Nov 8, 2022

ping @erikzhang @shargon @igormcoelho

@igormcoelho
Copy link
Contributor

Sorry for the delay @devhawk , let's see this.
First, congratulations for the amazing work, I fully agree that most compatibility should be kept, regarding the "C# family". We see similar situation in C++, where people try to keep oldest possible standard, however there are interesting cases here and novelty, at least for me, that maybe future C# versions can remove existing things? That's different.
In any case, this example you mentioned is very clarifying: I really think that NeoVM should be as efficient as possible, however, since it is the only reference implementation we have, it should be as clear as possible. And if it's not overwhelming performance difference, I really believe we should take the most clear implementation. So, I certainly prefer nested if-else structures.
Still reviewing details, will come back soon!

@devhawk
Copy link
Contributor Author

devhawk commented Nov 9, 2022

First, congratulations for the amazing work, I fully agree that most compatibility should be kept, regarding the "C# family". We see similar situation in C++, where people try to keep oldest possible standard, however there are interesting cases here and novelty, at least for me, that maybe future C# versions can remove existing things? That's different.

AFAIK, there are no plans to remove things from dotnet at this point. CoreCLR 1.0 only supported a subset of what .NET Framework supported at the time, but at this point Microsoft has unified their .NET story around the xplat CoreCLR runtime and library. I don't see them removing things without a very very good reason.

In any case, this example you mentioned is very clarifying: I really think that NeoVM should be as efficient as possible, however, since it is the only reference implementation we have, it should be as clear as possible. And if it's not overwhelming performance difference, I really believe we should take the most clear implementation. So, I certainly prefer nested if-else structures.

I found an article about using C# 9 with older target frameworks. It's not technically supported, but I found a bunch of places in both the dotnet and Microsoft GitHub orgs that define IsExternalInit and/or SkipLocalsInitAttribute in order to use C# 9 on downlevel targets. That approach would allow us to continue to use C# 9 while still supporting .NET Standard 2.1 and would have less code churn than this PR which uses C# 8. I'll open a 2nd PR to see how that approach works out.

@logikonline
Copy link

@devhawk Unless I am not seeing it, I need more than just the VM to review. Where would I find the RPC that is compatible with .NET Standard 2.1?

@devhawk
Copy link
Contributor Author

devhawk commented Nov 9, 2022

@devhawk Unless I am not seeing it, I need more than just the VM to review. Where would I find the RPC that is compatible with .NET Standard 2.1?

This is the first step. After this, we need to carve out the parts of neo.dll that RPC Client depends on as a separate assembly that targets .NET standard 2.1 + .NET 6. I've been referring to that separate assembly as the "nucleus". "Neo.Nucleus" work is tracked by neo-project/neo#1865. I was working on it, but that work is blocked on NeoVM .NET standard 2.1 support.

Once we have a separate Neo.Nucleus package, we'll be able to update RpcClient that targets .NET Standard 2.1 and runs in non-full-node environments like Unity and .NET Maui

@logikonline
Copy link

@devhawk Unless I am not seeing it, I need more than just the VM to review. Where would I find the RPC that is compatible with .NET Standard 2.1?

This is the first step. After this, we need to carve out the parts of neo.dll that RPC Client depends on as a separate assembly that targets .NET standard 2.1 + .NET 6. I've been referring to that separate assembly as the "nucleus". "Neo.Nucleus" work is tracked by neo-project/neo#1865. I was working on it, but that work is blocked on NeoVM .NET standard 2.1 support.

Once we have a separate Neo.Nucleus package, we'll be able to update RpcClient that targets .NET Standard 2.1 and runs in non-full-node environments like Unity and .NET Maui

Sounds good, I'll keep an eye out for the rest. The VM looks fine imo.

@devhawk
Copy link
Contributor Author

devhawk commented Nov 9, 2022

Sounds good, I'll keep an eye out for the rest. The VM looks fine imo.

Also take a look at #499

@logikonline
Copy link

Sounds good, I'll keep an eye out for the rest. The VM looks fine imo.

Also take a look at #499

Pulled it and tried it, looks solid. Once I can test with the RPC, I'll try it in the latest XF to confirm.

Copy link
Member

@shargon shargon left a comment

Choose a reason for hiding this comment

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

@devhawk It's possible to add the workflow test for these net frameworks?

Comment on lines +42 to +43
if (item is CompoundType) return true;
if (item is Buffer) return true;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (item is CompoundType) return true;
if (item is Buffer) return true;
if (item is CompoundType || item is Buffer) return true;

#if NET5_0_OR_GREATER
init;
#else
set;
Copy link
Member

Choose a reason for hiding this comment

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

Maybe create a constructor for all of these "else"? otherwise it can be changed after the creation

@devhawk
Copy link
Contributor Author

devhawk commented Nov 10, 2022

Note, I'm not updating this PR in favor of #499. I think that is a better approach

@devhawk
Copy link
Contributor Author

devhawk commented Dec 5, 2022

closed in favor of #499

@devhawk devhawk closed this Dec 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants