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.

Sign up for GitHub

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

Don't give a subsequent formatter an empty snapshot after skipping #15483

Merged
merged 1 commit into from
May 15, 2022
Merged

Don't give a subsequent formatter an empty snapshot after skipping #15483

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 2 additions & 1 deletion src/python/pants/core/goals/fmt.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,7 +286,8 @@ async def fmt_language(language_fmt_request: _LanguageFmtRequest) -> _LanguageFm
continue
result = await Get(FmtResult, FmtRequest, request)
results.append(result)
prior_formatter_result = result.output
if not result.skipped:
prior_formatter_result = result.output
Copy link
Member

Choose a reason for hiding this comment

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

Question for @Eric-Arellano @stuhood

Is the intention for skipped formatters to return an empty digest or just forward through the original input digest (untouched)?

As in, is the core intention for formatters to be pipelined (in some deterministic order), or is this a workaround/side-effect because skipped formatters happen to have empty digests?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's a good point. Right now "skipped" is a magical combo of values. What would it look like if it was distinct?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I don't think it matters much how Skip is factored, as long we don't waste time trying to run it etc

return _LanguageFmtResults(
tuple(results),
input=original_sources.snapshot.digest,
Expand Down
66 changes: 31 additions & 35 deletions src/python/pants/core/goals/fmt_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
from pants.engine.fs import EMPTY_DIGEST, CreateDigest, Digest, FileContent
Copy link
Member Author

@thejcannon thejcannon May 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This file could use a good amount of TLC. It seems to under-test fmt pretty badly and has more than a few quirks.

It's out of scope for this PR though. I cleaned it up enough to test the fix (fails before, passes after)

from pants.engine.internals.native_engine import EMPTY_SNAPSHOT, Snapshot
from pants.engine.rules import Get, collect_rules, rule
from pants.engine.target import FieldSet, MultipleSourcesField, Target
from pants.engine.target import FieldSet, SingleSourceField, Target
from pants.engine.unions import UnionRule
from pants.testutil.rule_runner import RuleRunner
from pants.util.logging import LogLevel
Expand All @@ -24,20 +24,20 @@
SMALLTALK_FILE = FileContent("formatted.st", b"y := self size + super size.')\n")


class FortranSources(MultipleSourcesField):
class FortranSource(SingleSourceField):
pass


class FortranTarget(Target):
alias = "fortran"
core_fields = (FortranSources,)
core_fields = (FortranSource,)


@dataclass(frozen=True)
class FortranFieldSet(FieldSet):
required_fields = (FortranSources,)
required_fields = (FortranSource,)

sources: FortranSources
sources: FortranSource


class FortranFmtRequest(FmtRequest):
Expand All @@ -47,30 +47,28 @@ class FortranFmtRequest(FmtRequest):

@rule
async def fortran_fmt(request: FortranFmtRequest) -> FmtResult:
output = (
await Get(Snapshot, CreateDigest([FORTRAN_FILE]))
if any(fs.address.target_name == "needs_formatting" for fs in request.field_sets)
else EMPTY_SNAPSHOT
)
if not any(fs.address.target_name == "needs_formatting" for fs in request.field_sets):
return FmtResult.skip(formatter_name=request.name)
output = await Get(Snapshot, CreateDigest([FORTRAN_FILE]))
return FmtResult(
input=EMPTY_SNAPSHOT, output=output, stdout="", stderr="", formatter_name=request.name
input=request.snapshot, output=output, stdout="", stderr="", formatter_name=request.name
)


class SmalltalkSources(MultipleSourcesField):
class SmalltalkSource(SingleSourceField):
pass


class SmalltalkTarget(Target):
alias = "smalltalk"
core_fields = (SmalltalkSources,)
core_fields = (SmalltalkSource,)


@dataclass(frozen=True)
class SmalltalkFieldSet(FieldSet):
required_fields = (SmalltalkSources,)
required_fields = (SmalltalkSource,)

sources: SmalltalkSources
source: SmalltalkSource


class SmalltalkNoopRequest(FmtRequest):
Expand All @@ -80,11 +78,10 @@ class SmalltalkNoopRequest(FmtRequest):

@rule
async def smalltalk_noop(request: SmalltalkNoopRequest) -> FmtResult:
result_digest = await Get(Digest, CreateDigest([SMALLTALK_FILE]))
result_snapshot = await Get(Snapshot, Digest, result_digest)
assert request.snapshot != EMPTY_SNAPSHOT
return FmtResult(
input=result_snapshot,
output=result_snapshot,
input=request.snapshot,
output=request.snapshot,
stdout="",
stderr="",
formatter_name=request.name,
Expand All @@ -98,6 +95,7 @@ class SmalltalkSkipRequest(FmtRequest):

@rule
def smalltalk_skip(request: SmalltalkSkipRequest) -> FmtResult:
assert request.snapshot != EMPTY_SNAPSHOT
return FmtResult.skip(formatter_name=request.name)


Expand All @@ -116,16 +114,6 @@ def fmt_rule_runner(
)


def fortran_digest(rule_runner: RuleRunner) -> Digest:
return rule_runner.make_snapshot({FORTRAN_FILE.path: FORTRAN_FILE.content.decode()}).digest


def merged_digest(rule_runner: RuleRunner) -> Digest:
return rule_runner.make_snapshot(
{fc.path: fc.content.decode() for fc in (FORTRAN_FILE, SMALLTALK_FILE)}
).digest


def run_fmt(
rule_runner: RuleRunner, *, target_specs: List[str], only: list[str] | None = None
) -> str:
Expand All @@ -141,19 +129,26 @@ def run_fmt(
def test_summary() -> None:
rule_runner = fmt_rule_runner(
target_types=[FortranTarget, SmalltalkTarget],
fmt_request_types=[FortranFmtRequest, SmalltalkNoopRequest, SmalltalkSkipRequest],
# NB: Keep SmalltalkSkipRequest before SmalltalkNoopRequest so it runs first. This helps test
# a bug where a formatter run after a skipped formatter was receiving an empty snapshot.
# See https://github.com/pantsbuild/pants/issues/15406
fmt_request_types=[FortranFmtRequest, SmalltalkSkipRequest, SmalltalkNoopRequest],
)

rule_runner.write_files(
{
"BUILD": dedent(
"""\
fortran(name='f1')
fortran(name='needs_formatting')
smalltalk(name='s1')
smalltalk(name='s2')
fortran(name='f1', source="ft1.f98")
fortran(name='needs_formatting', source="formatted.f98")
smalltalk(name='s1', source="st1.st")
smalltalk(name='s2', source="formatted.st")
""",
),
"ft1.f98": "READ INPUT TAPE 5\n",
"formatted.f98": "READ INPUT TAPE 5",
"st1.st": "y := self size + super size.')",
"formatted.st": "y := self size + super size.')\n",
},
)

Expand All @@ -171,7 +166,8 @@ def test_summary() -> None:
smalltalk_file = Path(rule_runner.build_root, SMALLTALK_FILE.path)
assert fortran_file.is_file()
assert fortran_file.read_text() == FORTRAN_FILE.content.decode()
assert not smalltalk_file.is_file()
assert smalltalk_file.is_file()
assert smalltalk_file.read_text() == SMALLTALK_FILE.content.decode()

stderr = run_fmt(
rule_runner,
Expand Down