Skip to content

Use is not null syntax - #13286

Closed
xtqqczze wants to merge 3 commits into
PowerShell:masterfrom
xtqqczze:isnotnull
Closed

Use is not null syntax#13286
xtqqczze wants to merge 3 commits into
PowerShell:masterfrom
xtqqczze:isnotnull

Conversation

@xtqqczze

@xtqqczze xtqqczze commented Jul 27, 2020

Copy link
Copy Markdown
Contributor

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

@xtqqczze

xtqqczze commented Jul 27, 2020

Copy link
Copy Markdown
Contributor Author

Ilya (@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
Copy Markdown
Collaborator

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

@xtqqczze

Copy link
Copy Markdown
Contributor Author

Ilya (@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

xtqqczze commented Jul 27, 2020

Copy link
Copy Markdown
Contributor Author

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

@xtqqczze

Copy link
Copy Markdown
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
Copy Markdown
Collaborator

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

Comment thread test/Test.Common.props Outdated
@xtqqczze xtqqczze changed the title WIP: Use is not null syntax Use is not null syntax Jul 27, 2020
@xtqqczze

Copy link
Copy Markdown
Contributor Author

Ilya (@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 Ilya (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
Copy Markdown
Contributor Author

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

@iSazonov

Copy link
Copy Markdown
Collaborator

xtqqczze Please rebase to pass CIs.

@xtqqczze

Copy link
Copy Markdown
Contributor Author

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

@xtqqczze

xtqqczze commented Jul 30, 2020

Copy link
Copy Markdown
Contributor Author

First rebase on 1f904e4

@xtqqczze

Copy link
Copy Markdown
Contributor Author

Second rebase fixes up.

@daxian-dbw

Dongbo Wang (daxian-dbw) commented Jul 30, 2020

Copy link
Copy Markdown
Member

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
Copy Markdown
Contributor Author

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

@iSazonov

Copy link
Copy Markdown
Collaborator

Dongbo Wang (@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
Copy Markdown
Member

Quote from dotnet/roslyn#42368:

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

Ilya (@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
Copy Markdown
Collaborator

Dongbo Wang (@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
Copy Markdown
Member

Rain Sallow (/u/ta11ow) (@vexx32) Yes, there is such possibility, but I don't think it's practically a problem we need to concern about.

@xtqqczze

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

@xtqqczze

Copy link
Copy Markdown
Contributor Author

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

@iSazonov

Copy link
Copy Markdown
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
Copy Markdown
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

xtqqczze commented Oct 26, 2020

Copy link
Copy Markdown
Contributor Author

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