From 1f04f2823e8873a13a148868a434e1e31fb89c7a Mon Sep 17 00:00:00 2001 From: sj Date: Tue, 9 Jul 2024 01:02:58 -0400 Subject: [PATCH 1/8] Ran migration with new implicitly fixer over trufflehog --- .../pants/backend/tools/trufflehog/rules.py | 60 +++++++++---------- 1 file changed, 28 insertions(+), 32 deletions(-) diff --git a/src/python/pants/backend/tools/trufflehog/rules.py b/src/python/pants/backend/tools/trufflehog/rules.py index 1c0bf9f837b..c13a06ed811 100644 --- a/src/python/pants/backend/tools/trufflehog/rules.py +++ b/src/python/pants/backend/tools/trufflehog/rules.py @@ -4,23 +4,24 @@ from __future__ import annotations +from typing import Iterable + from pants.backend.tools.trufflehog.subsystem import Trufflehog from pants.core.goals.lint import LintFilesRequest, LintResult -from pants.core.util_rules.config_files import ConfigFiles, ConfigFilesRequest -from pants.core.util_rules.external_tool import DownloadedExternalTool, ExternalToolRequest +from pants.core.util_rules.config_files import find_config_file +from pants.core.util_rules.external_tool import download_external_tool from pants.core.util_rules.partitions import Partitions -from pants.engine.fs import ( - CreateDigest, - Digest, - DigestEntries, - FileEntry, - MergeDigests, - PathGlobs, - Snapshot, +from pants.engine.fs import CreateDigest, FileEntry, MergeDigests, PathGlobs +from pants.engine.intrinsics import ( + create_digest_to_digest, + digest_to_snapshot, + directory_digest_to_digest_entries, + merge_digests_request_to_digest, + process_request_to_process_result, ) from pants.engine.platform import Platform -from pants.engine.process import FallibleProcessResult, Process -from pants.engine.rules import Get, MultiGet, collect_rules, rule +from pants.engine.process import Process +from pants.engine.rules import Rule, collect_rules, concurrently, implicitly, rule from pants.source.filespec import FilespecMatcher from pants.util.logging import LogLevel from pants.util.strutil import pluralize @@ -53,37 +54,32 @@ async def run_trufflehog( ) -> LintResult: """Runs the trufflehog executable against the targeted files.""" - download_trufflehog_get = Get( - DownloadedExternalTool, ExternalToolRequest, trufflehog.get_request(platform) + download_trufflehog_get = download_external_tool(trufflehog.get_request(platform)) + config_files_get = find_config_file(trufflehog.config_request()) + downloaded_trufflehog, config_digest = await concurrently( + download_trufflehog_get, config_files_get ) - config_files_get = Get(ConfigFiles, ConfigFilesRequest, trufflehog.config_request()) - - downloaded_trufflehog, config_digest = await MultiGet(download_trufflehog_get, config_files_get) - # the downloaded files are going to contain the `exe`, readme and license. We only - # want the `exe` + # The downloaded files are going to contain the `exe`, readme and license. We only want the `exe` entry = next( e - for e in await Get(DigestEntries, Digest, downloaded_trufflehog.digest) + for e in await directory_digest_to_digest_entries(downloaded_trufflehog.digest) if isinstance(e, FileEntry) and e.path == "trufflehog" and e.is_executable ) - trufflehog_digest = await Get(Digest, CreateDigest([entry])) - - snapshot = await Get(Snapshot, PathGlobs(request.elements)) - input_digest = await Get( - Digest, + trufflehog_digest = await create_digest_to_digest(CreateDigest([entry])) + snapshot = await digest_to_snapshot(**implicitly(PathGlobs(request.elements))) + input_digest = await merge_digests_request_to_digest( MergeDigests( ( snapshot.digest, trufflehog_digest, config_digest.snapshot.digest, ) - ), + ) ) - process_result = await Get( - FallibleProcessResult, + process_result = await process_request_to_process_result( Process( argv=( downloaded_trufflehog.exe, @@ -102,13 +98,13 @@ async def run_trufflehog( description=f"Run Trufflehog on {pluralize(len(snapshot.files), 'file')}.", level=LogLevel.DEBUG, ), + **implicitly(), ) return LintResult.create(request, process_result) -def rules() -> list: - """Collect all the rules.""" - return [ +def rules() -> Iterable[Rule]: + return ( *collect_rules(), *TrufflehogRequest.rules(), - ] + ) From 2dfb2a559d49ea5bb68c9f4e818c949fd37b932b Mon Sep 17 00:00:00 2001 From: sj Date: Tue, 9 Jul 2024 01:22:56 -0400 Subject: [PATCH 2/8] Migrated semgrep to call-by-name --- .../pants/backend/tools/semgrep/rules.py | 117 +++++++++--------- 1 file changed, 61 insertions(+), 56 deletions(-) diff --git a/src/python/pants/backend/tools/semgrep/rules.py b/src/python/pants/backend/tools/semgrep/rules.py index e2e68cbe8bf..7212db88ba5 100644 --- a/src/python/pants/backend/tools/semgrep/rules.py +++ b/src/python/pants/backend/tools/semgrep/rules.py @@ -10,22 +10,21 @@ from typing import Iterable from pants.backend.python.util_rules import pex -from pants.backend.python.util_rules.pex import PexRequest, VenvPex, VenvPexProcess +from pants.backend.python.util_rules.pex import VenvPexProcess, create_venv_pex from pants.core.goals.lint import LintResult, LintTargetsRequest from pants.core.util_rules.partitions import Partition, Partitions -from pants.core.util_rules.source_files import SourceFiles, SourceFilesRequest +from pants.core.util_rules.source_files import SourceFilesRequest, determine_source_files from pants.engine.addresses import Address -from pants.engine.fs import ( - CreateDigest, - Digest, - FileContent, - MergeDigests, - PathGlobs, - Paths, - Snapshot, +from pants.engine.fs import CreateDigest, FileContent, MergeDigests, PathGlobs, Paths, Snapshot +from pants.engine.intrinsics import ( + create_digest_to_digest, + digest_to_snapshot, + merge_digests_request_to_digest, + path_globs_to_paths, + process_request_to_process_result, ) -from pants.engine.process import FallibleProcessResult, ProcessCacheScope -from pants.engine.rules import Get, MultiGet, Rule, collect_rules, rule +from pants.engine.process import ProcessCacheScope +from pants.engine.rules import Rule, collect_rules, concurrently, implicitly, rule from pants.engine.unions import UnionRule from pants.option.global_options import GlobalOptions from pants.util.logging import LogLevel @@ -101,7 +100,9 @@ def _group_by_semgrep_dir(all_paths: Paths) -> AllSemgrepConfigs: @rule async def find_all_semgrep_configs() -> AllSemgrepConfigs: - all_paths = await Get(Paths, PathGlobs([f"**/{file_glob}" for file_glob in _RULES_FILES_GLOBS])) + all_paths = await path_globs_to_paths( + PathGlobs([f"**/{file_glob}" for file_glob in _RULES_FILES_GLOBS]) + ) return _group_by_semgrep_dir(all_paths) @@ -123,7 +124,7 @@ async def infer_relevant_semgrep_configs( @rule async def all_semgrep_ignore_files() -> SemgrepIgnoreFiles: - snapshot = await Get(Snapshot, PathGlobs([f"**/{_IGNORE_FILE_NAME}"])) + snapshot = await digest_to_snapshot(**implicitly(PathGlobs([f"**/{_IGNORE_FILE_NAME}"]))) return SemgrepIgnoreFiles(snapshot) @@ -136,8 +137,8 @@ async def partition( if semgrep.skip: return Partitions() - all_configs = await MultiGet( - Get(RelevantSemgrepConfigs, RelevantSemgrepConfigsRequest(field_set)) + all_configs = await concurrently( + infer_relevant_semgrep_configs(RelevantSemgrepConfigsRequest(field_set), **implicitly()) for field_set in request.field_sets ) @@ -168,15 +169,18 @@ async def lint( semgrep: SemgrepSubsystem, global_options: GlobalOptions, ) -> LintResult: - config_files, semgrep_pex, input_files, settings = await MultiGet( - Get(Snapshot, PathGlobs(str(s) for s in request.partition_metadata.config_files)), - Get(VenvPex, PexRequest, semgrep.to_pex_request()), - Get(SourceFiles, SourceFilesRequest(field_set.source for field_set in request.elements)), - Get(Digest, CreateDigest([_DEFAULT_SETTINGS])), + config_files, semgrep_pex, input_files, settings = await concurrently( + digest_to_snapshot( + **implicitly(PathGlobs(str(s) for s in request.partition_metadata.config_files)) + ), + create_venv_pex(**implicitly(semgrep.to_pex_request())), + determine_source_files( + SourceFilesRequest(field_set.source for field_set in request.elements) + ), + create_digest_to_digest(CreateDigest([_DEFAULT_SETTINGS])), ) - input_digest = await Get( - Digest, + input_digest = await merge_digests_request_to_digest( MergeDigests( ( input_files.snapshot.digest, @@ -184,7 +188,7 @@ async def lint( settings, request.partition_metadata.ignore_files.digest, ) - ), + ) ) cache_scope = ProcessCacheScope.PER_SESSION if semgrep.force else ProcessCacheScope.SUCCESSFUL @@ -192,38 +196,39 @@ async def lint( # TODO: https://github.com/pantsbuild/pants/issues/18430 support running this with --autofix # under the fix goal... but not all rules have fixes, so we need to be running with # --error/checking exit codes, which FixResult doesn't currently support. - result = await Get( - FallibleProcessResult, - VenvPexProcess( - semgrep_pex, - argv=( - "scan", - *(f"--config={f}" for f in config_files.files), - "--jobs={pants_concurrency}", - "--error", - *semgrep.args, - # we don't pass the target files directly because that overrides .semgrepignore - # (https://github.com/returntocorp/semgrep/issues/4978), so instead we just tell its - # traversal to include all the source files in this partition. Unfortunately this - # include is implicitly unrooted (i.e. as if it was **/path/to/file), and so may - # pick up other files if the names match. The highest risk of this is within the - # semgrep PEX. - *(f"--include={f}" for f in input_files.files), - f"--exclude={semgrep_pex.pex_filename}", - ), - extra_env={ - "SEMGREP_FORCE_COLOR": "true", - # disable various global state/network requests - "SEMGREP_SETTINGS_FILE": _DEFAULT_SETTINGS.path, - "SEMGREP_ENABLE_VERSION_CHECK": "0", - "SEMGREP_SEND_METRICS": "off", - }, - input_digest=input_digest, - concurrency_available=len(input_files.files), - description=f"Run Semgrep on {pluralize(len(input_files.files), 'file')}.", - level=LogLevel.DEBUG, - cache_scope=cache_scope, - ), + result = await process_request_to_process_result( + **implicitly( + VenvPexProcess( + semgrep_pex, + argv=( + "scan", + *(f"--config={f}" for f in config_files.files), + "--jobs={pants_concurrency}", + "--error", + *semgrep.args, + # we don't pass the target files directly because that overrides .semgrepignore + # (https://github.com/returntocorp/semgrep/issues/4978), so instead we just tell its + # traversal to include all the source files in this partition. Unfortunately this + # include is implicitly unrooted (i.e. as if it was **/path/to/file), and so may + # pick up other files if the names match. The highest risk of this is within the + # semgrep PEX. + *(f"--include={f}" for f in input_files.files), + f"--exclude={semgrep_pex.pex_filename}", + ), + extra_env={ + "SEMGREP_FORCE_COLOR": "true", + # disable various global state/network requests + "SEMGREP_SETTINGS_FILE": _DEFAULT_SETTINGS.path, + "SEMGREP_ENABLE_VERSION_CHECK": "0", + "SEMGREP_SEND_METRICS": "off", + }, + input_digest=input_digest, + concurrency_available=len(input_files.files), + description=f"Run Semgrep on {pluralize(len(input_files.files), 'file')}.", + level=LogLevel.DEBUG, + cache_scope=cache_scope, + ) + ) ) return LintResult.create(request, result, output_simplifier=global_options.output_simplifier()) From 83d8c8e5f2ce4fa12ed3065969ca7a0816c59851 Mon Sep 17 00:00:00 2001 From: sj Date: Tue, 9 Jul 2024 01:23:21 -0400 Subject: [PATCH 3/8] Committing new CST code and tests to handle implicitly a bit better --- src/python/pants/goal/migrate_call_by_name.py | 98 ++++++++++++- .../pants/goal/migrate_call_by_name_test.py | 131 +++++++++++++++--- 2 files changed, 201 insertions(+), 28 deletions(-) diff --git a/src/python/pants/goal/migrate_call_by_name.py b/src/python/pants/goal/migrate_call_by_name.py index ed74acba363..2e5fcb8522a 100644 --- a/src/python/pants/goal/migrate_call_by_name.py +++ b/src/python/pants/goal/migrate_call_by_name.py @@ -12,6 +12,7 @@ from typing import Callable, Iterable, TypedDict import libcst as cst +import libcst.helpers as h import libcst.matchers as m import libcst.metadata from libcst.display import dump @@ -439,7 +440,7 @@ def map_no_args_get_to_new_syntax( args=[cst.Arg(value=cst.Call(cst.Name("implicitly")), star="**")], ) if called_funcdef := cstutil.extract_functiondef_from_module(module, new_function): - new_call = remove_unused_implicitly(new_call, called_funcdef) + new_call = fix_implicitly_usage(new_call, called_funcdef) imports = [cstutil.make_importfrom(module, new_function)] return new_call, imports @@ -484,6 +485,9 @@ def map_long_form_get_to_new_syntax( ], ) + if called_funcdef := cstutil.extract_functiondef_from_module(module, new_function): + new_call = fix_implicitly_usage(new_call, called_funcdef) + imports = [cstutil.make_importfrom(module, new_function)] return new_call, imports @@ -512,6 +516,10 @@ def map_short_form_get_to_new_syntax( cst.Arg(value=cst.Call(cst.Name("implicitly")), star="**"), ], ) + + if called_funcdef := cstutil.extract_functiondef_from_module(module, new_function): + new_call = fix_implicitly_usage(new_call, called_funcdef) + imports = [cstutil.make_importfrom(module, new_function)] return new_call, imports @@ -547,6 +555,9 @@ def map_dict_form_get_to_new_syntax( ], ) + if called_funcdef := cstutil.extract_functiondef_from_module(module, new_function): + new_call = fix_implicitly_usage(new_call, called_funcdef) + imports = [cstutil.make_importfrom(module, new_function)] return new_call, imports @@ -556,20 +567,95 @@ def map_dict_form_get_to_new_syntax( # ------------------------------------------------------------------------------------------ -def remove_unused_implicitly(call: cst.Call, called_func: cst.FunctionDef) -> cst.Call: +def fix_implicitly_usage(call: cst.Call, target_func: cst.FunctionDef) -> cst.Call: """The CallByNameSyntaxMapper aggressively adds `implicitly` for safety. This function removes - unnecessary ones. + unnecessary ones, and attempts to cleanup usage. The following cases are handled: - The called function takes no arguments - TODO: The called function takes the same number of arguments that are passed to it - TODO: Check the types of the passed in parameters, if they don't match, they need to be implicitly passed + + Parameters: + call: The replaced `Get` which now uses the migrated call-by-name syntax + target_func: The target called-by-name function """ call_func_name = cst.ensure_type(call.func, cst.Name).value - if call_func_name != called_func.name.value: + if call_func_name != target_func.name.value: return call - called_params = len(called_func.params.params) - if called_params == 0: + # If there are no `implicitly`s, there is nothing to do + implicit_calls = m.findall(call, m.Call(func=m.Name("implicitly"))) + if not implicit_calls: + return call + + # Only handling the 1-arg case (plus implicitly) for now, which is the overwhelming majority of usage + number_of_call_args = len(call.args) + if number_of_call_args > 2: + return call + + # If the target function takes no arguments, then there is nothing to `implicit` + number_of_target_args = len(target_func.params.params) + if number_of_target_args == 0: return call.with_changes(args=[]) + + target_annotations = [ + cst.ensure_type(a, cst.Annotation).annotation + for a in m.findall(target_func.params, m.Annotation()) + ] + target_types = [ + target_type + for a in target_annotations + if (target_type := h.get_full_name_for_node(a)) + ] + + # Positionally compare the target function's annotations with the call's arguments + # If they match, then there is no need for `implicitly` + + # Check if `implicitly` contains dict - as that needs special handling + implicit_call = cst.ensure_type(implicit_calls[0], cst.Call) + if implicit_call.args and isinstance(d := implicit_call.args[0].value, cst.Dict): + if len(d.elements) > 1: + # Not handling cases with larger than 1 element + return call + + element = cst.ensure_type(d.elements[0], cst.DictElement) + if h.get_full_name_for_node(element.value) == target_types[0]: + # If arg and target match, we can strip `implicitly` call + return call.with_changes(args=[cst.Arg(element.key)]) + else: + # If arg and target don't match, keep `implicitly` call, but remove dict for normal call + new_arg = cst.Arg( + cst.Call(cst.Name("implicitly"), args=[cst.Arg(element.key)]), star="**" + ) + return call.with_changes(args=[new_arg]) + + # If the target function takes in the same number of arguments as we've already passed in, + # and they are of the same type, then we don't need to pass in `implicitly` + if number_of_call_args - 1 == len(target_types): + arg = cst.ensure_type(call.args[0].value, cst.Call) + new_arg = call.args[0].with_changes(comma=cst.MaybeSentinel.DEFAULT) + if h.get_full_name_for_node(arg.func) == target_types[0]: + # If arg and target match, we can strip `implicitly` call + return call.with_changes(args=[new_arg]) + else: + # If arg and target don't match, keep `implicitly` call + new_arg = cst.Arg( + cst.Call(cst.Name("implicitly"), args=[new_arg]), star="**" + ) + return call.with_changes(args=[new_arg]) + + # If the target function takes in more arguments than we've passed in, then we need to pass in `implicitly` + # This checks if it should be a trailing implicitly, or if we should wrap the first arg + if number_of_call_args - 1 < len(target_types): + arg = cst.ensure_type(call.args[0].value, cst.Call) + if h.get_full_name_for_node(arg.func) == target_types[0]: + return call + else: + new_arg = call.args[0].with_changes(comma=cst.MaybeSentinel.DEFAULT) + new_arg = cst.Arg( + cst.Call(cst.Name("implicitly"), args=[new_arg]), star="**" + ) + return call.with_changes(args=[new_arg]) + return call diff --git a/src/python/pants/goal/migrate_call_by_name_test.py b/src/python/pants/goal/migrate_call_by_name_test.py index 85536d49f0b..d0dfbb09e56 100644 --- a/src/python/pants/goal/migrate_call_by_name_test.py +++ b/src/python/pants/goal/migrate_call_by_name_test.py @@ -8,8 +8,9 @@ from typing import Final, Iterable import libcst as cst +import pytest -from pants.goal.migrate_call_by_name import Replacement, remove_unused_implicitly +from pants.goal.migrate_call_by_name import Replacement, fix_implicitly_usage from pants.util.cstutil import make_importfrom_attr as import_attr OLD_FUNC_NAME: Final[str] = "hello" @@ -87,30 +88,116 @@ def test_replacement_sanitizes_shadowed_code(): assert imp.names[0].asname.deep_equals(cst.AsName(cst.Name(f"{NEW_FUNC_NAME}_get"))) -def test_remove_unused_implicity_noop(): - call = cst.Call( - func=cst.Name("do_foo"), args=[cst.Arg(cst.Call(func=cst.Name("implicitly")), star="**")] - ) - called_func = _default_func_def() - - new_call = remove_unused_implicitly(call, called_func) +def test_fix_implicitly_noop(): + call = _parse_call("no_op()") + target_func = _parse_funcdef("async def no_op(): ...") + new_call = fix_implicitly_usage(call, target_func) assert new_call.deep_equals(call) + assert new_call.deep_equals(_parse_call("no_op()")) -def test_remove_unused_implicity_(): - call = cst.Call( - func=cst.Name("do_foo"), args=[cst.Arg(cst.Call(func=cst.Name("implicitly")), star="**")] - ) - called_func = _default_func_def().with_changes(name=cst.Name("do_foo")) +@pytest.mark.parametrize( + "source, target, expected", + [ + ( + "some_random_func(**implicitly())", + "async def unrelated_func(): ...", + "some_random_func(**implicitly())", + ), + ( + "multi_arg_call(arg1, arg2, **implicitly())", + "async def multi_arg_call(arg1: str, arg2: str): ...", + "multi_arg_call(arg1, arg2, **implicitly())", + ), + ( + "create_archive(CreateArchive(EMPTY_SNAPSHOT), **implicitly())", + "async def create_archive(request: CreateArchive, system_binaries_environment: SystemBinariesSubsystem.EnvironmentAware) -> Digest: ...", + "create_archive(CreateArchive(EMPTY_SNAPSHOT), **implicitly())", + ), + ], +) +def test_fix_implicitly_keeps_required(source: str, target: str, expected: str): + call = _parse_call(source) + target_func = _parse_funcdef(target) + new_call = fix_implicitly_usage(call, target_func) + + dummy_module = cst.parse_module("") + assert new_call.deep_equals( + call + ), f"Expected {dummy_module.code_for_node(new_call)} to equal {dummy_module.code_for_node(call)}" + assert new_call.deep_equals( + _parse_call(expected) + ), f"Expected {dummy_module.code_for_node(new_call)} to equal {expected}" + + +@pytest.mark.parametrize( + "source, target, expected", + [ + ( + "find_all_targets(**implicitly())", + "async def find_all_targets() -> AllTargets: ...", + "find_all_targets()", + ), + ( + "digest_to_snapshot(Digest('a', 1), **implicitly())", + "async def digest_to_snapshot(digest: Digest) -> Snapshot: ...", + "digest_to_snapshot(Digest('a', 1))", + ), + ( + "create_pex(**implicitly({clangformat.to_pex_request(): PexRequest}))", + "async def create_pex(request: PexRequest) -> Pex: ...", + "create_pex(clangformat.to_pex_request())", + ), + ], +) +def test_fix_implicitly_usage_removes_unneeded(source: str, target: str, expected: str): + call = _parse_call(source) + target_func = _parse_funcdef(target) + new_call = fix_implicitly_usage(call, target_func) + + dummy_module = cst.parse_module("") + assert not new_call.deep_equals( + call + ), f"Expected {dummy_module.code_for_node(new_call)} to not equal {dummy_module.code_for_node(call)}" + assert new_call.deep_equals( + _parse_call(expected) + ), f"Expected {dummy_module.code_for_node(new_call)} to equal {expected}" + + +@pytest.mark.parametrize( + "source, target, expected", + [ + ( + "create_venv_pex(**implicitly({clangformat.to_pex_request(): PexRequest}))", + "async def create_venv_pex(request: VenvPexRequest, bash: BashBinary, pex_environment: PexEnvironment) -> VenvPex: ...", + "create_venv_pex(**implicitly(clangformat.to_pex_request()))", + ), + ( + "digest_to_snapshot(DigestSubset(config_files.snapshot.digest, PathGlobs([config_file])), **implicitly())", + "async def digest_to_snapshot(digest: Digest) -> Snapshot: ...", + "digest_to_snapshot(**implicitly(DigestSubset(config_files.snapshot.digest, PathGlobs([config_file]))))", + ), + ( + "process_request_to_process_result(VenvPexProcess(arg1, arg2, arg3), **implicitly())", + "async def process_request_to_process_result(process: Process, process_execution_environment: ProcessExecutionEnvironment) -> FallibleProcessResult: ...", + "process_request_to_process_result(**implicitly(VenvPexProcess(arg1, arg2, arg3)))", + ), + ], +) +def test_fix_implicitly_usage_modification(source: str, target: str, expected: str): + call = _parse_call(source) + target_func = _parse_funcdef(target) + new_call = fix_implicitly_usage(call, target_func) + + dummy_module = cst.parse_module("") + assert new_call.deep_equals( + _parse_call(expected) + ), f"Expected {dummy_module.code_for_node(new_call)} to equal {expected}" + - new_call = remove_unused_implicitly(call, called_func) - assert not new_call.deep_equals(call) - assert len(new_call.args) == 0 +def _parse_call(source: str) -> cst.Call: + return cst.ensure_type(cst.parse_expression(source), cst.Call) -def _default_func_def() -> cst.FunctionDef: - return cst.FunctionDef( - name=cst.Name("REPLACE_ME"), - params=cst.Parameters(), - body=cst.IndentedBlock(body=[cst.SimpleStatementLine([cst.Pass()])]), - ).deep_clone() +def _parse_funcdef(source: str) -> cst.FunctionDef: + return cst.ensure_type(cst.parse_statement(source), cst.FunctionDef) From 45121aa76508b1fe0567edfb44bc3a3924f6e215 Mon Sep 17 00:00:00 2001 From: sj Date: Tue, 9 Jul 2024 09:01:19 -0400 Subject: [PATCH 4/8] Migrated yamllint to call-by-name --- .../pants/backend/tools/yamllint/rules.py | 70 ++++++++++--------- src/python/pants/util/cstutil.py | 2 +- 2 files changed, 38 insertions(+), 34 deletions(-) diff --git a/src/python/pants/backend/tools/yamllint/rules.py b/src/python/pants/backend/tools/yamllint/rules.py index 9bff99905c3..40b83201429 100644 --- a/src/python/pants/backend/tools/yamllint/rules.py +++ b/src/python/pants/backend/tools/yamllint/rules.py @@ -1,5 +1,6 @@ # Copyright 2022 Pants project contributors (see CONTRIBUTORS.md). # Licensed under the Apache License, Version 2.0 (see LICENSE). + from __future__ import annotations import os @@ -7,18 +8,22 @@ from dataclasses import dataclass from typing import Any -from pants.backend.python.util_rules.pex import Pex, PexProcess, PexRequest +from pants.backend.python.util_rules.pex import PexProcess, create_pex from pants.backend.tools.yamllint.subsystem import Yamllint from pants.core.goals.lint import LintFilesRequest, LintResult from pants.core.util_rules.config_files import ( GatherConfigFilesByDirectoriesRequest, - GatheredConfigFilesByDirectories, + gather_config_files_by_workspace_dir, ) from pants.core.util_rules.partitions import Partition, Partitions -from pants.engine.fs import Digest, DigestSubset, MergeDigests, PathGlobs +from pants.engine.fs import DigestSubset, MergeDigests, PathGlobs from pants.engine.internals.native_engine import FilespecMatcher, Snapshot -from pants.engine.process import FallibleProcessResult -from pants.engine.rules import Get, MultiGet, collect_rules, rule +from pants.engine.intrinsics import ( + digest_to_snapshot, + merge_digests_request_to_digest, + process_request_to_process_result, +) +from pants.engine.rules import collect_rules, concurrently, implicitly, rule from pants.util.dirutil import group_by_dir from pants.util.logging import LogLevel from pants.util.strutil import pluralize @@ -51,14 +56,13 @@ async def partition_inputs( includes=yamllint.file_glob_include, excludes=yamllint.file_glob_exclude ).matches(request.files) - config_files = await Get( - GatheredConfigFilesByDirectories, + config_files = await gather_config_files_by_workspace_dir( GatherConfigFilesByDirectoriesRequest( tool_name=yamllint.name, config_filename=yamllint.config_file_name, filepaths=tuple(sorted(matching_filepaths)), orphan_filepath_behavior=yamllint.orphan_files_behavior, - ), + ) ) default_source_files: set[str] = set() @@ -71,8 +75,10 @@ async def partition_inputs( else: default_source_files.update(files) - config_file_snapshots = await MultiGet( - Get(Snapshot, DigestSubset(config_files.snapshot.digest, PathGlobs([config_file]))) + config_file_snapshots = await concurrently( + digest_to_snapshot( + **implicitly(DigestSubset(config_files.snapshot.digest, PathGlobs([config_file]))) + ) for config_file in source_files_by_config_file ) @@ -101,14 +107,11 @@ async def partition_inputs( async def run_yamllint( request: YamllintRequest.Batch[str, PartitionInfo], yamllint: Yamllint ) -> LintResult: - yamllint_bin = await Get(Pex, PexRequest, yamllint.to_pex_request()) - partition_info = request.partition_metadata - snapshot = await Get(Snapshot, PathGlobs(request.elements)) - - input_digest = await Get( - Digest, + yamllint_bin = await create_pex(yamllint.to_pex_request()) + snapshot = await digest_to_snapshot(**implicitly(PathGlobs(request.elements))) + input_digest = await merge_digests_request_to_digest( MergeDigests( ( snapshot.digest, @@ -119,26 +122,27 @@ async def run_yamllint( else () ), ) - ), + ) ) - process_result = await Get( - FallibleProcessResult, - PexProcess( - yamllint_bin, - argv=( - *( - ("-c", partition_info.config_snapshot.files[0]) - if partition_info.config_snapshot - else () + process_result = await process_request_to_process_result( + **implicitly( + PexProcess( + yamllint_bin, + argv=( + *( + ("-c", partition_info.config_snapshot.files[0]) + if partition_info.config_snapshot + else () + ), + *yamllint.args, + *snapshot.files, ), - *yamllint.args, - *snapshot.files, - ), - input_digest=input_digest, - description=f"Run yamllint on {pluralize(len(request.elements), 'file')}.", - level=LogLevel.DEBUG, - ), + input_digest=input_digest, + description=f"Run yamllint on {pluralize(len(request.elements), 'file')}.", + level=LogLevel.DEBUG, + ) + ) ) return LintResult.create(request, process_result) diff --git a/src/python/pants/util/cstutil.py b/src/python/pants/util/cstutil.py index 0f17cd1161c..2a15c6196ea 100644 --- a/src/python/pants/util/cstutil.py +++ b/src/python/pants/util/cstutil.py @@ -50,5 +50,5 @@ def extract_functiondef_from_module(module: str, func: str) -> cst.FunctionDef | with open(spec.origin) as f: source_code = f.read() tree = cst.parse_module(source_code) - results = m.findall(tree, matcher=m.FunctionDef(m.Name(func))) + results = m.findall(tree, matcher=m.FunctionDef(m.Name(func), asynchronous=m.Asynchronous())) return cst.ensure_type(results[0], cst.FunctionDef) if results else None From ac269cf89d6970a3211e8fd69ea1644341700520 Mon Sep 17 00:00:00 2001 From: sj Date: Tue, 9 Jul 2024 09:13:20 -0400 Subject: [PATCH 5/8] Fixed migrate tests --- src/python/pants/goal/migrate_call_by_name.py | 12 +++--------- .../migrate_call_by_name_integration_test.py | 18 +++++++++--------- 2 files changed, 12 insertions(+), 18 deletions(-) diff --git a/src/python/pants/goal/migrate_call_by_name.py b/src/python/pants/goal/migrate_call_by_name.py index 2e5fcb8522a..9ca0ede0862 100644 --- a/src/python/pants/goal/migrate_call_by_name.py +++ b/src/python/pants/goal/migrate_call_by_name.py @@ -604,9 +604,7 @@ def fix_implicitly_usage(call: cst.Call, target_func: cst.FunctionDef) -> cst.Ca for a in m.findall(target_func.params, m.Annotation()) ] target_types = [ - target_type - for a in target_annotations - if (target_type := h.get_full_name_for_node(a)) + target_type for a in target_annotations if (target_type := h.get_full_name_for_node(a)) ] # Positionally compare the target function's annotations with the call's arguments @@ -640,9 +638,7 @@ def fix_implicitly_usage(call: cst.Call, target_func: cst.FunctionDef) -> cst.Ca return call.with_changes(args=[new_arg]) else: # If arg and target don't match, keep `implicitly` call - new_arg = cst.Arg( - cst.Call(cst.Name("implicitly"), args=[new_arg]), star="**" - ) + new_arg = cst.Arg(cst.Call(cst.Name("implicitly"), args=[new_arg]), star="**") return call.with_changes(args=[new_arg]) # If the target function takes in more arguments than we've passed in, then we need to pass in `implicitly` @@ -653,9 +649,7 @@ def fix_implicitly_usage(call: cst.Call, target_func: cst.FunctionDef) -> cst.Ca return call else: new_arg = call.args[0].with_changes(comma=cst.MaybeSentinel.DEFAULT) - new_arg = cst.Arg( - cst.Call(cst.Name("implicitly"), args=[new_arg]), star="**" - ) + new_arg = cst.Arg(cst.Call(cst.Name("implicitly"), args=[new_arg]), star="**") return call.with_changes(args=[new_arg]) return call diff --git a/src/python/pants/goal/migrate_call_by_name_integration_test.py b/src/python/pants/goal/migrate_call_by_name_integration_test.py index eb0a27eb617..7e1104ea2fc 100644 --- a/src/python/pants/goal/migrate_call_by_name_integration_test.py +++ b/src/python/pants/goal/migrate_call_by_name_integration_test.py @@ -61,10 +61,10 @@ class ContrivedGoal(Goal): @goal_rule async def setup_migrateme(black: Black) -> ContrivedGoal: - foo = await variants(**implicitly({black: Black})) - bar = await multiline(**implicitly({black: Black})) - qux = await shadowed(**implicitly({black: Black})) - thud = await multiget(**implicitly({black: Black})) + foo = await variants(black) + bar = await multiline(black) + qux = await shadowed(black) + thud = await multiget(black) def rules(): return [*collect_rules(), *rules1(), *rules2()] @@ -195,7 +195,7 @@ class Foo: @rule async def variants(black: Black, local_env: ChosenLocalEnvironmentName) -> Foo: all_targets = await find_all_targets() - pex = await create_venv_pex(**implicitly({black.to_pex_request(): PexRequest})) + pex = await create_venv_pex(**implicitly(black.to_pex_request())) digest = await create_archive(CreateArchive(EMPTY_SNAPSHOT), **implicitly()) paths = await find_binary(**implicitly({BinaryPathRequest(binary_name='time', search_path=['/usr/bin']): BinaryPathRequest, local_env.val: EnvironmentName})) try: @@ -219,7 +219,7 @@ class Bar: @rule(desc="Ensure multi-line calls are migrated") async def multiline(black: Black) -> Bar: - pex = await create_venv_pex(**implicitly({black.to_pex_request(): PexRequest})) + pex = await create_venv_pex(**implicitly(black.to_pex_request())) class Thud: pass @@ -231,12 +231,12 @@ async def multiget(black: Black) -> Thud: multigot = await concurrently( find_all_targets(), all_targets_get, - create_venv_pex(**implicitly({black.to_pex_request(): PexRequest})), + create_venv_pex(**implicitly(black.to_pex_request())), digest_get ) multigot_sameline = await concurrently( find_all_targets(), all_targets_get, - create_venv_pex(**implicitly({black.to_pex_request(): PexRequest})), digest_get + create_venv_pex(**implicitly(black.to_pex_request())), digest_get ) multigot_forloop = await concurrently( create_archive(CreateArchive(EMPTY_SNAPSHOT), **implicitly()) @@ -279,7 +279,7 @@ class Qux: @rule(desc="Ensure assignment name is not shadowed by new syntax") async def shadowed(black: Black) -> Qux: - create_venv_pex = await create_venv_pex_get(**implicitly({black.to_pex_request(): PexRequest})) + create_venv_pex = await create_venv_pex_get(**implicitly(black.to_pex_request())) def rules(): return collect_rules() From 725d140489ac62fbddf2de0e025d973ffa2c2686 Mon Sep 17 00:00:00 2001 From: sj Date: Tue, 9 Jul 2024 09:19:51 -0400 Subject: [PATCH 6/8] Updated changelog --- docs/notes/2.23.x.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/docs/notes/2.23.x.md b/docs/notes/2.23.x.md index 51d3a40b2db..0ee961d3c9f 100644 --- a/docs/notes/2.23.x.md +++ b/docs/notes/2.23.x.md @@ -170,8 +170,11 @@ Pants has a new mechanism for `@rule` invocation in backends. In this release th - `cue` - `debian` - `makeself` +- `semgrep` - `sql` - `swift` +- `trufflehog` +- `yamllint` ## Full Changelog From 78ec22f7e47e5abb5e4104349855a9eade1e9f74 Mon Sep 17 00:00:00 2001 From: sj Date: Tue, 9 Jul 2024 09:31:35 -0400 Subject: [PATCH 7/8] Updated focs --- src/python/pants/goal/migrate_call_by_name.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/python/pants/goal/migrate_call_by_name.py b/src/python/pants/goal/migrate_call_by_name.py index 9ca0ede0862..48ed3623039 100644 --- a/src/python/pants/goal/migrate_call_by_name.py +++ b/src/python/pants/goal/migrate_call_by_name.py @@ -571,10 +571,12 @@ def fix_implicitly_usage(call: cst.Call, target_func: cst.FunctionDef) -> cst.Ca """The CallByNameSyntaxMapper aggressively adds `implicitly` for safety. This function removes unnecessary ones, and attempts to cleanup usage. - The following cases are handled: - - The called function takes no arguments - - TODO: The called function takes the same number of arguments that are passed to it - - TODO: Check the types of the passed in parameters, if they don't match, they need to be implicitly passed + Examples: + find_all_targets(**implicitly()) -> find_all_targets() + create_pex(**implicitly({req: PexRequest})) -> create_pex(req) + create_venv_pex(**implicitly({req: PexRequest})) -> create_venv_pex(**implicitly(req)) + + Refer to `migrate_call_by_name_test.py` for more examples. Parameters: call: The replaced `Get` which now uses the migrated call-by-name syntax From c0ffda7ca93bbb3a9c44143071ca7c015fa1a9f1 Mon Sep 17 00:00:00 2001 From: sj Date: Tue, 9 Jul 2024 09:41:47 -0400 Subject: [PATCH 8/8] Fixed format blunder on cstutil --- src/python/pants/util/cstutil.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/python/pants/util/cstutil.py b/src/python/pants/util/cstutil.py index 2a15c6196ea..384ec6901e5 100644 --- a/src/python/pants/util/cstutil.py +++ b/src/python/pants/util/cstutil.py @@ -50,5 +50,7 @@ def extract_functiondef_from_module(module: str, func: str) -> cst.FunctionDef | with open(spec.origin) as f: source_code = f.read() tree = cst.parse_module(source_code) - results = m.findall(tree, matcher=m.FunctionDef(m.Name(func), asynchronous=m.Asynchronous())) + results = m.findall( + tree, matcher=m.FunctionDef(m.Name(func), asynchronous=m.Asynchronous()) + ) return cst.ensure_type(results[0], cst.FunctionDef) if results else None