search

planetarium / libplanet

Blockchain in C#/.NET for on-chain, decentralized gaming
https://docs.libplanet.io/
GNU Lesser General Public License v2.1
505 stars 139 forks source link

Turn on C# 8's null checker #458

Open dahlia opened 4 years ago

dahlia commented 4 years ago

Edit: There is a way to introduce C# 8 with still targeting .NET Standard 2.0. For several reasons, we decided to use C# 8, but at the moment we use it without <Nullable>enable</Nullable> option. I hope we turn it on as well in the near future!

Although C# 8 will have non-nullable reference types, it will requires .NET Standard 2.1 (note that we currently depend on .NET Standard 2.0), which is unlikely to supported by Unity in the near future.

Instead, I suggest to introduce JetBrain's NotNullAttribute until Unity becomes to support .NET Standard 2.1. There's a Roslyn Analyzer implementation named NullGuard.Fody which does null-checks according to NotNullAttribute at compile time.

Projects (test projects are excluded as of now):

stale[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions.

dahlia commented 4 years ago

If we decide to use C# 8.0 with .NET Standard 2.0 (see #780) this approach is probably not that useful.

dahlia commented 4 years ago

If we decide to use C# 8.0 with .NET Standard 2.0 (see #780) this approach is probably not that useful.

We decided to use C# 8.0 with .NET Standard 2.0, not for null checks, but async streams. Anyway we would be able to use null checks as well, if we introduce C# 8.0 soon.

stale[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions.

dahlia commented 3 years ago

Update: although we still haven't enabled null checker project-wide, we've already enabled it in several files (the recently added files for the most part). For gradual progression, I suggest the @planetarium/libplanet team the methodology to adopt null checker with the minimum pain (and actually it is what we've recently done in few months indeed):

@libplanet Opinions?

dahlia commented 3 years ago

The current status:

$ find Libplanet/ -name '*.cs' | wc -l
193
$ git grep '#nullable enable' Libplanet/ | wc -l
37

The easiest files to enable null checker would be exception classes:

$ for f in $(find Libplanet -name '*Exception.cs'); do grep '#nullable enable' "$f" > /dev/null || echo "$f"; done
Libplanet/Blocks/InvalidBlockNonceException.cs
Libplanet/Blocks/InvalidBlockDifficultyException.cs
Libplanet/Blocks/InvalidBlockPreviousHashException.cs
Libplanet/Blocks/InvalidBlockTimestampException.cs
Libplanet/Blocks/InvalidBlockIndexException.cs
Libplanet/Blocks/InvalidGenesisBlockException.cs
Libplanet/Blocks/InvalidBlockHashException.cs
Libplanet/Blocks/InvalidBlockException.cs
Libplanet/Crypto/InvalidCiphertextException.cs
Libplanet/Net/SwarmException.cs
Libplanet/Net/DifferentAppProtocolVersionException.cs
Libplanet/Net/InvalidStateTargetException.cs
Libplanet/Net/InvalidMessageException.cs
Libplanet/Net/IceServerException.cs
Libplanet/Net/PeerNotFoundException.cs
Libplanet/Net/NoSwarmContextException.cs
Libplanet/Net/Protocols/PingTimeoutException.cs
Libplanet/Net/Protocols/PeerDiscoveryException.cs
Libplanet/KeyStore/IncorrectPassphraseException.cs
Libplanet/KeyStore/InvalidKeyJsonException.cs
Libplanet/KeyStore/KeyJsonException.cs
Libplanet/KeyStore/UnsupportedKeyJsonException.cs
Libplanet/KeyStore/NoKeyException.cs
Libplanet/KeyStore/KeyStoreException.cs
Libplanet/KeyStore/MismatchedAddressException.cs
Libplanet/Tx/InvalidTxNonceException.cs
Libplanet/Tx/InvalidTxException.cs
Libplanet/Tx/InvalidTxGenesisHashException.cs
Libplanet/Tx/InvalidTxPublicKeyException.cs
Libplanet/Tx/InvalidTxSignatureException.cs
Libplanet/Tx/InvalidTxUpdatedAddressesException.cs
Libplanet/Tx/TxViolatingBlockPolicyException.cs
Libplanet/Tx/InvalidTxIdException.cs
Libplanet/Blockchain/IncompleteBlockStatesException.cs
Libplanet/Action/MissingActionTypeException.cs
Libplanet/Action/UnexpectedlyTerminatedActionException.cs
Libplanet/Store/ChainIdNotFoundException.cs
stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions.

stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions.

dahlia commented 2 years ago

The current status:

$ find Libplanet/ -name '*.cs' | wc -l
231
$ git grep '#nullable enable' Libplanet/ | wc -l
137

Progress: 59.3%.

dahlia commented 2 years ago

The current status:

$ find Libplanet/ -name '*.cs' | wc -l
248
$ git grep '#nullable enable' Libplanet/ | wc -l
197

Progress: 79.4%.

Now we have more null-checked files than unchecked files, we can enable it project-wide (in Libplanet/Libplanet.csproj file), and opt out only exceptional files.

stale[bot] commented 2 years ago

This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions.

riemannulus commented 5 months ago

@s2quake got this.

s2quake commented 5 months ago

The status of the entire solution before starting the task is as follows.

Libplanet.Stun/Libplanet.Stun.csproj
    cs      : 44
    nullable: 34
    progress: 77.27%
Libplanet/Libplanet.csproj
    cs      : 54
    nullable: 13
    progress: 24.07%
Libplanet.RocksDBStore/Libplanet.RocksDBStore.csproj
    cs      : 6
    nullable: 1
    progress: 16.67%
Libplanet.Tools/Libplanet.Tools.csproj
    cs      : 2
    nullable: 0
    progress: 0.00%
Libplanet.Analyzers/Libplanet.Analyzers.csproj
    cs      : 4
    nullable: 0
    progress: 0.00%
Libplanet.Explorer/Libplanet.Explorer.csproj
    cs      : 43
    nullable: 8
    progress: 18.60%
Libplanet.Extensions.Cocona/Libplanet.Extensions.Cocona.csproj
    cs      : 19
    nullable: 0
    progress: 0.00%
Libplanet.Extensions.Cocona.Tests/Libplanet.Extensions.Cocona.Tests.csproj
    cs      : 9
    nullable: 2
    progress: 22.22%
Libplanet.Net/Libplanet.Net.csproj
    cs      : 112
    nullable: 10
    progress: 8.93%
Libplanet.Net.Tests/Libplanet.Net.Tests.csproj
    cs      : 50
    nullable: 17
    progress: 34.00%
Libplanet.Crypto.Secp256k1/Libplanet.Crypto.Secp256k1.csproj
    cs      : 3
    nullable: 0
    progress: 0.00%
Libplanet.Crypto.Secp256k1.Tests/Libplanet.Crypto.Secp256k1.Tests.csproj
    cs      : 2
    nullable: 0
    progress: 0.00%
Libplanet.Explorer.Cocona/Libplanet.Explorer.Cocona.csproj
    cs      : 3
    nullable: 0
    progress: 0.00%
Libplanet.Common/Libplanet.Common.csproj
    cs      : 15
    nullable: 3
    progress: 20.00%
Libplanet.Store/Libplanet.Store.csproj
    cs      : 43
    nullable: 6
    progress: 13.95%
Libplanet.Action/Libplanet.Action.csproj
    cs      : 62
    nullable: 3
    progress: 4.84%
Libplanet.Crypto/Libplanet.Crypto.csproj
    cs      : 12
    nullable: 0
    progress: 0.00%
Libplanet.Types/Libplanet.Types.csproj
    cs      : 68
    nullable: 0
    progress: 0.00%
# pwsh script
dotnet sln list | Where-Object {
    Test-Path -Path $_
} | ForEach-Object {
    $directory = Split-Path -Path $_ -Parent
    $isNullable = Select-Xml -Path $_ -XPath "/Project/PropertyGroup/Nullable"
    if ($isNullable) {
        $csFiles = Get-ChildItem -Path $directory -Recurse -Filter *.cs
        $nullableFiles = @($csFiles | Where-Object { Select-String -Path $_ -Pattern "#nullable disable" } | ForEach-Object { $_ })
        $progress = $nullableFiles.Length / $csFiles.Length
        Write-Host "$($_)"
        Write-Host "    cs      : $($csFiles.Length)"
        Write-Host "    nullable: $($nullableFiles.Length)"
        Write-Host "    progress: $($progress.ToString('p2'))"
    }
}

I will proceed with the task of removing the #nullable disable keyword in projects that include the <Nullable> property.

s2quake commented 5 months ago

~PR https://github.com/planetarium/libplanet/pull/3620 for stun project~

s2quake commented 5 months ago

https://github.com/planetarium/libplanet/pull/3622 Applied to Common, Action, and Explorer projects

s2quake commented 5 months ago

https://github.com/planetarium/libplanet/pull/3644 Applied to Store project

s2quake commented 5 months ago

https://github.com/planetarium/libplanet/pull/3651 Applied to RocksDBStore project