Skip to content

Latest commit

 

History

History
1115 lines (791 loc) · 42.5 KB

WARNINGS.md

File metadata and controls

1115 lines (791 loc) · 42.5 KB
Raw

Buildifier warnings

Warning categories supported by buildifier's linter:

How to disable warnings

All warnings can be disabled / suppressed / ignored by adding a special comment # buildifier: disable=<category_name> to the expression that causes the warning. Historically comments with buildozer instead of buildifier are also supported, they are equivalent.

Examples

# buildifier: disable=no-effect
"""
A multiline comment as a string literal.

Docstrings don't trigger the warning if they are first statements of a file or a function.
"""

if debug:
    print("Debug information:", foo)  # buildifier: disable=print

cfg = "data" for attr definitions has no effect

The Configuration cfg = "data" is deprecated and has no effect. Consider removing it.

attr.license() is deprecated and shouldn't be used

  • Category name: attr-license
  • Flag in Bazel: --incompatible_no_attr_license
  • Automatic fix: no
  • Suppress the warning: # buildifier: disable=attr-license

The attr.license() method is almost never used and being deprecated.

non_empty attribute for attr definitions is deprecated

  • Category name: attr-non-empty
  • Flag in Bazel: --incompatible_disable_deprecated_attr_params
  • Automatic fix: yes
  • Suppress the warning: # buildifier: disable=attr-non-empty

The non_empty attribute for attr definitions is deprecated, please use allow_empty with an opposite value instead.

The default parameter for attr.output()is deprecated

The default parameter of attr.output() is bug-prone, as two targets of the same rule would be unable to exist in the same package under default behavior. Use Starlark macros to specify defaults for these attributes instead.

single_file is deprecated

  • Category name: attr-single-file
  • Flag in Bazel: --incompatible_disable_deprecated_attr_params
  • Automatic fix: yes
  • Suppress the warning: # buildifier: disable=attr-single-file

The single_file attribute is deprecated, please use allow_single_file instead.

*args and **kwargs are not allowed in BUILD files

  • Category name: build-args-kwargs
  • Flag in Bazel: --incompatible_no_kwargs_in_build_files
  • Automatic fix: no
  • Suppress the warning: # buildifier: disable=build-args-kwargs

Having *args or **kwargs makes BUILD files hard to read and manipulate. The list of arguments should be explicit.

Module shouldn't be used directly

  • Category name: bzl-visibility
  • Automatic fix: no
  • Suppress the warning: # buildifier: disable=bzl-visibility

If a directory foo contains a subdirectory internal or private, only files located under foo can access it.

For example, dir/rules_mockascript/private/foo.bzl can be loaded from dir/rules_mockascript/private/bar.bzl or dir/rules_mockascript/sub/public.bzl, but not from dir/other_rule/file.bzl.

Never use l, I, or O as names

  • Category name: confusing-name
  • Automatic fix: no
  • Suppress the warning: # buildifier: disable=confusing-name

The names l, I, or O can be easily confused with I, l, or 0 correspondingly.

Glob pattern has no wildcard ('*')

  • Category name: constant-glob
  • Automatic fix: no
  • Suppress the warning: # buildifier: disable=constant-glob

Glob function is used to get a list of files from the depot. The patterns (the first argument) typically include a wildcard (* character). A pattern without a wildcard is often useless and sometimes harmful.

To fix the warning, move the string out of the glob:

- glob(["*.cc", "test.cpp"])
+ glob(["*.cc"]) + ["test.cpp"]

There’s one important difference: before the change, Bazel would silently ignore test.cpp if file is missing; after the change, Bazel will throw an error if file is missing.

If test.cpp doesn’t exist, the fix becomes:

- glob(["*.cc", "test.cpp"])
+ glob(["*.cc"])

which improves maintenance and readability.

If no pattern has a wildcard, just remove the glob. It will also improve build performance (glob can be relatively slow):

- glob(["test.cpp"])
+ ["test.cpp"]

ctx.{action_name} is deprecated

The following actions are deprecated, please use the new API:

ctx.actions.args().add() for multiple arguments is deprecated

It's deprecated to use the add method of ctx.actions.args() to add a list (or a depset) of variables. Please use either add_all or add_joined, depending on the desired behavior.

The function is deprecated

  • Category name: deprecated-function
  • Automatic fix: no
  • Suppress the warning: # buildifier: disable=deprecated-function

The function defined in another .bzl file has a docstring stating that it's deprecated, i.e. it contains a Deprecated: section. The convention for function docstrings is described by the function-docstring warning.

Depset's "items" parameter is deprecated

The items parameter for depset is deprecated. In it's old form it's either a list of direct elements to be added (use the direct or unnamed first parameter instead) or a depset that becomes a transitive element of the new depset (use the transitive parameter instead).

Depset iteration is deprecated

Depsets are complex structures, iterations over them and lookups require flattening them to a list which may be a heavy operation. To make it more obvious it's now required to call the .to_list() method on them in order to be able to iterate their items:

deps = depset()
[x.path for x in deps]  # deprecated
[x.path for x in deps.to_list()]  # recommended

Depsets should be joined using the depset constructor

The following ways to merge two depsets are deprecated:

depset1 + depset2
depset1 | depset2
depset1.union(depset2)

Please use the depset constructor instead:

depset(transitive = [depset1, depset2])

When fixing this issue, make sure you understand depsets and try to reduce the number of calls to depset.

Dictionary concatenation is deprecated

The + operator to concatenate dicts is deprecated. The operator used to create a new dict and copy the data to it. There are several ways to avoid it, for example, instead of d = d1 + d2 + d3 you can use one of the following:

load("@bazel_skylib//lib:dicts.bzl", "dicts")

d = dicts.add(d1, d2, d3)
  • The same if you don't want to use Skylib:
d = dict(d1.items() + d2.items() + d3.items())
  • The same in several steps:
d = dict(d1)  # If you don't want `d1` to be mutated
d.update(d2)
d.update(d3)

A rule with name foo was already found on line

  • Category name: duplicated-name
  • Automatic fix: no
  • Suppress the warning: # buildifier: disable=duplicated-name

Each label in Bazel has a unique name, and Bazel doesn’t allow two rules to have the same name. With macros, this may be accepted by Bazel (if each macro generates different rules):

my_first_macro(name = "foo")
my_other_macro(name = "foo")

Although the build may work, this code can be very confusing. It can confuse users reading a BUILD file (if they look for the rule “foo”, they may read see only one of the macros). It will also confuse tools that edit BUILD files.

To fix the issue just change the name attribute of one rule/macro.

The FileType function is deprecated

The function FileType is deprecated. Instead of using it as an argument to the rule function just use a list of strings.

Function docstring

  • Category names:
    • function-docstring
    • function-docstring-header
    • function-docstring-args
    • function-docstring-return
  • Automatic fix: no
  • Suppress the warning: # buildifier: disable=function-docstring, # buildifier: disable=function-docstring-header, # buildifier: disable=function-docstring-args, # buildifier: disable=function-docstring-return

Public functions should have docstrings describing functions and their signatures. A docstring is a string literal (not a comment) which should be the first statement of a function (it may follow comment lines). Function docstrings are expected to be formatted in the following way:

"""One-line summary: must be followed and may be preceded by a blank line.

Optional additional description like this.

If it's a function docstring and the function has more than one argument, the docstring has
to document these parameters as follows:

Args:
  parameter1: description of the first parameter. Each parameter line
    should be indented by one, preferably two, spaces (as here).
  parameter2: description of the second
    parameter that spans two lines. Each additional line should have a
    hanging indentation of at least one, preferably two, additional spaces (as here).
  another_parameter (unused, mutable): a parameter may be followed
    by additional attributes in parentheses

Returns:
  Description of the return value.
  Should be indented by at least one, preferably two spaces (as here)
  Can span multiple lines.

Deprecated:
  Optional, description of why the function is deprecated and what should be used instead.
"""

Docstrings are required for all public functions with at least 5 statements. If a docstring exists it should start with a one-line summary line followed by an empty line. If a docstring is required or it describes some arguments, it should describe all of them. If a docstring is required and the function returns a value, it should be described.

Function git_repository is not global anymore

Native git_repository and new_git_repository functions are removed. Please use the Starlark versions instead:

load("@bazel_tools//tools/build_defs/repo:git.bzl", "git_repository", "new_git_repository")

Function http_archive is not global anymore

Native http_archive function is removed. Please use the Starlark versions instead:

load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")

The / operator for integer division is deprecated

The / operator is deprecated in favor of //, please use the latter for integer division:

a = b // c
d //= e

Keyword parameter should be positional

  • Category name: keyword-positional-params
  • Automatic fix: yes
  • Suppress the warning: # buildifier: disable=keyword-positional-params

Some parameters for builtin functions in Starlark are keyword for legacy reasons; their names are not meaningful (e.g. x). Making them positional-only will improve the readability.

Prefer using .append() to adding a single element list

  • Category name: list-append
  • Automatic fix: yes
  • Suppress the warning: # buildifier: disable=list-append

Transforming x += [expr] to x.append(expr) avoids a list allocation.

Loaded symbol is unused

Background

load is used to import definitions in a BUILD file. If the definition is not used in the file, the load can be safely removed. If a symbol is loaded two times, you will get a warning on the second occurrence.

How to fix it

Delete the line. When load is used to import multiple symbols, you can remove the unused symbols from the list. To fix your BUILD files automatically, try this command:

$ buildozer 'fix unusedLoads' path/to/BUILD

If you want to keep the load, you can disable the warning by adding a comment # @unused.

Load statements should be at the top of the file

Load statements should be first statements (with the exception of WORKSPACE files), they can follow only comments and docstrings.

The file has no module docstring

  • Category name: module-docstring
  • Automatic fix: no
  • Suppress the warning: # buildifier: disable=module-docstring

.bzl files should have docstrings on top of them. A docstring is a string literal (not a comment) which should be the first statement of the file (it may follow comment lines). For example:

"""
This module contains build rules for my project.
"""

...

Name conventions

  • Category name: name-conventions
  • Automatic fix: no
  • Suppress the warning: # buildifier: disable=name-conventions

By convention, all variables should be lower_snake_case, constant should be UPPER_SNAKE_CASE, and providers should be UpperCamelCase ending with Info.

All Android build rules should be loaded from Starlark

The Android build rules should be loaded from Starlark.

Update: the plans for disabling native rules have been postponed, at the moment it's not required to load Starlark rules.

The native module shouldn't be used in BUILD files

  • Category name: native-build
  • Automatic fix: yes
  • Suppress the warning: # buildifier: disable=native-build

There's no need in using native. in BUILD files, its members are available as global symbols there.

All C++ build rules should be loaded from Starlark

The CC build rules should be loaded from Starlark.

Update: the plans for disabling native rules have been postponed, at the moment it's not required to load Starlark rules.

All Java build rules should be loaded from Starlark

The Java build rules should be loaded from Starlark.

Update: the plans for disabling native rules have been postponed, at the moment it's not required to load Starlark rules.

native.package() shouldn't be used in .bzl files

  • Category name: native-package
  • Automatic fix: no
  • Suppress the warning: # buildifier: disable=native-package

It's discouraged and will be disallowed to use native.package() in .bzl files. It can silently modify the semantics of a BUILD file and makes it hard to maintain.

All Proto build rules and symbols should be loaded from Starlark

The Proto build rules should be loaded from Starlark.

Update: the plans for disabling native rules have been postponed, at the moment it's not required to load Starlark rules.

All Python build rules should be loaded from Starlark

The Python build rules should be loaded from Starlark.

Update: the plans for disabling native rules have been postponed, at the moment it's not required to load Starlark rules.

Expression result is not used

The statement has no effect. Consider removing it or storing its result in a variable.

Load statements should be ordered by their labels

Load statements should be ordered by their first argument - extension file label. This makes it easier to developers to locate loads of interest and reduces chances for conflicts when performing large-scale automated refactoring.

When applying automated fixes, it's highly recommended to also use load-on-top fixes, since otherwise the relative order of a symbol load and its usage can change resulting in runtime error.

ctx.attr.dep.output_group is deprecated

The output_group field of a target is deprecated in favor of the OutputGroupInfo provider.

The depset is potentially overly nested

  • Category name: overly-nested-depset
  • Automatic fix: no
  • Suppress the warning: # buildifier: disable=overly-nested-depset

If a depset is iteratively chained in a for loop, e.g. the following pattern is used:

for ...:
    x = depset(..., transitive = [..., x, ...])

this can result in an overly nested depset with a long chain of transitive elements. Such patterns can lead to performance problems, consider refactoring the code to create a flat list of transitive elements and call the depset constructor just once:

transitive = []

for ...:
    transitive += ...

x = depset(..., transitive = transitive)

Or in simple cases you can use list comprehensions instead:

x = depset(..., transitive = [y.deps for y in ...])

For more information, read Bazel documentation about depsets and reducing the number of calls to depset.

Global variable PACKAGE_NAME is deprecated

The global variable PACKAGE_NAME is deprecated, please use native.package_name() instead.

Package declaration should be at the top of the file

  • Category name: package-on-top
  • Automatic fix: no
  • Suppress the warning: # buildifier: disable=package-on-top

Here is a typical structure of a BUILD file:

  • load() statements
  • package()
  • calls to rules, macros

Instantiating a rule and setting the package defaults later can be very confusing, and has been a source of bugs (tools and humans sometimes believe package applies to everything in a BUILD file). This might become an error in the future.

What can be used before package()?

The linter allows the following to be before package():

  • comments
  • load()
  • variable declarations
  • package_group()
  • licenses()

Keyword arguments should be used over positional arguments

  • Category name: positional-args
  • Automatic fix: no
  • Suppress the warning: # buildifier: disable=positional-args

All top level calls (except for some built-ins) should use keyword args over positional arguments. Positional arguments can cause subtle errors if the order is switched or if an argument is removed. Keyword args also greatly improve readability.

- my_macro("foo", "bar")
+ my_macro(name = "foo", env = "bar")

The linter allows the following functions to be called with positional arguments:

  • load()
  • vardef()
  • export_files()
  • licenses()
  • print()

print() is a debug function and shouldn't be submitted

Using the print() function for warnings is discouraged: they are often spammy and non actionable, the people who see the warning are usually not the people who can fix the code to make the warning disappear, and the actual maintainers of the code may never see the warning.

Calls to provider should specify a list of fields and a documentation

  • Category name: provider-params
  • Automatic fix: no
  • Suppress the warning: # buildifier: disable=provider-params

Calls to provider should specify a documentation string and a list of fields:

ServerAddressInfo = provider(
    "The address of an HTTP server. Fields are host (string) and port (int).",
    fields = ["host", "port"]
)

Fields should also be documented when needed:

ServerAddressInfo = provider(
    "The address of an HTTP server.",
    fields = {
        "host": "string, e.g. 'example.com'",
        "port": "int, a TCP port number",
    }
)

Note that specifying a list of fields is a breaking change. It is an error if a call to the provider uses undeclared fields. If you cannot declare the list of fields, you may explicitly set it to None (and explain why in a comment).

AllInfo = provider("This provider accepts any field.", fields = None)

NoneInfo = provider("This provider cannot have fields.", fields = [])

See the documentation for providers.

Variable has already been defined

  • Category name: redefined-variable
  • Automatic fix: no
  • Suppress the warning: # buildifier: disable=redefined-variable

Background

In .bzl files, redefining a global variable is already forbidden. This helps both humans and tools reason about the code. For consistency, we want to bring this restriction also to BUILD files.

How to fix it

Rename one of the variables.

Note that the content of lists and dictionaries can still be modified. We will forbid reassignment, but not every side-effect.

Global variable REPOSITORY_NAME is deprecated

The global variable REPOSITORY_NAME is deprecated, please use native.repository_name() instead.

Some but not all execution paths of a function return a value

  • Category name: return-value
  • Automatic fix: no
  • Suppress the warning: # buildifier: disable=return-value

Some but not all execution paths of a function return a value. Either there's an explicit empty return statement, or an implicit return in the end of a function. If it is intentional, make it explicit using return None. If you know certain parts of the code cannot be reached, add the statement fail("unreachable") to them.

Avoid using the legacy provider syntax

  • Category name: rule-impl-return
  • Automatic fix: no
  • Suppress the warning: # buildifier: disable=rule-impl-return

Returning structs from rule implementation functions is deprecated, consider using providers or lists of providers instead.

Same label is used for multiple loads

  • Category name: same-origin-load
  • Automatic fix: yes
  • Suppress the warning: # buildifier: disable=same-origin-load

Background

load is used to import definitions in a BUILD file. If the same label is used for loading symbols more the ones, all such loads can be merged into a single one.

How to fix it

Merge all loads into a single one. For example,

load(":f.bzl", "s1")
load(":f.bzl", "s2")

can be written more compactly as

load(":f.bzl", "s1", "s2")

"Skylark" is an outdated name of the language, please use "starlark" instead

  • Category names:
    • skylark-comment
    • skylark-docstring
  • Automatic fix: yes
  • Suppress the warning: # buildifier: disable=skylark-comment, # buildifier: disable=skylark-docstring

The configuration language for Bazel is called "Starlark" now, the name "Skylark" is outdated and shouldn't be used.

String iteration is deprecated

Iteration over strings often leads to confusion with iteration over a sequence of strings, therefore strings won't be recognized as sequences of 1-element strings (like in Python). Use string indexing and len instead:

my_string = "hello world"
for i in range(len(my_string)):
    char = my_string[i]
    # do something with char

Variable may not have been initialized

  • Category name: uninitialized
  • Automatic fix: no
  • Suppress the warning: # buildifier: disable=uninitialized

The local value can be not initialized at the time of execution. It may happen if it's initialized in one of the if-else clauses but not in all of them, or in a for-loop which can potentially be empty.

The macro should have a keyword argument called "name"

  • Category name: unnamed-macro
  • Automatic fix: no
  • Suppress the warning: # buildifier: disable=unnamed-macro

By convention all macro functions should have a keyword argument called name (even if they don't use it). This is important for tooling and automation.

A macro is a function that calls a rule (either directly or indirectly by calling other macros).

If this function is a helper function that's not supposed to be used outside of its file, please make it private (rename it so that the name starts with _), this will prevent loading the function from BUILD files and suppress the warning.

The statement is unreachable

  • Category name: unreachable
  • Automatic fix: no
  • Suppress the warning: # buildifier: disable=unreachable

The statement is unreachable because it follows a return, break, continue, or fail() statement.

Dictionary items should be ordered by their keys

Dictionary items should be sorted lexicographically by their keys. This makes it easier to find the item of interest and reduces chances of conflicts when performing large-scale automated refactoring.

The order is affected by NamePriority dictionary passed using -tables or -add_tables flags.

If you want to preserve the original dictionary items order, you can disable the warning by adding a comment # @unsorted-dict-items to the dictionary expression or any of its enclosing expressions (binary, if etc). For example,

# @unsorted-dict-items
d = {
    "b": "bvalue",
    "a": "avalue",
}

will not be reported as an issue because the assignment operation that uses the dictionary with unsorted items has a comment disabling this warning.

Variable is unused

  • Category name: unused-variable
  • Automatic fix: no
  • Suppress the warning: # buildifier: disable=unused-variable

This happens when a variable is set but not used in the file, e.g.

x = [1, 2]

The line can often be safely removed.

If you want to keep the variable, you can disable the warning by adding a comment # @unused.

x = [1, 2] # @unused