Patterns: Allow non-negative and full integer sets to merge (intersect or union) #71968

jcouv · 2024-02-06T02:32:54Z

For background, the issue is that we treat certain Count property patterns as only resulting in a non-negative integer set, but in the problematic scenario we need to perform operations (intersection/union) between a non-negative integer set resulting from a Count property with the special treatment (on an IList) which is countable and indexable) and an integer set resulting from a Count property in a context where it's not treated specially (on an ICollection).

I considered a few possible avenues for fixing this:

expanding the non-negative integer set to a regular integer set when the operation is performed
implement NonNegativeIntValueSetFactory on the basis of IntTC (ie. remove NonNegativeIntTC)
don't give the special treatment (ie. don't set BoundPropertySubpattern.IsLengthOrCount or BoundDagPropertyEvaluation.IsLengthOrCount) when we detect such mix usage. This can be done by not setting the flag in the first place or by erasing it after all the tests have been prepared.
adjust equality (BoundDabTemp.Equals) to account for differences in IsLengthOrCount (thx @alrz for suggestion)

This PR initially implemented option 1, but we ended up shifting towards a new implementation of option 2. The type parameters for type classes were removed in favor of arguments holding instances of specific algorithms. This allows having two instances of IntTC, one for full integer set and one for non-negative sets. That flag doesn't need to be merged carefully when two sets get merged (intersection/union) at the moment, although that's something we could refine if it turns out necessary.

Tagging @alrz in case you have some other thought.

alrz · 2024-02-06T11:08:32Z

Would it make sense to not relate the two props in the first place? that is, including IsLengthOrCount in BoundDagPropertyEvaluation equality.

jcouv · 2024-02-06T15:43:28Z

@alrz Thanks much for the suggestion. I'd not considered that. I'll give it a try (seems simpler).

jcouv · 2024-02-06T18:22:27Z

@alrz I took a stab at it, but I don't think tweaking the comparison is a good approach after all. It's better for all branches of the DAG that test the Count property to share the same knowledge of remaining values. I added a test that I think illustrates the problem.

CyrusNajmabadi · 2024-02-06T21:29:41Z

and an integer set resulting from a Count property in a context where it's not treated specially (on an ICollection).

Curious why we don't think of that as special. I assumed we were using .Count from teh highest interface, not just IList.

jcouv · 2024-02-06T21:33:07Z

Curious why we don't think of that as special. I assumed we were using .Count from teh highest interface, not just IList.

ICollection lacks an indexer (it could not be used in a list-pattern) so it's Count doesn't count.

CyrusNajmabadi · 2024-02-06T21:44:11Z

I get that part :)

My point though is that the Count property comes from ICollection:

Not IList itself. So it seems ok to me that if we are to assume that that Count is non-negative, we do that for all ICollection's, not just ILists. Does that make more sense?

jcouv · 2024-02-06T23:21:39Z

Clarified offline with @CyrusNajmabadi. Independently of the change in this PR, he's surprised that we don't make the non-negative assumption of well-behavedness for some other well-known BCL Count properties such as ICollection.Count. That could be made into a csharplang proposal if someone wants to champion it.

alrz · 2024-02-07T07:41:35Z

(it could not be used in a list-pattern)

It could with enumerable list-patterns. Checking Count as a short-circuiting failure is already mentioned in the proposal, I think non-negative value range could apply there.

jcouv · 2024-02-15T19:52:52Z

@dotnet/roslyn-compiler for review. Thanks

AlekseyTs · 2024-02-16T15:35:47Z

src/Compilers/CSharp/Portable/Utilities/ValueSetFactory.NumericValueSet.cs

+                // But we need to upgrade them to regular integers to perform operations against full integer sets.
+                if (this is NumericValueSet<int, NonNegativeIntTC> nonNegativeThis && o is NumericValueSet<int, IntTC>)
+                {
+                    return ((IValueSet<T>)ExpandToIntegerRange(nonNegativeThis)).Intersect(o);


ExpandToIntegerRange

It feels like when we are intersecting a non-negative value set with full value set the result will be a set of non-negative values. Therefore, I think we should "shrink" the full value set to a non-negative set instead. #Closed

I've tested this suggestion but it has no visible effect (what matters is the set of remaining values being tracked, which are the same in both designs). I think I prefer only using one adjustment (expansion) for both union and intersection, as it's a bit simpler.

AlekseyTs · 2024-02-16T15:43:09Z

src/Compilers/CSharp/Portable/Utilities/ValueSetFactory.NumericValueSet.cs

+                // But we need to upgrade them to regular integers to perform operations against full integer sets.
+                if (this is NumericValueSet<int, NonNegativeIntTC> nonNegativeThis && o is NumericValueSet<int, IntTC>)
+                {
+                    return ((IValueSet<T>)ExpandToIntegerRange(nonNegativeThis)).Union(o);


ExpandToIntegerRange

I am not sure what is the scenario for hitting this code path (what new test needs this change?), but it is quite possible that we don't want to expand the set in this case either. We determined that the property doesn't have negative values, therefore, it doesn't seem useful to include negative values into the set of possible values, even if the other check didn't eliminate them on its own. #Closed

We need to modify the Union logic for the following tests: MixedCountPatterns_ReverseOrder, MixedCountPatterns_NegativeTestAfterRegularPropertyPattern and MixedCountPatterns_NegativeTest (which otherwise fail with a cast exception as we try to union sets of different kinds).
That said, it looks like I only managed to hit the second branch (below). I'll see if I can manage to hit this first branch too.

I've removed the handling of two mirror cases that I don't think we need after all. Thanks

AlekseyTs · 2024-02-16T15:43:55Z

Done with review pass (commit 3)

AlekseyTs · 2024-02-16T15:58:26Z

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

+}
+""";
+        CompileAndVerify(source, expectedOutput: "012").VerifyDiagnostics(
+            // (13,18): warning CS8509: The switch expression does not handle all possible values of its input type (it is not exhaustive). For example, the pattern '{ Count: 1 }' is not covered.


// (13,18): warning CS8509: The switch expression does not handle all possible values of its input type (it is not exhaustive). For example, the pattern '{ Count: 1 }' is not covered.

This warning looks unexpected to me. Both Count references refer to the same ICollection.Count property on the same instance. We checked it for 0 and for values >0, therefore '{ Count: 1 }' is also covered. #Closed

We only checked for values >0 when the type is IList, so not for the general case.

We only checked for values >0 when the type is IList, so not for the general case.

Yes, I missed the fact. Thanks.

alrz · 2024-02-22T10:00:02Z

src/Compilers/CSharp/Portable/Utilities/ValueSetFactory.NumericValueSet.cs

@@ -164,6 +164,17 @@ public IValueSet<T> Complement()

            public IValueSet<T> Intersect(IValueSet<T> o)
            {
+                // We use non-negative integers for Count/Length on types that list-patterns can be used on (ie.countable and indexable ones).
+                // But we need to upgrade them to regular integers to perform operations against full integer sets.
+                if (this is NumericValueSet<int, NonNegativeIntTC> nonNegativeThis && o is NumericValueSet<int, IntTC>)


There's one place that a similar situation is handled (though this is a test while we want to track an evaluation):

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

Lines 929 to 933 in b0ebfe7

if (foundExplicitNullTest && d is BoundDagNonNullTest { IsExplicitTest: false } t)

{

// Turn an "implicit" non-null test into an explicit one

state.SelectedTest = new BoundDagNonNullTest(t.Syntax, isExplicitTest: true, t.Input, t.HasErrors);

}

~~I think a flag could find IsLengthOrCount=true and tweak the selected node with a matching IsLengthOrCount.~~

(edit: I missed the fact that value set is not affected there unless Test.Input is changed)

AlekseyTs · 2024-02-23T17:11:37Z

src/Compilers/CSharp/Portable/Utilities/ValueSetFactory.NumericValueSet.cs

@@ -48,6 +49,8 @@ internal NumericValueSet(ImmutableArray<(T first, T last)> intervals)
                }
 #endif
                _intervals = intervals;
+                _tc = tc;
+                _numericValueSetFactory = new NumericValueSetFactory<T, TTC>(tc);


_numericValueSetFactory

Since this is a struct, we should be able to create it on the fly right when it is needed. #Closed

AlekseyTs · 2024-02-23T17:21:52Z

src/Compilers/CSharp/Portable/Utilities/ValueSetFactory.FloatingValueSet.cs

@@ -18,32 +18,33 @@ internal static partial class ValueSetFactory
        /// </summary>
        /// <typeparam name="TFloating">A floating-point type.</typeparam>
        /// <typeparam name="TFloatingTC">A typeclass supporting that floating-point type.</typeparam>
-        private sealed class FloatingValueSet<TFloating, TFloatingTC> : IValueSet<TFloating> where TFloatingTC : struct, FloatingTC<TFloating>
+        private sealed class FloatingValueSet<TFloating, TFloatingTC> : IValueSet<TFloating> where TFloatingTC : class, FloatingTC<TFloating>


TFloatingTC

I think we should be able to get rid of this type parameter #Closed

AlekseyTs · 2024-02-23T17:26:08Z

src/Compilers/CSharp/Portable/Utilities/ValueSetFactory.FloatingValueSetFactory.cs

@@ -8,35 +8,38 @@ namespace Microsoft.CodeAnalysis.CSharp
 {
    internal static partial class ValueSetFactory
    {
-        private sealed class FloatingValueSetFactory<TFloating, TFloatingTC> : IValueSetFactory<TFloating> where TFloatingTC : struct, FloatingTC<TFloating>
+        private sealed class FloatingValueSetFactory<TFloating, TFloatingTC> : IValueSetFactory<TFloating> where TFloatingTC : class, FloatingTC<TFloating>


TFloatingTC

I think we should be able to get rid of this type parameter #Closed

AlekseyTs · 2024-02-23T17:30:36Z

src/Compilers/CSharp/Portable/Utilities/ValueSetFactory.IntTC.cs

@@ -46,7 +58,7 @@ int INumericTC<int>.Next(int value)

            int INumericTC<int>.Prev(int value)
            {
-                Debug.Assert(value != int.MinValue);
+                Debug.Assert(value != (nonNegative ? 0 : int.MinValue));


(nonNegative ? 0 : int.MinValue)

MinValue? #Closed

MinValue is a private interface implementation

MinValue is a private interface implementation

It doesn't have to be private. And we can avoid code duplication.

AlekseyTs · 2024-02-23T17:34:14Z

src/Compilers/CSharp/Portable/Utilities/ValueSetFactory.NintValueSetFactory.cs

@@ -13,6 +13,7 @@ internal static partial class ValueSetFactory
        private sealed class NintValueSetFactory : IValueSetFactory<int>, IValueSetFactory
        {
            public static readonly NintValueSetFactory Instance = new NintValueSetFactory();
+            private static readonly NumericValueSetFactory<int, IntTC> s_numericValueSetFactory = new NumericValueSetFactory<int, IntTC>(IntTC.DefaultInstance);


s_numericValueSetFactory

Is it worth caching this struct value? #Closed

AlekseyTs · 2024-02-23T17:35:41Z

src/Compilers/CSharp/Portable/Utilities/ValueSetFactory.NonNegativeIntValueSetFactory.cs

@@ -15,49 +15,50 @@ private sealed class NonNegativeIntValueSetFactory : IValueSetFactory<int>
        {
            public static readonly NonNegativeIntValueSetFactory Instance = new NonNegativeIntValueSetFactory();

-            private NonNegativeIntValueSetFactory() { }
+            private static readonly IValueSetFactory<int> s_underlying = new NumericValueSetFactory<int, IntTC>(IntTC.NonNegativeInstance);


new NumericValueSetFactory<int, IntTC>(IntTC.NonNegativeInstance)

Is it worth caching this struct value? #Closed

We're caching the boxed struct

AlekseyTs · 2024-02-23T17:37:51Z

src/Compilers/CSharp/Portable/Utilities/ValueSetFactory.NuintValueSetFactory.cs

@@ -13,6 +13,7 @@ internal static partial class ValueSetFactory
        private sealed class NuintValueSetFactory : IValueSetFactory<uint>, IValueSetFactory
        {
            public static readonly NuintValueSetFactory Instance = new NuintValueSetFactory();
+            private static readonly NumericValueSetFactory<uint, UIntTC> s_numericValueSetFactory = new NumericValueSetFactory<uint, UIntTC>(UIntTC.Instance);


new NumericValueSetFactory<uint, UIntTC>(UIntTC.Instance);

Is it worth caching this struct value? #Closed

AlekseyTs · 2024-02-23T17:45:48Z

Done with review pass (commit 12). It looks like the title and the description are obsolete now. Make sure to adjust them during merging.

AlekseyTs · 2024-02-23T18:06:27Z

src/Compilers/CSharp/Portable/Utilities/ValueSetFactory.IntTC.cs

        {
-            int INumericTC<int>.MinValue => int.MinValue;
+            // Note: whenever we intersect or union two sets of IntTCs,
+            // we just keep the nonNegative flag of the set we're merging into.


// we just keep the nonNegative flag of the set we're merging into.

If we are uncomfortable relying on this behavior, we could add Intersect/Union APIs to INumericTC and implement them for this type by preferring non-negative flavor among intersected, and preferring full-set among union-ed. NumericValueSet.Intersect/Union will call the same named APIs to get the resulting INumericTC to use. #WontFix

AlekseyTs · 2024-02-23T18:07:58Z

src/Compilers/CSharp/Portable/Utilities/ValueSetFactory.IntTC.cs

-            int INumericTC<int>.MinValue => int.MinValue;
+            // Note: whenever we intersect or union two sets of IntTCs,
+            // we just keep the nonNegative flag of the set we're merging into.
+            public bool nonNegative;


public bool nonNegative;

I really like the fact that we no longer have to use different types to represent this difference in behavior #Closed

AlekseyTs · 2024-02-24T01:26:55Z

@jcouv It looks like the build is broken

AlekseyTs · 2024-02-24T01:31:41Z

src/Compilers/CSharp/Portable/Utilities/ValueSetFactory.DecimalValueSetFactory.cs

@@ -12,20 +12,20 @@ private sealed class DecimalValueSetFactory : IValueSetFactory<decimal>, IValueS
        {
            public static readonly DecimalValueSetFactory Instance = new DecimalValueSetFactory();

-            private readonly IValueSetFactory<decimal> _underlying = NumericValueSetFactory<decimal, DecimalTC>.Instance;
+            private readonly IValueSetFactory<decimal> _underlying = new NumericValueSetFactory<decimal>(DecimalTC.Instance);


new NumericValueSetFactory(DecimalTC.Instance);

Is this a struct? Is it worth caching its value? #Closed

Is this a struct? Is it worth caching its value?

I see, we are caching an interface instance not a struct value directly

AlekseyTs · 2024-02-24T01:52:57Z

Done with review pass (commit 17)

jcouv · 2024-02-26T21:15:46Z

@dotnet/roslyn-compiler for another review. I updated OP to reflect the new design.
In short, we're removing the use of type parameters to parameterize algorithms in patterns "value sets", instead using actual parameters. We remove NonNegativeIntTC and add a nonNegative flag to IntTC instead. This way we no longer have the problem of computing intersection/union of sets using NonNegativeIntTC and IntTC, as they both use IntTC.

AlekseyTs

LGTM (commit 19)

jcouv · 2024-02-28T23:35:52Z

@dotnet/roslyn-compiler for second review. Thanks

jcouv · 2024-03-04T20:12:23Z

@dotnet/roslyn-compiler for second review. Thanks

src/Compilers/CSharp/Portable/Utilities/ValueSetFactory.NumericValueSet.cs

…cValueSet.cs Co-authored-by: Rikki Gibson <rikkigibson@gmail.com>

jcouv added the Area-Compilers label Feb 6, 2024

jcouv self-assigned this Feb 6, 2024

dotnet-issue-labeler bot added the untriaged Issues and PRs which have not yet been triaged by a lead label Feb 6, 2024

Patterns: Expand non-negative sets when performing certain operations

45bd785

jcouv force-pushed the mixed-count branch from efaf8b2 to 45bd785 Compare February 6, 2024 05:41

jcouv marked this pull request as ready for review February 6, 2024 15:35

jcouv requested a review from a team as a code owner February 6, 2024 15:35

jcouv marked this pull request as draft February 6, 2024 15:42

Add test

9f74cdb

jcouv marked this pull request as ready for review February 6, 2024 18:22

Fix test

af16224

jcouv requested a review from 333fred February 8, 2024 22:18

AlekseyTs reviewed Feb 16, 2024

View reviewed changes

alrz reviewed Feb 22, 2024

View reviewed changes

jcouv added 2 commits February 22, 2024 12:07

Add tests and experiment with shrinking

1bb85ae

Stick with expansion

e6da7f4

jcouv requested a review from AlekseyTs February 22, 2024 20:14

Use instances for INumericTC implementations

e0597e4

AlekseyTs reviewed Feb 23, 2024

View reviewed changes

jcouv added 5 commits February 23, 2024 11:43

Remove TTC type parameters

d617b80

restore NonNegativeIntValueSetFactory

aa29183

Add assertions

355aa2c

Address feedback

78678e6

Restore cached interface in NonNegativeIntValueSetFactory

043cebb

jcouv changed the title ~~Patterns: Expand non-negative sets when performing certain operations~~ Patterns: Allow non-negative and full integer sets to merge (intersect or union) Feb 23, 2024

AlekseyTs reviewed Feb 24, 2024

View reviewed changes

jcouv added 2 commits February 26, 2024 08:29

Typo

768d63f

Re-use MinValue

58ce1ee

AlekseyTs approved these changes Feb 26, 2024

View reviewed changes

RikkiGibson self-assigned this Mar 6, 2024

RikkiGibson approved these changes Mar 6, 2024

View reviewed changes

src/Compilers/CSharp/Portable/Utilities/ValueSetFactory.NumericValueSet.cs Outdated Show resolved Hide resolved

Update src/Compilers/CSharp/Portable/Utilities/ValueSetFactory.Numeri…

8bb84f9

…cValueSet.cs Co-authored-by: Rikki Gibson <rikkigibson@gmail.com>

jcouv enabled auto-merge (squash) March 6, 2024 02:41

jcouv merged commit 3ae3ef9 into dotnet:main Mar 6, 2024
24 checks passed

	if (foundExplicitNullTest && d is BoundDagNonNullTest { IsExplicitTest: false } t)
	{
	// Turn an "implicit" non-null test into an explicit one
	state.SelectedTest = new BoundDagNonNullTest(t.Syntax, isExplicitTest: true, t.Input, t.HasErrors);
	}

Patterns: Allow non-negative and full integer sets to merge (intersect or union) #71968

Patterns: Allow non-negative and full integer sets to merge (intersect or union) #71968

Conversation

jcouv commented Feb 6, 2024 • edited Loading

alrz commented Feb 6, 2024

jcouv commented Feb 6, 2024

jcouv commented Feb 6, 2024

CyrusNajmabadi commented Feb 6, 2024

jcouv commented Feb 6, 2024

CyrusNajmabadi commented Feb 6, 2024

jcouv commented Feb 6, 2024

alrz commented Feb 7, 2024

jcouv commented Feb 15, 2024

AlekseyTs Feb 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Feb 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs commented Feb 16, 2024

AlekseyTs Feb 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alrz Feb 22, 2024 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Feb 23, 2024 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Feb 23, 2024 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Feb 23, 2024 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Feb 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Feb 23, 2024 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Feb 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Feb 23, 2024 • edited Loading

Choose a reason for hiding this comment

AlekseyTs commented Feb 23, 2024

AlekseyTs Feb 23, 2024 • edited by jcouv Loading

Choose a reason for hiding this comment

AlekseyTs Feb 23, 2024 • edited Loading

Choose a reason for hiding this comment

AlekseyTs commented Feb 24, 2024

AlekseyTs Feb 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs commented Feb 24, 2024

jcouv commented Feb 26, 2024

AlekseyTs left a comment

Choose a reason for hiding this comment

jcouv commented Feb 28, 2024

jcouv commented Mar 4, 2024

jcouv commented Feb 6, 2024 •

edited

Loading

AlekseyTs Feb 16, 2024 •

edited

Loading

AlekseyTs Feb 16, 2024 •

edited

Loading

AlekseyTs Feb 16, 2024 •

edited

Loading

alrz Feb 22, 2024 •

edited

Loading

AlekseyTs Feb 23, 2024 •

edited

Loading

AlekseyTs Feb 23, 2024 •

edited

Loading

AlekseyTs Feb 23, 2024 •

edited

Loading

AlekseyTs Feb 23, 2024 •

edited

Loading

AlekseyTs Feb 23, 2024 •

edited

Loading

AlekseyTs Feb 23, 2024 •

edited

Loading

AlekseyTs Feb 23, 2024 •

edited

Loading

AlekseyTs Feb 23, 2024 •

edited by jcouv

Loading

AlekseyTs Feb 23, 2024 •

edited

Loading

AlekseyTs Feb 24, 2024 •

edited

Loading