From d9760bcce1f1030740c5a582e93b53c9c1e9b32d Mon Sep 17 00:00:00 2001 From: Blank Spruce <32396809+BlankSpruce@users.noreply.github.com> Date: Wed, 18 Oct 2023 18:00:54 +0200 Subject: [PATCH] Warn about conflicting definitions --- .github/workflows/test.yml | 2 +- gersemi/command_invocation_dumper.py | 5 +- gersemi/custom_command_definition_finder.py | 31 +++++++++--- gersemi/runner.py | 32 +++++++++--- .../poem_parsearg_edition.cmake | 4 ++ .../poem_straightarg_edition.cmake | 3 ++ .../conflicting_definitions/CMakeLists.txt | 1 + .../conflicting_definitions/foo1.cmake | 3 ++ .../conflicting_definitions/foo2.cmake | 11 +++++ ...ssue_0011_conflicting_definitions.in.cmake | 2 + ...sue_0011_conflicting_definitions.out.cmake | 5 ++ .../test_custom_command_dumper_generation.py | 9 +++- tests/test_custom_command_formatting.py | 11 +++-- tests/test_executable.py | 49 +++++++++++++++++++ 14 files changed, 147 insertions(+), 21 deletions(-) create mode 100644 tests/custom_commands/conflicting_definitions/poem_parsearg_edition.cmake create mode 100644 tests/custom_commands/conflicting_definitions/poem_straightarg_edition.cmake create mode 100644 tests/executable/conflicting_definitions/CMakeLists.txt create mode 100644 tests/executable/conflicting_definitions/foo1.cmake create mode 100644 tests/executable/conflicting_definitions/foo2.cmake create mode 100644 tests/formatter/issue_0011_conflicting_definitions.in.cmake create mode 100644 tests/formatter/issue_0011_conflicting_definitions.out.cmake diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 039146b..b83c945 100644 --- a/.github/workflows/test.yml +++ b/.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' diff --git a/gersemi/command_invocation_dumper.py b/gersemi/command_invocation_dumper.py index 70dcc68..a5553c7 100644 --- a/gersemi/command_invocation_dumper.py +++ b/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: diff --git a/gersemi/custom_command_definition_finder.py b/gersemi/custom_command_definition_finder.py index 6de9021..a8a1e15 100644 --- a/gersemi/custom_command_definition_finder.py +++ b/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 diff --git a/gersemi/runner.py b/gersemi/runner.py index daf23fb..919df25 100644 --- a/gersemi/runner.py +++ b/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,10 +69,20 @@ 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 @@ -77,9 +90,16 @@ def find_all_custom_command_definitions( 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): diff --git a/tests/custom_commands/conflicting_definitions/poem_parsearg_edition.cmake b/tests/custom_commands/conflicting_definitions/poem_parsearg_edition.cmake new file mode 100644 index 0000000..7c7ac61 --- /dev/null +++ b/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() diff --git a/tests/custom_commands/conflicting_definitions/poem_straightarg_edition.cmake b/tests/custom_commands/conflicting_definitions/poem_straightarg_edition.cmake new file mode 100644 index 0000000..0b0df1d --- /dev/null +++ b/tests/custom_commands/conflicting_definitions/poem_straightarg_edition.cmake @@ -0,0 +1,3 @@ +function(poem) + message(FATAL_ERROR ${ARGN}) +endfunction() diff --git a/tests/executable/conflicting_definitions/CMakeLists.txt b/tests/executable/conflicting_definitions/CMakeLists.txt new file mode 100644 index 0000000..624c367 --- /dev/null +++ b/tests/executable/conflicting_definitions/CMakeLists.txt @@ -0,0 +1 @@ +foo(ONE TWO x THREE y z) diff --git a/tests/executable/conflicting_definitions/foo1.cmake b/tests/executable/conflicting_definitions/foo1.cmake new file mode 100644 index 0000000..018d549 --- /dev/null +++ b/tests/executable/conflicting_definitions/foo1.cmake @@ -0,0 +1,3 @@ +function(foo) + cmake_parse_arguments("ARG" "ONE" "TWO" "THREE" ${ARGN}) +endfunction() diff --git a/tests/executable/conflicting_definitions/foo2.cmake b/tests/executable/conflicting_definitions/foo2.cmake new file mode 100644 index 0000000..a69d91d --- /dev/null +++ b/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() diff --git a/tests/formatter/issue_0011_conflicting_definitions.in.cmake b/tests/formatter/issue_0011_conflicting_definitions.in.cmake new file mode 100644 index 0000000..36921f0 --- /dev/null +++ b/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") diff --git a/tests/formatter/issue_0011_conflicting_definitions.out.cmake b/tests/formatter/issue_0011_conflicting_definitions.out.cmake new file mode 100644 index 0000000..55aedca --- /dev/null +++ b/tests/formatter/issue_0011_conflicting_definitions.out.cmake @@ -0,0 +1,5 @@ +poem( + LINES + "foo" + "bar" +) diff --git a/tests/test_custom_command_dumper_generation.py b/tests/test_custom_command_dumper_generation.py index d6ff0e7..1396269 100644 --- a/tests/test_custom_command_dumper_generation.py +++ b/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) diff --git a/tests/test_custom_command_formatting.py b/tests/test_custom_command_formatting.py index 9c09b4a..39b447d 100644 --- a/tests/test_custom_command_formatting.py +++ b/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) diff --git a/tests/test_executable.py b/tests/test_executable.py index cc5ccb4..dc5ea5c 100644 --- a/tests/test_executable.py +++ b/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, + )