List patterns: Slice value is assumed to be never null by alrz · Pull Request #57457 · dotnet/roslyn

alrz · 2021-10-28T23:32:19Z

Spec is updated as follows:

The member that makes the type sliceable is assumed to be well-behaved, that is, the return value is never null and that it is a proper subslice of the containing list.

~~In a nullable-enabled context, the annotation on the output type is ignored to avoid producing nonsensical warnings.~~
~~(no longer needed to ignore, we're emitting the test but assuming the false branch is unreachable)~~ not an option.

Updated LDM decision (ended up agreeing with this approach): dotnet/csharplang#5599

Test plan: #51289

src/Compilers/CSharp/Test/Semantic/Semantics/PatternMatchingTests_ListPatterns.cs

jcouv · 2021-10-30T07:40:23Z

I'm not following. The LDM decision was to honor the annotation on Slice method. The current behavior is correct, no?

src/Compilers/CSharp/Test/Semantic/Semantics/PatternMatchingTests_ListPatterns.cs

src/Compilers/CSharp/Test/Semantic/Semantics/PatternMatchingTestBase.cs

src/Compilers/CSharp/Test/Semantic/Semantics/PatternMatchingTests_ListPatterns.cs

src/Compilers/CSharp/Portable/Binder/DecisionDagBuilder.cs

src/Compilers/CSharp/Test/Semantic/Semantics/PatternMatchingTests_ListPatterns.cs

jcouv

Done with review pass (iteration 9)

jcouv

LGTM Thanks (iteration 17)

jcouv · 2022-01-11T18:07:50Z

@333fred @AlekseyTs @dotnet/roslyn-compiler for second review. Thanks

333fred

Done review pass (commit 17)

333fred · 2022-01-12T01:23:18Z

src/Compilers/CSharp/Test/Semantic/Semantics/PatternMatchingTests_ListPatterns.cs

+            Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "slice").WithLocation(12, 13),
+            // (14,13): warning CS8602: Dereference of a possibly null reference.
+            //             list.ToString();
+            Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "list").WithLocation(14, 13)


I'm not sure I understand this one. Shouldn't the [] pattern say that it's a non-null slice, since it has to dereference the slice to determine if the slice is empty? #Closed

Makes sense. I'd defer to @jcouv for that. My solution was to ignore and report the annotation on the return type.

If we consider slice.Length evaluation (or any other) as a dereference with no warning on forthcoming derefs, shouldn't we inform the user about it?

I think we can do something in between: ignore and report only if there's a dereference i.e. when the result is being used - that would exclude var patterns. Not sure how that would play out with Nullable<T> as the result.

Thanks for spotting this. There's indeed a good question here.
From the LDM notes: "We will adjust our expectations around slice patterns to assume that Slice will never return a null value, and we will not insert a null test into the reachability or codegen DAGs."

I think it should follow that both the .. var slice and .. [] slice2 cases should yield not-null state for those variables.

@jcouv Can you please take another look? thanks.

src/Compilers/CSharp/Test/Semantic/Semantics/PatternMatchingTests_ListPatterns.cs

AlekseyTs · 2022-01-12T16:03:47Z

Done with review pass (commit 17)

alrz · 2022-01-19T19:15:16Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker_Patterns.cs

                                        addToTempMap(output, outputSlot, type.Type);
+                                        this.State[outputSlot] = NullableFlowState.NotNull; // Slice value is assumed to be never null


❓ Should we also strip the annotation on the return type for semantic model?

I am not sure if hovering over [.. var variable] shows a nullable type (probably shouldn't?)

The var will definitely have a ?, because it always does. Theoretically, though, it should say that variable is considered not null here.

333fred · 2022-01-19T19:29:33Z

src/Compilers/CSharp/Test/Semantic/Semantics/PatternMatchingTests_ListPatterns.cs


        if (new C<int?>() is [1, ..var rest2])
-            rest2.Value.ToString(); // 2
+            rest2.Value.ToString(); // 2 (assumed not-null)


This isn't quite what I meant: the numbers are wrong, because warning 2 is no longer there.

I renumbered the one you commented on. Will update the rest in the next commit.

jcouv

LGTM Thanks (iteration 18).
Nit: some line-number comments in tests could be adjusted, if no longer producing a diagnostic

333fred

LGTM (commit 19)

jcouv · 2022-01-20T06:11:38Z

Thanks @alrz! :-)

* upstream/main: (1035 commits) Add missing header Mark IVSTypeScriptFormattingServiceImplementation as optional, but require it in the constructor Fix Go To Base for a symbol with a single metadata location (dotnet#58965) [EnC] Store entire spans instead of line deltas (dotnet#58831) Delete CodeAnalysisRules.ruleset (dotnet#58968) Allow xml docs to still be created when 'emit metadata only' is on. (dotnet#57667) Fix ParseVBErrorOrWarning (dotnet#47833) Update parameter nullability to match implementation Ensure CSharpUseParameterNullCheckingDiagnosticAnalyzer works with nullable parameters Add tests for issues fixed by previous PR (dotnet#58764) Update src/Features/CSharp/Portable/Completion/CompletionProviders/ExplicitInterfaceMemberCompletionProvider.CompletionSymbolDisplay.cs Disallow null checking discard parameters (dotnet#58952) Add extension method Escape type arguments Few fixes Update tests. Add Analyzers layer to CODEOWNERS Add formatting analyzer test for param nullchecking (dotnet#58936) Move reading HideAdvancedMembers option up (dotnet#58747) List patterns: Slice value is assumed to be never null (dotnet#57457) ...

ghost added Community The pull request was submitted by a contributor who is not a Microsoft employee. Area-Compilers labels Oct 28, 2021

alrz commented Oct 28, 2021

View reviewed changes

src/Compilers/CSharp/Test/Semantic/Semantics/PatternMatchingTests_ListPatterns.cs Outdated Show resolved Hide resolved

alrz force-pushed the slice-value branch from f975e80 to 2f0ab47 Compare October 29, 2021 19:40

alrz mentioned this pull request Oct 29, 2021

Update list-patterns.md dotnet/csharplang#5364

Merged

alrz closed this Oct 30, 2021

alrz reopened this Oct 30, 2021

alrz force-pushed the slice-value branch from 2f0ab47 to 396342e Compare October 30, 2021 06:30

This comment has been minimized.

Sign in to view

Slice value is always assumed to be not null

104c8d9

alrz force-pushed the slice-value branch from 396342e to 104c8d9 Compare November 1, 2021 16:32

runfoapp bot mentioned this pull request Nov 3, 2021

Flaky test: Roslyn.VisualStudio.IntegrationTests.CSharp.CSharpFindReferences.FindReferencesToCtor #57552

Closed

Add test

a128b59

alrz commented Nov 4, 2021

View reviewed changes

src/Compilers/CSharp/Test/Semantic/Semantics/PatternMatchingTests_ListPatterns.cs Show resolved Hide resolved

alrz marked this pull request as ready for review November 4, 2021 09:37

alrz requested a review from a team as a code owner November 4, 2021 09:37

jcouv added the Feature - List Patterns label Nov 8, 2021

jcouv self-assigned this Nov 9, 2021

alrz requested a review from jcouv November 10, 2021 19:06

The annotation on the output type is ignored

5127879

alrz force-pushed the slice-value branch from f856097 to 5127879 Compare November 10, 2021 19:10

alrz changed the title ~~Slice value is always assumed to be not null~~ List patterns: Slice value is assumed to be never null Nov 10, 2021

jcouv reviewed Nov 10, 2021

View reviewed changes

src/Compilers/CSharp/Test/Semantic/Semantics/PatternMatchingTestBase.cs Outdated Show resolved Hide resolved

jcouv reviewed Nov 10, 2021

View reviewed changes

src/Compilers/CSharp/Test/Semantic/Semantics/PatternMatchingTests_ListPatterns.cs Outdated Show resolved Hide resolved

alrz commented Nov 10, 2021

View reviewed changes

src/Compilers/CSharp/Test/Semantic/Semantics/PatternMatchingTests_ListPatterns.cs Outdated Show resolved Hide resolved

jcouv reviewed Nov 11, 2021

View reviewed changes

Revert

b893fbc

alrz force-pushed the slice-value branch from c4ca6c2 to 37714bf Compare November 12, 2021 09:03

alrz commented Nov 12, 2021

View reviewed changes

src/Compilers/CSharp/Test/Semantic/Semantics/PatternMatchingTests_ListPatterns.cs Outdated Show resolved Hide resolved

jcouv reviewed Jan 9, 2022

View reviewed changes

alrz added 7 commits January 11, 2022 19:44

Revert

2515b23

Merge remote-tracking branch 'origin/main' into slice-value

e8d9f21

Update baseline

0e76c3b

Revert

0ee9dca

Revert resources

8109213

Revert

1ec2275

Adjust comments

99c7618

alrz force-pushed the slice-value branch from 492d607 to 99c7618 Compare January 11, 2022 16:40

Numbering

8e75b6c

alrz requested review from 333fred and jcouv January 11, 2022 16:55

jcouv approved these changes Jan 11, 2022

View reviewed changes

333fred reviewed Jan 12, 2022

View reviewed changes

PR feedback

26694e8

alrz commented Jan 19, 2022

View reviewed changes

333fred reviewed Jan 19, 2022

View reviewed changes

jcouv approved these changes Jan 19, 2022

View reviewed changes

Numbering

5b3120e

333fred reviewed Jan 19, 2022

View reviewed changes

333fred approved these changes Jan 19, 2022

View reviewed changes

jcouv enabled auto-merge (squash) January 19, 2022 21:31

jcouv merged commit 305a91c into dotnet:main Jan 20, 2022

ghost added this to the Next milestone Jan 20, 2022

RikkiGibson modified the milestones: Next, 17.2.P1 Feb 4, 2022

		addToTempMap(output, outputSlot, type.Type);
		this.State[outputSlot] = NullableFlowState.NotNull; // Slice value is assumed to be never null

Conversation

alrz commented Oct 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

jcouv commented Oct 30, 2021

Uh oh!

This comment has been minimized.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

jcouv commented Jan 11, 2022

Uh oh!

333fred left a comment

Choose a reason for hiding this comment

Uh oh!

333fred Jan 12, 2022 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alrz Jan 12, 2022

Choose a reason for hiding this comment

Uh oh!

alrz Jan 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Jan 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alrz Jan 19, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

AlekseyTs commented Jan 12, 2022

Uh oh!

alrz Jan 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

333fred Jan 19, 2022

Choose a reason for hiding this comment

Uh oh!

333fred Jan 19, 2022

Choose a reason for hiding this comment

Uh oh!

alrz Jan 19, 2022

Choose a reason for hiding this comment

Uh oh!

alrz Jan 19, 2022

Choose a reason for hiding this comment

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

333fred left a comment

Choose a reason for hiding this comment

Uh oh!

jcouv commented Jan 20, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

alrz commented Oct 28, 2021 •

edited

Loading

333fred Jan 12, 2022 •

edited by jcouv

Loading

alrz Jan 12, 2022 •

edited

Loading

jcouv Jan 18, 2022 •

edited

Loading

alrz Jan 19, 2022 •

edited

Loading