Macro-without-name warning conflicts with Starlark test practice

[allevato]

The new warning about macros lacking a name argument conflicts with the established practice that Skylib has been using for Starlark-based tests, which has also been adopted by rules_apple and rules_swift (and possibly others?).

Examples:

• https://github.com/bazelbuild/rules_swift/blob/master/test/BUILD
• https://github.com/bazelbuild/rules_swift/blob/master/test/generated_header_tests.bzl#L55

Failures as a result: https://buildkite.com/bazel/rules-swift-swift/builds/1520#840f0fdb-6fc3-4963-9a4a-f9d6a1dba2cf

The pattern is to define a macro with no arguments, and the macro hardcodes a name prefix used for all of the test targets that it generates (which is also used as a tag), and then it creates a final test_suite with that name and tag. (This is done either manually, or through some wrapper like Skylib's unittest.suite, depending on the nature of the tests.)

We could move the name into a macro argument, but that's kind of redundant since the macro will never be called more than once. The check could also be avoided by possibly wrapping the native.test_suite call inside the macro in its own macro, but that seems like unnecessary trickery.

Can that warning be refined in some way to allow this pattern, without having to disable it for the whole repo or individually disable it at each call site?

[vladmos]

Will it be enough to allow macros to have no name argument if its name ends with _test_suite?

[vladmos]

Or is it better to ignore calls to native.test_suite()? E.g. if a function calls just this rule and no other rules (maybe transitively via other macros), it's ok that it doesn't have the name argument.

[vladmos]

The check could also be avoided by possibly wrapping the native.test_suite call inside the macro in its own macro

That shouldn't work, buildifier can resolve transitive calls to rules even if the intermediate macros are defined in different files (but in the same repository).

[allevato]

Or is it better to ignore calls to native.test_suite()? E.g. if a function calls just this rule and no other rules (maybe transitively via other macros), it's ok that it doesn't have the name argument.

I don't think this would work, because in the generated_headers_tests.bzl example above, those calls to generate_header_and_module_map_provider_test, generate_header_and_module_map_failure_test, and no_generate_header_provider_test are also rules.

Allowing the macro to omit name if it ends with _test_suite feels like a bit of a weird special case and could lead to not diagnosing things that you might want to diagnose, but I also can't think of anything cleaner right now.

[vladmos]

In this particular case ignoring native.test_suite() will work, because:

1. The test rules mentioned above are rules because they call analysistest.make() which is defined in another repository, and cross-repository analyisis is not supported (therefore buildifier won't know it's a rule),
2. It doesn't call a rule directly (or via other functions) but calls a returned value of a function which generates a rule (buildifier is not that smart to resolve that)
3. Buildifier currently doesn't resolve calls via struct fields (e.g. analysistest.make() is not a top level function but a field of the analysistest struct), however this restriction is almost trivial to lift, at least for top-level constant structs, and may actually be lifted soon.

But I don't know if can also be applied to most of the cases. From buildifier's point of view it's better to get rid of false positives than keep all actual findings, it already doesn't promise to find everything (e.g. cross-file analysis was introduced just a month ago).