Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[internal] Add infrastructure to support deprecating field names #13666

Merged
merged 4 commits into from
Nov 26, 2021
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
109 changes: 108 additions & 1 deletion src/python/pants/core/goals/update_build_files.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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."""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about FrozenDict[str, FrozenDict[str, str]], which maps target types to field renames. Then you can rewrite the implementation I think to more eagerly exit if the target type is not a match?

I'm not sure I followed the level code correctly...the level should be 1. A nested target should not exist - targets are always top-level.

At this point, might also be worth switching to a frozen dataclass rather than subclassing FrozenDict so that you can give a name to the variable like target_types_to_field_renames.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about FrozenDict[str, FrozenDict[str, str]], which maps target types to field renames. Then you can rewrite the implementation I think to more eagerly exit if the target type is not a match?

I think you're referring to eagerly exit out of should_be_renamed, correct?
OK, will try it out.. see what it'll look like (not convinced it will win much, but perhaps the data structure will be easier to comprehend?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I followed the level code correctly...the level should be 1. A nested target should not exist - targets are always top-level.

Agreed, there's a check that we only ever operate when level==1.

This is to protect against constructs, such as this, as an example:

python_distribution(
  provides=setup_py(my_field="value"),
)

As we do not want to change my_field here... The level code might be overkill, but it plugs a potential hole.
And, as I write this, I realize I don't only look at the field name, but the enclosing target name as well (setup_py in this example) and as such, it would be safe to ignore the level all together.. thanks for the nudge, will get rid of it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, wait, no, the level is important for this reason..

python_distribution(
  name="dist",
  provides=setup_py(....),
  old_field="value",
)

We must keep track of when we're seeing fields in setup_py, which we should ignore, and when we're back in python_distritbution, so we know that old_field is deprecated and should be replaced.

It's hard to make this distinction unless I keep track of when I enter and exit constructs, such as setup_py there..

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made some refactorings that will hopefully be easier to follow now.



@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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe call this pants_target. I wasn't sure until reading this for a few minutes if "target" meant some generic parsing term.

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)}`"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about this? (Still using colors)

Rename the field deprecated to new for target type tgt_type

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),
Expand Down
53 changes: 53 additions & 0 deletions src/python/pants/core/goals/update_build_files_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
)
Expand Down Expand Up @@ -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)
33 changes: 33 additions & 0 deletions src/python/pants/engine/internals/graph.py
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@
Dependencies,
DependenciesRequest,
ExplicitlyProvidedDependencies,
Field,
FieldSet,
FieldSetsPerTarget,
FieldSetsPerTargetRequest,
Expand Down Expand Up @@ -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,
Expand All @@ -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(
Expand Down
11 changes: 10 additions & 1 deletion src/python/pants/engine/internals/graph_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down Expand Up @@ -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:
Expand Down
29 changes: 20 additions & 9 deletions src/python/pants/engine/target.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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], ...]:
Expand Down