[Wildcard Variables] `UNUSED_LOCAL_VARIABLE` support

[pq]

As per the discussion in https://github.com/dart-lang/linter/issues/4968, we want to start reporting UNUSED_LOCAL_VARIABLE for non wildcard underscore cases (e.g., __, ___, etc).

UPDATE: non-wildcard underscores will produce diagnostics but whether we want to start reporting on other identifiers (e.g., e or exception) is still an open question. (See proposal below.)

[pq]

Follow-up from https://github.com/dart-lang/sdk/issues/57132,

Proposal: report UNUSED_LOCAL_VARIABLE on all unused bound catch clause parameters

Specifically, this means we start flagging the following (which we don't today) with an UNUSED_LOCAL_VARIABLE diagnostic:

try {
} catch(e) { }

providing a quick-fix conversion to:

try {
} catch(_) { }

Since the idiom of catching (and ignoring) an exception named e or similar is well established, this will produce a bunch of diagnostics in some projects but with dart fix code can effortlessly be migrated.

/cc @dart-lang/language-team @dart-lang/analyzer-team

[kallentu]

This resurfaced while testing const evaluation in the analyzer, in a comment here.

We should probably remove the UNUSED_LOCAL_VARIABLE diagnostic that's being reported here and other places.

test() {
  const _ = true; // Should not have `UNUSED_LOCAL_VARIABLE`, but currently does.
  const c = _;
}

EDIT: WAI; see below.

[pq]

Revisiting this case, I think it's actually working as intended and we're getting tripped up by the test framework.

Here's the test:

  test_visitSimpleIdentifier_wildcard_local() async {
    await assertErrorsInCode(r'''
test() {
  const _ = true;
  const c = _;
}
''', [
      error(WarningCode.UNUSED_LOCAL_VARIABLE, 35, 1),
      error(CompileTimeErrorCode.UNDEFINED_IDENTIFIER, 39, 1),
      error(CompileTimeErrorCode.CONST_INITIALIZED_WITH_NON_CONSTANT_VALUE, 39,
          1),
    ]);
  }

The unused variable being reported is actually c (the _ is at offset 17).

Offset-based tests test expectations are easy to get tripped up by!

@kallentu: do you recall the intention of the test?

If it's valuable as-is, we could make it a little more explicit by updating the expectation like so:

error(WarningCode.UNUSED_LOCAL_VARIABLE, 35, 1, messageContains: ["'c'"]),

[kallentu]

Ah, that makes a lot more sense. Woops, sorry for the noise.

The test was mainly trying to make sure we would produce an error if we tried to access/use the const _. I think it's useful to have still.