lowRISC / opentitan

OpenTitan: Open source silicon root of trust
https://www.opentitan.org
Apache License 2.0
2.44k stars 730 forks source link

[Bazel] using multable list as a default value in many of the rules #23953

Open machshev opened 1 week ago

machshev commented 1 week ago

There are a number of bzl files with functions taking default values of []. Which could be a source of unexpected behaviour, as (like Python) the default value objects are created when the function is defined, not called.

Which means if the function is called and the default value is used, then the default value object can be mutated inside the function or any function that value is then passed to. The next time the function is called, the default value will be the mutated value and not the expected empty list [].

While this is a potential issue, it's not clear if the specific use case is present or not already. However I think it's worth fixing this up to avoid hitting this issue in the future.

See https://github.com/bazelbuild/starlark/blob/master/spec.md#functions


If a function parameter's default value is a mutable expression, modifications to the value during one call may be observed by subsequent calls. Beware of this when using lists or dicts as default values. If the function becomes frozen, its parameters' default values become frozen too.

# module a.sky
def f(x, list=[]):
  list.append(x)
  return list

f(4, [1,2,3])           # [1, 2, 3, 4]
f(1)                    # [1]
f(2)                    # [1, 2], not [2]!

# module b.sky
load("a.sky", "f")
f(3)                    # error: cannot append to frozen list
pamaury commented 1 week ago

Although this could in theory be an issue, the fact that the list is frozen when one imports a function from a module should make this issue unlikely because it would be quite unusual to call a macro from within its own .bzl file at the root scope. Do you know of any instance in our codebase where that's the case?

pamaury commented 1 week ago

Thinking about this a bit more, one way to fix the problem is to create a new bzl file somewhere:

# empty_list.bzl
EMPTY_LIST = []

then use it as follows:

load("empty_list.bzl", "EMPTY_LIST")

def f(x, list=EMPTY_LIST):
  list.append(x)
  print(list)
  return list

# This will not work:
f(4, [1,2,3])           # [1, 2, 3, 4]
f(1)                    # [1]
f(2)                    # [1, 2], not [2]!

For this to work, it's very important that EMPTY_LIST be in its own bazel file. Also maybe it should be called FROZEN_EMPTY_LIST.