Warn about conflicting definitions, fixes #11

.github/workflows/test.yml

@@ -38,5 +38,5 @@ jobs:
         run: tox -e tests
 
       - name: Run quality tests
-        run: tox -e lint -e format -e mypy
+        run: tox -e format && tox -e mypy
         if: matrix.name == 'Quality tests'

CHANGELOG.md

@@ -1,5 +1,9 @@
 # Changelog
 
+## [0.9.3] 2023-10-18
+### Fixed
+- warn about conflicting definitions for macros and functions, make usage of conflicting definitions consistent and deterministic (#11)
+
 ## [0.9.2] 2023-06-15
 ### Changed
 - allow PyYAML version 6 as a dependency

README.md

@@ -68,7 +68,7 @@ You can use gersemi with a pre-commit hook by adding the following to `.pre-comm
 ```yaml
 repos:
 - repo: https://github.com/BlankSpruce/gersemi
-  rev: 0.9.2
+  rev: 0.9.3
   hooks:
   - id: gersemi
 ```

gersemi/__version__.py

@@ -4,4 +4,4 @@
 __license__ = "MPL 2.0"
 __title__ = "gersemi"
 __url__ = "https://github.com/BlankSpruce/gersemi"
-__version__ = "0.9.2"
+__version__ = "0.9.3"

gersemi/command_invocation_dumper.py

@@ -42,12 +42,13 @@ class CommandInvocationDumper(
 ):
     @contextmanager
     def patched(self, patch):
-        old_class = self.__class__
+        old_class = type(self)
         try:
+            # pylint: disable=attribute-defined-outside-init
             self.__class__ = create_patch(patch, old_class)
             yield self
         finally:
-            self.__class__ = old_class  # pylint: disable=invalid-class-object
+            self.__class__ = old_class  # pylint: disable=invalid-class-object,
 
     def _get_patch(self, command_name):
         if command_name in BUILTIN_COMMAND_MAPPING:

gersemi/custom_command_definition_finder.py

@@ -30,13 +30,14 @@ def line_comment(self, children):
 
 
 class CMakeInterpreter(Interpreter):
-    def __init__(self, stack=None):
+    def __init__(self, filepath, stack=None):
         self.stack = {} if stack is None else stack
         self.found_commands = {}
+        self.filepath = filepath
 
     @property
     def _inner_scope(self):
-        return type(self)(stack=self.stack.copy())
+        return type(self)(filepath=self.filepath, stack=self.stack.copy())
 
     def _eval_variables(self, arg):
         for name, value in self.stack.items():
@@ -85,6 +86,15 @@ def _should_definition_be_ignored(self, block):
             )
         return False
 
+    def _add_command(self, name, arguments):
+        key = name.lower()
+        if key not in self.found_commands:
+            self.found_commands[key] = []
+
+        self.found_commands[key].append(
+            (arguments, f"{self.filepath}:{name.line}:{name.column}")
+        )
+
     def block(self, tree):
         if self._should_definition_be_ignored(tree):
             return
@@ -98,9 +108,9 @@ def block(self, tree):
 
         keywords, *_ = body
         if len(keywords) > 0:
-            self.found_commands[name.lower()] = positional_arguments, keywords[0]
+            self._add_command(name, (positional_arguments, keywords[0]))
         else:
-            self.found_commands[name.lower()] = positional_arguments, Keywords()
+            self._add_command(name, (positional_arguments, Keywords()))
 
     def block_body(self, tree):
         return [
@@ -134,6 +144,15 @@ def quoted_argument(self, tree):
     unquoted_argument = _extract_first
 
 
-def find_custom_command_definitions(tree):
+def find_custom_command_definitions(tree, filepath="---"):
     tree = DropIrrelevantElements(visit_tokens=True).transform(tree)
-    return CMakeInterpreter().visit(tree)
+    return CMakeInterpreter(filepath).visit(tree)
+
+
+def get_just_definitions(definitions):
+    result = {}
+    for name, info in definitions.items():
+        sorted_info = list(sorted(info, key=lambda item: item[1]))
+        arguments, _ = sorted_info[0]
+        result[name] = arguments
+    return result

gersemi/runner.py

@@ -8,7 +8,10 @@
 from typing import Callable, Dict, Iterable, Tuple
 from gersemi.cache import create_cache
 from gersemi.configuration import Configuration
-from gersemi.custom_command_definition_finder import find_custom_command_definitions
+from gersemi.custom_command_definition_finder import (
+    find_custom_command_definitions,
+    get_just_definitions,
+)
 from gersemi.formatter import create_formatter, Formatter
 from gersemi.mode import Mode
 from gersemi.parser import PARSER as parser
@@ -57,7 +60,7 @@ def find_custom_command_definitions_in_file_impl(filepath: Path) -> Dict[str, Ke
         return {}
 
     parse_tree = parser.parse(code)
-    return find_custom_command_definitions(parse_tree)
+    return find_custom_command_definitions(parse_tree, filepath)
 
 
 def find_custom_command_definitions_in_file(
@@ -66,20 +69,37 @@ def find_custom_command_definitions_in_file(
     return apply(find_custom_command_definitions_in_file_impl, filepath)
 
 
+def check_conflicting_definitions(definitions):
+    for name, info in definitions.items():
+        if len(info) > 1:
+            print_to_stderr(f"Warning: conflicting definitions for '{name}':")
+            places = sorted(where for _, where in info)
+            for index, where in enumerate(places):
+                kind = "(used)   " if index == 0 else "(ignored)"
+                print_to_stderr(f"{kind} {where}")
+
+
 def find_all_custom_command_definitions(
     paths: Iterable[Path], pool
 ) -> Dict[str, Keywords]:
-    result = {}
+    result: Dict = {}
 
     files = get_files(paths)
     find = find_custom_command_definitions_in_file
 
     for defs in pool.imap_unordered(find, files, chunksize=CHUNKSIZE):
         if isinstance(defs, Error):
             print_to_stderr(get_error_message(defs))
-        else:
-            result.update(defs)
-    return result
+            continue
+
+        for name, info in defs.items():
+            if name in result:
+                result[name].extend(info)
+            else:
+                result[name] = info
+
+    check_conflicting_definitions(result)
+    return get_just_definitions(result)
 
 
 def select_task(mode: Mode, configuration: Configuration):

tests/custom_commands/conflicting_definitions/poem_parsearg_edition.cmake

@@ -0,0 +1,4 @@
+function(poem)
+    cmake_parse_arguments("ARG" "" "" "LINES" ${ARGN})
+    message(FATAL_ERROR ${ARG_LINES})
+endfunction()

tests/custom_commands/conflicting_definitions/poem_straightarg_edition.cmake

@@ -0,0 +1,3 @@
+function(poem)
+    message(FATAL_ERROR ${ARGN})
+endfunction()

tests/executable/conflicting_definitions/CMakeLists.txt

@@ -0,0 +1 @@
+foo(ONE TWO x THREE y z)

tests/executable/conflicting_definitions/foo1.cmake

@@ -0,0 +1,3 @@
+function(foo)
+    cmake_parse_arguments("ARG" "ONE" "TWO" "THREE" ${ARGN})
+endfunction()

tests/executable/conflicting_definitions/foo2.cmake

@@ -0,0 +1,11 @@
+function(foo)
+    cmake_parse_arguments("ARG" "FOUR" "FIVE" "SIX" ${ARGN})
+endfunction()
+
+function(foo)
+    cmake_parse_arguments("ARG" "FOUR" "FIVE" "SIX" ${ARGN})
+endfunction()
+
+function(foo)
+    cmake_parse_arguments("ARG" "FOUR" "FIVE" "SIX" ${ARGN})
+endfunction()

tests/formatter/issue_0011_conflicting_definitions.in.cmake

@@ -0,0 +1,2 @@
+### {list_expansion: favour-expansion, definitions: [tests/custom_commands/conflicting_definitions]}
+poem(LINES "foo" "bar")

tests/formatter/issue_0011_conflicting_definitions.out.cmake

@@ -0,0 +1,5 @@
+poem(
+    LINES
+        "foo"
+        "bar"
+)

tests/test_custom_command_dumper_generation.py

@@ -1,4 +1,7 @@
-from gersemi.custom_command_definition_finder import find_custom_command_definitions
+from gersemi.custom_command_definition_finder import (
+    find_custom_command_definitions,
+    get_just_definitions,
+)
 from gersemi.dumper import Dumper
 from .tests_generator import generate_input_only_tests
 
@@ -36,7 +39,9 @@ def test_custom_command_generated_dumper(
     parsed_function_def = parser_with_postprocessing.parse(case.content)
     parsed_function = parser_with_postprocessing.parse(custom_command_to_format)
 
-    definitions = find_custom_command_definitions(parsed_function_def)
+    definitions = get_just_definitions(
+        find_custom_command_definitions(parsed_function_def)
+    )
     dumper = create_dumper(definitions)
 
     custom_command_formatted = dumper.visit(parsed_function)

tests/test_custom_command_formatting.py

@@ -1,5 +1,8 @@
 import pytest
-from gersemi.custom_command_definition_finder import find_custom_command_definitions
+from gersemi.custom_command_definition_finder import (
+    find_custom_command_definitions,
+    get_just_definitions,
+)
 from gersemi.dumper import Dumper
 
 
@@ -34,7 +37,7 @@ def test_custom_command_without_keyworded_arguments_formatting(
 """
 
     parsed = parser_with_postprocessing.parse(given)
-    definitions = find_custom_command_definitions(parsed)
+    definitions = get_just_definitions(find_custom_command_definitions(parsed))
     dumper = Dumper(width=80, custom_command_definitions=definitions)
 
     formatted = dumper.visit(parsed)
@@ -71,7 +74,7 @@ def test_custom_command_without_keyworded_arguments_with_disabled_reformatting(
 """
 
     parsed = parser_with_postprocessing.parse(given)
-    definitions = find_custom_command_definitions(parsed)
+    definitions = get_just_definitions(find_custom_command_definitions(parsed))
     dumper = Dumper(width=80, custom_command_definitions=definitions)
 
     formatted = dumper.visit(parsed)
@@ -104,7 +107,7 @@ def test_can_deal_with_empty_body_in_custom_command_definition(
 """
 
     parsed = parser_with_postprocessing.parse(given)
-    definitions = find_custom_command_definitions(parsed)
+    definitions = get_just_definitions(find_custom_command_definitions(parsed))
     dumper = Dumper(width=80, custom_command_definitions=definitions)
 
     formatted = dumper.visit(parsed)

tests/test_executable.py

@@ -3,6 +3,7 @@
 import filecmp
 import os
 from pathlib import Path
+import re
 import shutil
 from stat import S_IREAD, S_IRGRP, S_IROTH
 import subprocess
@@ -805,3 +806,51 @@ def test_cache_is_not_updated_when_input_is_from_stdin():
         inspector.assert_that_has_initialized_tables()
         assert len(inspector.get_files()) == 0
         assert len(inspector.get_formatted()) == 0
+
+
+def test_check_project_with_conflicting_command_definitions():
+    with temporary_dir_copy(case("conflicting_definitions")) as copy:
+        completed_process = gersemi("--check", copy, "--definitions", copy)
+        assert completed_process.returncode == 0
+        assert completed_process.stdout == ""
+        assert re.search(
+            f"""Warning: conflicting definitions for 'foo':
+\\(used\\)    .*foo1\\.cmake:1:10
+\\(ignored\\) .*foo2\\.cmake:1:10
+\\(ignored\\) .*foo2\\.cmake:5:10
+\\(ignored\\) .*foo2\\.cmake:9:10
+""",
+            completed_process.stderr,
+            re.MULTILINE,
+        )
+
+
+def test_format_file_with_conflicting_command_definitions():
+    with temporary_dir_copy(case("conflicting_definitions")) as copy:
+        completed_process = gersemi(
+            f"{copy}/CMakeLists.txt",
+            "--definitions",
+            copy,
+            "--list-expansion=favour-expansion",
+        )
+        assert completed_process.returncode == 0
+        assert (
+            completed_process.stdout
+            == """foo(ONE
+    TWO x
+    THREE
+        y
+        z
+)
+"""
+        )
+        assert re.search(
+            f"""Warning: conflicting definitions for 'foo':
+\\(used\\)    .*foo1\\.cmake:1:10
+\\(ignored\\) .*foo2\\.cmake:1:10
+\\(ignored\\) .*foo2\\.cmake:5:10
+\\(ignored\\) .*foo2\\.cmake:9:10
+""",
+            completed_process.stderr,
+            re.MULTILINE,
+        )