Merge branch 'working-main' into exception-logging

Author: JessicaBell00

tools/pylint-extensions/azure-pylint-guidelines-checker/README.md

@@ -93,7 +93,11 @@ In the case of a false positive, use the disable command to remove the pylint er
 | docstring-keyword-should-match-keyword-only        | Docstring keyword arguments and keyword-only method arguments should match.                                                                                          | pylint:disable=docstring-keyword-should-match-keyword-only          | [link](https://azure.github.io/azure-sdk/python_documentation.html#docstrings)                          |
 | docstring-type-do-not-use-class                    | Docstring type is formatted incorrectly. Do not use `:class` in docstring type.                                                                                      | pylint:disable=docstring-type-do-not-use-class                      | [link](https://sphinx-rtd-tutorial.readthedocs.io/en/latest/docstrings.html)                            |
 | no-typing-import-in-type-check                     | Do not import typing under TYPE_CHECKING.                                                                                                                            | pylint:disable=no-typing-import-in-type-check                       | No Link.                                                                                                |
+| invalid-use-of-overload                            | Do not mix async and synchronous overloads                                                                                                                           | pylint:disable=invalid-use-of-overload                              | No Link.                                                                                                |
 | do-not-log-raised-errors                           | Do not log errors at `error` or `warning` level when error is raised in an exception block.                                                                          | pylint:disable=do-not-log-raised-errors                             | No Link.                                                                                                |
-| do-not-use-legacy-typing                           | Do not use legacy (<Python 3.8) type hinting comments                                                                                                             | pylint:disable=do-not-use-legacy-typing                             | No Link.
+| do-not-use-legacy-typing                           | Do not use legacy (<Python 3.8) type hinting comments                                                                                                             | pylint:disable=do-not-use-legacy-typing                             | No Link.                                                                                                |
 | do-not-import-asyncio                              | Do not import asyncio directly.                                                                                                                                      | pylint:disable=do-not-import-asyncio                                | No Link.                                                                                                |
-| do-not-log-exceptions                              | Do not log exceptions in levels other than debug, otherwise it can reveal sensitive information                                                            | pylint:disable=do-not-log-exceptions                                | [link](https://azure.github.io/azure-sdk/python_implementation.html#python-logging-sensitive-info)                                                                                                |
+| do-not-log-exceptions                              | Do not log exceptions in levels other than debug, otherwise it can reveal sensitive information                                                                      | pylint:disable=do-not-log-exceptions                                | [link](https://azure.github.io/azure-sdk/python_implementation.html#python-logging-sensitive-info)      |                                                                                                                            |                                                                     |                                                                                                         |
+| unapproved-client-method-name-prefix               | Clients should use preferred verbs for method names                                                                                                                  | pylint:disable=unapproved-client-method-name-prefix                 | [link](https://azure.github.io/azure-sdk/python_design.html#naming)                                     |
+| do-not-hardcode-connection-verify                  | Do not hardcode a boolean value to connection_verify                                                                                                                 | pylint:disable=do-not-hardcode-connection-verify                    | No LInk.                                                                                                |
+| TODO                                               | Address Commented out Pylint Custom Plugin Checkers #3228                                                                                                            |                                                                     |                                                                                                         |

tools/pylint-extensions/azure-pylint-guidelines-checker/pylint_guidelines_checker.py

@@ -161,79 +161,93 @@ class ClientHasApprovedMethodNamePrefix(BaseChecker):
             " https://azure.github.io/azure-sdk/python_design.html#service-operations",
             "unapproved-client-method-name-prefix",
             "All clients should use the preferred verbs for method names.",
-        )
-    }
-    options = (
-        (
-            "ignore-unapproved-client-method-name-prefix",
-            {
-                "default": False,
-                "type": "yn",
-                "metavar": "<y_or_n>",
-                "help": "Allow clients to not use preferred method name prefixes",
-            },
         ),
-    )
+    }
 
-    ignore_clients = [
+    ignore_clients = [  
         "PipelineClient",
         "AsyncPipelineClient",
         "ARMPipelineClient",
         "AsyncARMPipelineClient",
     ]
 
+    approved_prefixes = [
+        "get",
+        "list",
+        "create",
+        "upsert",
+        "set",
+        "update",
+        "replace",
+        "append",
+        "add",
+        "delete",
+        "remove",
+        "begin",
+        "upload",
+        "download", # standard verbs
+        "close",    # very common verb
+        "cancel",
+        "clear",
+        "subscribe",
+        "send",
+        "query",    # common verbs
+        "analyze",
+        "train",
+        "detect",   # future proofing
+        "from",     # special case
+    ]
+
+    ignored_decorators = [
+        "property",
+    ]
+
     def __init__(self, linter=None):
         super(ClientHasApprovedMethodNamePrefix, self).__init__(linter)
+        self.process_class = None
+        self.namespace = None
+    
+    def _check_decorators(self, node):
+        if not node.decorators:
+            return False
+        for decorator in node.decorators.nodes:
+            if isinstance(decorator, astroid.nodes.Name) and decorator.name in self.ignored_decorators:
+                return True
+        return False
+
+    def visit_module(self, node):
+        self.namespace = node.name
 
     def visit_classdef(self, node):
-        """Visits every class in file and checks if it is a client. If it is a client, checks
-        that approved method name prefixes are present.
+        if all((
+            node.name.endswith("Client"),
+            node.name not in self.ignore_clients,
+            not node.name.startswith("_"),
+            not '._' in self.namespace,
+        )):
+            self.process_class = node
 
-        :param node: class node
-        :type node: ast.ClassDef
-        :return: None
-        """
-        try:
-            if node.name.endswith("Client") and node.name not in self.ignore_clients:
-                client_methods = [
-                    child for child in node.get_children() if child.is_function
-                ]
-
-                approved_prefixes = [
-                    "get",
-                    "list",
-                    "create",
-                    "upsert",
-                    "set",
-                    "update",
-                    "replace",
-                    "append",
-                    "add",
-                    "delete",
-                    "remove",
-                    "begin",
-                ]
-                for idx, method in enumerate(client_methods):
-                    if (
-                        method.name.startswith("__")
-                        or "_exists" in method.name
-                        or method.name.startswith("_")
-                        or method.name.startswith("from")
-                    ):
-                        continue
-                    prefix = method.name.split("_")[0]
-                    if prefix.lower() not in approved_prefixes:
-                        self.add_message(
-                            msgid="unapproved-client-method-name-prefix",
-                            node=client_methods[idx],
-                            confidence=None,
-                        )
-        except AttributeError:
-            logger.debug(
-                "Pylint custom checker failed to check if client has approved method name prefix."
+    def visit_functiondef(self, node):
+        if any((
+            self.process_class is None, # not in a client class
+            node.name.startswith("_"), # private method
+            node.name.endswith("_exists"), # special case
+            self._check_decorators(node), # property
+            node.parent != self.process_class, # nested method
+        )):
+            return
+        
+        # check for approved prefix
+        parts = node.name.split("_")
+        if parts[0].lower() not in self.approved_prefixes:
+            self.add_message(
+                msgid="unapproved-client-method-name-prefix",
+                node=node,
+                confidence=None,
             )
-            pass
 
+    def leave_classdef(self, node):
+        self.process_class = None
 
 class ClientMethodsUseKwargsWithMultipleParameters(BaseChecker):
     name = "client-method-multiple-parameters"
@@ -2805,6 +2819,75 @@ def visit_import(self, node):
             pass
 
 
+class InvalidUseOfOverload(BaseChecker):
+    """Rule to check that use of the @overload decorator matches the async/sync nature of the underlying function"""
+
+    name = "invalid-use-of-overload"
+    priority = -1
+    msgs = {
+        "C4765": (
+            "Do not mix async and synchronous overloads",
+            "invalid-use-of-overload",
+            "Functions and their overloads must be either all async or all synchronous.",
+        ),
+    }
+
+    def visit_classdef(self, node):
+        """Check that use of the @overload decorator matches the async/sync nature of the underlying function"""
+
+        # Obtain a list of all functions and function names
+        functions = []
+        try:
+            node.body
+            for item in node.body:
+                if hasattr(item, 'name'):
+                    functions.append(item)
+
+            # Dictionary of lists of all functions by name
+            overloadedfunctions = {}
+            for item in functions:
+                if item.name in overloadedfunctions:
+                    overloadedfunctions[item.name].append(item)
+                else:
+                    overloadedfunctions[item.name] = [item]
+
+
+            # Loop through the overloaded functions and check they are the same type
+            for funct in overloadedfunctions.values():
+                if len(funct) > 1:  # only need to check if there is more than 1 function with the same name
+                    function_is_async = None
+
+                    for item in funct:
+                        if function_is_async is None:
+                            function_is_async = self.is_function_async(item)
+
+                        else:
+                            if function_is_async != self.is_function_async(item):
+                                self.add_message(
+                                    msgid=f"invalid-use-of-overload",
+                                    node=item,
+                                    confidence=None,
+                                )
+        except:
+            pass
+
+
+    def is_function_async(self, node):
+        try:
+            str(node.__class__).index("Async")
+            return True
+        except:
+            if node.returns is None:
+                return False
+            try:
+                if node.returns.value.name == "Awaitable":
+                    return True
+                else:
+                    return False
+            except:
+                return False
+
+
 class DoNotUseLegacyTyping(BaseChecker):
 
     """ Rule to check that we aren't using legacy typing using comments. """
@@ -2835,7 +2918,6 @@ class DoNotImportAsyncio(BaseChecker):
 
     name = "do-not-import-asyncio"
     priority = -1
-    # TODO Find message number
     msgs = {
         "C4763": (
             "Do not import the asyncio package directly in your library",
@@ -2864,8 +2946,6 @@ def visit_import(self, node):
                 )
 
 
-# [Pylint] custom linter check for invalid use of @overload #3229
-
 
 class DoNotLogExceptions(BaseChecker):
 
@@ -2940,8 +3020,83 @@ def check_for_logging(self, node, exception_name):
 
 
 # [Pylint] Address Commented out Pylint Custom Plugin Checkers #3228
-# [Pylint] Add a check for connection_verify hardcoded settings #35355
-# [Pylint] Investigate pylint rule around missing dependency #3231
+
+
+class DoNotHardcodeConnectionVerify(BaseChecker):
+
+    """Rule to check that developers do not hardcode a boolean to connection_verify."""
+
+    name = "do-not-hardcode-connection-verify"
+    priority = -1
+    msgs = {
+        "C4767": (
+            "Do not hardcode a boolean value to connection_verify",
+            "do-not-hardcode-connection-verify",
+            "Do not hardcode a boolean value to connection_verify. It's up to customers who use the code to be able to set it",
+        ),
+    }
+
+
+    def visit_call(self, node):
+        """Visit function calls to ensure it isn't used as a parameter"""
+        try:
+            for keyword in node.keywords:
+                if keyword.arg == "connection_verify":
+                    if type(keyword.value.value) == bool:
+                        self.add_message(
+                            msgid=f"do-not-hardcode-connection-verify",
+                            node=keyword,
+                            confidence=None,
+                        )
+        except:
+            pass
+
+
+    def visit_assign(self, node):
+        """Visiting variable Assignments"""
+        try: # Picks up self.connection_verify = True
+            if node.targets[0].attrname == "connection_verify":
+                if type(node.value.value) == bool:
+                    self.add_message(
+                        msgid=f"do-not-hardcode-connection-verify",
+                        node=node,
+                        confidence=None,
+                    )
+        except:
+            try: # connection_verify = True
+                if node.targets[0].name == "connection_verify":
+                    if type(node.value.value) == bool:
+                        self.add_message(
+                            msgid=f"do-not-hardcode-connection-verify",
+                            node=node,
+                            confidence=None,
+                        )
+            except:
+                pass
+
+
+    def visit_annassign(self, node):
+        """Visiting variable annotated assignments"""
+        try: # self.connection_verify: bool = True
+            if node.target.attrname == "connection_verify":
+                if type(node.value.value) == bool:
+                    self.add_message(
+                        msgid=f"do-not-hardcode-connection-verify",
+                        node=node,
+                        confidence=None,
+                    )
+        except:  # Picks up connection_verify: bool = True
+            try:
+                if node.target.name == "connection_verify":
+                    if type(node.value.value) == bool:
+                        self.add_message(
+                            msgid=f"do-not-hardcode-connection-verify",
+                            node=node,
+                            confidence=None,
+                        )
+            except:
+                pass
+
 
 
 # if a linter is registered in this function then it will be checked with pylint
@@ -2977,22 +3132,20 @@ def register(linter):
     linter.register_checker(DoNotImportAsyncio(linter))
     linter.register_checker(NoLegacyAzureCoreHttpResponseImport(linter))
     linter.register_checker(NoImportTypingFromTypeCheck(linter))
+    linter.register_checker(InvalidUseOfOverload(linter))
     linter.register_checker(DoNotUseLegacyTyping(linter))
     linter.register_checker(DoNotLogErrorsEndUpRaising(linter))
     linter.register_checker(DoNotLogExceptions(linter))
-
-    # [Pylint] custom linter check for invalid use of @overload #3229
     # [Pylint] Address Commented out Pylint Custom Plugin Checkers #3228
-    # [Pylint] Add a check for connection_verify hardcoded settings #35355
-    # [Pylint] Investigate pylint rule around missing dependency #3231
+    linter.register_checker(DoNotHardcodeConnectionVerify(linter))
 
     # disabled by default, use pylint --enable=check-docstrings if you want to use it
     linter.register_checker(CheckDocstringParameters(linter))
 
 
     # Rules are disabled until false positive rate improved
     # linter.register_checker(CheckForPolicyUse(linter))
-    # linter.register_checker(ClientHasApprovedMethodNamePrefix(linter))
+    linter.register_checker(ClientHasApprovedMethodNamePrefix(linter))
 
     # linter.register_checker(ClientDocstringUsesLiteralIncludeForCodeExample(linter))
     # linter.register_checker(ClientLROMethodsUseCorePolling(linter))