Skip to content

Latest commit

 

History

History
783 lines (532 loc) · 29.7 KB

WARNINGS.md

File metadata and controls

783 lines (532 loc) · 29.7 KB
Raw

Buildifier warnings

Warning categories supported by buildifier's linter:

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

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

non_empty attribute for attr definitions are deprecated

  • Category name: attr-non-empty
  • Flag in Bazel: --incompatible_disable_deprecated_attr_params
  • Automatic fix: yes

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

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

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

Never use l, I, or O as names

  • Category name: confusing-name
  • Automatic fix: no

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

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"]

How to disable this warning

You can disable this warning by adding # buildozer: disable=constant-glob on the line or at the beginning of a rule.

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.

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])

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:

  • Use Skylib:

    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

Background

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.

How to fix it

Just change the name attribute of one rule/macro.

How to disable this warning

You can disable this warning by adding # buildozer: disable=duplicated-name on the line or at the beginning of a rule.

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

Public functions should have docstrings describing functions and their signatures. A docstring is a string statement which should be the first statement of the file (it may follow comment lines). 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.
"""

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 docsrting is required or it describe some of the 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 being removed. Please use the Starklark 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 are being removed. Please use the Starklark 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

Loaded symbol is unused

  • Category name: load
  • Automatic fix: yes

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.

How to disable this warning

You can disable this warning by adding # buildozer: disable=load on the line or at the beginning of a rule.

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

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

Name conventions

  • Category name: name-conventions
  • Automatic fix: no

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

  • Category name: native-android
  • Automatic fix: yes

The Android build rules should be loaded from Starlark. The native rules will be disabled.

The native module shouldn't be used in BUILD files

  • Category name: native-build
  • Automatic fix: yes

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

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

  • Category name: native-package
  • Automatic fix: no

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.

Expression result is not used

  • Category name: no-effect
  • Automatic fix: no

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.

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

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 (but it requires large-scale changes in google3).

What can be used before package()?

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

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

How to disable this warning

You can disable this warning by adding # buildozer: disable=package-on-top on the line or at the beginning of a rule.

Keyword arguments should be used over positional arguments

  • Category name: positional-args
  • Automatic fix: no

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()

How to disable this warning

You can disable this warning by adding # buildozer: disable=positional-args on the line or at the beginning of a rule.

Variable has already been defined

  • Category name: redefined-variable
  • Automatic fix: no

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.

How to disable this warning

You can disable this warning by adding # buildozer: disable=unused-variable on the line or at the beginning of a rule.

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

Some but not all execution paths of a function return a value. Either there's an explicit empty return statement, or an implcit 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

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

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")

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

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 statement is unreachable

  • Category name: unreachable
  • Automatic fix: no

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 lexicagraphically 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 expressins (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

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

How to disable this warning

You can disable this warning by adding # buildozer: disable=unused-variable on the line or at the beginning of a rule.