Disable cell-segmentation of import paths with buckconfig

[cormacrelf]

Fixes #683 if you enable the config.

Also fixes #890.

This is achieved by ignoring dropping the build_file_cell during ImportPath construction. I can confirm this fixes the test case on that issue (https://github.com/cormacrelf/buck2-tset-bug).

This is a big improvement for users outside of Meta because previously it was practically impossible to use transitive sets. I have been writing some rules and have had to emulate the feature with regular starlark sets.

Previous version of this PR had cfg flags; now I made a buckconfig for it. Because it does break some things.

This will break any code that reads read_config / get_cell_name at the top level of a BZL file outside of the prelude. It will also break code that uses read_config / get_cell_name during analysis (which should never have worked). It is therefore not activated for anyone by default. However it is not super difficult to migrate. To test it, you can use

# root .buckconfig
[buck2]
    disable_cell_segmentation = true

Breakage (behind buckconfig)

Before (non-prelude cell)

# all four calls relate to the build file cell and will all agree.
STATIC_CONFIG = read_config(...)
STATIC_CELL = native.get_cell_name()
def macro():
    _ = read_config(...)
    _ = native.get_cell_name()

After

# These two relate to the .bzl file's cell
STATIC_CONFIG = read_config(...)
STATIC_CELL = native.get_cell_name()

def macro():
    # these two relate to build file cell as before
    _ = read_config(...)
    _ = native.get_cell_name()

For the prelude specifically

No change, the "after" is already how it works in the prelude.

read_config during analysis is no longer accepted

Previously this was unofficially and perhaps accidentally supported:

def _impl(ctx: AnalysisContext) -> list[Provider]:
    xyz = read_config("abc", "xyz")
    ...

rule = rule(impl = _impl)

The reason being that read_config was #[starlark(speculative_exec_safe)], so it was actually being inlined when the entire file was parsed. It can no longer be inlined so this no longer works. It fails with

BUILD FAILED
Error running analysis for `root//rust:test (prelude//platforms:default#7171795d350c5b11)`

Caused by:
    Traceback (most recent call last):
      File <builtin>, in <module>
      * prelude/rust/rust_binary.bzl:519, in rust_test_impl
          re_executor, executor_overrides = get_re_executors_from_props(ctx)
      * prelude/tests/re_utils.bzl:68, in get_re_executors_from_props
          re_arg = _get_re_arg(ctx)
      * prelude/tests/re_utils.bzl:20, in _get_re_arg
          force_local = read_config("fbcode", "disable_re_tests", default = False)

    error: This function is unavailable during analysis (usual solution is to place the information on a toolchain)
      --> prelude/tests/re_utils.bzl:20:19
       |
    20 |     force_local = read_config("fbcode", "disable_re_tests", default = False)
       |                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
       |

Review questions

• Is parser.cell_segmentation the best buckconfig name? Almost every other config is in the buck2 section but this is one of those rare occasions that maybe parser is right? Feel free to change it in app/buck2_interpreter/src/import_paths.rs. You can call the config whatever you like, just fix the tests, or I am happy to change it. I suppose it doesn't matter since it's a temporary thing anyway. Ok I changed it to buck2.disable_cell_segmentation.
• I don't know how to run the integration test that I wrote. Fingers crossed it works.
• There may be a need for disabling this in a more fine-grained way. Especially if people are using a bunch of external cells and they have not been updated to support this new mode.

[facebook-github-bot]

@facebook-github-bot has imported this pull request. If you are a Meta employee, you can view this in D83033385. (Because this pull request was imported automatically, there will not be any future comments.)

[meta-codesync]

@facebook-github-bot has imported this pull request. If you are a Meta employee, you can view this in D83033385. (Because this pull request was imported automatically, there will not be any future comments.)

[cormacrelf]

Right above this code is where we first check if the path is in the prelude and use ImportPath::new_same_cell(). When cell segmentation is disabled, ImportPath::new_with_build_file_cells behaves exactly like this.

[cormacrelf]

Draft because basically this breaks some usages of get_cell_name being called in a .bzl file.

# one//:bfc.bzl

# returns one, only evaluated once
ACTIVE_CELL = native.get_cell_name()

def macro():
    # works
    return native.get_cell_name()

# two//BUCK
load("one//:bfc.bzl", "macro", "ACTIVE_CELL")

# if you import ACTIVE_CELL, you get "one".
# but if you call the macro, it returns "two"
# works
macro()

So we just have to fix some usages like ACTIVE_CELL to be like macro().