From 4e696c8328428bfa00f754cfcd059609bfa8a11f Mon Sep 17 00:00:00 2001 From: Andreas Stenius Date: Thu, 18 Nov 2021 13:11:42 +0100 Subject: [PATCH 1/3] [internal] Add infrastructure to support deprecating field names Signed-off-by: Andreas Stenius # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels] --- src/python/pants/engine/internals/graph.py | 33 +++++++++++++++++++ .../pants/engine/internals/graph_test.py | 11 ++++++- src/python/pants/engine/target.py | 29 +++++++++++----- 3 files changed, 63 insertions(+), 10 deletions(-) diff --git a/src/python/pants/engine/internals/graph.py b/src/python/pants/engine/internals/graph.py index b91e1a05276..1038dae9921 100644 --- a/src/python/pants/engine/internals/graph.py +++ b/src/python/pants/engine/internals/graph.py @@ -50,6 +50,7 @@ Dependencies, DependenciesRequest, ExplicitlyProvidedDependencies, + Field, FieldSet, FieldSetsPerTarget, FieldSetsPerTargetRequest, @@ -141,6 +142,32 @@ def warn_deprecated_target_type(request: _WarnDeprecatedTargetRequest) -> _WarnD return _WarnDeprecatedTarget() +# We use a rule for this warning so that it gets memoized, i.e. doesn't get repeated for every +# offending field. +class _WarnDeprecatedField: + pass + + +@dataclass(frozen=True) +class _WarnDeprecatedFieldRequest: + field_type: type[Field] + + +@rule +def warn_deprecated_field_type(request: _WarnDeprecatedFieldRequest) -> _WarnDeprecatedField: + field_type = request.field_type + assert field_type.deprecated_alias_removal_version is not None + warn_or_error( + removal_version=field_type.deprecated_alias_removal_version, + entity=f"the field name {field_type.deprecated_alias}", + hint=( + f"Instead, use `{field_type.alias}`, which behaves the same. Run `./pants " + "update-build-files` to automatically fix your BUILD files." + ), + ) + return _WarnDeprecatedField() + + @rule async def resolve_target( address: Address, @@ -162,6 +189,12 @@ async def resolve_target( ): await Get(_WarnDeprecatedTarget, _WarnDeprecatedTargetRequest(target_type)) target = target_type(target_adaptor.kwargs, address, union_membership) + for field_type in target.field_types: + if ( + field_type.deprecated_alias is not None + and field_type.deprecated_alias in target_adaptor.kwargs + ): + await Get(_WarnDeprecatedField, _WarnDeprecatedFieldRequest(field_type)) return WrappedTarget(target) wrapped_generator_tgt = await Get( diff --git a/src/python/pants/engine/internals/graph_test.py b/src/python/pants/engine/internals/graph_test.py index 683f58e8b20..c2204da7dae 100644 --- a/src/python/pants/engine/internals/graph_test.py +++ b/src/python/pants/engine/internals/graph_test.py @@ -86,6 +86,8 @@ class MockDependencies(Dependencies): supports_transitive_excludes = True + deprecated_alias = "deprecated_field" + deprecated_alias_removal_version = "9.9.9.dev0" class SpecialCasedDeps1(SpecialCasedDependencies): @@ -540,11 +542,18 @@ def test_dep_no_cycle_indirect(transitive_targets_rule_runner: RuleRunner) -> No } +def test_deprecated_field_name(transitive_targets_rule_runner: RuleRunner, caplog) -> None: + transitive_targets_rule_runner.write_files({"BUILD": "target(name='t', deprecated_field=[])"}) + transitive_targets_rule_runner.get_target(Address("", target_name="t")) + assert len(caplog.records) == 1 + assert "Instead, use `dependencies`" in caplog.text + + def test_resolve_deprecated_target_name(transitive_targets_rule_runner: RuleRunner, caplog) -> None: transitive_targets_rule_runner.write_files({"BUILD": "deprecated_target(name='t')"}) transitive_targets_rule_runner.get_target(Address("", target_name="t")) assert len(caplog.records) == 1 - assert "Instead, use `target`" + assert "Instead, use `target`" in caplog.text def test_resolve_generated_target(transitive_targets_rule_runner: RuleRunner) -> None: diff --git a/src/python/pants/engine/target.py b/src/python/pants/engine/target.py index 36a3365c67b..1b434c6e538 100644 --- a/src/python/pants/engine/target.py +++ b/src/python/pants/engine/target.py @@ -123,6 +123,9 @@ def compute_value(cls, raw_value: Optional[int], *, address: Address) -> Optiona removal_version: ClassVar[str | None] = None removal_hint: ClassVar[str | None] = None + deprecated_alias: ClassVar[str | None] = None + deprecated_alias_removal_version: ClassVar[str | None] = None + @final def __init__(self, raw_value: Optional[Any], address: Address) -> None: self._check_deprecated(raw_value, address) @@ -328,38 +331,46 @@ def __init__( warn_or_error( self.removal_version, entity=f"the {repr(self.alias)} target type", - hint=( - f"Using the `{self.alias}` target type for {address}. " f"{self.removal_hint}" - ), + hint=f"Using the `{self.alias}` target type for {address}. {self.removal_hint}", ) self.address = address self.plugin_fields = self._find_plugin_fields(union_membership or UnionMembership({})) - self.residence_dir = residence_dir if residence_dir is not None else address.spec_path + self.field_values = self._calculate_field_values(unhydrated_values, address) + self.validate() + @final + def _calculate_field_values( + self, unhydrated_values: dict[str, Any], address: Address + ) -> FrozenDict[type[Field], Field]: field_values = {} - aliases_to_field_types = {field_type.alias: field_type for field_type in self.field_types} + valid_aliases = set() + aliases_to_field_types = {} + for field_type in self.field_types: + valid_aliases.add(field_type.alias) + aliases_to_field_types[field_type.alias] = field_type + if field_type.deprecated_alias is not None: + aliases_to_field_types[field_type.deprecated_alias] = field_type for alias, value in unhydrated_values.items(): if alias not in aliases_to_field_types: raise InvalidFieldException( f"Unrecognized field `{alias}={value}` in target {address}. Valid fields for " - f"the target type `{self.alias}`: {sorted(aliases_to_field_types.keys())}.", + f"the target type `{self.alias}`: {sorted(valid_aliases)}.", ) field_type = aliases_to_field_types[alias] field_values[field_type] = field_type(value, address) + # For undefined fields, mark the raw value as None. for field_type in set(self.field_types) - set(field_values.keys()): field_values[field_type] = field_type(None, address) - self.field_values = FrozenDict( + return FrozenDict( sorted( field_values.items(), key=lambda field_type_to_val_pair: field_type_to_val_pair[0].alias, ) ) - self.validate() - @final @property def field_types(self) -> Tuple[Type[Field], ...]: From 7d7b002cff313eb8a15bdc8bb30ea8cfaa37c7c8 Mon Sep 17 00:00:00 2001 From: Andreas Stenius Date: Thu, 18 Nov 2021 16:14:36 +0100 Subject: [PATCH 2/3] [internal] Teach update-build-files to fix deprecated field names. # Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels] --- .../pants/core/goals/update_build_files.py | 109 +++++++++++++++++- .../core/goals/update_build_files_test.py | 53 +++++++++ 2 files changed, 161 insertions(+), 1 deletion(-) diff --git a/src/python/pants/core/goals/update_build_files.py b/src/python/pants/core/goals/update_build_files.py index 59b9a6902fe..3d3684e2074 100644 --- a/src/python/pants/core/goals/update_build_files.py +++ b/src/python/pants/core/goals/update_build_files.py @@ -10,7 +10,7 @@ from collections import defaultdict from dataclasses import dataclass from io import BytesIO -from typing import DefaultDict, cast +from typing import DefaultDict, Tuple, cast from colors import green, red @@ -380,11 +380,118 @@ def should_be_renamed(token: tokenize.TokenInfo) -> bool: ) +# ------------------------------------------------------------------------------------------ +# Rename deprecated field types fixer +# ------------------------------------------------------------------------------------------ + + +class RenameDeprecatedFieldsRequest(DeprecationFixerRequest): + pass + + +class RenamedFieldTypes(FrozenDict[Tuple[str, str], str]): + """Deprecated field type names for target to new names.""" + + +@rule +def determine_renamed_field_types( + target_types: RegisteredTargetTypes, union_membership: UnionMembership +) -> RenamedFieldTypes: + renamed_field_types = {} + for tgt in target_types.types: + for field_type in tgt.class_field_types(union_membership): + if field_type.deprecated_alias is None: + continue + renamed_field_types[(tgt.alias, field_type.deprecated_alias)] = field_type.alias + if tgt.deprecated_alias is not None: + renamed_field_types[ + (tgt.deprecated_alias, field_type.deprecated_alias) + ] = field_type.alias + return RenamedFieldTypes(renamed_field_types) + + +@rule(desc="Check for deprecated field type names", level=LogLevel.DEBUG) +def maybe_rename_deprecated_fields( + request: RenameDeprecatedFieldsRequest, + renamed_field_types: RenamedFieldTypes, +) -> RewrittenBuildFile: + target = None + level = 0 + tokens = iter(request.tokenize()) + applied_renames: set[tuple[str, str]] = set() + + def parse_level(token: tokenize.TokenInfo) -> bool: + nonlocal target + nonlocal level + + if target is None or token.type is not tokenize.OP: # type: ignore[unreachable] + return False + if token.string == "(": # type: ignore[unreachable] + level += 1 + elif token.string == ")": + level -= 1 + if level < 1: + target = None + else: + return False + return True + + def next_op_is(string: str) -> bool: + for next_token in tokens: + if next_token.type is tokenize.NL: + continue + parse_level(next_token) + return next_token.type is tokenize.OP and next_token.string == string + return False + + def should_be_renamed(token: tokenize.TokenInfo) -> bool: + nonlocal target + nonlocal level + + if parse_level(token): + return False + + if token.type is not tokenize.NAME: + return False + + if target is None and next_op_is("("): + target = token.string + level = 1 + return False + + if level > 1 or not next_op_is("="): + return False + + return (target, token.string) in renamed_field_types + + updated_text_lines = list(request.lines) + for token in tokens: + if not should_be_renamed(token): + continue + line_index = token.start[0] - 1 + line = request.lines[line_index] + prefix = line[: token.start[1]] + suffix = line[token.end[1] :] + new_symbol = renamed_field_types[(cast(str, target), token.string)] + applied_renames.add((f"{target}.{token.string}", f"{target}.{new_symbol}")) + updated_text_lines[line_index] = f"{prefix}{new_symbol}{suffix}" + + return RewrittenBuildFile( + request.path, + tuple(updated_text_lines), + change_descriptions=tuple( + f"Rename `{request.red(deprecated)}` to `{request.green(new)}`" + for deprecated, new in sorted(applied_renames) + ), + ) + + def rules(): return ( *collect_rules(), *pex.rules(), UnionRule(RewrittenBuildFileRequest, RenameDeprecatedTargetsRequest), + UnionRule(RewrittenBuildFileRequest, RenameDeprecatedFieldsRequest), # NB: We want this to come at the end so that running Black happens after all our # deprecation fixers. UnionRule(RewrittenBuildFileRequest, FormatWithBlackRequest), diff --git a/src/python/pants/core/goals/update_build_files_test.py b/src/python/pants/core/goals/update_build_files_test.py index c809f17d63a..075e02a44c9 100644 --- a/src/python/pants/core/goals/update_build_files_test.py +++ b/src/python/pants/core/goals/update_build_files_test.py @@ -12,13 +12,16 @@ from pants.backend.python.util_rules import pex from pants.core.goals.update_build_files import ( FormatWithBlackRequest, + RenameDeprecatedFieldsRequest, RenameDeprecatedTargetsRequest, + RenamedFieldTypes, RenamedTargetTypes, RewrittenBuildFile, RewrittenBuildFileRequest, UpdateBuildFilesGoal, UpdateBuildFilesSubsystem, format_build_file_with_black, + maybe_rename_deprecated_fields, maybe_rename_deprecated_targets, update_build_files, ) @@ -225,3 +228,53 @@ def test_rename_deprecated_target_types_rewrite(lines: list[str], expected: list ) assert result.change_descriptions assert result.lines == tuple(expected) + + +# ------------------------------------------------------------------------------------------ +# Renamed field types fixer +# ------------------------------------------------------------------------------------------ + + +@pytest.mark.parametrize( + "lines", + ( + # Already valid. + ["target(new_name='')"], + ["target(new_name = 56 ) "], + ["target(foo=1, new_name=2)"], + ["target(", "new_name", "=3)"], + # Unrelated lines. + ["", "123", "target()", "name='new_name'"], + ["unaffected(deprecated_name='not this target')"], + ["target(nested=here(deprecated_name='too deep'))"], + ), +) +def test_rename_deprecated_field_types_noops(lines: list[str]) -> None: + result = maybe_rename_deprecated_fields( + RenameDeprecatedFieldsRequest("BUILD", tuple(lines), colors_enabled=False), + RenamedFieldTypes( + { + ("target", "deprecated_name"): "new_name", + } + ), + ) + assert not result.change_descriptions + assert result.lines == tuple(lines) + + +@pytest.mark.parametrize( + "lines,expected", + ( + (["tgt1(deprecated_name='')"], ["tgt1(new_name='')"]), + (["tgt1 ( deprecated_name = ' ', ", ")"], ["tgt1 ( new_name = ' ', ", ")"]), + (["tgt1(deprecated_name='') # comment"], ["tgt1(new_name='') # comment"]), + (["tgt1(", "deprecated_name", "=", ")"], ["tgt1(", "new_name", "=", ")"]), + ), +) +def test_rename_deprecated_field_types_rewrite(lines: list[str], expected: list[str]) -> None: + result = maybe_rename_deprecated_fields( + RenameDeprecatedTargetsRequest("BUILD", tuple(lines), colors_enabled=False), + RenamedFieldTypes({("tgt1", "deprecated_name"): "new_name"}), + ) + assert result.change_descriptions + assert result.lines == tuple(expected) From 22edec86b668337094637eddb6aee2bd40ae143a Mon Sep 17 00:00:00 2001 From: Andreas Stenius Date: Thu, 25 Nov 2021 10:02:50 +0100 Subject: [PATCH 3/3] Refactoring after review feedback. --- .../pants/core/goals/update_build_files.py | 111 +++++++++++------- .../core/goals/update_build_files_test.py | 8 +- 2 files changed, 73 insertions(+), 46 deletions(-) diff --git a/src/python/pants/core/goals/update_build_files.py b/src/python/pants/core/goals/update_build_files.py index 3d3684e2074..d3d7ad41529 100644 --- a/src/python/pants/core/goals/update_build_files.py +++ b/src/python/pants/core/goals/update_build_files.py @@ -10,7 +10,7 @@ from collections import defaultdict from dataclasses import dataclass from io import BytesIO -from typing import DefaultDict, Tuple, cast +from typing import DefaultDict, Mapping, cast from colors import green, red @@ -389,25 +389,44 @@ class RenameDeprecatedFieldsRequest(DeprecationFixerRequest): pass -class RenamedFieldTypes(FrozenDict[Tuple[str, str], str]): - """Deprecated field type names for target to new names.""" +@dataclass(frozen=True) +class RenamedFieldTypes: + """Map deprecated field names to their new name, per target.""" + + target_field_renames: FrozenDict[str, FrozenDict[str, str]] + + @classmethod + def from_dict(cls, data: Mapping[str, Mapping[str, str]]) -> RenamedFieldTypes: + return cls( + FrozenDict( + { + target_name: FrozenDict( + { + deprecated_field_name: new_field_name + for deprecated_field_name, new_field_name in field_renames.items() + } + ) + for target_name, field_renames in data.items() + } + ) + ) @rule def determine_renamed_field_types( target_types: RegisteredTargetTypes, union_membership: UnionMembership ) -> RenamedFieldTypes: - renamed_field_types = {} + target_field_renames: DefaultDict[str, dict[str, str]] = defaultdict(dict) for tgt in target_types.types: + field_renames = target_field_renames[tgt.alias] + if tgt.deprecated_alias is not None: + target_field_renames[tgt.deprecated_alias] = field_renames + for field_type in tgt.class_field_types(union_membership): - if field_type.deprecated_alias is None: - continue - renamed_field_types[(tgt.alias, field_type.deprecated_alias)] = field_type.alias - if tgt.deprecated_alias is not None: - renamed_field_types[ - (tgt.deprecated_alias, field_type.deprecated_alias) - ] = field_type.alias - return RenamedFieldTypes(renamed_field_types) + if field_type.deprecated_alias is not None: + field_renames[field_type.deprecated_alias] = field_type.alias + + return RenamedFieldTypes.from_dict(target_field_renames) @rule(desc="Check for deprecated field type names", level=LogLevel.DEBUG) @@ -415,54 +434,66 @@ def maybe_rename_deprecated_fields( request: RenameDeprecatedFieldsRequest, renamed_field_types: RenamedFieldTypes, ) -> RewrittenBuildFile: - target = None - level = 0 + pants_target: str = "" + level: int = 0 + applied_renames: set[tuple[str, str, str]] = set() tokens = iter(request.tokenize()) - applied_renames: set[tuple[str, str]] = set() def parse_level(token: tokenize.TokenInfo) -> bool: - nonlocal target + """Returns true if token was consumed.""" nonlocal level - if target is None or token.type is not tokenize.OP: # type: ignore[unreachable] + if level == 0 or token.type is not tokenize.OP or token.string not in ["(", ")"]: return False - if token.string == "(": # type: ignore[unreachable] + + if token.string == "(": level += 1 elif token.string == ")": level -= 1 - if level < 1: - target = None - else: - return False - return True - def next_op_is(string: str) -> bool: - for next_token in tokens: - if next_token.type is tokenize.NL: - continue - parse_level(next_token) - return next_token.type is tokenize.OP and next_token.string == string - return False + return True - def should_be_renamed(token: tokenize.TokenInfo) -> bool: - nonlocal target + def parse_target(token: tokenize.TokenInfo) -> bool: + """Returns true if we're parsing a field name for a top level target.""" + nonlocal pants_target nonlocal level if parse_level(token): + # Consumed parenthesis operator. return False if token.type is not tokenize.NAME: return False - if target is None and next_op_is("("): - target = token.string + if level == 0 and next_token_is("("): level = 1 + pants_target = token.string + # Current token consumed. return False - if level > 1 or not next_op_is("="): + return level == 1 + + def next_token_is(string: str, token_type=tokenize.OP) -> bool: + for next_token in tokens: + if next_token.type is tokenize.NL: + continue + parse_level(next_token) + return next_token.type is token_type and next_token.string == string + return False + + def should_be_renamed(token: tokenize.TokenInfo) -> bool: + nonlocal pants_target + + if not parse_target(token): + return False + + if pants_target not in renamed_field_types.target_field_renames: return False - return (target, token.string) in renamed_field_types + return ( + next_token_is("=") + and token.string in renamed_field_types.target_field_renames[pants_target] + ) updated_text_lines = list(request.lines) for token in tokens: @@ -472,16 +503,16 @@ def should_be_renamed(token: tokenize.TokenInfo) -> bool: line = request.lines[line_index] prefix = line[: token.start[1]] suffix = line[token.end[1] :] - new_symbol = renamed_field_types[(cast(str, target), token.string)] - applied_renames.add((f"{target}.{token.string}", f"{target}.{new_symbol}")) + new_symbol = renamed_field_types.target_field_renames[pants_target][token.string] + applied_renames.add((pants_target, token.string, new_symbol)) updated_text_lines[line_index] = f"{prefix}{new_symbol}{suffix}" return RewrittenBuildFile( request.path, tuple(updated_text_lines), change_descriptions=tuple( - f"Rename `{request.red(deprecated)}` to `{request.green(new)}`" - for deprecated, new in sorted(applied_renames) + f"Rename the field `{request.red(deprecated)}` to `{request.green(new)}` for target type `{target}`" + for target, deprecated, new in sorted(applied_renames) ), ) diff --git a/src/python/pants/core/goals/update_build_files_test.py b/src/python/pants/core/goals/update_build_files_test.py index 075e02a44c9..bbf3eeb5922 100644 --- a/src/python/pants/core/goals/update_build_files_test.py +++ b/src/python/pants/core/goals/update_build_files_test.py @@ -252,11 +252,7 @@ def test_rename_deprecated_target_types_rewrite(lines: list[str], expected: list def test_rename_deprecated_field_types_noops(lines: list[str]) -> None: result = maybe_rename_deprecated_fields( RenameDeprecatedFieldsRequest("BUILD", tuple(lines), colors_enabled=False), - RenamedFieldTypes( - { - ("target", "deprecated_name"): "new_name", - } - ), + RenamedFieldTypes.from_dict({"target": {"deprecated_name": "new_name"}}), ) assert not result.change_descriptions assert result.lines == tuple(lines) @@ -274,7 +270,7 @@ def test_rename_deprecated_field_types_noops(lines: list[str]) -> None: def test_rename_deprecated_field_types_rewrite(lines: list[str], expected: list[str]) -> None: result = maybe_rename_deprecated_fields( RenameDeprecatedTargetsRequest("BUILD", tuple(lines), colors_enabled=False), - RenamedFieldTypes({("tgt1", "deprecated_name"): "new_name"}), + RenamedFieldTypes.from_dict({"tgt1": {"deprecated_name": "new_name"}}), ) assert result.change_descriptions assert result.lines == tuple(expected)