Buildifier: Variables prefixed with _ should be ignored by unused-variable checks

[UebelAndre]

I and seemingly other contributors to rules_rust (and surely other rules) have had the expectation that variables prefixed with _ were unused. Needing to additionally annotate variables with a comment feels unnecessary and can increase the burden of reviewers where a user may call some native or external macro which returns multiple values but without a _ prefix it's unclear which is actually unused.

# @unused
compilation_ctx, cc_compilation_outputs = cc_common.compile(...)

vs

compilation_ctx, _cc_compilation_outputs = cc_common.compile(...)

The latter makes it clear that a specific result of this external function is not used and allows the warning to catch cases where the second variable becomes unused, this informing users the entire snippet can be removed.

[vladmos]

Would it help if buildifier ignores such variables only if they are defined together with another variable? Because if you write

_compilation_ctx, cc_compilation_outputs = cc_common.compile(...)

(and don't use both) I think it's still valid to warn about unused variables, WDYT?

[UebelAndre]

In the case of

_compilation_ctx, cc_compilation_outputs = cc_common.compile(...)

I don't think it's valid to warn on _compilation_ctx being unused because the _ prefix should be an indication that the variable is expected to be unused. However, if cc_compilation_outputs were to be unused, I would want a warning. This pattern helps identify when things can be removed, which I don't think you'd get by applying @unused to the entire statement.

[vladmos]

Sorry, I meant both variables with underscores:

# safe to remove the LHS
_compilation_ctx, _cc_compilation_outputs = cc_common.compile(...)

or just

# safe to remove the entire statement
_x = 3

(all variables in the examples are unused). The formal criteria would be "don't warn if an unused variable starts with an underscore and is defined in the same statement with another used variable". I agree with your usecase but don't think that suppressing the warning for all variables starting with _ is a good idea.

[UebelAndre]

That would work for me.

One other case I've found is in function arguments

def _wasm_bindgen_transition(_settings, _attr):
    return {"//command_line_option:platforms": str(Label("//rust/platform:wasm"))}

wasm_bindgen_transition = transition(
    implementation = _wasm_bindgen_transition,
    inputs = [],
    outputs = ["//command_line_option:platforms"],
)

I would expect this to not yield any warnings since I don't have control over the API of the function here.

If both the examples were to be addressed then I'd feel comfortable enabling the new warning (and I'd really like to since I think it's a very good thing to have).

edit:
Note that in the example above both variables are unused but this is not always the case.

[vladmos]

@UebelAndre feel free to review #1047, it should cover all of the cases described above.

enabling the new warning

in fact it's not new, it was just not implemented properly and because of that didn't catch most of the cases, not it should be more reliable and useful.

[UebelAndre]

@UebelAndre feel free to review #1047, it should cover all of the cases described above.

This looks perfect! Thank you so much for the quick change!

in fact it's not new, it was just not implemented properly and because of that didn't catch most of the cases, not it should be more reliable and useful.

Yeah, sorry about that, I realized that some time later yesterday but forgot to update the ticket. It felt new because we just started seeing the defects.

[UebelAndre]

@vladmos What do you expect the turn-around time to be for getting the changes merged and cutting a new release?

[vladmos]

What do you expect the turn-around time to be for getting the changes merged and cutting a new release?

I'll release 5.0.1 with the fix tomorrow.

Yeah, sorry about that, I realized that some time later yesterday but forgot to update the ticket. It felt new because we just started seeing the defects.

No worries, I just wanted to explain why an existing warning suddenly started yielding a lot of (hopefully not false positive) warnings. Because of primitive implementation it was only detecting unused constants in BUILD files and disabled completely for .bzl files because some variables defined there can be imported from other files. Now it's supposed to handle such cases correctly and e.g. warn about unused local variables inside functions.

[UebelAndre]

Once again, thank you so much!

[vladmos]

You're welcome, please let me know if you see any other examples of warnings that are rather noisy than useful (especially if that's a false positive finding), I refactored a dozen of different warning categories so it's expected that some new warnings appear.