Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 20 commits into from
Mar 6, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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>)
Copy link
Member

@alrz alrz Feb 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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)

{
return ((IValueSet<T>)ExpandToIntegerRange(nonNegativeThis)).Intersect(o);
Copy link
Contributor

@AlekseyTs AlekseyTs Feb 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
else if (o is NumericValueSet<int, NonNegativeIntTC> nonNegativeOther && this is NumericValueSet<int, IntTC>)
{
return ((IValueSet<T>)ExpandToIntegerRange(nonNegativeOther)).Intersect(this);
}

var other = (NumericValueSet<T, TTC>)o;
TTC tc = default;
var builder = ArrayBuilder<(T first, T last)>.GetInstance();
Expand Down Expand Up @@ -203,6 +214,12 @@ public IValueSet<T> Intersect(IValueSet<T> o)
}

return new NumericValueSet<T, TTC>(builder.ToImmutableAndFree());

}

jcouv marked this conversation as resolved.
Show resolved Hide resolved
private static IValueSet<int> ExpandToIntegerRange(NumericValueSet<int, NonNegativeIntTC> nonNegativeThis)
{
return new NumericValueSet<int, IntTC>(nonNegativeThis._intervals);
}

/// <summary>
Expand Down Expand Up @@ -243,6 +260,17 @@ private static T Max(T a, T b)

public IValueSet<T> Union(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>)
{
return ((IValueSet<T>)ExpandToIntegerRange(nonNegativeThis)).Union(o);
Copy link
Contributor

@AlekseyTs AlekseyTs Feb 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
else if (o is NumericValueSet<int, NonNegativeIntTC> nonNegativeOther && this is NumericValueSet<int, IntTC>)
{
return ((IValueSet<T>)ExpandToIntegerRange(nonNegativeOther)).Union(this);
}

var other = (NumericValueSet<T, TTC>)o;
TTC tc = default;
var builder = ArrayBuilder<(T first, T last)>.GetInstance();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8915,4 +8915,211 @@ public void NotExhaustive_LongList()
// _ = a switch { { Length: < 1000 } => 0 };
Diagnostic(ErrorCode.WRN_SwitchExpressionNotExhaustive, "switch").WithArguments("{ Length: 1000 }").WithLocation(2, 7));
}

[Fact, WorkItem("https://github.com/dotnet/roslyn/issues/71660")]
public void MixedCountPatterns()
{
// One of the property patterns is treated as a non-negative Count pattern,
// while the other is a regular property pattern.
var source = """
using System;
using System.Collections;
using System.Collections.Generic;

System.Console.Write(select(new List<int>()));
System.Console.Write(select(new List<int>() { 42 }));
System.Console.Write(select(new C()));

int select(ICollection c)
{
try
{
return c switch
{
{ Count: 0 } => 0,
IList { Count: > 0 } => 1,
};
}
catch
{
return 2;
}
}

class C : System.Collections.ICollection
{
public int Count => 1;
public object SyncRoot => throw new NotImplementedException();
public bool IsSynchronized => throw new NotImplementedException();
public void CopyTo(Array array, int index) => throw new NotImplementedException();
public IEnumerator GetEnumerator() => throw new NotImplementedException();
}
""";
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.
Copy link
Contributor

@AlekseyTs AlekseyTs Feb 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// (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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Yes, I missed the fact. Thanks.

// return c switch
Diagnostic(ErrorCode.WRN_SwitchExpressionNotExhaustive, "switch").WithArguments("{ Count: 1 }").WithLocation(13, 18)
);
}

[Fact, WorkItem("https://github.com/dotnet/roslyn/issues/71660")]
public void MixedCountPatterns_ReverseOrder()
{
var source = """
using System;
using System.Collections;
using System.Collections.Generic;

System.Console.Write(select(new List<int>()));
System.Console.Write(select(new List<int>() { 42 }));
System.Console.Write(select(new C()));

int select(ICollection c)
{
try
{
return c switch
{
IList { Count: > 0 } => 1,
{ Count: 0 } => 0,
};
}
catch
{
return 2;
}
}

class C : System.Collections.ICollection
{
public int Count => 1;
public object SyncRoot => throw new NotImplementedException();
public bool IsSynchronized => throw new NotImplementedException();
public void CopyTo(Array array, int index) => throw new NotImplementedException();
public IEnumerator GetEnumerator() => throw new NotImplementedException();
}
""";
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.
// return c switch
Diagnostic(ErrorCode.WRN_SwitchExpressionNotExhaustive, "switch").WithArguments("{ Count: 1 }").WithLocation(13, 18)
);
}

[Fact, WorkItem("https://github.com/dotnet/roslyn/issues/71660")]
public void MixedCountPatterns_NegativeTest()
{
var source = """
using System.Collections;
using System.Collections.Generic;

var result = (ICollection)new List<int>() switch
{
IList { Count: > 0 } => throw null,
IList { Count: < 0 } => throw null,
{ Count: 0 } => "ran",
};

System.Console.Write(result);
""";
var comp = CreateCompilation(source);
comp.VerifyDiagnostics(
// (4,43): 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.
// var result = (ICollection)new List<int>() switch
Diagnostic(ErrorCode.WRN_SwitchExpressionNotExhaustive, "switch").WithArguments("{ Count: 1 }").WithLocation(4, 43),
// (7,5): error CS8510: The pattern is unreachable. It has already been handled by a previous arm of the switch expression or it is impossible to match.
// IList { Count: < 0 } => throw null,
Diagnostic(ErrorCode.ERR_SwitchArmSubsumed, "IList { Count: < 0 }").WithLocation(7, 5)
);
}

[Fact, WorkItem("https://github.com/dotnet/roslyn/issues/71660")]
public void MixedCountPatterns_NegativeTestAfterRegularPropertyPattern()
{
var source = """
using System.Collections;
using System.Collections.Generic;

_ = (ICollection)new List<int>() switch
{
IList { Count: > 0 } => 0,
{ Count: 0 } => 0,
IList { Count: < 0 } => 0,
};
""";
var comp = CreateCompilation(source);
comp.VerifyDiagnostics(
// (4,34): 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.
// _ = (ICollection)new List<int>() switch
Diagnostic(ErrorCode.WRN_SwitchExpressionNotExhaustive, "switch").WithArguments("{ Count: 1 }").WithLocation(4, 34),
// (8,5): error CS8510: The pattern is unreachable. It has already been handled by a previous arm of the switch expression or it is impossible to match.
// IList { Count: < 0 } => 0,
Diagnostic(ErrorCode.ERR_SwitchArmSubsumed, "IList { Count: < 0 }").WithLocation(8, 5)
);
}

[Fact, WorkItem("https://github.com/dotnet/roslyn/issues/71660")]
public void MixedCountPatterns_ExplicitICollectionType()
{
var source = """
using System;
using System.Collections;
using System.Collections.Generic;

System.Console.Write(select(new List<int>()));
System.Console.Write(select(new List<int>() { 42 }));
System.Console.Write(select(new C()));

int select(ICollection c)
{
try
{
return c switch
{
ICollection { Count: 0 } => 0,
IList { Count: 1 } => 1,
};
}
catch
{
return 2;
}
}

class C : System.Collections.ICollection
{
public int Count => 1;
public object SyncRoot => throw new NotImplementedException();
public bool IsSynchronized => throw new NotImplementedException();
public void CopyTo(Array array, int index) => throw new NotImplementedException();
public IEnumerator GetEnumerator() => throw new NotImplementedException();
}
""";
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.
// return c switch
Diagnostic(ErrorCode.WRN_SwitchExpressionNotExhaustive, "switch").WithArguments("{ Count: 1 }").WithLocation(13, 18)
);
}

[Fact, WorkItem("https://github.com/dotnet/roslyn/issues/71660")]
public void MixedCountPatterns_Subsumed()
{
var source = """
using System.Collections;

_ = (ICollection)null switch
{
{ Count: <0 or >0 } => 0,
IList { Count: >0 } => 1,
{ Count: 0 } => 2,
};
""";
var comp = CreateCompilation(source);
comp.VerifyDiagnostics(
// (6,5): error CS8510: The pattern is unreachable. It has already been handled by a previous arm of the switch expression or it is impossible to match.
// IList { Count: >0 } => 1,
Diagnostic(ErrorCode.ERR_SwitchArmSubsumed, "IList { Count: >0 }").WithLocation(6, 5)
);
}
}
Loading