MSTEST0039 CodeFix adds redundant (& unexpected) Discard-Operators

[LukasGelke]

Describe the bug

The CodeFix for MSTEST0039 adds some additional Discards, even if one is already present.

It also adds a Discard to an AssignmentOperation. Yes assignments do emit their value, but this (at least by us) is used rather rarely.

Steps To Reproduce

[TestClass]
public sealed class Test1
{
  [TestMethod]
  public void TestMethod1()
  {
    int[] numbers = [1];

    Assert.ThrowsException<ArgumentException>(() => Error(1));
    Assert.ThrowsException<ArgumentException>(() => GetValue(1));
    Assert.ThrowsException<ArgumentException>(() => _ = GetValue(1));
    Assert.ThrowsException<ArgumentException>(() => new Helper());
    Assert.ThrowsException<ArgumentException>(() => _ = new Helper());
    Assert.ThrowsException<ArgumentException>(() => numbers[0] = 4);
  }

  private void Error(object o) => _ = o;
  private int GetValue(object o)
  {
    _ = o;
    return 1;
  }

  private sealed class Helper
  {

  }
}

after fix all:

    Assert.ThrowsExactly<ArgumentException>(() => Error(1));
    Assert.ThrowsExactly<ArgumentException>(() => _ = GetValue(1));
    Assert.ThrowsExactly<ArgumentException>(() => _ = _ = GetValue(1));
    Assert.ThrowsExactly<ArgumentException>(() => _ = new Helper());
    Assert.ThrowsExactly<ArgumentException>(() => _ = _ = new Helper());
    Assert.ThrowsExactly<ArgumentException>(() => _ = numbers[0] = 4);

Expected behavior

Only add Discards where appropriate/excepted (?)

Actual behavior

Adds Discard to every Operation/Expression that emits some value. It ignores Discards already present

Additional context

Packages: MsTest v3.8.2

[nohwnd]

Reasons for why this was added are in comment here: https://github.com/microsoft/testfx/blame/7d906dd55222ce529b7ca7dfc8ec8368608b846b/src/Analyzers/MSTest.Analyzers/UseNewerAssertThrowsAnalyzer.cs#L66-L72

@Youssef1313 maybe it would be valid to add the Func<object?> overload back to the new assertions so we avoid the need for the discard?

Or was this analyzed and there was a reason why it should not be done?

[LukasGelke]

oh, ok. I guess just having a single Action halves the number of methods.

From my little Testing, it seems only stuff like Field/Property-Access needs Discard. A method with return value works Just fine, ie:

But I don't mind having discards per-se.
No matter what the reason/solution is then: at least those double-discards should be avoided?
Yes it compiles, and does not break tests, but it just "doesn't look right".
At least a simple "find and replace" for _ = _ = works.

[Youssef1313]

The double discard is definitely something that can/should be fixed.

For adding Func<object?> overload, it's something we can do, but I'm not sure it provides much value given that a little _ = would get things to work.

[nohwnd]

The _ = discard is not super obvious from the current lightbulb fixes for cs0201, it suggests changing the return type of the test method.

Is it possible to specifically add code fix for that case?

[Youssef1313]

@nohwnd I opened an issue on Roslyn side to consider improving the codefix. Meanwhile, we may still consider adding the Func<object>.

[nohwnd]

Gotcha saw it. I think it would not hurt to add the overload, it is in place on the old assertions, and it would lower the friction for users when moving from the older assertions.

[nohwnd]

oh, ok. I guess just having a single Action halves the number of methods.

Sorry if that sounded like RTFM. That was not my intention, I was just documenting the reason why it is in place 🙂

[Youssef1313]

The codefix is being adjusted in #5117. For adding new overloads, opened #5119 to track that request.