IDE0016 false positive

[ljw1004]

Version Used: VS2017 15.1 (26403.3) Release

Steps to Reproduce:

Do File>New>C# Console App and type this code.

Expected Behavior:

You should either offer this quickfix which preserves the meaning of the code

void Foo(object o) {
  var s = o as string ?? throw new InvalidArgumentException("expected string");
  _i = 1;
  _s = s;
}

Or if you continue to offer the current quickfix, then it should NOT be accompanied by grayed-out text and a message in the ErrorList.

Actual Behavior:

There is something in the Error List, and the "if null" check is grayed out. If I apply the fix then I get this:

void Foo(object o) {
  var s = o as string;
  _i = 1;
  _s = s ?? throw new InvalidOperationException("expected string");
}

Look, I know you recently closed this as "won't fix" in #17185 but I think you made the wrong call and I want to re-raise the issue. @Pilchie @gafter @CyrusNajmabadi

My code is perfectly good. If I apply your current quickfix change then it will change the meaning of the program, and in my case it introduced a semantic bug.

Moreover, the refactored code after applying the quickfix for IDE0016 actually looked worse than the code before. It's a well-established convention that we do argument validation at the top of our method, and then write the body cleanly. The refactoring has broken that convention by shifting argument validation inside the logic.

Look, I understand that "throw is an exception" is a lovely new feature, and you want to educate folks on how to use it. But this is too heavy-handed. It's not acceptable to pollute my error list with messages that result in incorrect and uglier code. By all means please offer it as a lightbulb without graying out my text and without showing in the error list. (I don't know how to disable built-in warnings within VS, and I shouldn't have to figure out how just to compensate for too much heavy-handedness).

Your objective in introducing such a heavy-handed warning is to educate people about the new feature so they end up using it more generally. You can still get this user-education benefit if there are common code patterns that folks write which would assuredly benefit from the refactoring, and then you limit your heavy-handedness to just those places. I think there is at least one common pattern: an as test followed by a throw:

void f(object o) {
  var s = o as string;
  if (s == null) throw new InvalidArgumentException();
  _s = s;
}

==>

void f(object o) {
  var s = o as string ?? throw new InvalidArgumentException();
  _s = s;
}

// NOTE: don't you think the above correct refactoring is much more beautiful than
// IDE0016's currently incorrect refactoring? ...
void f(object o) {
  var s = o as string;
  _i = 1;
  _s = s ?? throw new InvalidOperationException("expected string");
}

Just to be clear: I believe you should (1) continue to offer the lightbulb in all cases you can find, (2) limit the greying-out and message display to fewer idioms such as the one above, (3) change the quickfix as per the above.

[CyrusNajmabadi]

Hi lucian. Great to hear from you. This was fixed with: #18431

[CyrusNajmabadi]

A few notes: #17185 is a different sort of case, and we believe it is reasonable to offer the feature in that sort of case.

Your case is clearly one where important semantics where changed, and that is def not good.

Moreover, the refactored code after applying the quickfix for IDE0016 actually looked worse than the code before.

We agree. :)

It's not acceptable to pollute my error list with messages that result in incorrect and uglier code.

We agree.

You can still get this user-education benefit if there are common code patterns that folks write which would assuredly benefit from the refactoring, and then you limit your heavy-handedness to just those places.

We agree. That's the intended design of the feature. What you ran into was the implementation not matching our intended design.

--

I want to be clear, we did not close the linked bug with the intent of saying that the feature was flawless and that and that everything it was doing was appropriate. We closed the linked bug because that specific issue that was reported was something we felt was an acceptable and appropriate change for the feature to be suggesting. Your issue is not hte same as the linked bug. In this case the feature was not behaving as intended, and it was just a logic bug that led to a suboptimal result. You should see this fix, along with several others in this area for the next VS release.

[ljw1004]

Hmm... if I understand right, the fix would disable the message in this case?

Is it possible to instead show the message and suggest the quickfix I adopted?

[CyrusNajmabadi]

yes. It's possible. But it's a feature request for the existing feature to detect and offer that sort of change based on the pattern you have. I'm definitely totally supportive of us detecting and offering fixes in more cases. :)

If possible though i would prefer that be tracked in another issue. Or i would like it if this issue were made clearer. Right now it's both a bug report about something that has been fixed, and a feature request for new functionality. Having them both together makes things harder to track and makes the conversation/discussion around this muddied (IMO at least).

[ljw1004]

Okay, thanks Cyrus! Will do.

[CyrusNajmabadi]

Also, please let me know what additional patterns you'd like to see detected. As an example, i know i want to add support for finding:

if (!(o is T))
    return;

var t = (T)o;

And suggesting:

if (!(o is T t))
    return;

This would be under another option though as i can see some people not likely the potentially clutered look of if (!(o is T t))