Skip to content

Commit

Permalink
Fix fmt and lint re-runs under pantsd (#9869)
Browse files Browse the repository at this point in the history
### Problem

The `FmtFieldSets` and `LinterFieldSets` types extended `Collection` (which implements useful `eq`/`hash`), but then added an additional optional field, `prior_formatter_result`. But that additional field was not included in the `eq`/`hash` for the type, and this meant that instances of these objects would be equal regardless of the previous result. This resulted in `pantsd` memoizing the wrong input for runs: in short, havoc.

### Solution

Extract `StyleRequest` to replace `FmtFieldSets` and `LinterFieldSets`, and fix `__eq__` for that type by extending dataclass. Additionally, align all `Fmt` and `Lint` `FieldSets` types around the name `Request`.

### Result

Operation of `fmt` and `lint` is correct under `pantsd`.

[ci skip-rust-tests]
[ci skip-jvm-tests]
  • Loading branch information
stuhood committed May 26, 2020
1 parent c832672 commit 8d95788
Show file tree
Hide file tree
Showing 20 changed files with 260 additions and 220 deletions.
21 changes: 11 additions & 10 deletions src/python/pants/backend/python/lint/bandit/rules.py
Expand Up @@ -15,7 +15,7 @@
from pants.backend.python.subsystems import python_native_code, subprocess_environment
from pants.backend.python.subsystems.subprocess_environment import SubprocessEncodingEnvironment
from pants.backend.python.target_types import PythonInterpreterCompatibility, PythonSources
from pants.core.goals.lint import LinterFieldSet, LinterFieldSets, LintResult
from pants.core.goals.lint import LintRequest, LintResult
from pants.core.util_rules import determine_source_files, strip_source_roots
from pants.core.util_rules.determine_source_files import (
AllSourceFilesRequest,
Expand All @@ -26,21 +26,22 @@
from pants.engine.process import FallibleProcessResult, Process
from pants.engine.rules import SubsystemRule, named_rule
from pants.engine.selectors import Get, MultiGet
from pants.engine.target import FieldSetWithOrigin
from pants.engine.unions import UnionRule
from pants.option.global_options import GlobMatchErrorBehavior
from pants.python.python_setup import PythonSetup
from pants.util.strutil import pluralize


@dataclass(frozen=True)
class BanditFieldSet(LinterFieldSet):
class BanditFieldSet(FieldSetWithOrigin):
required_fields = (PythonSources,)

sources: PythonSources
compatibility: PythonInterpreterCompatibility


class BanditFieldSets(LinterFieldSets):
class BanditRequest(LintRequest):
field_set_type = BanditFieldSet


Expand All @@ -55,7 +56,7 @@ def generate_args(*, specified_source_files: SourceFiles, bandit: Bandit) -> Tup

@named_rule(desc="Lint using Bandit")
async def bandit_lint(
field_sets: BanditFieldSets,
request: BanditRequest,
bandit: Bandit,
python_setup: PythonSetup,
subprocess_encoding_environment: SubprocessEncodingEnvironment,
Expand All @@ -66,7 +67,7 @@ async def bandit_lint(
# NB: Bandit output depends upon which Python interpreter version it's run with. See
# https://github.com/PyCQA/bandit#under-which-version-of-python-should-i-install-bandit.
interpreter_constraints = PexInterpreterConstraints.create_from_compatibility_fields(
(field_set.compatibility for field_set in field_sets), python_setup=python_setup
(field_set.compatibility for field_set in request.field_sets), python_setup=python_setup
) or PexInterpreterConstraints(bandit.default_interpreter_constraints)
requirements_pex_request = Get[Pex](
PexRequest(
Expand All @@ -87,11 +88,11 @@ async def bandit_lint(
)

all_source_files_request = Get[SourceFiles](
AllSourceFilesRequest(field_set.sources for field_set in field_sets)
AllSourceFilesRequest(field_set.sources for field_set in request.field_sets)
)
specified_source_files_request = Get[SourceFiles](
SpecifiedSourceFilesRequest(
(field_set.sources, field_set.origin) for field_set in field_sets
(field_set.sources, field_set.origin) for field_set in request.field_sets
)
)

Expand All @@ -114,7 +115,7 @@ async def bandit_lint(
)

address_references = ", ".join(
sorted(field_set.address.reference() for field_set in field_sets)
sorted(field_set.address.reference() for field_set in request.field_sets)
)

process = requirements_pex.create_process(
Expand All @@ -123,7 +124,7 @@ async def bandit_lint(
pex_path="./bandit.pex",
pex_args=generate_args(specified_source_files=specified_source_files, bandit=bandit),
input_digest=input_digest,
description=f"Run Bandit on {pluralize(len(field_sets), 'target')}: {address_references}.",
description=f"Run Bandit on {pluralize(len(request.field_sets), 'target')}: {address_references}.",
)
result = await Get[FallibleProcessResult](Process, process)
return LintResult.from_fallible_process_result(result, linter_name="Bandit")
Expand All @@ -133,7 +134,7 @@ def rules():
return [
bandit_lint,
SubsystemRule(Bandit),
UnionRule(LinterFieldSets, BanditFieldSets),
UnionRule(LintRequest, BanditRequest),
*download_pex_bin.rules(),
*determine_source_files.rules(),
*pex.rules(),
Expand Down
Expand Up @@ -3,7 +3,7 @@

from typing import List, Optional

from pants.backend.python.lint.bandit.rules import BanditFieldSet, BanditFieldSets
from pants.backend.python.lint.bandit.rules import BanditFieldSet, BanditRequest
from pants.backend.python.lint.bandit.rules import rules as bandit_rules
from pants.backend.python.target_types import PythonInterpreterCompatibility, PythonLibrary
from pants.base.specs import FilesystemLiteralSpec, OriginSpec, SingleAddress
Expand All @@ -27,7 +27,7 @@ class BanditIntegrationTest(ExternalToolTestBase):

@classmethod
def rules(cls):
return (*super().rules(), *bandit_rules(), RootRule(BanditFieldSets))
return (*super().rules(), *bandit_rules(), RootRule(BanditRequest))

def make_target_with_origin(
self,
Expand Down Expand Up @@ -68,7 +68,7 @@ def run_bandit(
return self.request_single_product(
LintResult,
Params(
BanditFieldSets(BanditFieldSet.create(tgt) for tgt in targets),
BanditRequest(BanditFieldSet.create(tgt) for tgt in targets),
create_options_bootstrapper(args=args),
),
)
Expand Down
35 changes: 18 additions & 17 deletions src/python/pants/backend/python/lint/black/rules.py
Expand Up @@ -7,7 +7,7 @@
from typing import List, Optional, Tuple, Union, cast

from pants.backend.python.lint.black.subsystem import Black
from pants.backend.python.lint.python_fmt import PythonFmtFieldSets
from pants.backend.python.lint.python_fmt import PythonFmtRequest
from pants.backend.python.rules import download_pex_bin, pex
from pants.backend.python.rules.pex import (
Pex,
Expand All @@ -18,8 +18,8 @@
from pants.backend.python.subsystems import python_native_code, subprocess_environment
from pants.backend.python.subsystems.subprocess_environment import SubprocessEncodingEnvironment
from pants.backend.python.target_types import PythonSources
from pants.core.goals.fmt import FmtFieldSet, FmtFieldSets, FmtResult
from pants.core.goals.lint import LinterFieldSets, LintResult
from pants.core.goals.fmt import FmtRequest, FmtResult
from pants.core.goals.lint import LintRequest, LintResult
from pants.core.util_rules import determine_source_files, strip_source_roots
from pants.core.util_rules.determine_source_files import (
AllSourceFilesRequest,
Expand All @@ -30,26 +30,27 @@
from pants.engine.process import FallibleProcessResult, Process, ProcessResult
from pants.engine.rules import SubsystemRule, named_rule, rule
from pants.engine.selectors import Get, MultiGet
from pants.engine.target import FieldSetWithOrigin
from pants.engine.unions import UnionRule
from pants.option.global_options import GlobMatchErrorBehavior
from pants.python.python_setup import PythonSetup
from pants.util.strutil import pluralize


@dataclass(frozen=True)
class BlackFieldSet(FmtFieldSet):
class BlackFieldSet(FieldSetWithOrigin):
required_fields = (PythonSources,)

sources: PythonSources


class BlackFieldSets(FmtFieldSets):
class BlackRequest(FmtRequest, LintRequest):
field_set_type = BlackFieldSet


@dataclass(frozen=True)
class SetupRequest:
field_sets: BlackFieldSets
request: BlackRequest
check_only: bool


Expand Down Expand Up @@ -106,11 +107,11 @@ async def setup(
)

all_source_files_request = Get[SourceFiles](
AllSourceFilesRequest(field_set.sources for field_set in request.field_sets)
AllSourceFilesRequest(field_set.sources for field_set in request.request.field_sets)
)
specified_source_files_request = Get[SourceFiles](
SpecifiedSourceFilesRequest(
(field_set.sources, field_set.origin) for field_set in request.field_sets
(field_set.sources, field_set.origin) for field_set in request.request.field_sets
)
)

Expand All @@ -119,16 +120,16 @@ async def setup(
config_snapshot_request,
specified_source_files_request,
]
if request.field_sets.prior_formatter_result is None:
if request.request.prior_formatter_result is None:
requests.append(all_source_files_request)
requirements_pex, config_snapshot, specified_source_files, *rest = cast(
Union[Tuple[Pex, Snapshot, SourceFiles], Tuple[Pex, Snapshot, SourceFiles, SourceFiles]],
await MultiGet(requests),
)

all_source_files_snapshot = (
request.field_sets.prior_formatter_result
if request.field_sets.prior_formatter_result
request.request.prior_formatter_result
if request.request.prior_formatter_result
else rest[0].snapshot
)

Expand All @@ -139,7 +140,7 @@ async def setup(
)

address_references = ", ".join(
sorted(field_set.address.reference() for field_set in request.field_sets)
sorted(field_set.address.reference() for field_set in request.request.field_sets)
)

process = requirements_pex.create_process(
Expand All @@ -154,14 +155,14 @@ async def setup(
input_digest=input_digest,
output_files=all_source_files_snapshot.files,
description=(
f"Run Black on {pluralize(len(request.field_sets), 'target')}: {address_references}."
f"Run Black on {pluralize(len(request.request.field_sets), 'target')}: {address_references}."
),
)
return Setup(process, original_digest=all_source_files_snapshot.digest)


@named_rule(desc="Format using Black")
async def black_fmt(field_sets: BlackFieldSets, black: Black) -> FmtResult:
async def black_fmt(field_sets: BlackRequest, black: Black) -> FmtResult:
if black.options.skip:
return FmtResult.noop()
setup = await Get[Setup](SetupRequest(field_sets, check_only=False))
Expand All @@ -175,7 +176,7 @@ async def black_fmt(field_sets: BlackFieldSets, black: Black) -> FmtResult:


@named_rule(desc="Lint using Black")
async def black_lint(field_sets: BlackFieldSets, black: Black) -> LintResult:
async def black_lint(field_sets: BlackRequest, black: Black) -> LintResult:
if black.options.skip:
return LintResult.noop()
setup = await Get[Setup](SetupRequest(field_sets, check_only=True))
Expand All @@ -191,8 +192,8 @@ def rules():
black_fmt,
black_lint,
SubsystemRule(Black),
UnionRule(PythonFmtFieldSets, BlackFieldSets),
UnionRule(LinterFieldSets, BlackFieldSets),
UnionRule(PythonFmtRequest, BlackRequest),
UnionRule(LintRequest, BlackRequest),
*download_pex_bin.rules(),
*determine_source_files.rules(),
*pex.rules(),
Expand Down
Expand Up @@ -3,7 +3,7 @@

from typing import List, Optional, Tuple

from pants.backend.python.lint.black.rules import BlackFieldSet, BlackFieldSets
from pants.backend.python.lint.black.rules import BlackFieldSet, BlackRequest
from pants.backend.python.lint.black.rules import rules as black_rules
from pants.backend.python.target_types import PythonLibrary
from pants.base.specs import FilesystemLiteralSpec, OriginSpec, SingleAddress
Expand All @@ -30,7 +30,7 @@ class BlackIntegrationTest(ExternalToolTestBase):

@classmethod
def rules(cls):
return (*super().rules(), *black_rules(), RootRule(BlackFieldSets))
return (*super().rules(), *black_rules(), RootRule(BlackRequest))

def make_target_with_origin(
self, source_files: List[FileContent], *, origin: Optional[OriginSpec] = None,
Expand Down Expand Up @@ -61,7 +61,7 @@ def run_black(
options_bootstrapper = create_options_bootstrapper(args=args)
field_sets = [BlackFieldSet.create(tgt) for tgt in targets]
lint_result = self.request_single_product(
LintResult, Params(BlackFieldSets(field_sets), options_bootstrapper)
LintResult, Params(BlackRequest(field_sets), options_bootstrapper)
)
input_sources = self.request_single_product(
SourceFiles,
Expand All @@ -73,7 +73,7 @@ def run_black(
fmt_result = self.request_single_product(
FmtResult,
Params(
BlackFieldSets(field_sets, prior_formatter_result=input_sources.snapshot),
BlackRequest(field_sets, prior_formatter_result=input_sources.snapshot),
options_bootstrapper,
),
)
Expand Down

0 comments on commit 8d95788

Please sign in to comment.