Refactor fqcn to recommend use of canonical names (#2604)

Closs: #2572

Author: ssbarnea

examples/playbooks/test_skip_inside_yaml.yml

@@ -5,8 +5,8 @@
     - skip_ansible_lint # should disable error at playbook level
   tasks:
     - name: Test # <-- 0 latest[hg]
-      action: ansible.builtin.hg
-    - name: Test latest[hg] (skipped) # noqa latest[hg]
+      action: ansible.builtin.hg # noqa fqcn[canonical]
+    - name: Test latest[hg] (skipped) # noqa latest[hg] fqcn[canonical]
       action: ansible.builtin.hg
 
     - name: Test latest[git] and partial-become

examples/roles/role_for_no_same_owner/tasks/pass.yml

@@ -1,12 +1,12 @@
 ---
 - name: Synchronize-delegate
-  ansible.builtin.synchronize:
+  ansible.posix.synchronize:
     src: dummy
     dest: dummy
   delegate_to: localhost
 
 - name: Synchronize-no-same-owner
-  ansible.builtin.synchronize:
+  ansible.posix.synchronize:
     src: dummy
     dest: dummy
     owner: false

src/ansiblelint/rules/fqcn.md

@@ -7,9 +7,9 @@ This avoids ambiguity and conflicts that can cause operations to fail or produce
 
 The `fqcn` rule has the following checks:
 
-- `fqcn[action]` - Checks all actions for FQCNs.
+- `fqcn[action]` - Use FQCN for module actions, such ...
 - `fqcn[action-core]` - Checks for FQCNs from the `ansible.legacy` or `ansible.builtin` collection.
-- `fqcn[action-redirect]` - Provides the correct FQCN to replace short actions.
+- `fqcn[canonical]` - You should use canonical module name ... instead of ...
 
 ```{note}
 In most cases you should declare the `ansible.builtin` collection for internal Ansible actions.
@@ -22,6 +22,21 @@ The `collections` keyword provided a temporary mechanism transitioning to Ansibl
 You should rewrite any content that uses the `collections:` key and avoid it where possible.
 ```
 
+## Canonical module names
+
+Canonical module names are also known as **resolved module names** and they
+are to be preferred for most cases. Many Ansible modules have multiple aliases
+and redirects, as these were created over time while the content was refactored.
+Still, all of them do finally resolve to the same module name, but not without
+adding some performance overhead. As very old aliases are at some point removed,
+it makes to just refresh the content to make it point to the current canonical
+name.
+
+The only exception for using a canonical name is if your code still needs to
+be compatible with a very old version of Ansible, one that does not know how
+to resolve that name. If you find yourself in such a situation, feel free to
+add this rule to the ignored list.
+
 ## Problematic Code
 
 ```yaml

src/ansiblelint/rules/fqcn.py

@@ -1,13 +1,18 @@
 """Rule definition for usage of fully qualified collection names for builtins."""
 from __future__ import annotations
 
+import logging
 import sys
 from typing import Any
 
+from ansible.plugins.loader import module_loader
+
 from ansiblelint.errors import MatchError
 from ansiblelint.file_utils import Lintable
 from ansiblelint.rules import AnsibleLintRule
 
+_logger = logging.getLogger(__name__)
+
 builtins = [
     # spell-checker:disable
     "add_host",
@@ -92,33 +97,62 @@ class FQCNBuiltinsRule(AnsibleLintRule):
     )
     tags = ["formatting"]
     version_added = "v6.8.0"
+    module_aliases: dict[str, str] = {"block/always/rescue": "block/always/rescue"}
 
     def matchtask(
         self, task: dict[str, Any], file: Lintable | None = None
     ) -> list[MatchError]:
         result = []
         module = task["action"]["__ansible_module_original__"]
-        if module in builtins:
-            result.append(
-                self.create_matcherror(
-                    message=f"Use FQCN for builtin module actions ({module}).",
-                    details=f"Use `ansible.builtin.{module}` or `ansible.legacy.{module}` instead.",
-                    filename=file,
-                    linenumber=task["__line__"],
-                    tag="fqcn[action-core]",
-                )
-            )
-        # Add here implementation for fqcn[action-redirect]
-        elif module != "block/always/rescue" and module.count(".") < 2:
-            result.append(
-                self.create_matcherror(
-                    message=f"Use FQCN for module actions, such `<namespace>.<collection>.{module}`.",
-                    details=f"Action `{module}` is not FQCN.",
-                    filename=file,
-                    linenumber=task["__line__"],
-                    tag="fqcn[action]",
+
+        if module not in self.module_aliases:
+            loaded_module = module_loader.find_plugin_with_context(module)
+            target = loaded_module.resolved_fqcn
+            self.module_aliases[module] = target
+            if target is None:
+                _logger.warning("Unable to resolve FQCN for module %s", module)
+                self.module_aliases[module] = module
+                return []
+            if target not in self.module_aliases:
+                self.module_aliases[target] = target
+
+        if module != self.module_aliases[module]:
+            module_alias = self.module_aliases.get(module, "")
+            if module_alias.startswith("ansible.builtin"):
+                result.append(
+                    self.create_matcherror(
+                        message=f"Use FQCN for builtin module actions ({module}).",
+                        details=f"Use `ansible.builtin.{module}` or `ansible.legacy.{module}` instead.",
+                        filename=file,
+                        linenumber=task["__line__"],
+                        tag="fqcn[action-core]",
+                    )
                 )
-            )
+            else:
+                if module.count(".") < 2:
+                    result.append(
+                        self.create_matcherror(
+                            message=f"Use FQCN for module actions, such `{self.module_aliases[module]}`.",
+                            details=f"Action `{module}` is not FQCN.",
+                            filename=file,
+                            linenumber=task["__line__"],
+                            tag="fqcn[action]",
+                        )
+                    )
+                # TODO(ssbarnea): Remove the c.g. and c.n. exceptions from here once
+                # community team is flattening these.
+                # See: https://github.com/ansible-community/community-topics/issues/147
+                elif not module.startswith("community.general.") or module.startswith(
+                    "community.network."
+                ):
+                    result.append(
+                        self.create_matcherror(
+                            message=f"You should use canonical module name `{self.module_aliases[module]}` instead of `{module}`.",
+                            filename=file,
+                            linenumber=task["__line__"],
+                            tag="fqcn[canonical]",
+                        )
+                    )
         return result
 
 
@@ -160,10 +194,7 @@ def test_fqcn_builtin_fail(rule_runner: RunFromText) -> None:
         assert results[0].tag == "fqcn[action-core]"
         assert "Use FQCN for builtin module actions" in results[0].message
         assert results[1].tag == "fqcn[action]"
-        assert (
-            "Use FQCN for module actions, such `<namespace>.<collection>"
-            in results[1].message
-        )
+        assert "Use FQCN for module actions, such" in results[1].message
 
     @pytest.mark.parametrize(
         "rule_runner", (FQCNBuiltinsRule,), indirect=["rule_runner"]