False positive fix for "Remove the '?'"

[tenhobi]

If we have code like

UnknownType? getValue() {
  return null;
}

if (getValue() case final valueX?) {
  return Text(valueX);
}

When my UnknownType type is InvalidType because like in cases when maybe imports are wrong, or generated code is not generated, fix (we have auto fix on save) removes the ? while it is NOT CORRECT and will lead to errors when non null expected below.

The fix is there because of this code https://github.com/dart-lang/sdk/blob/main/pkg/analysis_server/lib/src/services/correction/dart/remove_question_mark.dart#L45 which is probably missing some check for InvalidType in which case it should not suggest the fix.

---

Edit: The same thing for null assert.

[dart-github-bot]

Summary: The "Remove the '?'" quick fix incorrectly suggests removing the optional type (?) from a variable declared in a case statement when the type is InvalidType due to missing imports or code generation issues. The fix should be disabled in such cases.

[tenhobi]

Previous issues with tests

I tried to implement this change to not suggest this fix (removing ?) when the type of condition is InvalidType.

Maybe something like this

    if (targetNode is NullCheckPattern) {
      var targetIfStatement = targetNode.enclosingIfStatement;
      var condition = targetIfStatement?.expression;
      // Ignore if the condition is of invalid type.
      if (condition?.staticType is InvalidType) {
        return;
      }

      var questionMark = targetNode.operator;
      await builder.addDartFileEdit(file, (builder) {
        builder.addDeletion(range.token(questionMark));
      });
    }

But the issue is with test -- I cannot figure out how to make it work. I tried something like this:

  Future<void> test_casePatternInvalidType() async {
    await resolveTestCode('''
UnknownType? getValue() => null;
void f() {
  if (getValue() case final valueX?) {
    print(valueX);
  }
}
''');
    await assertNoFix();
  }

When I try to model the example, and assert that no fix is suggested, it keeps crashing with "Undefined class 'InvalidType'":

00:00 +2 -1: RemoveQuestionMarkTest | test_casePatternInvalidType [E]

  Expected one error, found:
    /home/test/lib/test.dart(0..10): Undefined class 'UnknownType'. [CompileTimeErrorCode.UNDEFINED_CLASS]
    /home/test/lib/test.dart(46..56): Undefined class 'UnknownType'. [CompileTimeErrorCode.UNDEFINED_CLASS]
    /home/test/lib/test.dart(108..108): The null-check pattern will have no effect because the matched type isn't nullable. [StaticWarningCode.UNNECESSARY_NULL_CHECK_PATTERN]


  package:matcher/src/expect/expect.dart 149:31                                              fail
  pkg/analysis_server/test/src/services/correction/fix/fix_processor.dart 63:7               BaseFixProcessorTest._findErrorToFix
  pkg/analysis_server/test/src/services/correction/fix/fix_processor.dart 400:23             FixProcessorTest.assertNoFix
  pkg/analysis_server/test/src/services/correction/fix/remove_question_mark_test.dart 80:11  RemoveQuestionMarkTest.test_casePatternInvalidType

So if somebody can help me how I can do the test so I can ignore this error and test that in such cases no fix is suggested, I would appreciate it because then I will be able to finish a PR for this fix.

---

EDIT: I figured out I can put errorFilter there like

    await assertNoFix(
        errorFilter: (error) =>
            error.errorCode != CompileTimeErrorCode.UNDEFINED_CLASS);

[tenhobi]

Implemented in https://dart-review.googlesource.com/c/sdk/+/394403

[bwilkerson]

I don't know why it would be having a problem finding InvalidType. The only thing I can think of to suggest is to ensure that you didn't accidentally run dart pub in your checkout of the sdk. Doing so can sometimes cause problems like this.

But I'd like to suggest that the better fix would be to ensure that StaticWarningCode.UNNECESSARY_NULL_CHECK_PATTERN isn't reported in this situation. We shouldn't say that it can't be null when we don't know what type it is. (And the absence of the bogus diagnostic will also prevent the fix from being offered.)

[tenhobi]

@bwilkerson sorry for maybe misleading error message. The error was about my UnknownType (I just used InvalidType before directly first that is why it's confusing). I also noticed I can filter this error in the test.

But I will check that StaticWarningCode.UNNECESSARY_NULL_CHECK_PATTERN later. It sounds as a better approach. Thanks.

[tenhobi]

@bwilkerson I made changes so we don't report StaticWarningCode.UNNECESSARY_NULL_CHECK_PATTERN in this case. But I kept tests in remove ? since we probably wanna make sure this will not change. :)

[bwilkerson]

We generally don't have tests for situations that are known to not occur. I haven't looked at the updated CL yet, but if there are tests to ensure that the diagnostic isn't reported when it shouldn't be, then we don't need tests to ensure that the fix isn't offered in those cases.