Deprecate --build-file-imports defaulting to warn and having the …

…option `allow` (#9047)

### Problem
Imports are not safe in BUILD files, as described in:

#9040 (comment)

We did not have a mechanism to warn about them until December 2017 via #5180, so we seem to have only maintained the misfeature due to backward compatibility. 

### Solution
We want to remove support for them, but most do so incrementally per the deprecation policy. First, we deprecate `--build-file-imports=allow` and at the same time deprecate the default being `warn` instead of `error`. Then, in 1.27.0.dev0, we will change the default to `error` and deprecate the option entirely.

### Result
We will have remove a whole class of bugs for users. We'll also be able to make more assumptions about BUILD files. From #5180:

> It also opens up potential future opimisations such as parsing with a
custom parser rather than a whole python environment.

pants.ini

@@ -89,6 +89,7 @@ pants_ignore: +[
     '/build-support/bin/native/src',
   ]
 
+build_file_imports: error
 
 [cache]
 # Caching is on globally by default, but we disable it here for development purposes.

src/python/pants/engine/legacy/parser.py

@@ -16,6 +16,7 @@
 from pants.engine.mapper import UnaddressableObjectError
 from pants.engine.objects import Serializable
 from pants.engine.parser import ParseError, Parser, SymbolTable
+from pants.option.global_options import BuildFileImportsBehavior
 from pants.util.memo import memoized_property
 
 
@@ -33,15 +34,17 @@ class LegacyPythonCallbacksParser(Parser):
   """
 
   def __init__(
-    self, symbol_table: SymbolTable, aliases: BuildFileAliases, build_file_imports_behavior: str,
+    self,
+    symbol_table:SymbolTable,
+    aliases: BuildFileAliases,
+    build_file_imports_behavior: BuildFileImportsBehavior,
   ) -> None:
     """
     :param symbol_table: A SymbolTable for this parser, which will be overlaid with the given
       additional aliases.
     :param aliases: Additional BuildFileAliases to register.
     :param build_file_imports_behavior: How to behave if a BUILD file being parsed tries to use
-      import statements. Valid values: "allow", "warn", "error".
-    :type build_file_imports_behavior: string
+      import statements.
     """
     super().__init__()
     self._symbols, self._parse_context = self._generate_symbols(symbol_table, aliases)
@@ -165,7 +168,7 @@ def parse(self, filepath: str, filecontent: bytes):
     # Note that this is incredibly poor sandboxing. There are many ways to get around it.
     # But it's sufficient to tell most users who aren't being actively malicious that they're doing
     # something wrong, and it has a low performance overhead.
-    if "globs" in python or (self._build_file_imports_behavior != 'allow' and 'import' in python):
+    if "globs" in python or (self._build_file_imports_behavior != BuildFileImportsBehavior.allow and 'import' in python):
       io_wrapped_python = StringIO(python)
       for token in tokenize.generate_tokens(io_wrapped_python.readline):
         token_str = token[1]
@@ -174,9 +177,9 @@ def parse(self, filepath: str, filecontent: bytes):
         self.check_for_deprecated_globs_usage(token_str, filepath, lineno)
 
         if token_str == 'import':
-          if self._build_file_imports_behavior == "allow":
+          if self._build_file_imports_behavior == BuildFileImportsBehavior.allow:
             continue
-          elif self._build_file_imports_behavior == 'warn':
+          elif self._build_file_imports_behavior == BuildFileImportsBehavior.warn:
             logger.warning(
               f'Import used in {filepath} at line {lineno}. Import statements should '
               f'be avoided in BUILD files because they can easily break Pants caching and lead to '

src/python/pants/init/engine_initializer.py

@@ -14,6 +14,7 @@
 from pants.backend.python.targets.python_tests import PythonTests
 from pants.base.build_environment import get_buildroot
 from pants.base.build_root import BuildRoot
+from pants.base.deprecated import deprecated_conditional
 from pants.base.exiter import PANTS_SUCCEEDED_EXIT_CODE
 from pants.base.file_system_project_tree import FileSystemProjectTree
 from pants.base.specs import Specs
@@ -58,6 +59,7 @@
 from pants.init.options_initializer import BuildConfigInitializer, OptionsInitializer
 from pants.option.global_options import (
   DEFAULT_EXECUTION_OPTIONS,
+  BuildFileImportsBehavior,
   ExecutionOptions,
   GlobMatchErrorBehavior,
 )
@@ -288,6 +290,7 @@ def setup_legacy_graph(
     """Construct and return the components necessary for LegacyBuildGraph construction."""
     build_root = get_buildroot()
     bootstrap_options = options_bootstrapper.bootstrap_options.for_global_scope()
+
     glob_expansion_failure_configured = not bootstrap_options.is_default("glob_expansion_failure")
     files_not_found_behavior_configured = not bootstrap_options.is_default("files_not_found_behavior")
     if glob_expansion_failure_configured and files_not_found_behavior_configured:
@@ -300,6 +303,36 @@ def setup_legacy_graph(
       if files_not_found_behavior_configured else
       bootstrap_options.glob_expansion_failure
     )
+
+    deprecated_conditional(
+      lambda: cast(bool, bootstrap_options.build_file_imports == BuildFileImportsBehavior.allow),
+      removal_version="1.27.0.dev0",
+      entity_description="Using `--build-file-imports=allow`",
+      hint_message=(
+        "Import statements should be avoided in BUILD files because they can easily break Pants "
+        "caching and lead to stale results. It is not safe to ignore warnings of imports, so the "
+        "`allow` option is being removed.\n\nTo prepare for this change, either set "
+        "`--build-file-imports=warn` or `--build-file-imports=error` (we recommend using `error`)."
+        "\n\nIf you still need to keep the functionality you have from the import statement, "
+        "consider rewriting your code into a Pants plugin: "
+        "https://www.pantsbuild.org/howto_plugin.html"
+      )
+    )
+    deprecated_conditional(
+      lambda: bootstrap_options.is_default("build_file_imports"),
+      removal_version="1.27.0.dev0",
+      entity_description="Defaulting to `--build-file-imports=warn`",
+      hint_message=(
+        "Import statements should be avoided in BUILD files because they can easily break Pants "
+        "caching and lead to stale results. The default behavior will change from warning to "
+        "erroring in 1.27.0.dev0, and the option will be removed in 1.29.0.dev0.\n\nTo prepare for "
+        "this change, please explicitly set the option `--build-file-imports=warn` or "
+        "`--build-file-imports=error` (we recommend using `error`).\n\nIf you still need to keep "
+        "the functionality you have from import statements, consider rewriting your code into a "
+        "Pants plugin: https://www.pantsbuild.org/howto_plugin.html"
+      )
+    )
+
     return EngineInitializer.setup_legacy_graph_extended(
       OptionsInitializer.compute_pants_ignore(build_root, bootstrap_options),
       bootstrap_options.local_store_dir,
@@ -320,7 +353,7 @@ def setup_legacy_graph(
   def setup_legacy_graph_extended(
     pants_ignore_patterns,
     local_store_dir,
-    build_file_imports_behavior,
+    build_file_imports_behavior: BuildFileImportsBehavior,
     options_bootstrapper: OptionsBootstrapper,
     build_configuration: BuildConfiguration,
     build_root: Optional[str] = None,
@@ -338,8 +371,7 @@ def setup_legacy_graph_extended(
                                        usually taken from the '--pants-ignore' global option.
     :param local_store_dir: The directory to use for storing the engine's LMDB store in.
     :param build_file_imports_behavior: How to behave if a BUILD file being parsed tries to use
-      import statements. Valid values: "allow", "warn", "error".
-    :type build_file_imports_behavior: string
+                                        import statements.
     :param build_root: A path to be used as the build root. If None, then default is used.
     :param native: An instance of the native-engine subsystem.
     :param options_bootstrapper: A `OptionsBootstrapper` object containing bootstrap options.

src/python/pants/option/global_options.py

@@ -51,6 +51,12 @@ def to_glob_match_error_behavior(self) -> GlobMatchErrorBehavior:
     return GlobMatchErrorBehavior(self.value)
 
 
+class BuildFileImportsBehavior(Enum):
+  allow = "allow"
+  warn = "warn"
+  error = "error"
+
+
 @dataclass(frozen=True)
 class ExecutionOptions:
   """A collection of all options related to (remote) execution of processes.
@@ -434,8 +440,8 @@ def register_bootstrap_options(cls, register):
 
     # This option changes the parser behavior in a fundamental way (which currently invalidates
     # all caches), and needs to be parsed out early, so we make it a bootstrap option.
-    register('--build-file-imports', choices=['allow', 'warn', 'error'], default='warn',
-             advanced=True,
+    register('--build-file-imports', type=BuildFileImportsBehavior,
+             default=BuildFileImportsBehavior.warn, advanced=True,
              help='Whether to allow import statements in BUILD files')
 
     register('--local-store-dir', advanced=True,

src/python/pants/testutil/test_base.py

@@ -29,6 +29,7 @@
 from pants.engine.selectors import Params
 from pants.init.engine_initializer import EngineInitializer
 from pants.init.util import clean_global_runtime_state
+from pants.option.global_options import BuildFileImportsBehavior
 from pants.option.options_bootstrapper import OptionsBootstrapper
 from pants.source.source_root import SourceRootConfig
 from pants.source.wrapped_globs import EagerFilesetWithSpec
@@ -401,7 +402,7 @@ def _init_engine(self, local_store_dir: Optional[str] = None) -> None:
     graph_session = EngineInitializer.setup_legacy_graph_extended(
       pants_ignore_patterns=None,
       local_store_dir=local_store_dir,
-      build_file_imports_behavior='allow',
+      build_file_imports_behavior=BuildFileImportsBehavior.error,
       native=init_native(),
       options_bootstrapper=options_bootstrapper,
       build_root=self.build_root,

tests/python/pants_test/engine/legacy/test_parser.py

@@ -6,13 +6,18 @@
 from pants.build_graph.build_file_aliases import BuildFileAliases
 from pants.engine.legacy.parser import LegacyPythonCallbacksParser
 from pants.engine.parser import SymbolTable
+from pants.option.global_options import BuildFileImportsBehavior
 
 
 class LegacyPythonCallbacksParserTest(unittest.TestCase):
 
   def test_no_import_sideeffects(self) -> None:
     # A parser with no symbols registered.
-    parser = LegacyPythonCallbacksParser(SymbolTable({}), BuildFileAliases(), build_file_imports_behavior='allow')
+    parser = LegacyPythonCallbacksParser(
+      SymbolTable({}),
+      BuildFileAliases(),
+      build_file_imports_behavior=BuildFileImportsBehavior.warn,
+    )
     # Call to import a module should succeed.
     parser.parse('/dev/null', b'''import os; os.path.join('x', 'y')''')
     # But the imported module should not be visible as a symbol in further parses.