Show context for reserved variable warnings (#85324)

* Show context for reserved variable warnings

* Update integration test

* Use var origin not value origin

* Use a list

* Ensure tagged varname is used

(cherry picked from commit d922398c4d)

changelogs/fragments/warn-on-reserved.yml

@@ -0,0 +1,2 @@
+minor_changes:
+  - variables - Warnings about reserved variable names now show context where the variable was defined.

lib/ansible/vars/manager.py

@@ -407,7 +407,7 @@ class VariableManager:
         all_vars = _combine_and_track(all_vars, self._extra_vars, "extra vars")
 
         # before we add 'reserved vars', check we didn't add any reserved vars
-        warn_if_reserved(all_vars.keys())
+        warn_if_reserved(all_vars)
 
         # magic variables
         all_vars = _combine_and_track(all_vars, magic_variables, "magic vars")
@@ -563,7 +563,8 @@ class VariableManager:
         if not isinstance(facts, Mapping):
             raise AnsibleAssertionError("the type of 'facts' to set for host_facts should be a Mapping but is a %s" % type(facts))
 
-        warn_if_reserved(facts.keys())
+        warn_if_reserved(facts)
+
         try:
             host_cache = self._fact_cache.get(host)
         except KeyError:
@@ -587,7 +588,8 @@ class VariableManager:
         if not isinstance(facts, Mapping):
             raise AnsibleAssertionError("the type of 'facts' to set for nonpersistent_facts should be a Mapping but is a %s" % type(facts))
 
-        warn_if_reserved(facts.keys())
+        warn_if_reserved(facts)
+
         try:
             self._nonpersistent_fact_cache[host] |= facts
         except KeyError:
@@ -599,6 +601,7 @@ class VariableManager:
         """
 
         warn_if_reserved([varname])
+
         if host not in self._vars_cache:
             self._vars_cache[host] = dict()
 

lib/ansible/vars/reserved.py

@@ -66,8 +66,7 @@ def get_reserved_names(include_private: bool = True) -> set[str]:
 
 
 def warn_if_reserved(myvars: c.Iterable[str], additional: c.Iterable[str] | None = None) -> None:
-    """ this function warns if any variable passed conflicts with internally reserved names """
+    """Issue a warning for any variable which conflicts with an internally reserved name."""
-
     if additional is None:
         reserved = _RESERVED_NAMES
     else:
@@ -76,8 +75,11 @@ def warn_if_reserved(myvars: c.Iterable[str], additional: c.Iterable[str] | None
     varnames = set(myvars)
     varnames.discard('vars')  # we add this one internally, so safe to ignore
 
-    for varname in varnames.intersection(reserved):
+    if conflicts := varnames.intersection(reserved):
-        display.warning(f'Found variable using reserved name {varname!r}.')
+        # Ensure the varname used for obj is the tagged one from myvars and not the untagged one from reserved.
+        # This can occur because tags do not affect value equality, and intersection can return values from either the left or right side.
+        for varname in (name for name in myvars if name in conflicts):
+            display.warning(f'Found variable using reserved name {varname!r}.', obj=varname)
 
 
 def is_reserved_name(name: str) -> bool:

test/integration/targets/var_reserved/tasks/main.yml

@@ -25,9 +25,9 @@
   - name: check they all complain about bad defined var
     assert:
       that:
-        - item.stderr == warning_message
+        - item.stderr.startswith(warning_message)
     loop: '{{play_out.results}}'
     loop_control:
       label: '{{item.item.file}}'
     vars:
-      warning_message: "[WARNING]: {{ canary }} '{{ item.item.name }}'."
+      warning_message: "[WARNING]: {{ canary }} '{{ item.item.name }}'.\nOrigin: "