`unnecessary_underscores` false-positive when variable is referenced in docs

[FMorschel]

I'm not sure why anyone would ever mention a variable that starts with an underscore in a doc comment, but I believe that @bwilkerson's #61431 (comment) should be considered with unnecessary_underscores:

I could not see any mention of it at #56595.

/// This [__] here.
void f(int __) {
}

I don't feel like this would make a huge impact on our users, but it would make the implementation of #61431 easier by being sure the lint already handles this correctly.

WDYT @kallentu @pq?

I volunteer to make this change if you all agree to it.

[FMorschel]

Probably could also use #60420 when the above exists

[RohitSaily]

would it lint that underscores should not be referenced at all ie the entire reference should be removed? i'm seeing it as an attempt to use/reference a wildcard which is not supposed to be possible. it could be ambiguous eg

/// who knows what this [_] refers to
f(int _, String _)
{
}

[FMorschel]

No, it wouldn't.

1. The lint's only job is to warn against multiple _ to name a variable.
2. That would probably be a job for comment reference_to_private_declarations lint suggestion #60420 (mentioned in the comment above).

The only two reasons I'm suggesting that we stop warning here are:

1. Exactly your repro. If we rename so that more than one parameter is _, the docs don't know which one it references.
2. It would make the change for the quick-fix related to it to automatic (able to run dart fix), basically to create a new constructor (way easier).

[bwilkerson]

I wouldn't make the warning part of reference_to_private_declarations. Underscores aren't 'private declarations', they're wildcard variables, and I think we should just produce an enabled-by-default warning. If there is a wildcard variable being referenced in a doc comment reference then it would make sense to warn the user that the name can't be referenced (or is undefined) as we do for

void f(int _) {
  print(_);
}

[FMorschel]

But two or more underscores are not wildcard variables, right? Would you want to still trigger the same warning for those? (I've not verified anything related to this) - I'll open an issue later so we can place this discussion there and handle it separately.

[RohitSaily]

But two or more underscores are not wildcard variables, right?

just tested it and this is correct, they are not treated as wildcards directly by the language, but unnecessary_underscores considers them as intended wildcards? it's a bit confusing to me right now

---

it seems sensible to not lint unnecessary_underscores and do as @bwilkerson said ie

it would make sense to warn the user that the name can't be referenced (or is undefined) as we do for [...]

since even though it is technically not a wildcard it seems to be treated as if it were intended to be a wildcard (based on how unnecessary_underscores work)

---

but technically, these kinds of identifiers are only considered wildcards if unnecessary_underscores is used. otherwise without lints, the language recognizes them as normal declarations. so actually it should be part of unnecessary_underscores but the fix should be to remove it and not reference such a thing at all

[bwilkerson]

But two or more underscores are not wildcard variables, right?

Correct. They're normal variables.

Would you want to still trigger the same warning for those?

If you're asking whether the lint should fire for a multi-underscore parameter that's referenced in a doc comment, then no. It shouldn't report any variable if doing so would break an existing reference to the variable.

If you're asking whether we should report that a multi-underscore parameter that's referenced in a doc comment is undefined, then also no. It is defined and the reference should work as expected.

... but unnecessary_underscores considers them as intended wildcards? it's a bit confusing to me right now

Before the wildcard feature was added a single underscore was a valid variable name. By convention it indicated that the intent was to not reference it, and in cases where there were multiple such parameters in the same parameter list they all had to have unique names, so multiple underscores were used.

The lint just flags places where the use of multi-underscore names might be better represented by a real wildcard variable now that it's fine to have multiple wildcard parameters in the same parameter list.

[RohitSaily]

Before the wildcard feature was added a single underscore was a valid variable name. By convention it indicated that the intent was to not reference it, and in cases where there were multiple such parameters in the same parameter list they all had to have unique names, so multiple underscores were used.

The lint just flags places where the use of multi-underscore names might be better represented by a real wildcard variable now that it's fine to have multiple wildcard parameters in the same parameter list.

that makes sense. i still believe it makes sense to do as you said and have a warning that the comment reference should not be used at all but i think it should be part of unnecessary_underscores since that lint seems to be intended for checking for the aforementioned convention

[FMorschel]

I disagree. The unnecessary_underscores lint doesn't have to handle all of the problems related to the old convention. I believe that:

1. Yes, the reference to a wildcard variable in a doc comment should be warned
2. This lint shouldn't trigger for variables referenced in doc comments
3. We should have comment reference_to_private_declarations lint suggestion #60420 for warning about all private references (any variable that starts with _ falls into this category) on docs
4. Maybe a new lint to warn when we have the old convention positional-only-underscores variable being referenced only in docs, so people still get warned for (suggestive name can be changed - unnecessary_underscores_references):

/// We don't use [_] or [__].
void f(int _, String __) {}
//                   ^ unnecessary_underscores_references

[bwilkerson]

Maybe a new lint to warn when we have the old convention positional-only-underscores variable being referenced only in docs ...

What would the suggest fix be? The user can't convert it to a wildcard variable if they really want to reference it in the docs. That leaves using a name containing something other than underscores (which negates the signal the user was probably trying to send. I don't see a good alternative, so I don't think this is something we could reasonably lint for.

[FMorschel]

Yes, this is why I wrote maybe. If we have a warning about doc references to private members, the user will already handle that (either adding ignore or removing):

1. ignore: means they know that happens and want to keep it this way.
2. Removing the reference: this could then trigger unnecessary_underscores, so the user already has a fix

I also agree that this is not needed.

[RohitSaily]

i now see how your suggestion for linting it as a private member can simplify things and avoid introducing a new lint (which i think would also be fine, but if it can be done without adding another lint that's probably better)

then would any parameter (positional or named) prefixed with _ be considered a private member because it uses the library-private naming? even though it is in a local context

[FMorschel]

then would any parameter (positional or named) prefixed with _ be considered a private member because it uses the library-private naming? even though it is in a local context

I guess this could be discussed at the other issue. Maybe some exceptions could be thought but I can't articulate any atm, so basically, yes.

[RohitSaily]

i think that would make sense. i will add to the discussion there regarding that

in that case i currently like this approach the best:

Yes, this is why I wrote maybe. If we have a warning about doc references to private members, the user will already handle that (either adding ignore or removing):

1. ignore: means they know that happens and want to keep it this way.
2. Removing the reference: this could then trigger unnecessary_underscores, so the user already has a fix

I also agree that this is not needed.