❗️ (Unless not possible) it would be good to retain the original if statement as well as the new one.

I just changed this test to cover negative case.

➡️ Sounds good, thanks

❓ Is the ability to handle a cast to object new for both the normal and negated forms? If so, we should add a test for the newly supported normal form.

Yes, it's new, I'll add the test for (object)x != null as well.

💭 This will rule out the feature for many types. We may be able to improve the experience by allowing user-defined operators, but ensuring a single Fix All operation only corrects operations that use the same operator. For example:

• If all uses of == null and != null for a variable use the default operator, a Fix All on the variable will fix all cases where no user-defined operators are involved
• If one or more uses of == or != involve a user-defined operator for a variable with the declared type T, then a Fix All on that variable will fix all instances involving variables with declared type T, whether or not they use user-defined operators. Alternately, this scenario could fix all instances involving variables with declared type T which use one or more user-defined operators.

The equivalence ID of the code fix can be used to "bucket" fixes according to the above conditions.

If you like the idea but aren't ready to implement it, feel free to leave this as a follow-up issue by filing a new bug referencing this comment.

This is not part of this PR, the only diff here is notEquals -> comparison

Filed #23374

❓ To help me get my head around this, can you provide an example of why this block is necessary?

can you put this in as a comment.

Wait, this is only correct if if has an unreachable endpoint. I'll remove this check.

💭 This seems like an unnecessary change

Relates to #21962

I can revert it if you want.

is it worth to use Task.WhenAll here?

Nope. :)

This would be a candidate for using Theory instead of Fact. With that said, Theory is used sparingly (very rarely currently) and not everyone has agreed that it's a good idea. I'm only showing the difference here as a comparison. The one advantage I've found is a test failure shows more information about the specific failing case, as opposed to just a line number in a method with many tests.

[Theory, Trait(Traits.Feature, Traits.Features.CodeActionsInlineTypeCheck)]
[InlineData("x != null", "o is string x")]
[InlineData("null != x", "o is string x")]
[InlineData("(object)x != null", "o is string x")]
[InlineData("null != (object)x", "o is string x")]
// Other test cases added as additional attributes...
public async Task InlineTypeCheck1(string input, string output)
{
  await TestStatement($"if ({input}) {{ }}", $"if ({output}) {{ }}");
  await TestStatement($"var y = {input};", $"var y = {output};");
  await TestStatement($"return {input};", $"return {output};");
}

This would be a candidate for using Theory instead of Fact. With that said, Theory is used sparingly (very rarely currently) and not everyone has agreed that it's a good idea. I'm only showing the difference here as a comparison. The one advantage I've found is a test failure shows more information about the specific failing case, as opposed to just a line number in a method with many tests.

[Theory, Trait(Traits.Feature, Traits.Features.CodeActionsInlineTypeCheck)]
[InlineData("x != null", "o is string x")]
[InlineData("null != x", "o is string x")]
[InlineData("(object)x != null", "o is string x")]
[InlineData("null != (object)x", "o is string x")]
// Other test cases added as additional attributes...
public async Task InlineTypeCheck1(string input, string output)
{
  await TestStatement($"if ({input}) {{ }}", $"if ({output}) {{ }}");
  await TestStatement($"var y = {input};", $"var y = {output};");
  await TestStatement($"return {input};", $"return {output};");
}

📝 This test case covers the original InlineTypeCheck1.

📝 This test case covers the original InlineTypeCheckInvertedCheck1.

📝 This test case replaces the original TestMissingOnNullEquality, showing new functionality.

📝 This test case covers the original TestInlineNullCheck1.

❗️ Use an explicitly-typed variable rather than a cast. This ensures a compilation error occurs for any case where an implicit cast is not available (which in theory ensures the operation is a widening conversion, though we have cases even in this codebase where that is not the case).

💡 IsKind

I use Kind() where the code asserts that the variable is never null i.e the path is unreachable if it was. This is also the convention used on the compiler side.

I suppose on the IDE side we traditionally use IsKind to not throw if anything ever failed.

💡 definitely

💡 I find this block pleasantly readable, but it may be because I have context from seeing the recent bug. Consider expanding this comment by adding the following:

// For example:
//
// if ((x = o as string) != null || SomeExpression) {
//   // x would not be definitely assigned here if pattern matching were used
// }

💡 A comment regarding the return value would be handy

💡 Assuming my understanding is correct, the following might be good to add.

// For example:
//
// if (!(o is string x)) {
//   return;
// }
//
// // The 'return' statement above ensures x is definitely assigned here
// Console.WriteLine(x);

Done.

There is a bug here, should be missing but it's not:

    void M()
    {
        [|var|] x = o as string;
        if (x != null)
        {
        }
        Console.WriteLine(x);
        x = "";
    }

x is always assigned but there is a read before assignment - probably need to revert code to iterate over statements one by one, @CyrusNajmabadi, can you confirm if that was the original purpose of doing so instead of region analysis?

There is a bug here, should be missing but it's not:

It's missing for me. I'm not able to see this locally.

Note: iterating over statements is not safe because assignments could be anywhere in an expression,

Console.WriteLine(x = Read(x));

@CyrusNajmabadi I meant in this branch. The previous revision check for nodes statement by statement so doesn't have this bug.

However, with either approaches, this one is not missing.

    string Use(string x) => x;
    void M()
    {
        var x = o as string;
        if (x != null)
        {
        }
        Console.WriteLine(x = Use(x));
    }

From data flow analysis results, we can't see if the read is happened before write..

@CyrusNajmabadi, can you confirm if that was the original purpose of doing so instead of region analysis?

Yes, i believe that was the intent.

Note: iterating over statements is not safe because assignments could be anywhere in an expression,

I thought we checked assignment on a per-reference basis. So we would specifically check the Use(x) and see the problem. Right?

I think we return early in the previous code. we see the write first and bail. I'll try a mixed approach, if we are AlwaysAssgined, check if there is a read, if yes, need to walk through tree to see it doesn't come first. early return here could be safe.

Oh, i think i see what you're saying. The logic here is buggy:

Specifically, because it sees the x =, it assumes that 'x' is always assigned, and thus the change is safe. But it's not. Because the right side of the assignment executes first, and will do a read of the unassigned variable.

1. That sucks. But it's also a very corner case.
2. We could consider fixing it. Namely that if the reference is the left of an assign, that we have to ensure that the right side is fine as well. Annoying, but doable.

Note: using AlwaysAssigned is necessary to handle branches e.g. #21097

I see. The logic here has changed a bit, and i'll admit i don't really fully understand the algorithm as it stands now, so i'm not sure the exact right fix. But i do think this can be solved.

Also, an efficiency point:

                    var symbol = semanticModel.GetSymbolInfo(id, cancellationToken).GetAnySymbol();
                    if (localVariable.Equals(symbol))
                    {

We should check if the id.ValueText matches the local variable name before bothering to do the binding. That will avoid a lot of expensive semantic checks.

it woudl be helpful to have examples shown in the code of what this is catching.

it woudl be helpful to have examples shown in the code of what this is catching.