Merge `main` branch into `list-patterns` branch #57425

alrz · 2021-10-27T22:31:44Z

Relates to test plan #51289

Use Roslyn.sln, not Compilers.sln, to build more projects during source-build. Update ExcludeFromSourceBuild properties to include more projects and exclude a few projects that shouldn't be in source-build. The newly included projects are used by downstream repos.

This makes source-build stop producing the Microsoft.CodeAnalysis.Collections package. Downstream repos (MSBuild) can't safely use a source-built version.

By default, the projects reference Microsoft.Build 16.5.0, which has netcoreapp2.1 and net472 support. The new 17.0.0-... version built during source-build only has net6.0. This causes Microsoft.CodeAnalysis.Workspaces.MSBuild.csproj to fail. Adding 16.5.0 to SBRP and dodging the override by renaming each package version property is the safe way to fix it.

AlekseyTs · 2021-10-28T16:25:42Z

Checks that do not depend on Source should be performed before we look at sources.

alrz · 2021-10-28T16:27:42Z

We're doing just that.

For the indexer case, we have to first find the top-level input (in case of nested slices)
For the rest we do that in the when clause after the null check.

There's no earlier place that we could check those at the same time.

AlekseyTs · 2021-10-28T16:36:34Z

                case (BoundDagIndexerEvaluation s1, BoundDagIndexerEvaluation s2):

When we enter this case, where do we check that s1Input.Index == s2Input.Index? #Closed

Refers to: src/Compilers/CSharp/Portable/Binder/DecisionDagBuilder.cs:1376 in 362472f. [](commit_id = 362472f, deletion_comment = False)

alrz · 2021-10-28T16:40:50Z

Line 1384 (notice we're depending on the Source)

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

Lines 1379 to 1384 in 90ee5ef

    
           (s1Input, BoundDagTemp s1LengthTemp, int s1Index) = GetCanonicalInput(s1); 
        
           (s2Input, BoundDagTemp s2LengthTemp, int s2Index) = GetCanonicalInput(s2); 
        
           Debug.Assert(s1LengthTemp.Syntax is ListPatternSyntax); 
        
           Debug.Assert(s2LengthTemp.Syntax is ListPatternSyntax); 
        
           // Ignore input source as it will be matched in the subsequent iterations. 
        
           if (s1Input.IsEquivalentTo(s2Input) &&

AlekseyTs · 2021-10-28T16:49:07Z

Line 1384 (notice we're depending on the Source)

This line checks different inputs, note that lines 1379 and 1380 overwrite values in s1Input and s2Input. If you are saying that it would be harmful to require s1Input.Index == s2Input.Index before entering the switch. I would like to hear an explanation. I believe this requirement should be met regardless of values in s1Input.Source and s2Input.Source.

alrz · 2021-10-28T16:55:20Z

I think you mean we put the entire switch in if (s1Input.IsEquivalentTo(s2Input)) which is fine. Nevertheless, if GetCanonicalInput returns a different temp, we'd expect Index to be always zero because those are only contained within list patterns. I'll also add a couple of assertions to that effect.

alrz · 2021-10-28T16:59:11Z

That would change the behavior though which is against well-behaved assumption from spec. If Slice on the outer list returns a different type from Slice on the inner list, we no longer consider those related with this check.

AlekseyTs · 2021-10-28T17:02:02Z

I think you mean we put the entire switch in if (s1Input.IsEquivalentTo(s2Input)) which is fine.

I think the type check will be inappropriate here as well.

Nevertheless, if GetCanonicalInput returns a different temp, we'd expect Index to be always zero because those are only contained within list patterns. I'll also add a couple of assertions to that effect.

I prefer us to not rely on assumptions like that, even if they are valid assumptions today. An approach like that is not robust in the long term should BoundDagTemp evolve. The "equivalence" of inputs that doesn't depend on their sources, should be checked before we do anything with sources.

AlekseyTs · 2021-10-28T17:08:38Z

Note, that the change that you are merging is sayin that "equivalence" of inputs doesn't require equality of types by default. Equality of types is required when types are indeed important. The merge doesn't preserve this principle and I think it should be preserved.

alrz · 2021-10-28T17:08:57Z

I think the type check will be inappropriate here as well.

That would work (different Slice return types will be ignored) but I'm not sure what does that entail for the rest of usages of IsEquivalentTo. It's possible we will need another helper for those.

AlekseyTs · 2021-10-28T17:12:22Z

That would work (different Slice return types will be ignored) but I'm not sure what does that entail for the rest of usages of IsEquivalentTo. It's possible we will need another helper for those.

I do not believe I suggested to simply make changes to IsEquivalentTo, or call it in some place, I simply made comments about specific call sites that I find problematic.

In reply to: 954040600

AlekseyTs · 2021-10-29T00:05:26Z

        while (!s1Input.Equals(s2Input)); // if we have found two identical input fragments, there's no point to continue.

Is there an advantage to do this check, which potentially does a very deep check on every iteration) instead of simply letting the loop to descend one step at a time and handle reaching null source on either or both sides? #Closed

Refers to: src/Compilers/CSharp/Portable/Binder/DecisionDagBuilder.cs:1442 in e6d7842. [](commit_id = e6d7842, deletion_comment = False)

AlekseyTs · 2021-10-29T00:10:37Z

src/Compilers/CSharp/Portable/BoundTree/BoundDagTemp.cs

+        /// <summary>
+        /// Determine if two <see cref="BoundDagTemp"/>s represent the same value for the purpose of a pattern evaluation.
+        /// </summary>
+        public bool IsSameValue(BoundDagTemp other)


IsSameValue

I think the name no longer accurately reflrects what this function is doing. And the doc comment as well. Consider renaming to something like "IsEquivalentIgnoringType" and adjusting the comment accordingly.

AlekseyTs · 2021-10-29T00:16:04Z

Done with review pass (commit 84), it looks like there are legitimate test failures.

AlekseyTs

LGTM (commit 86)

AlekseyTs · 2021-10-29T15:31:38Z

I also only reviewed commits passed 79. Consider squashing commits 80 and up into a single commit before merging.

jcouv · 2021-10-29T17:08:25Z

Taking another look to understand the difference and what I'd missed. Go ahead and squash the non-merge portion of the PR, then we'll merge. Thanks

jcouv · 2021-10-29T17:23:30Z

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

+                    case var (s1, s2) when s1 == s2:
+                        if (conditions != null)
+                        {
+                            relationCondition = Tests.AndSequence.Create(conditions);


Not related to this PR: would be clearer if we renamed this overload to CreateAndFree

jcouv · 2021-10-29T17:37:58Z

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

@@ -4984,34 +4979,33 @@ void Test(int[][] a)
            Diagnostic(ErrorCode.ERR_SwitchCaseSubsumed, "[[42]]").WithLocation(9, 18));

        VerifyDecisionDagDump<SwitchStatementSyntax>(comp,
-@"[0]: t0 != null ? [1] : [27]
+@"[0]: t0 != null ? [1] : [26]


Consider adding a test like Subsumption_15 or Subsumption_16 (which involve multiple preconditions to match temps) where the subsumption results from types (one is I and Base and the other is Derived) rather than remaining values.

jcouv

LGTM Thanks (iteration 86) with a test suggestion

jonfortescue and others added 30 commits September 3, 2021 15:00

Use 1ES pools

cdea40b

Pipeline variable changes

ed51a30

Remove reliance on pipeline variables

ad47867

Add single quotes

314029d

Hardcode the queues

37f0b44

Add error output Capture-Screenshot

b4b1786

re-enable fail-fast

8f59392

benchmarks

562753b

random benchmark stuff

43e7663

Temporary: Switch to int testing pools

7c5b4e9

use the correct benchmark path

ced72a3

cleanup

fcd0f08

removed unnecessary files

c1ddd8f

added cleanup

2b394d5

Merge branch 'main' into benchmarks

88112fc

Switch to production pools

3c09789

nav to benchmark change

2734317

fixes after having to delete everything bc git

0748222

removed rename issue

d36c03d

redo classification benchmark

59fdbb6

deleted generated files

5ca2a04

dont do entire sln

bb813b1

Add Features projects to SourceBuild

674f43e

Merge branch 'release/dev17.0' into 1esPools170

fa1751d

Fix Debugger.Contracts package condition

32c506a

Refine SourceBuild Features exclusions

0569f3d

Exclude src/Dependencies from source-build

8b1f3cb

This makes source-build stop producing the Microsoft.CodeAnalysis.Collections package. Downstream repos (MSBuild) can't safely use a source-built version.

Remove SourceBuild project excludes

8fa8561

alrz mentioned this pull request Oct 28, 2021

List patterns: Slice value is assumed to be never null #57457

Merged

alrz requested a review from AlekseyTs October 28, 2021 23:38

AlekseyTs reviewed Oct 29, 2021

View reviewed changes

alrz requested review from jcouv and AlekseyTs October 29, 2021 09:27

alrz force-pushed the epilogue branch 2 times, most recently from ec65c8d to 5e4d260 Compare October 29, 2021 11:26

AlekseyTs approved these changes Oct 29, 2021

View reviewed changes

jcouv reviewed Oct 29, 2021

View reviewed changes

jcouv approved these changes Oct 29, 2021

View reviewed changes

Follow-up on merge

f50a8dd

alrz force-pushed the epilogue branch from 5e4d260 to f50a8dd Compare October 29, 2021 18:33

jcouv enabled auto-merge October 29, 2021 18:38

jcouv merged commit 67a9216 into dotnet:features/list-patterns Oct 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge `main` branch into `list-patterns` branch #57425

Merge `main` branch into `list-patterns` branch #57425

alrz commented Oct 27, 2021 •

edited by jcouv

Loading

AlekseyTs commented Oct 28, 2021

alrz commented Oct 28, 2021 •

edited

Loading

AlekseyTs commented Oct 28, 2021 •

edited

Loading

alrz commented Oct 28, 2021

AlekseyTs commented Oct 28, 2021

alrz commented Oct 28, 2021

alrz commented Oct 28, 2021 •

edited

Loading

AlekseyTs commented Oct 28, 2021

AlekseyTs commented Oct 28, 2021

alrz commented Oct 28, 2021

AlekseyTs commented Oct 28, 2021 •

edited

Loading

AlekseyTs commented Oct 29, 2021 •

edited

Loading

AlekseyTs Oct 29, 2021

AlekseyTs commented Oct 29, 2021

AlekseyTs left a comment

AlekseyTs commented Oct 29, 2021

jcouv commented Oct 29, 2021

jcouv Oct 29, 2021

jcouv Oct 29, 2021

jcouv left a comment

Merge main branch into list-patterns branch #57425

Merge main branch into list-patterns branch #57425

Conversation

alrz commented Oct 27, 2021 • edited by jcouv Loading

AlekseyTs commented Oct 28, 2021

alrz commented Oct 28, 2021 • edited Loading

AlekseyTs commented Oct 28, 2021 • edited Loading

alrz commented Oct 28, 2021

AlekseyTs commented Oct 28, 2021

alrz commented Oct 28, 2021

alrz commented Oct 28, 2021 • edited Loading

AlekseyTs commented Oct 28, 2021

AlekseyTs commented Oct 28, 2021

alrz commented Oct 28, 2021

AlekseyTs commented Oct 28, 2021 • edited Loading

AlekseyTs commented Oct 29, 2021 • edited Loading

AlekseyTs Oct 29, 2021

Choose a reason for hiding this comment

AlekseyTs commented Oct 29, 2021

AlekseyTs left a comment

Choose a reason for hiding this comment

AlekseyTs commented Oct 29, 2021

jcouv commented Oct 29, 2021

jcouv Oct 29, 2021

Choose a reason for hiding this comment

jcouv Oct 29, 2021

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

Merge `main` branch into `list-patterns` branch #57425

Merge `main` branch into `list-patterns` branch #57425

alrz commented Oct 27, 2021 •

edited by jcouv

Loading

alrz commented Oct 28, 2021 •

edited

Loading

AlekseyTs commented Oct 28, 2021 •

edited

Loading

alrz commented Oct 28, 2021 •

edited

Loading

AlekseyTs commented Oct 28, 2021 •

edited

Loading

AlekseyTs commented Oct 29, 2021 •

edited

Loading