Add is not type constraint (different from is not pattern) [breaking change!]

[seanblue]

See this topic for full details and previous discussion: dotnet/roslyn#50671

Since you can read the full discussion in the other github issue, here's a summary:
is can currently be two things: a type check and a pattern. On the other hand, is not can only be a pattern. In theory you would think this shouldn't matter, but it actually results in odd situations where there is inconsistent behavior. I brought it up in the above issue as a bug, but I was told it was implemented correctly to the spec, so this discussion is to consider changing the spec to include an is not type operator instead of relying on the is not pattern, so that it works consistently with the is type operator.

Here's what it boils down to though:

public class SomeType
{
}

public class Version1
{
	private const string SomeType = "some type";

	public void Execute(object data)
	{
		if (data is SomeType)
			Console.WriteLine("data is SomeType");
		else
			Console.WriteLine("data is not SomeType");
	}
}

new Version1().Execute(new SomeType()); // data is SomeType
new Version1().Execute(""); // data is not SomeType
new Version1().Execute("some type"); // data is not SomeType

// THIS CASE MAKES NO SENSE
public class Version2
{
	private const string SomeType = "some type";

	public void Execute(object data)
	{
		if (data is not SomeType)
			Console.WriteLine("data is not SomeType");
		else
			Console.WriteLine("data is SomeType");
	}
}

new Version2().Execute(new SomeType()); // data is not SomeType
new Version2().Execute(""); // data is not SomeType
new Version2().Execute("some type"); // data is SomeType

public class Version3
{
	private const string SomeType = "some type";

	public void Execute(object data)
	{
		switch(data)
		{
			case SomeType:
				Console.WriteLine("data is SomeType");
				break;
			default:
				Console.WriteLine("data is not SomeType");
				break;
		};
	}
}

new Version3().Execute(new SomeType()); // data is not SomeType
new Version3().Execute(""); // data is not SomeType
new Version3().Execute("some type"); // data is SomeType

public class Version4
{
	private const string SomeType = "some type";

	public void Execute(object data)
	{
		switch(data)
		{
			case not SomeType:
				Console.WriteLine("data is not SomeType");
				break;
			default:
				Console.WriteLine("data is SomeType");
				break;
		};
	}
}

new Version4().Execute(new SomeType()); // data is not SomeType
new Version4().Execute(""); // data is not SomeType
new Version4().Execute("some type"); // data is SomeType

Demo here

---

Now that that long example is out of the way, here's what's going on. We're trying to check against the type, but because there is a const with the same name, the const is taking priority. (If it's something other than a const, it actually fails to compile, as detailed in the other github issue.) If there wasn't a const or other member with the type name in scope, it would properly check against the type. The main thing I take issue with is that if (data is not SomeType) doesn't work the same as if (data is SomeType). You should be able to invert a condition/block knowing that the behavior will perfectly match. In fact Visual Studio currently gives suggestions to invert conditions which could result in different or uncompilable code. If we add an is not type operator to be the parallel to the is type operator instead of relying on the is not pattern, these if conditions would both check the type, even if a member with the same name was in scope.

There is however, a major drawback to this proposal: It would be a breaking change. In cases where it doesn't compile (e.g. if you had a property with the same name as the type), no problem. But if you did have a const and were doing if (data is not SomeType), it would change behavior from checking against the const to checking against the type. For what it's worth in the issue on the roslyn github, there were views that this behavior was unintended and that a breaking change would likely be acceptable in this case. That is, until they determined it was implemented to the spec and told me to move the discussion over here. The main use case here is when you have a property returning a type and the property has the same name as the type. I don't know how often this comes up, but I did encounter it, which is how I first found this oddity.

I'm curious to here what others think about this issue.

[HaloFour]

Seems like you can force it with:

if (o is not SomeType _)

But it does feel weird that is prioritizes matching against the type whereas is not prioritizes matching against the constant. You'd expect the not pattern to be the inverse of the existing type pattern, not something else entirely.

[RikkiGibson]

I am wondering if it's possible/desirable to use different rules when binding a pattern within an 'is' versus a pattern within a switch statement or switch expression. Basically, prefer types over constants in 'is', but prefer constants over types in 'switch'.

It does feel like a fairly significant breaking change at this point, though.

[seanblue]

@HaloFour It's true that you can force it by using the discard, but it feels so strange. Also, I think at that point, many people desiring clean code would remove the "unnecessary" discard, and be surprised to see it changed behavior.

It does feel like a fairly significant breaking change at this point, though.

@RikkiGibson I guess it's a question of whether this is one of those "we're fixing likely bugs" breaking changes. How many people are using the a is not myConst syntax rather than a != myConst? How many people are doing this in a situation where the const has the same name as a type? Unless we can get some kind of statistics around that, it's hard to know how big a deal changing this would be.

I'd also like to reiterate (since I only glossed over it in the original post) that while data is not SomeType selects the const over the type if SomeType is a const in scope, it will simply fail to compile if there is a non-const member with that name. It would be nice to at least get those cases compiling and handled properly. If data is SomeType selects the type when there's a property called SomeType, it seems really odd to fail to compile by doing data is not SomeType.