Skip to content

Conversation

@xtqqczze
Copy link
Contributor

@xtqqczze xtqqczze commented Jul 27, 2020

PR Summary

  • -replace '!= null', 'is not null' for C# 9.0.
  • Enable C# 9.0 in xUnit.tests.

PR Context

Follow-up #13277.

PR Checklist


This change is Reviewable

@ghost ghost assigned daxian-dbw Jul 27, 2020
@xtqqczze
Copy link
Contributor Author

xtqqczze commented Jul 27, 2020

@iSazonov I think these changes should wait until we have updated code to use features available in C# 8, such as null operators and pattern matching.

@iSazonov
Copy link
Collaborator

The pattern comes from C# 9.0 and is already enabled.

@xtqqczze
Copy link
Contributor Author

@iSazonov What is mean is I think we should go for "low-hanging fruit" such as using null conditional access from C# 6.0 (RCS1146) etc before making C# 9.0 changes.

@xtqqczze
Copy link
Contributor Author

xtqqczze commented Jul 27, 2020

@iSazonov I have started work on related changes in #13286.

@xtqqczze
Copy link
Contributor Author

Build failures seem to be because preview features not enabled in xUnit.tests.

https://dev.azure.com/powershell/PowerShell/_build/results?buildId=58427&view=logs&j=ce47e5d9-6379-5300-b411-5c447691f9de&t=2718afa2-89c3-5dee-4660-3339d525f0c9&l=37

csharp/test_PSConfiguration.cs(100,47): error CS8652: The feature 'not pattern' is currently in Preview and *unsupported*. To use Preview features, use the 'preview' language version. [/Users/runner/work/1/s/test/xUnit/xUnit.tests.csproj]

@iSazonov
Copy link
Collaborator

You can exclude xUnit files from the PR or enable C# 9.0 in the csproj.

@xtqqczze xtqqczze changed the title WIP: Use is not null syntax Use is not null syntax Jul 27, 2020
@xtqqczze
Copy link
Contributor Author

@iSazonov If you would like, you can start to review, as we are not isolating C# 9.0 changes. This is already a huge PR so it seems to be the best plan. Any more changes can be in follow-up PR.

@xtqqczze xtqqczze marked this pull request as ready for review July 27, 2020 19:45
@xtqqczze xtqqczze marked this pull request as draft July 27, 2020 20:40
@xtqqczze xtqqczze marked this pull request as ready for review July 27, 2020 20:44
@iSazonov iSazonov added the CL-CodeCleanup Indicates that a PR should be marked as a Code Cleanup change in the Change Log label Jul 28, 2020
@xtqqczze
Copy link
Contributor Author

Macos failures look like Test-Connection DNS issues related to #12935.

@iSazonov
Copy link
Collaborator

@xtqqczze Please rebase to pass CIs.

@xtqqczze
Copy link
Contributor Author

@iSazonov Too many merge conflicts, I will push new commits rather then resolving.

@xtqqczze
Copy link
Contributor Author

xtqqczze commented Jul 30, 2020

First rebase on 1f904e4

@xtqqczze
Copy link
Contributor Author

Second rebase fixes up.

@daxian-dbw
Copy link
Member

daxian-dbw commented Jul 30, 2020

@xtqqczze thanks for your efforts in cleaning up the code, but I don't think the is not language feature is for replacing != null. There is nothing wrong using != null. So I don't think we can merge this PR.

@xtqqczze
Copy link
Contributor Author

@daxian-dbw This change was actually requested by @iSazonov (#13090 (comment)). Are you against is not generally or just a huge change like this PR?

@iSazonov
Copy link
Collaborator

@daxian-dbw What do you mean? The pattern "is not null" is for replacement !(x is not null) dotnet/roslyn#42368

@daxian-dbw
Copy link
Member

Quote from dotnet/roslyn#42368:

Add feature to convert from !(x is null) to x is not null

@iSazonov That conversion makes sense. But most changes in this PR are converting x != null to x is not null, which is not what is suggested. Unlike !(x is null), x != null is explicit and clear, it's conventional and there is no readability issue about it.

Even the !(x is null) is pretty contrived. IMO, is not feature is for cases where we currently use !(x is <a-type-name>).

@vexx32
Copy link
Collaborator

vexx32 commented Jul 30, 2020

@daxian-dbw it's worth noting that == and != operators can both (potentially) be overridden by a type with an implementation that returns unexpected results for a comparison with null; x is not null avoids this possibility.

@daxian-dbw
Copy link
Member

@vexx32 Yes, there is such possibility, but I don't think it's practically a problem we need to concern about.

@xtqqczze
Copy link
Contributor Author

Just to say that if x is a value type, x != null could result in undefined behaviour (see #13285 for examples). x is not null will result in a compiler error instead .

@daxian-dbw
Copy link
Member

@xtqqczze I agree that for value types, x != null can actually be considered a bug. I will review #13285 shortly.

@daxian-dbw
Copy link
Member

Given that #13277 is reverted, I will close this PR as well. The idea is the same as in #13277 (comment), that we shouldn't force is not null over != null -- neither should be universally preferred; they are appropriate in different scenarios.

@daxian-dbw daxian-dbw closed this Jul 30, 2020
@xtqqczze xtqqczze deleted the isnotnull branch July 30, 2020 23:32
@xtqqczze
Copy link
Contributor Author

@daxian-dbw I have made !(x is <a-type-name>) changes in #13323

@iSazonov
Copy link
Collaborator

I think we should re-review this again since .Net team added new Roslyn analyzer to force using new syntax "is null" and "is not".

@xtqqczze
Copy link
Contributor Author

I think we should re-review this again since .Net team added new Roslyn analyzer to force using new syntax "is null" and "is not".

Yep, see dotnet/runtime#42692

@xtqqczze
Copy link
Contributor Author

xtqqczze commented Oct 26, 2020

We should wait until #13333 and #13338 are merged to avoid conflicts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CL-CodeCleanup Indicates that a PR should be marked as a Code Cleanup change in the Change Log

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants