From cba73f9b4549f085cfa924374def5caa382182a1 Mon Sep 17 00:00:00 2001 From: andrecs <12188364+andrecsilva@users.noreply.github.com> Date: Mon, 27 Nov 2023 08:44:03 -0300 Subject: [PATCH] Refactoring, better descriptions, and documentation --- src/codemodder/codemods/utils.py | 6 +- .../docs/pixee_python_file-resource-leak.md | 2 +- src/core_codemods/file_resource_leak.py | 62 ++++++------------- 3 files changed, 23 insertions(+), 47 deletions(-) diff --git a/src/codemodder/codemods/utils.py b/src/codemodder/codemods/utils.py index d50721b06..7305ae580 100644 --- a/src/codemodder/codemods/utils.py +++ b/src/codemodder/codemods/utils.py @@ -121,16 +121,14 @@ def __init__(self, context: CodemodContext) -> None: if dependencies: wrapper = self.context.wrapper if wrapper is None: - # pylint: disable-next=broad-exception-raised - raise Exception( + raise ValueError( f"Attempting to instantiate {self.__class__.__name__} outside of " + "an active transform. This means that metadata hasn't been " + "calculated and we cannot successfully create this visitor." ) for dep in dependencies: if dep not in wrapper._metadata: - # pylint: disable-next=broad-exception-raised - raise Exception( + raise ValueError( f"Attempting to access metadata {dep.__name__} that was not a " + "declared dependency of parent transform! This means it is " + "not possible to compute this value. Please ensure that all " diff --git a/src/core_codemods/docs/pixee_python_file-resource-leak.md b/src/core_codemods/docs/pixee_python_file-resource-leak.md index f5377777a..8c3e9d42c 100644 --- a/src/core_codemods/docs/pixee_python_file-resource-leak.md +++ b/src/core_codemods/docs/pixee_python_file-resource-leak.md @@ -1,4 +1,4 @@ -This codemod wraps assignments of `open` calls in a with statement. Without explicit closing, these resources will be "leaked", and won't be re-claimed until garbage collection. In situations where these resources are leaked rapidly (either through malicious repetitive action or unusually spiky usage), connection pool or file handle exhaustion will occur. These types of failures tend to be catastrophic, resulting in downtime and many times affect downstream applications. +This codemod wraps assignments of `open` calls in a with statement. Without explicit closing, these resources will be "leaked" and won't be re-claimed until garbage collection. In situations where these resources are leaked rapidly (either through malicious repetitive action or unusually spiky usage), connection pool or file handle exhaustion will occur. These types of failures tend to be catastrophic, resulting in downtime and many times affect downstream applications. Our changes look something like this: diff --git a/src/core_codemods/file_resource_leak.py b/src/core_codemods/file_resource_leak.py index b58b66af0..54a870fef 100644 --- a/src/core_codemods/file_resource_leak.py +++ b/src/core_codemods/file_resource_leak.py @@ -1,8 +1,8 @@ from typing import Optional, Sequence +from codemodder.utils.utils import extract_targets_of_assignment import libcst as cst from libcst import ensure_type, matchers from libcst.codemod import ( - Codemod, CodemodContext, ContextAwareVisitor, ) @@ -14,34 +14,30 @@ ) from codemodder.change import Change from codemodder.codemods.base_codemod import ( - BaseCodemod, - CodemodMetadata, ReviewGuidance, ) -from codemodder.codemods.base_visitor import UtilsMixin +from codemodder.codemods.api import BaseCodemod from codemodder.codemods.utils import MetadataPreservingTransformer from codemodder.codemods.utils_mixin import AncestorPatternsMixin, NameResolutionMixin from codemodder.file_context import FileContext from functools import partial -class FileResourceLeak(BaseCodemod, UtilsMixin, Codemod): - SUMMARY = "Automatically close resources" - METADATA = CodemodMetadata( - DESCRIPTION=SUMMARY, - NAME="file-resource-leak", - REVIEW_GUIDANCE=ReviewGuidance.MERGE_WITHOUT_REVIEW, - REFERENCES=[ - { - "url": "https://cwe.mitre.org/data/definitions/772.html", - "description": "", - }, - { - "url": "https://cwe.mitre.org/data/definitions/404.html", - "description": "", - }, - ], - ) +class FileResourceLeak(BaseCodemod): + NAME = "file-resource-leak" + SUMMARY = "Automatically Close Resources" + REVIEW_GUIDANCE = ReviewGuidance.MERGE_WITHOUT_REVIEW + DESCRIPTION = SUMMARY + REFERENCES = [ + { + "url": "https://cwe.mitre.org/data/definitions/772.html", + "description": "", + }, + { + "url": "https://cwe.mitre.org/data/definitions/404.html", + "description": "", + }, + ] CHANGE_DESCRIPTION = "Wrapped opened resource in a with statement." METADATA_DEPENDENCIES = ( @@ -59,9 +55,7 @@ def __init__( self.changed_nodes: dict[ cst.CSTNode, cst.CSTNode | cst.RemovalSentinel | cst.FlattenSentinel ] = {} - BaseCodemod.__init__(self, file_context, *codemod_args) - UtilsMixin.__init__(self, []) - Codemod.__init__(self, context) + BaseCodemod.__init__(self, context, file_context, *codemod_args) def transform_module_impl(self, tree: cst.Module) -> cst.Module: fr = FindResources(self.context) @@ -250,7 +244,7 @@ def _find_direct_name_assignment_targets( for node in (access.node for access in accesses): maybe_assigned = self.is_value_of_assignment(node) if maybe_assigned: - targets = self._extract_targets_of_assignment(maybe_assigned) + targets = extract_targets_of_assignment(maybe_assigned) name_targets.extend(targets) return name_targets @@ -286,7 +280,7 @@ def _find_transitive_assignment_targets( maybe_assigned = self.is_value_of_assignment(expr) if maybe_assigned: named_targets, other_targets = self._sieve_targets( - self._extract_targets_of_assignment(maybe_assigned) + extract_targets_of_assignment(maybe_assigned) ) for n in named_targets: n_named_targets, n_other_targets = self._find_name_assignment_targets(n) @@ -295,22 +289,6 @@ def _find_transitive_assignment_targets( return named_targets, other_targets return ([], []) - def _extract_targets_of_assignment( - self, assignment: cst.AnnAssign | cst.Assign | cst.WithItem | cst.NamedExpr - ) -> list[cst.BaseExpression]: - match assignment: - case cst.AnnAssign(): - if assignment.target: - return [assignment.target] - case cst.Assign(): - return [t.target for t in assignment.targets] - case cst.NamedExpr(): - return [assignment.target] - case cst.WithItem(): - if assignment.asname: - return [assignment.asname.name] - return [] - # pylint: disable-next=too-many-arguments def _wrap_in_with_statement( self,