From b12b81b5755b67b64d2dba8c3fa70eced3153651 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Smail=20Galija=C5=A1evi=C4=87?= <32690613+SmailG@users.noreply.github.com> Date: Tue, 10 Nov 2026 13:19:14 +0100 Subject: [PATCH 01/29] fix: make ST lint nesting handling linear in the nesting depth lint_st rescanned the whole block stack for every token: the conditional and in-loop checks, the EXIT/CONTINUE check, the scope lookup and the closer search when nothing matched. "IF a THEN " repeated 32000 times took 24 s and doubling the depth quadrupled the time. Keep a per-keyword count of the open blocks, updated where frames are pushed and where a closer discards them, and give every frame a pointer to its nearest scope frame at push time. Findings and analysis fields are unchanged. --- scripts/plccheck/st.py | 33 ++++++++++++++++++++++++--------- 1 file changed, 24 insertions(+), 9 deletions(-) diff --git a/scripts/plccheck/st.py b/scripts/plccheck/st.py index 7910754..12e0566 100644 --- a/scripts/plccheck/st.py +++ b/scripts/plccheck/st.py @@ -18,6 +18,7 @@ from __future__ import annotations import re +from collections import Counter from dataclasses import dataclass, field from .findings import Finding, error, warning @@ -242,6 +243,7 @@ class _Frame: line: int scope: dict[str, int] | None = None # declared names, for scope openers body: bool = False # statements allowed at this level + owner: "_Frame | None" = field(default=None, repr=False) # nearest scope frame at or below this one def _is_zero(t: Tok) -> bool: @@ -317,16 +319,25 @@ def lint_st(src: str, mode: str = "file") -> tuple[list[Finding], StAnalysis]: toks, findings = tokenize(src) an = StAnalysis() root = _Frame("", 0, scope={}, body=(mode != "declaration")) + root.owner = root stack: list[_Frame] = [root] + open_kw: Counter[str] = Counter() # open frames per keyword, so "is any X open" is not a stack scan pending: tuple[str, str, int] | None = None # (awaited keyword, opener, line) stmt: list[Tok] = [] paren = 0 + def push(fr: _Frame) -> None: + fr.owner = fr if fr.scope is not None else stack[-1].owner + stack.append(fr) + open_kw[fr.kw] += 1 + + def is_open(kinds: set[str]) -> bool: + return any(open_kw[k] for k in kinds) + def scope_frame() -> _Frame: - for fr in reversed(stack): - if fr.scope is not None: - return fr - return root + owner = stack[-1].owner + assert owner is not None + return owner def in_decl() -> bool: return stack[-1].kw in DECL_OPENERS @@ -415,8 +426,8 @@ def _declaration(decl: list[Tok]) -> None: an.has_body = True name = t.text.lstrip("#").upper() nxt = toks[idx + 1] if idx + 1 < len(toks) else None - conditional = any(fr.kw in CONDITIONALS for fr in stack) - in_loop = any(fr.kw in LOOPS for fr in stack) + conditional = is_open(CONDITIONALS) + in_loop = is_open(LOOPS) if not t.member: if _is_op(nxt, "("): an.calls.append((name, t.line, conditional, in_loop)) @@ -459,7 +470,7 @@ def _declaration(decl: list[Tok]) -> None: k += 1 if k < len(toks) and toks[k].kind in ("word", "dstr"): an.header = (kw, toks[k].text.strip('"')) - stack.append(fr) + push(fr) stmt = [] continue @@ -480,7 +491,7 @@ def _declaration(decl: list[Tok]) -> None: stmt = [t] continue if kw in ("EXIT", "CONTINUE"): - if not any(fr.kw in LOOPS for fr in stack): + if not is_open(LOOPS): findings.append(error("ST005", f"{kw} outside FOR/WHILE/REPEAT", t.line)) stmt.append(t) continue @@ -491,13 +502,17 @@ def _declaration(decl: list[Tok]) -> None: if kw in CLOSERS: expected = CLOSERS[kw] - pos = next((k for k in range(len(stack) - 1, 0, -1) if stack[k].kw in expected), None) + pos = None + if is_open(expected): # else the scan would walk the whole stack to find nothing + pos = next((k for k in range(len(stack) - 1, 0, -1) if stack[k].kw in expected), None) if pos is None: findings.append(error("ST002", f"{kw} without a matching {'/'.join(sorted(expected))}", t.line)) else: for fr in stack[pos + 1:]: findings.append(error("ST002", f"{fr.kw} opened at line {fr.line} is not closed before {kw}", fr.line)) closed = stack[pos] + for fr in stack[pos:]: + open_kw[fr.kw] -= 1 del stack[pos:] if closed.kw in VAR_KINDS: scope_frame().body = True From 4b1dad3d58efc6c56dfaf7f0b00cc95acd444eb1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Smail=20Galija=C5=A1evi=C4=87?= <32690613+SmailG@users.noreply.github.com> Date: Tue, 10 Nov 2026 13:19:14 +0100 Subject: [PATCH 02/29] test: pin ST lint scaling with nesting depth and the open-block state it carries A ratio test over four deeply nested inputs (one per rescan site) in the style of the XML text-join test, plus behaviour tests for conditional/in-loop/EXIT state after closers, closers that discard unclosed blocks, and declaration scope through nested blocks. --- tests/test_formats.py | 39 +++++++++++++++++++++++++++++++++ tests/test_st.py | 50 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 89 insertions(+) diff --git a/tests/test_formats.py b/tests/test_formats.py index 7032694..069d76c 100644 --- a/tests/test_formats.py +++ b/tests/test_formats.py @@ -18,6 +18,7 @@ sys.path.insert(0, str(HERE.parent / "scripts")) import plc_validate # noqa: E402 +from plccheck.st import lint_st # noqa: E402 from plccheck.xmlutil import XmlError, parse_bytes, sniff_root # noqa: E402 TC6_NS = "http://www.plcopen.org/xml/tc6_0201" @@ -606,6 +607,44 @@ def test_text_join_time_grows_linearly_with_the_chunk_count(self): f"{self.SMALL} chunks took {t_small:.3f}s, {self.BIG} chunks took {t_big:.3f}s") +class StNestingScaling(unittest.TestCase): + """ST lint time must grow linearly with the nesting depth: no rescan of the open blocks per token.""" + + RUNS = 3 + SMALL, BIG = 2_000, 8_000 # BIG is 4x SMALL: linear work is about 4x, quadratic about 16x + MAX_RATIO = 8 + TIMER_FLOOR = 0.01 # seconds; keeps the ratio meaningful if the small run is below timer noise + + # Each input leaves every block open, so the stack is n deep where a rescan would run. A scan for + # a keyword stops at the first match, so the IF input only exposes the loop scan and the FOR input the + # conditional scan. + INPUTS = { + "word tokens and scope lookup in n open IF": lambda n: "IF a THEN " * n, + "word tokens in n open FOR (no IF/CASE to stop the conditional scan early)": lambda n: "FOR a DO " * n, + "EXIT with n open IF and no loop": lambda n: "IF a THEN " * n + "EXIT;" * n, + "closer matching nothing over n open IF": lambda n: "IF a THEN " * n + "END_FOR;" * n, + } + + def best_of_runs(self, src: str) -> float: + best = float("inf") + for _ in range(self.RUNS): + start = time.perf_counter() + lint_st(src) + best = min(best, time.perf_counter() - start) + return best + + def test_lint_time_grows_linearly_with_the_nesting_depth(self): + # A ratio, not an absolute bound, so a slow CI runner does not flake: both sizes slow + # down together. Best of three drops a run that a scheduling hiccup inflated, which would + # otherwise push the ratio up. Rescanning the whole stack for every token is quadratic and + # lands near 16x; per-block counts and a scope pointer land near 4x. + for name, make in self.INPUTS.items(): + with self.subTest(name): + t_small, t_big = self.best_of_runs(make(self.SMALL)), self.best_of_runs(make(self.BIG)) + self.assertLess(t_big, self.MAX_RATIO * max(t_small, self.TIMER_FLOOR), + f"depth {self.SMALL} took {t_small:.3f}s, depth {self.BIG} took {t_big:.3f}s") + + class ShippedTemplates(unittest.TestCase): """Every template a skill hands to the model must itself pass the validator.""" diff --git a/tests/test_st.py b/tests/test_st.py index f01413b..022cb0d 100644 --- a/tests/test_st.py +++ b/tests/test_st.py @@ -192,6 +192,56 @@ def test_split_declaration_and_body(self): self.assertEqual(codes(scan_call_findings([an1, an2])), ["ST030"]) +class OpenBlockStateTests(unittest.TestCase): + """What the open-block bookkeeping answers: conditional, in a loop, in a loop at all, which scope.""" + + def prog(self, body: str) -> str: + return ("PROGRAM P\nVAR\n tonA : TON;\n rtB : R_TRIG;\n x : BOOL;\n i : INT;\nEND_VAR\nrtB(CLK := x);\n" + + body + "\nEND_PROGRAM\n") + + def test_timer_after_end_if_is_unconditional(self): + self.assertEqual(codes(check_st_file(self.prog("IF x THEN i := 1; END_IF;\ntonA(IN := x, PT := T#1S);"))), []) + + def test_timer_in_for_inside_if_is_conditional_and_in_loop(self): + f = check_st_file(self.prog("IF x THEN FOR i := 1 TO 3 DO tonA(IN := x, PT := T#1S); END_FOR; END_IF;")) + self.assertEqual(codes(f), ["ST030", "ST032"]) + + def test_timer_after_end_for_inside_if_is_conditional_but_not_in_loop(self): + f = check_st_file(self.prog("IF x THEN FOR i := 1 TO 3 DO i := i; END_FOR; tonA(IN := x, PT := T#1S); END_IF;")) + self.assertEqual(codes(f), ["ST030"]) + + def test_timer_after_closer_that_discards_an_open_if_is_unconditional(self): + f = check_st_file(self.prog("FOR i := 1 TO 3 DO IF x THEN END_FOR;\ntonA(IN := x, PT := T#1S);")) + self.assertEqual(codes(f), ["ST002"]) + self.assertIn("IF opened", f[0].message) + + def test_exit_after_the_loop_closed_is_outside_a_loop(self): + f = check_st_file("WHILE a DO x := 1; END_WHILE;\nEXIT;\n") + self.assertEqual(codes(f), ["ST005"]) + self.assertEqual(f[0].line, 2) + + def test_exit_after_closer_that_discards_an_open_loop_is_outside_a_loop(self): + f = check_st_file("IF a THEN WHILE b DO END_IF;\nEXIT;\n") + self.assertEqual(codes(f), ["ST002", "ST005"]) + + def test_closer_of_another_kind_matches_nothing_and_keeps_the_stack(self): + f = check_st_file("IF a THEN END_FOR; x := 1; END_IF;\n") + self.assertEqual(codes(f), ["ST002"]) + self.assertIn("END_FOR without a matching FOR", f[0].message) + + def test_closer_finds_its_opener_under_other_open_blocks(self): + self.assertEqual(codes(check_st_file("WHILE a DO IF b THEN CASE c OF 1: EXIT; END_CASE; END_IF; END_WHILE;\n")), []) + + def test_scope_follows_the_nearest_pou_through_nested_blocks_and_closers(self): + src = ("FUNCTION_BLOCK F\nVAR a : INT; END_VAR\n" + "METHOD M\nVAR a : INT; END_VAR\n" # own scope: not a duplicate of F's a + "IF a THEN IF a THEN IF a THEN x = 1; END_IF; END_IF; END_IF;\n" # body seen 3 blocks deep: ST010 + "END_METHOD\n" + "VAR a : INT; END_VAR\n" # back in F's scope: duplicate + "END_FUNCTION_BLOCK\n") + self.assertEqual(codes(check_st_file(src)), ["ST010", "ST040"]) + + class DialectTests(unittest.TestCase): def test_siemens_scl_source(self): src = '''FUNCTION_BLOCK "FB_Conveyor" From 7d45e310d0066cfd793346b7a4bb422082c5a735 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Smail=20Galija=C5=A1evi=C4=87?= <32690613+SmailG@users.noreply.github.com> Date: Tue, 10 Nov 2026 13:19:14 +0100 Subject: [PATCH 03/29] chore: bump version to 0.2.2 --- .claude-plugin/plugin.json | 2 +- plugin.json | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.claude-plugin/plugin.json b/.claude-plugin/plugin.json index 01668c1..79560aa 100644 --- a/.claude-plugin/plugin.json +++ b/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "$schema": "https://anthropic.com/claude-code/plugin.schema.json", "name": "plc-programming", - "version": "0.2.1", + "version": "0.2.2", "description": "PLC programming across IEC 61131-3 (ST, LD, FBD, SFC, IL), PLCopen XML / IEC 61131-10, Siemens TIA (SCL, SimaticML, SIMATIC SD), Schneider (Machine Expert, Control Expert), CODESYS, TwinCAT, ABB Freelance, Rockwell L5X/L5K, IEC 61499, AutomationML, DEXPI, PackML/ISA-88 and OPC UA: design, write, convert, review and debug programs, with a stdlib-only structural validator.", "author": { "name": "SmailG", diff --git a/plugin.json b/plugin.json index b97a096..e4b35fd 100644 --- a/plugin.json +++ b/plugin.json @@ -1,7 +1,7 @@ { "name": "plc-programming", "displayName": "PLC Programming", - "version": "0.2.1", + "version": "0.2.2", "description": "PLC programming across IEC 61131-3 (ST, LD, FBD, SFC, IL), PLCopen XML / IEC 61131-10, Siemens TIA, Schneider, CODESYS, TwinCAT, ABB Freelance, Rockwell L5X/L5K, IEC 61499, AutomationML, DEXPI, PackML/ISA-88 and OPC UA with a stdlib-only structural validator.", "author": { "name": "SmailG", From 5b0cef573e3334dff6a50bddf17dfd7a16cf2177 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Smail=20Galija=C5=A1evi=C4=87?= <32690613+SmailG@users.noreply.github.com> Date: Tue, 10 Nov 2026 15:35:26 +0100 Subject: [PATCH 04/29] fix: keep the scope-frame owner out of dataclass equality A scope frame's owner is the frame itself, so the generated __eq__ recursed without end (RecursionError) when comparing two distinct scope frames. Nothing compares frames today; mark the field compare=False to remove the trap. No test: _Frame is internal and no current code path reaches the comparison. --- scripts/plccheck/st.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/plccheck/st.py b/scripts/plccheck/st.py index 12e0566..ee37a19 100644 --- a/scripts/plccheck/st.py +++ b/scripts/plccheck/st.py @@ -243,7 +243,7 @@ class _Frame: line: int scope: dict[str, int] | None = None # declared names, for scope openers body: bool = False # statements allowed at this level - owner: "_Frame | None" = field(default=None, repr=False) # nearest scope frame at or below this one + owner: "_Frame | None" = field(default=None, repr=False, compare=False) # nearest scope frame at or below this one def _is_zero(t: Tok) -> bool: From c5341e7c8fbc54f53a91187114c4364e9d4a1c97 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Smail=20Galija=C5=A1evi=C4=87?= <32690613+SmailG@users.noreply.github.com> Date: Tue, 10 Nov 2026 15:35:26 +0100 Subject: [PATCH 05/29] test: time scaling tests in CPU time and pin ST open-block sets Measure the XML text-join and ST nesting scaling tests with time.process_time(): both are single-thread CPU-bound, so process time drops scheduler noise on a shared runner. Best-of-3, floor and threshold are unchanged. Add two tests that pin set membership nobody tested: a timer called only inside CASE gives ST030, and EXIT inside REPEAT gives no ST005. They also pass on the previous st.py. --- tests/test_formats.py | 18 ++++++++++-------- tests/test_st.py | 6 ++++++ 2 files changed, 16 insertions(+), 8 deletions(-) diff --git a/tests/test_formats.py b/tests/test_formats.py index 069d76c..cb7bab8 100644 --- a/tests/test_formats.py +++ b/tests/test_formats.py @@ -581,7 +581,7 @@ def test_text_line_is_the_first_chunk_line(self): class XmlTextJoinScaling(unittest.TestCase): - """Text split into many chunks (one per entity reference) must parse in linear time.""" + """Text split into many chunks (one per entity reference) must parse in linear time (CPU time).""" RUNS = 3 SMALL, BIG = 40_000, 160_000 # BIG is 4x SMALL: linear work is about 4x, quadratic about 16x @@ -592,14 +592,15 @@ def best_of_runs(self, n: int) -> float: data = (f'' + "x<" * n + "").encode() best = float("inf") for _ in range(self.RUNS): - start = time.perf_counter() + start = time.process_time() parse_bytes(data) - best = min(best, time.perf_counter() - start) + best = min(best, time.process_time() - start) return best def test_text_join_time_grows_linearly_with_the_chunk_count(self): # A ratio, not an absolute bound, so a slow CI runner does not flake: both sizes slow - # down together. Best of three drops a run that a scheduling hiccup inflated, which would + # down together. Process (CPU) time, not wall time: both are single-thread CPU-bound, and it + # drops scheduler noise. Best of three drops a run that a hiccup inflated, which would # otherwise push the ratio up. Appending each chunk to node.text copies the text so far # each time (quadratic) and lands near 16x; one join per element lands near 4x. t_small, t_big = self.best_of_runs(self.SMALL), self.best_of_runs(self.BIG) @@ -608,7 +609,7 @@ def test_text_join_time_grows_linearly_with_the_chunk_count(self): class StNestingScaling(unittest.TestCase): - """ST lint time must grow linearly with the nesting depth: no rescan of the open blocks per token.""" + """ST lint CPU time must grow linearly with the nesting depth: no rescan of the open blocks per token.""" RUNS = 3 SMALL, BIG = 2_000, 8_000 # BIG is 4x SMALL: linear work is about 4x, quadratic about 16x @@ -628,14 +629,15 @@ class StNestingScaling(unittest.TestCase): def best_of_runs(self, src: str) -> float: best = float("inf") for _ in range(self.RUNS): - start = time.perf_counter() + start = time.process_time() lint_st(src) - best = min(best, time.perf_counter() - start) + best = min(best, time.process_time() - start) return best def test_lint_time_grows_linearly_with_the_nesting_depth(self): # A ratio, not an absolute bound, so a slow CI runner does not flake: both sizes slow - # down together. Best of three drops a run that a scheduling hiccup inflated, which would + # down together. Process (CPU) time, not wall time: both are single-thread CPU-bound, and it + # drops scheduler noise. Best of three drops a run that a hiccup inflated, which would # otherwise push the ratio up. Rescanning the whole stack for every token is quadratic and # lands near 16x; per-block counts and a scope pointer land near 4x. for name, make in self.INPUTS.items(): diff --git a/tests/test_st.py b/tests/test_st.py index 022cb0d..656be17 100644 --- a/tests/test_st.py +++ b/tests/test_st.py @@ -210,6 +210,12 @@ def test_timer_after_end_for_inside_if_is_conditional_but_not_in_loop(self): f = check_st_file(self.prog("IF x THEN FOR i := 1 TO 3 DO i := i; END_FOR; tonA(IN := x, PT := T#1S); END_IF;")) self.assertEqual(codes(f), ["ST030"]) + def test_timer_inside_case_is_conditional(self): + self.assertEqual(codes(check_st_file(self.prog("CASE i OF 1: tonA(IN := x, PT := T#1S); END_CASE;"))), ["ST030"]) + + def test_exit_inside_repeat_is_inside_a_loop(self): + self.assertEqual(codes(check_st_file("REPEAT EXIT; UNTIL x END_REPEAT;\n")), []) + def test_timer_after_closer_that_discards_an_open_if_is_unconditional(self): f = check_st_file(self.prog("FOR i := 1 TO 3 DO IF x THEN END_FOR;\ntonA(IN := x, PT := T#1S);")) self.assertEqual(codes(f), ["ST002"]) From e30a7cf71a9889bda9852247519e301ac8ff1eb5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Smail=20Galija=C5=A1evi=C4=87?= <32690613+SmailG@users.noreply.github.com> Date: Tue, 10 Nov 2026 15:46:19 +0100 Subject: [PATCH 06/29] fix: lint each Control Expert ST source once, for its nearest DFB An STSource inside nested FBSource elements was linted once per enclosing FBSource, so findings and memory multiplied with the nesting depth. --- scripts/plccheck/schneider.py | 18 ++++++- tests/test_linear_time.py | 94 +++++++++++++++++++++++++++++++++++ 2 files changed, 111 insertions(+), 1 deletion(-) create mode 100644 tests/test_linear_time.py diff --git a/scripts/plccheck/schneider.py b/scripts/plccheck/schneider.py index 2284da6..c01f6bd 100644 --- a/scripts/plccheck/schneider.py +++ b/scripts/plccheck/schneider.py @@ -10,6 +10,22 @@ ROOTS = {"FEFExchangeFile", "VariablesExchangeFile"} +def _own_st_sources(fb: Node): + """STSource elements of this DFB, in document order, not descending into a nested FBSource. + + A nested FBSource is visited by the caller's own loop. Iterating the whole subtree per + FBSource would lint an STSource once for every FBSource around it. + """ + stack = list(reversed(fb.children)) + while stack: + node = stack.pop() + if node.tag == "FBSource": + continue + if node.tag == "STSource": + yield node + stack.extend(reversed(node.children)) + + def check_control_expert(root: Node) -> list[Finding]: out: list[Finding] = [] if root.tag not in ROOTS: @@ -40,7 +56,7 @@ def check_control_expert(root: Node) -> list[Finding]: if name.upper() in seen: out.append(error("CX012", f"DFB parameter/variable '{name}' declared twice", var.line, ctx)) seen[name.upper()] = var.line - for st in fb.iter("STSource"): + for st in _own_st_sources(fb): f, _ = lint_st(st.text, "body") out.extend(with_context(f, f"{ctx} (ST)", (st.text_line or st.line) - 1)) diff --git a/tests/test_linear_time.py b/tests/test_linear_time.py new file mode 100644 index 0000000..b55f953 --- /dev/null +++ b/tests/test_linear_time.py @@ -0,0 +1,94 @@ +"""A crafted file must not make a checker spin or multiply its memory. + +Each scaling test feeds a checker an input whose size grows 4x and asserts the CPU time grows +about 4x, not 16x (a rescan per element). Each semantics test beside it pins what the linear +version must keep returning on legitimate input. +""" + +from __future__ import annotations + +import sys +import time +import unittest +from pathlib import Path + +HERE = Path(__file__).resolve().parent +sys.path.insert(0, str(HERE.parent / "scripts")) + +from plccheck.schneider import check_control_expert # noqa: E402 +from plccheck.xmlutil import parse_bytes # noqa: E402 + + +class LinearTime(unittest.TestCase): + """Base class: the ratio helper.""" + + RUNS = 3 + MAX_RATIO = 8 + TIMER_FLOOR = 0.01 # seconds; keeps the ratio meaningful if the small run is below timer noise + + def best_of_runs(self, run) -> float: + best = float("inf") + for _ in range(self.RUNS): + start = time.process_time() + run() + best = min(best, time.process_time() - start) + return best + + def assert_linear(self, make, small, big): + """`make(n)` builds the input outside the timer and returns the thunk to time. + + A ratio, not an absolute bound, so a slow CI runner does not flake: both sizes slow down + together. Process (CPU) time drops scheduler noise, and the best of three drops a run a + hiccup inflated. Linear work lands near big/small, a rescan per element near its square. + """ + run_small, run_big = make(small), make(big) + t_small, t_big = self.best_of_runs(run_small), self.best_of_runs(run_big) + self.assertLess(t_big, self.MAX_RATIO * max(t_small, self.TIMER_FLOOR), + f"size {small} took {t_small:.3f}s, size {big} took {t_big:.3f}s") + + +def xml_root(text: str): + return parse_bytes(text.encode()) + + +def xef(body: str) -> str: + return f"{body}" + + +class ControlExpertNestedDfb(LinearTime): + """An STSource is linted once, for the nearest FBSource around it, not once per FBSource around it.""" + + def nested(self, depth: int, st: str) -> str: + return xef('' * depth + f"{st}" + "" * depth) + + def test_lint_time_grows_linearly_with_depth_and_source_size_together(self): + # Depth and ST length grow together: relinting the ST once per enclosing FBSource is + # quadratic in the file size (about 16x for 4x), one lint per STSource is linear (about 4x). + def make(n): + root = xml_root(self.nested(n, "IF a THEN " * (10 * n))) + return lambda: check_control_expert(root) + self.assert_linear(make, 15, 60) + + def test_deeply_nested_dfb_reports_the_findings_of_a_single_lint(self): + st = "IF a THEN " + single = check_control_expert(xml_root(self.nested(1, st))) + self.assertEqual(len(single), 1, single) + self.assertEqual(check_control_expert(xml_root(self.nested(50, st))), single) + + def test_nested_st_source_belongs_to_the_nearest_dfb(self): + root = xml_root(xef( + 'IF a THEN ' + 'FOR i := 1 TO 2 DO ')) + got = sorted((f.code, f.context) for f in check_control_expert(root)) + self.assertEqual(got, [("ST002", "DFB Inner (ST)"), ("ST002", "DFB Outer (ST)")]) + + def test_sibling_dfbs_keep_their_own_context_and_line(self): + root = xml_root(xef( + '\nIF a THEN \n' + '\nFOR i := 1 TO 2 DO ')) + got = [(f.code, f.line, f.context) for f in check_control_expert(root)] + self.assertEqual(got, [("ST002", 2, "DFB A (ST)"), ("ST002", 4, "DFB B (ST)")]) + + +if __name__ == "__main__": + unittest.main() From 192aaa87f1802081d5802e9063ce174b47be0dfd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Smail=20Galija=C5=A1evi=C4=87?= <32690613+SmailG@users.noreply.github.com> Date: Tue, 10 Nov 2026 15:46:53 +0100 Subject: [PATCH 07/29] fix: collect IEC 61131-10 connections in one pass over the document Every BodyContent rescanned its whole subtree for ids and Connection elements, so nested BodyContent elements cost time quadratic in the depth. Each node now counts for its nearest enclosing BodyContent; files with non-nested bodies give identical findings. --- scripts/plccheck/plcopen.py | 29 ++++++++++++++++++--- tests/test_linear_time.py | 52 +++++++++++++++++++++++++++++++++++++ 2 files changed, 78 insertions(+), 3 deletions(-) diff --git a/scripts/plccheck/plcopen.py b/scripts/plccheck/plcopen.py index e1f9885..f35f741 100644 --- a/scripts/plccheck/plcopen.py +++ b/scripts/plccheck/plcopen.py @@ -242,15 +242,38 @@ def check_iec61131_10(root: Node) -> list[Finding]: return [error("PX101", f"root element is <{root.tag}>, expected ", root.line)] if root.get("schemaVersion") is None: out.append(warning("PX102", " has no schemaVersion", root.line)) - for bc in root.iter("BodyContent"): + for bc, outs, conns in _bodies_with_connections(root): kind = (bc.get("type") or "").rsplit(":", 1)[-1] st = bc.find("ST") if kind == "ST" and st is not None: f, _ = lint_st(st.text, "body") out.extend(with_context(f, "IEC 61131-10 ST body", (st.text_line or st.line) - 1)) - outs = {n.get("connectionPointOutId") for n in bc.iter() if n.get("connectionPointOutId")} - for conn in bc.iter("Connection"): + for conn in conns: ref = conn.get("refConnectionPointOutId") if ref and ref not in outs: out.append(warning("PX121", f"Connection refers to connectionPointOutId {ref}, not found in this body", conn.line)) return out + + +def _bodies_with_connections(root: Node) -> list[tuple[Node, set[str], list[Node]]]: + """Each BodyContent in document order, with its connectionPointOutId values and Connection elements. + + One walk over the document: a node counts for its nearest enclosing BodyContent. Scanning the + subtree per BodyContent is quadratic when BodyContent elements nest, which a valid file never + does. For such a file the scope of PX121 is the nearest body, not every body around it. + """ + bodies: list[tuple[Node, set[str], list[Node]]] = [] + stack: list[tuple[Node, tuple[Node, set[str], list[Node]] | None]] = [(root, None)] + while stack: + node, current = stack.pop() + if node.tag == "BodyContent": + current = (node, set(), []) + bodies.append(current) + if current is not None: + out_id = node.get("connectionPointOutId") + if out_id: + current[1].add(out_id) + if node.tag == "Connection": + current[2].append(node) + stack.extend((child, current) for child in reversed(node.children)) + return bodies diff --git a/tests/test_linear_time.py b/tests/test_linear_time.py index b55f953..67a21b4 100644 --- a/tests/test_linear_time.py +++ b/tests/test_linear_time.py @@ -15,6 +15,7 @@ HERE = Path(__file__).resolve().parent sys.path.insert(0, str(HERE.parent / "scripts")) +from plccheck.plcopen import check_iec61131_10 # noqa: E402 from plccheck.schneider import check_control_expert # noqa: E402 from plccheck.xmlutil import parse_bytes # noqa: E402 @@ -90,5 +91,56 @@ def test_sibling_dfbs_keep_their_own_context_and_line(self): self.assertEqual(got, [("ST002", 2, "DFB A (ST)"), ("ST002", 4, "DFB B (ST)")]) +class Iec61131_10NestedBodies(LinearTime): + """Connections and ids are collected in one pass, each for its nearest BodyContent.""" + + def project(self, body: str) -> str: + return f'{body}' + + def px121(self, body: str) -> list[tuple[str, int]]: + found = check_iec61131_10(xml_root(self.project(body))) + self.assertEqual({f.code for f in found}, {"PX121"} if found else set()) + return [(f.message.split()[4], f.line) for f in found] + + def test_check_time_grows_linearly_with_the_nesting_depth(self): + # Scanning the whole subtree of every BodyContent for ids and Connection elements is + # quadratic in the depth (about 16x for 4x); one walk is linear (about 4x). + def make(n): + root = xml_root(self.project('' * n + "" * n)) + return lambda: check_iec61131_10(root) + self.assert_linear(make, 500, 2000) + + def test_a_missing_ref_warns_and_a_resolving_one_does_not(self): + got = self.px121( + '\n\n' + '\n') + self.assertEqual(got, [("9,", 4)]) + + def test_ids_resolve_only_inside_their_own_body(self): + got = self.px121( + '\n' + '') + self.assertEqual(got, [("2,", 1), ("1,", 2)]) + + def test_the_body_element_itself_and_deeper_elements_supply_ids(self): + got = self.px121( + '' + '') + self.assertEqual(got, []) + + def test_elements_outside_any_body_are_ignored(self): + got = self.px121('' + '') + self.assertEqual(got, [("1,", 1)]) + + def test_nested_body_is_its_own_scope(self): + # Not a valid structure; pinned so the scope is a decision, not an accident of the walk. + got = self.px121( + '' + '' + '') + self.assertEqual(got, [("1,", 1), ("3,", 1), ("2,", 1)]) + + if __name__ == "__main__": unittest.main() From bf058ea4848bbec6044730ab8ec77473fe1eae41 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Smail=20Galija=C5=A1evi=C4=87?= <32690613+SmailG@users.noreply.github.com> Date: Tue, 10 Nov 2026 15:52:35 +0100 Subject: [PATCH 08/29] fix: strip TwinCAT comments in one pass instead of lazy regex retries Each opener that never closed rescanned the rest of the text, so a file of unclosed (*, /* or { openers took time quadratic in its size. Scanning with str.find stops at the first opener with no closer, and the output is byte-identical to the regexes. --- scripts/plccheck/twincat.py | 50 +++++++++++++++++++++++--- tests/test_linear_time.py | 70 +++++++++++++++++++++++++++++++++++++ 2 files changed, 116 insertions(+), 4 deletions(-) diff --git a/scripts/plccheck/twincat.py b/scripts/plccheck/twincat.py index cb09f60..28a4061 100644 --- a/scripts/plccheck/twincat.py +++ b/scripts/plccheck/twincat.py @@ -20,11 +20,53 @@ _FIRST_KW = re.compile(r"\b(PROGRAM|FUNCTION_BLOCK|FUNCTION|INTERFACE|METHOD|PROPERTY|TYPE|VAR_GLOBAL)\b", re.I) +def _blank_spans(text: str, opener: str, closer: str) -> str: + """Replace each non-nested `opener ... closer` span by one space. + + Single pass with str.find. The closer is searched after the opener's own characters, so + `(*)` is not a comment. An opener with no closer after it ends the scan: a later opener has + none either, and retrying from each one (as a lazy regex does) is quadratic. + """ + parts: list[str] = [] + pos = 0 + while True: + start = text.find(opener, pos) + if start < 0: + break + end = text.find(closer, start + len(opener)) + if end < 0: + break + parts.append(text[pos:start]) + parts.append(" ") + pos = end + len(closer) + parts.append(text[pos:]) + return "".join(parts) + + +def _blank_line_comments(text: str) -> str: + """Replace each `//` up to, not including, the end of its line by one space.""" + parts: list[str] = [] + pos = 0 + while True: + start = text.find("//", pos) + if start < 0: + break + end = text.find("\n", start + 2) + parts.append(text[pos:start]) + parts.append(" ") + if end < 0: + pos = len(text) + break + pos = end + parts.append(text[pos:]) + return "".join(parts) + + def _strip_comments(text: str) -> str: - text = re.sub(r"\(\*.*?\*\)", " ", text, flags=re.S) - text = re.sub(r"/\*.*?\*/", " ", text, flags=re.S) - text = re.sub(r"//[^\n]*", " ", text) - return re.sub(r"\{[^}]*\}", " ", text) + text = _blank_spans(text, "(*", "*)") + text = _blank_spans(text, "/*", "*/") + text = _blank_line_comments(text) + return _blank_spans(text, "{", "}") def check_twincat(root: Node, path: Path | None = None) -> list[Finding]: diff --git a/tests/test_linear_time.py b/tests/test_linear_time.py index 67a21b4..411fe6c 100644 --- a/tests/test_linear_time.py +++ b/tests/test_linear_time.py @@ -17,6 +17,7 @@ from plccheck.plcopen import check_iec61131_10 # noqa: E402 from plccheck.schneider import check_control_expert # noqa: E402 +from plccheck.twincat import _blank_spans, _strip_comments # noqa: E402 from plccheck.xmlutil import parse_bytes # noqa: E402 @@ -142,5 +143,74 @@ def test_nested_body_is_its_own_scope(self): self.assertEqual(got, [("1,", 1), ("3,", 1), ("2,", 1)]) +class TwinCatCommentStripping(LinearTime): + """Comment stripping scans once; an opener that never closes must not rescan the rest of the text.""" + + # (opener repeated, small, big). Every opener is unclosed, so a retry from each one walks to the + # end of the text. The pragma case needs a bigger text: its scan is a single character class. + INPUTS = { + "(* never closed": ("(* ", 1_500, 6_000), + "/* never closed": ("/* ", 1_500, 6_000), + "{ never closed": ("{ ", 5_000, 20_000), + } + + def test_strip_time_grows_linearly_with_the_unclosed_openers(self): + for name, (unit, small, big) in self.INPUTS.items(): + with self.subTest(name): + self.assert_linear(lambda n, unit=unit: (lambda text=unit * n: _strip_comments(text)), small, big) + + def test_an_opener_without_a_closer_ends_the_scan(self): + # The timing test cannot see a retry loop built on str.find (about 10x cheaper than a regex + # retry, so below timer noise at a test-sized input), so count the searches directly. + class CountingStr(str): + calls = 0 + + def find(self, *args): + CountingStr.calls += 1 + return super().find(*args) + + text = CountingStr("(* " * 1000) + self.assertEqual(_blank_spans(text, "(*", "*)"), text) + self.assertEqual(CountingStr.calls, 2) # one opener found, one failed closer search + CountingStr.calls = 0 + _blank_spans(CountingStr("(* a *) " * 1000), "(*", "*)") + self.assertLessEqual(CountingStr.calls, 2 * 1000 + 1) # an opener and a closer search per span, one last opener search + + def test_terminated_unterminated_and_adjacent_comments(self): + cases = { + "a (* c *) b": "a b", + "a (* c": "a (* c", + "(* x *)(* y *)": " ", + "a /* c */ b /* d": "a b /* d", + "/* x *//* y */z": " z", + "a (* x\ny *) b": "a b", + "{ pragma } a { open": " a { open", + "{a\n}b": " b", + "x // c\ny": "x \ny", + "x // c": "x ", + "//\n//\n": " \n \n", + } + for text, want in cases.items(): + with self.subTest(text=text): + self.assertEqual(_strip_comments(text), want) + + def test_an_opener_is_not_closed_by_its_own_characters(self): + for text in ("(*)", "a (*) b", "/*/", "a /*/ b"): + with self.subTest(text=text): + self.assertEqual(_strip_comments(text), text) + self.assertEqual(_strip_comments("(*)*)"), " ") + self.assertEqual(_strip_comments("/*/*/"), " ") + + def test_openers_do_not_nest_and_passes_run_in_order(self): + self.assertEqual(_strip_comments("(* (* *) *)"), " *)") + self.assertEqual(_strip_comments("(* // *)\nx"), " \nx") # the (* *) pass runs before // + self.assertEqual(_strip_comments("(* /* *) */"), " */") # (* *) first: it takes the /* inside + self.assertEqual(_strip_comments("/* (* */ *)"), "/* ") # then /* */ finds nothing left to pair with + self.assertEqual(_strip_comments("// (* \n *)"), " ") # so the (* *) span crosses the line end + self.assertEqual(_strip_comments("// x (* y\nz"), " \nz") # an unclosed (* is left for the // pass + self.assertEqual(_strip_comments("{ // }\nz"), "{ \nz") # { } runs last: // already took the } + self.assertEqual(_strip_comments("/* { */ }"), " }") + + if __name__ == "__main__": unittest.main() From b3b4f12bfe4a6d8168c1d826baa2a50f1d3283ac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Smail=20Galija=C5=A1evi=C4=87?= <32690613+SmailG@users.noreply.github.com> Date: Tue, 10 Nov 2026 15:53:52 +0100 Subject: [PATCH 09/29] fix: find the open L5K block for a closer without scanning the stack A closer matching no open block walked the whole stack of open blocks, so a file of many open blocks and unmatched closers took quadratic time. A count of open frames per keyword, as in the ST linter, skips the scan. --- scripts/plccheck/logix.py | 9 +++++++-- tests/test_linear_time.py | 41 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 2 deletions(-) diff --git a/scripts/plccheck/logix.py b/scripts/plccheck/logix.py index 6e31422..796cbe5 100644 --- a/scripts/plccheck/logix.py +++ b/scripts/plccheck/logix.py @@ -3,6 +3,7 @@ from __future__ import annotations import re +from collections import Counter from .findings import Finding, error, warning, with_context from .st import lint_st @@ -147,6 +148,7 @@ def check_l5k(text: str) -> list[Finding]: rungs are `N: neutral text;`; ST routine lines start with a single quote.""" out: list[Finding] = [] stack: list[tuple[str, int]] = [] + open_words: Counter[str] = Counter() # open frames per keyword, so "is X open" is not a stack scan ends = set(re.findall(r"\bEND_([A-Z][A-Z0-9_]*)\b", text)) st_lines: list[tuple[int, str]] = [] for lineno, raw in enumerate(text.splitlines(), 1): @@ -165,14 +167,17 @@ def check_l5k(text: str) -> list[Finding]: if not end: if word in L5K_BLOCKS or word in ends: stack.append((word, lineno)) + open_words[word] += 1 continue - pos = next((k for k in range(len(stack) - 1, -1, -1) if stack[k][0] == word), None) - if pos is None: + if not open_words[word]: # else the scan would walk the whole stack to find nothing out.append(error("LX050", f"END_{word} without an open {word}", lineno)) continue + pos = next(k for k in range(len(stack) - 1, -1, -1) if stack[k][0] == word) for kw, ln in stack[pos + 1:]: out.append(error("LX050", f"{kw} opened at line {ln} is not closed before END_{word}", ln)) closed = stack[pos] + for kw, _ in stack[pos:]: + open_words[kw] -= 1 del stack[pos:] if closed[0] == "ST_ROUTINE" and st_lines: src = "\n" * (st_lines[0][0] - 1) + "\n".join(t for _, t in st_lines) diff --git a/tests/test_linear_time.py b/tests/test_linear_time.py index 411fe6c..fbb7180 100644 --- a/tests/test_linear_time.py +++ b/tests/test_linear_time.py @@ -15,6 +15,7 @@ HERE = Path(__file__).resolve().parent sys.path.insert(0, str(HERE.parent / "scripts")) +from plccheck.logix import check_l5k # noqa: E402 from plccheck.plcopen import check_iec61131_10 # noqa: E402 from plccheck.schneider import check_control_expert # noqa: E402 from plccheck.twincat import _blank_spans, _strip_comments # noqa: E402 @@ -212,5 +213,45 @@ def test_openers_do_not_nest_and_passes_run_in_order(self): self.assertEqual(_strip_comments("/* { */ }"), " }") +class LogixL5kBlockStack(LinearTime): + """A closer that matches no open block is found without scanning the stack.""" + + def errors(self, text: str) -> list[tuple[int | None, str]]: + found = check_l5k(text) + self.assertEqual({f.code for f in found}, {"LX050"} if found else set()) + return [(f.line, f.message) for f in found] + + def test_check_time_grows_linearly_with_the_open_blocks(self): + # Every END_ZZ matches nothing, so each one used to walk all the open PROGRAM frames: + # quadratic in the depth (about 16x for 4x), a count lookup is linear (about 4x). + def make(n): + text = "PROGRAM " * n + "END_ZZ " * n + return lambda: check_l5k(text) + self.assert_linear(make, 1_000, 4_000) + + def test_a_closer_matching_nothing_reports_its_error(self): + self.assertEqual(self.errors("END_ROUTINE"), [(1, "END_ROUTINE without an open ROUTINE")]) + self.assertEqual(self.errors("PROGRAM p\nEND_ROUTINE\nEND_PROGRAM"), [(2, "END_ROUTINE without an open ROUTINE")]) + + def test_a_matched_closer_closes_its_block(self): + self.assertEqual(self.errors("PROGRAM p\nROUTINE r\nEND_ROUTINE\nEND_PROGRAM"), []) + self.assertEqual(self.errors("PROGRAM p\nEND_PROGRAM\nEND_PROGRAM"), [(3, "END_PROGRAM without an open PROGRAM")]) + self.assertEqual(self.errors("PROGRAM a PROGRAM b END_PROGRAM END_PROGRAM END_PROGRAM"), + [(1, "END_PROGRAM without an open PROGRAM")]) + + def test_a_closer_closes_the_blocks_inside_its_own_and_none_more(self): + got = self.errors("PROGRAM p\nROUTINE r\nTAG t\nEND_PROGRAM\nEND_ROUTINE\nEND_TAG\nEND_PROGRAM") + self.assertEqual(got, [ + (2, "ROUTINE opened at line 2 is not closed before END_PROGRAM"), + (3, "TAG opened at line 3 is not closed before END_PROGRAM"), + (5, "END_ROUTINE without an open ROUTINE"), + (6, "END_TAG without an open TAG"), + (7, "END_PROGRAM without an open PROGRAM"), + ]) + + def test_blocks_left_open_are_reported_at_the_end(self): + self.assertEqual(self.errors("PROGRAM p\nROUTINE r\nEND_ROUTINE"), [(1, "PROGRAM opened here is never closed")]) + + if __name__ == "__main__": unittest.main() From da5aabcf3a10aea7f8609a81b03628b5c3c6a31d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Smail=20Galija=C5=A1evi=C4=87?= <32690613+SmailG@users.noreply.github.com> Date: Tue, 10 Nov 2026 15:54:28 +0100 Subject: [PATCH 10/29] fix: find the open SIMATIC SD block for a closer without scanning the stack check_simatic_sd had the same per-closer stack scan as the L5K checker: a closer matching no open NETWORK or RUNG walked the whole stack. Count the open frames per keyword instead. --- scripts/plccheck/simaticml.py | 9 +++++++-- tests/test_linear_time.py | 38 +++++++++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 2 deletions(-) diff --git a/scripts/plccheck/simaticml.py b/scripts/plccheck/simaticml.py index 8d95c97..728ede2 100644 --- a/scripts/plccheck/simaticml.py +++ b/scripts/plccheck/simaticml.py @@ -8,6 +8,7 @@ from __future__ import annotations import re +from collections import Counter from .findings import Finding, error, warning from .xmlutil import Node @@ -179,6 +180,7 @@ def check_simatic_sd(text: str) -> list[Finding]: """SIMATIC SD (.s7dcl): only NETWORK/RUNG nesting and brace balance are checked.""" out: list[Finding] = [] stack: list[tuple[str, int]] = [] + open_words: Counter[str] = Counter() # open frames per keyword, so "is X open" is not a stack scan depth = 0 for lineno, raw in enumerate(text.splitlines(), 1): line = re.sub(r"//.*$", "", raw) @@ -187,14 +189,17 @@ def check_simatic_sd(text: str) -> list[Finding]: for word in re.findall(r"\b(END_NETWORK|END_RUNG|NETWORK|RUNG)\b", line): if word in ("NETWORK", "RUNG"): stack.append((word, lineno)) + open_words[word] += 1 continue opener = word[4:] - pos = next((k for k in range(len(stack) - 1, -1, -1) if stack[k][0] == opener), None) - if pos is None: + if not open_words[opener]: # else the scan would walk the whole stack to find nothing out.append(error("SD001", f"{word} without matching {opener}", lineno)) continue + pos = next(k for k in range(len(stack) - 1, -1, -1) if stack[k][0] == opener) for inner, ln in stack[pos + 1:]: out.append(error("SD001", f"{inner} opened at line {ln} is not closed before {word}", ln)) + for kw, _ in stack[pos:]: + open_words[kw] -= 1 del stack[pos:] for word, lineno in stack: out.append(error("SD001", f"{word} opened here is never closed", lineno)) diff --git a/tests/test_linear_time.py b/tests/test_linear_time.py index fbb7180..ee12c09 100644 --- a/tests/test_linear_time.py +++ b/tests/test_linear_time.py @@ -18,6 +18,7 @@ from plccheck.logix import check_l5k # noqa: E402 from plccheck.plcopen import check_iec61131_10 # noqa: E402 from plccheck.schneider import check_control_expert # noqa: E402 +from plccheck.simaticml import check_simatic_sd # noqa: E402 from plccheck.twincat import _blank_spans, _strip_comments # noqa: E402 from plccheck.xmlutil import parse_bytes # noqa: E402 @@ -253,5 +254,42 @@ def test_blocks_left_open_are_reported_at_the_end(self): self.assertEqual(self.errors("PROGRAM p\nROUTINE r\nEND_ROUTINE"), [(1, "PROGRAM opened here is never closed")]) +class SimaticSdBlockStack(LinearTime): + """SIMATIC SD: a closer that matches no open NETWORK/RUNG is found without scanning the stack.""" + + def errors(self, text: str) -> list[tuple[int | None, str]]: + found = check_simatic_sd(text) + self.assertEqual({f.code for f in found}, {"SD001"} if found else set()) + return [(f.line, f.message) for f in found] + + def test_check_time_grows_linearly_with_the_open_blocks(self): + # Every END_RUNG matches nothing, so each one used to walk all the open NETWORK frames: + # quadratic in the depth (about 16x for 4x), a count lookup is linear (about 4x). + def make(n): + text = "NETWORK " * n + "END_RUNG " * n + return lambda: check_simatic_sd(text) + self.assert_linear(make, 1_000, 4_000) + + def test_a_closer_matching_nothing_reports_its_error(self): + self.assertEqual(self.errors("END_RUNG"), [(1, "END_RUNG without matching RUNG")]) + self.assertEqual(self.errors("NETWORK\nEND_RUNG\nEND_NETWORK"), [(2, "END_RUNG without matching RUNG")]) + + def test_a_matched_closer_closes_its_block(self): + self.assertEqual(self.errors("NETWORK\nRUNG\nEND_RUNG\nEND_NETWORK"), []) + self.assertEqual(self.errors("NETWORK\nEND_NETWORK\nEND_NETWORK"), [(3, "END_NETWORK without matching NETWORK")]) + self.assertEqual(self.errors("RUNG RUNG END_RUNG END_RUNG END_RUNG"), [(1, "END_RUNG without matching RUNG")]) + + def test_a_closer_closes_the_blocks_inside_its_own_and_none_more(self): + got = self.errors("NETWORK\nRUNG\nEND_NETWORK\nEND_RUNG\nEND_NETWORK") + self.assertEqual(got, [ + (2, "RUNG opened at line 2 is not closed before END_NETWORK"), + (4, "END_RUNG without matching RUNG"), + (5, "END_NETWORK without matching NETWORK"), + ]) + + def test_blocks_left_open_are_reported_at_the_end(self): + self.assertEqual(self.errors("NETWORK\nRUNG\nEND_RUNG"), [(1, "NETWORK opened here is never closed")]) + + if __name__ == "__main__": unittest.main() From 0e53cfd90bcc8842a200f6676ed829361c85171a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Smail=20Galija=C5=A1evi=C4=87?= <32690613+SmailG@users.noreply.github.com> Date: Tue, 10 Nov 2026 15:56:15 +0100 Subject: [PATCH 11/29] fix: lint L5K ST routines from their own line instead of newline padding Each ST routine was padded with one newline per file line before it, so a file with many ST routines cost time quadratic in its size. lint_st now takes the file line of the text's first line; findings are unchanged, message text included. --- scripts/plccheck/logix.py | 3 +-- scripts/plccheck/st.py | 10 ++++++---- tests/test_linear_time.py | 28 ++++++++++++++++++++++++++++ 3 files changed, 35 insertions(+), 6 deletions(-) diff --git a/scripts/plccheck/logix.py b/scripts/plccheck/logix.py index 796cbe5..b80641b 100644 --- a/scripts/plccheck/logix.py +++ b/scripts/plccheck/logix.py @@ -180,8 +180,7 @@ def check_l5k(text: str) -> list[Finding]: open_words[kw] -= 1 del stack[pos:] if closed[0] == "ST_ROUTINE" and st_lines: - src = "\n" * (st_lines[0][0] - 1) + "\n".join(t for _, t in st_lines) - f, _ = lint_st(src, "body") + f, _ = lint_st("\n".join(t for _, t in st_lines), "body", st_lines[0][0]) out.extend(with_context(f, "L5K ST routine")) st_lines = [] for kw, ln in stack: diff --git a/scripts/plccheck/st.py b/scripts/plccheck/st.py index ee37a19..ea4e6fa 100644 --- a/scripts/plccheck/st.py +++ b/scripts/plccheck/st.py @@ -110,10 +110,10 @@ def shifted(self, offset: int) -> "StAnalysis": ) -def tokenize(src: str) -> tuple[list[Tok], list[Finding]]: +def tokenize(src: str, first_line: int = 1) -> tuple[list[Tok], list[Finding]]: toks: list[Tok] = [] findings: list[Finding] = [] - i, n, line = 0, len(src), 1 + i, n, line = 0, len(src), first_line while i < n: c = src[i] if c == "\n": @@ -315,8 +315,10 @@ def _designator_then_eq(stmt: list[Tok]) -> bool: return i < len(stmt) and _is_op(stmt[i], "=") -def lint_st(src: str, mode: str = "file") -> tuple[list[Finding], StAnalysis]: - toks, findings = tokenize(src) +def lint_st(src: str, mode: str = "file", first_line: int = 1) -> tuple[list[Finding], StAnalysis]: + """Lint ST text. `first_line` is the file line of its first line, for text taken from the + middle of a file: every line number in a finding, message text included, then names a file line.""" + toks, findings = tokenize(src, first_line) an = StAnalysis() root = _Frame("", 0, scope={}, body=(mode != "declaration")) root.owner = root diff --git a/tests/test_linear_time.py b/tests/test_linear_time.py index ee12c09..e57e35c 100644 --- a/tests/test_linear_time.py +++ b/tests/test_linear_time.py @@ -19,6 +19,7 @@ from plccheck.plcopen import check_iec61131_10 # noqa: E402 from plccheck.schneider import check_control_expert # noqa: E402 from plccheck.simaticml import check_simatic_sd # noqa: E402 +from plccheck.st import lint_st # noqa: E402 from plccheck.twincat import _blank_spans, _strip_comments # noqa: E402 from plccheck.xmlutil import parse_bytes # noqa: E402 @@ -254,6 +255,33 @@ def test_blocks_left_open_are_reported_at_the_end(self): self.assertEqual(self.errors("PROGRAM p\nROUTINE r\nEND_ROUTINE"), [(1, "PROGRAM opened here is never closed")]) +class LogixL5kStRoutineLines(LinearTime): + """An ST routine is linted from its own line, not behind a padding of one newline per file line before it.""" + + def test_lint_time_grows_linearly_with_the_number_of_routines(self): + # Routine k used to be padded with about 3k newlines, so n routines cost n^2 characters: + # quadratic in the file size (about 16x for 4x). + def make(n): + text = "ST_ROUTINE r\n'a := 1;\nEND_ST_ROUTINE\n" * n + return lambda: check_l5k(text) + self.assert_linear(make, 700, 2_800) + + def test_findings_name_file_lines_in_their_position_and_their_message(self): + text = "PROGRAM p\nROUTINE r\nEND_ROUTINE\nST_ROUTINE s\n'IF a THEN\n'x := 1;\nEND_ST_ROUTINE\n" \ + "ST_ROUTINE t\n'(* open\nEND_ST_ROUTINE\nEND_PROGRAM" + got = [(f.code, f.line, f.message, f.context) for f in check_l5k(text)] + self.assertEqual(got, [ + ("ST002", 5, "IF opened at line 5 is never closed", "L5K ST routine"), + ("ST001", 9, "unterminated comment opened with '(*'", "L5K ST routine"), + ]) + + def test_lint_st_numbers_lines_from_first_line(self): + findings, _ = lint_st("x := 1;\nIF a THEN", "body", 10) + self.assertEqual([(f.line, f.message) for f in findings], [(11, "IF opened at line 11 is never closed")]) + findings, _ = lint_st("x := 1;\nIF a THEN", "body") + self.assertEqual([(f.line, f.message) for f in findings], [(2, "IF opened at line 2 is never closed")]) + + class SimaticSdBlockStack(LinearTime): """SIMATIC SD: a closer that matches no open NETWORK/RUNG is found without scanning the stack.""" From d1a21afa7c9220ea43d4b517d0b8486458f88bc3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Smail=20Galija=C5=A1evi=C4=87?= <32690613+SmailG@users.noreply.github.com> Date: Tue, 10 Nov 2026 15:57:19 +0100 Subject: [PATCH 12/29] fix: check each L5X rung for its own routine, not every routine around it Every RLL routine iterated its whole subtree for rungs, so rungs under nested Routines elements were checked once per enclosing routine: time and findings grew with the nesting depth. A nested Routines element is now its own scope; files without nested Routines give identical findings. --- scripts/plccheck/logix.py | 17 ++++++++++- tests/test_linear_time.py | 60 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 75 insertions(+), 2 deletions(-) diff --git a/scripts/plccheck/logix.py b/scripts/plccheck/logix.py index b80641b..038b392 100644 --- a/scripts/plccheck/logix.py +++ b/scripts/plccheck/logix.py @@ -84,6 +84,21 @@ def check_rung(text: str, line: int, ctx: str) -> tuple[list[Finding], list[str] return out, otes +def _own_rungs(routine: Node): + """Rung elements of this routine in document order, not descending into a nested . + + A nested is visited by the caller as a scope of its own. Iterating the whole subtree + per routine would check a nested rung once for every routine around it. + """ + stack = list(reversed(routine.children)) + while stack: + node = stack.pop() + if node.tag == "Rung": + yield node + if node.tag != "Routines": + stack.extend(reversed(node.children)) + + def check_l5x(root: Node) -> list[Finding]: out: list[Finding] = [] if root.tag != "RSLogix5000Content": @@ -116,7 +131,7 @@ def check_l5x(root: Node) -> list[Finding]: if rtype not in ROUTINE_TYPES: out.append(warning("LX010", f"unknown routine Type '{rtype}'", r.line, ctx)) if rtype == "RLL": - for rung in r.iter("Rung"): + for rung in _own_rungs(r): text_node = rung.find("Text") if text_node is None: continue diff --git a/tests/test_linear_time.py b/tests/test_linear_time.py index e57e35c..5d441de 100644 --- a/tests/test_linear_time.py +++ b/tests/test_linear_time.py @@ -15,7 +15,7 @@ HERE = Path(__file__).resolve().parent sys.path.insert(0, str(HERE.parent / "scripts")) -from plccheck.logix import check_l5k # noqa: E402 +from plccheck.logix import check_l5k, check_l5x # noqa: E402 from plccheck.plcopen import check_iec61131_10 # noqa: E402 from plccheck.schneider import check_control_expert # noqa: E402 from plccheck.simaticml import check_simatic_sd # noqa: E402 @@ -319,5 +319,63 @@ def test_blocks_left_open_are_reported_at_the_end(self): self.assertEqual(self.errors("NETWORK\nRUNG\nEND_RUNG"), [(1, "NETWORK opened here is never closed")]) +class LogixL5xNestedRoutines(LinearTime): + """A rung is checked for the routine it is in, not once for every routine around it.""" + + def l5x(self, programs: str) -> str: + return ('' + f'{programs}') + + def routine(self, name: str, rungs: str, inner: str = "") -> str: + return f'{rungs}{inner}' + + def rung(self, number: int, text: str) -> str: + return f'{text}' + + def nested(self, depth: int, text: str) -> str: + """`depth` routines, each in the of the one before and each with one rung.""" + out = "" + for _ in range(depth): + out = self.routine("r", self.rung(0, text), f"{out}" if out else "") + return self.l5x(f'{out}') + + def check(self, programs: str) -> list[tuple[str, int, str | None]]: + return [(f.code, f.line, f.context) for f in check_l5x(xml_root(self.l5x(programs)))] + + def test_check_time_grows_linearly_with_the_nesting_depth(self): + # Rescanning the whole subtree of every routine for rungs is quadratic in the depth + # (about 16x for 4x); one visit per rung is linear (about 4x). + def make(n): + root = xml_root(self.nested(n, "XIC(a);")) + return lambda: check_l5x(root) + self.assert_linear(make, 150, 600) + + def test_deeply_nested_routines_report_each_rung_once(self): + found = check_l5x(xml_root(self.nested(50, "XIC(a)"))) # no ';': one LX020 per rung + self.assertEqual([f.code for f in found], ["LX020"] * 50) + + def test_sibling_routines_keep_their_own_rungs_and_context(self): + got = self.check('\n' + self.routine("a", self.rung(0, "XIC(a)")) + "\n" + + self.routine("b", self.rung(0, "XIC(b)") + self.rung(1, "OTE(c)")) + "") + self.assertEqual(got, [("LX020", 2, "p/a rung 0"), ("LX020", 3, "p/b rung 0"), ("LX020", 3, "p/b rung 1")]) + + def test_coils_are_counted_per_routines_scope(self): + got = self.check('' + self.routine("a", self.rung(0, "OTE(x);") + self.rung(1, "OTE(x);")) + + self.routine("b", self.rung(0, "OTE(x);")) + "") + self.assertEqual(got, [("LX040", 1, "p")]) + + def test_a_routine_nested_without_a_routines_wrapper_keeps_its_rungs_with_the_routine_around_it(self): + # Only a is visited as a scope of its own; a bare inside a routine is not. + got = self.check('' + self.routine("a", self.rung(0, "XIC(a);") + + self.routine("bare", self.rung(1, "XIC(b)"))) + "") + self.assertEqual(got, [("LX020", 1, "p/a rung 1")]) + + def test_rungs_of_a_nested_routines_scope_belong_to_it(self): + # Not a valid structure; pinned so the scope is a decision, not an accident of the walk. + got = self.check('' + self.routine("a", self.rung(0, "XIC(a)"), + "" + self.routine("in", self.rung(0, "XIC(b)")) + "") + "") + self.assertEqual(got, [("LX020", 1, "p/a rung 0"), ("LX020", 1, "a/in rung 0")]) + + if __name__ == "__main__": unittest.main() From 5f36e11959af9c6f2ca8821bdfbe4987943012a2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Smail=20Galija=C5=A1evi=C4=87?= <32690613+SmailG@users.noreply.github.com> Date: Tue, 10 Nov 2026 15:58:44 +0100 Subject: [PATCH 13/29] fix: check each SimaticML network once, for its own CompileUnit Every CompileUnit collected the network payloads of its whole subtree, so payloads under nested CompileUnit elements were checked once per enclosing unit: time and findings grew with the nesting depth. A nested CompileUnit is now its own scope; files without nested units give identical findings. --- scripts/plccheck/simaticml.py | 31 +++++++++++++----- tests/test_linear_time.py | 61 ++++++++++++++++++++++++++++++++++- 2 files changed, 83 insertions(+), 9 deletions(-) diff --git a/scripts/plccheck/simaticml.py b/scripts/plccheck/simaticml.py index 728ede2..db73294 100644 --- a/scripts/plccheck/simaticml.py +++ b/scripts/plccheck/simaticml.py @@ -101,7 +101,7 @@ def _check_block(block: Node) -> list[Finding]: iface = attrs.find("Interface") if iface is not None: out.extend(_check_interface(block.tag, iface, ctx)) - for cu in block.iter("SW.Blocks.CompileUnit"): + for cu in block.iter(COMPILE_UNIT): out.extend(_check_compile_unit(cu, ctx)) return out @@ -141,22 +141,37 @@ def _check_interface(kind: str, iface: Node, ctx: str) -> list[Finding]: PAYLOAD_ROOTS = ("FlgNet", "StructuredText", "StatementList", "Graph") +COMPILE_UNIT = "SW.Blocks.CompileUnit" + + +def _own_scopes(cu: Node) -> list[Node]: + """Payload roots in this CompileUnit, in document order, not inside a nested CompileUnit. + + A nested CompileUnit is visited by the caller as a unit of its own. Iterating the whole subtree + per unit would lint a payload once for every CompileUnit around it. + """ + scopes: list[Node] = [] + stack = list(reversed(cu.children)) + while stack: + node = stack.pop() + if node.tag == COMPILE_UNIT: + continue + if node.tag in PAYLOAD_ROOTS: + scopes.append(node) + stack.extend(reversed(node.children)) + return scopes def _check_compile_unit(cu: Node, ctx: str) -> list[Finding]: """UIds are unique per network payload; GRAPH nests one FlgNet per condition, each its own scope.""" out: list[Finding] = [] - scopes: dict[int, Node] = {} - for node in cu.iter(): - if node.tag in PAYLOAD_ROOTS: - scopes[id(node)] = node - for scope in scopes.values(): + for scope in _own_scopes(cu): nodes = [] stack = list(scope.children) - while stack: # stop at nested payload roots: they are separate scopes + while stack: # stop at nested payload roots and compile units: they are separate scopes n = stack.pop() nodes.append(n) - if n.tag not in PAYLOAD_ROOTS: + if n.tag not in PAYLOAD_ROOTS and n.tag != COMPILE_UNIT: stack.extend(n.children) uids: dict[str, int] = {} wires: set[str] = set() diff --git a/tests/test_linear_time.py b/tests/test_linear_time.py index 5d441de..1b70729 100644 --- a/tests/test_linear_time.py +++ b/tests/test_linear_time.py @@ -18,7 +18,7 @@ from plccheck.logix import check_l5k, check_l5x # noqa: E402 from plccheck.plcopen import check_iec61131_10 # noqa: E402 from plccheck.schneider import check_control_expert # noqa: E402 -from plccheck.simaticml import check_simatic_sd # noqa: E402 +from plccheck.simaticml import check_simaticml, check_simatic_sd # noqa: E402 from plccheck.st import lint_st # noqa: E402 from plccheck.twincat import _blank_spans, _strip_comments # noqa: E402 from plccheck.xmlutil import parse_bytes # noqa: E402 @@ -377,5 +377,64 @@ def test_rungs_of_a_nested_routines_scope_belong_to_it(self): self.assertEqual(got, [("LX020", 1, "p/a rung 0"), ("LX020", 1, "a/in rung 0")]) +class SimaticMlNestedCompileUnits(LinearTime): + """A network payload is checked for the CompileUnit it is in, not once for every CompileUnit around it.""" + + UNIT = "SW.Blocks.CompileUnit" + PART = '' + DUP = '' # one SM020 + + def unit(self, *payloads: str) -> str: + """A CompileUnit with each payload under its NetworkSource, as TIA writes it.""" + return f"<{self.UNIT}>" + "".join(f"{p}" for p in payloads) + f"" + + def document(self, units: str) -> str: + return ('B' + f"{units}") + + def nested(self, depth: int, payload: str) -> str: + return self.document(f"<{self.UNIT}>{payload}" * depth + f"" * depth) + + def check(self, units: str) -> list[tuple[str, int, str | None]]: + return [(f.code, f.line, f.context) for f in check_simaticml(xml_root(self.document(units)))] + + def test_check_time_grows_linearly_with_the_nesting_depth(self): + # Collecting the payloads of the whole subtree for every CompileUnit is quadratic in the + # depth (about 16x for 4x); one visit per payload is linear (about 4x). + def make(n): + root = xml_root(self.nested(n, self.PART)) + return lambda: check_simaticml(root) + self.assert_linear(make, 150, 600) + + def test_deeply_nested_compile_units_report_each_network_once(self): + found = check_simaticml(xml_root(self.nested(50, self.DUP))) + self.assertEqual([f.code for f in found], ["SM020"] * 50) + + def test_sibling_compile_units_are_separate_scopes(self): + self.assertEqual(self.check(self.unit(self.PART) + self.unit(self.PART)), []) + self.assertEqual(self.check(f"\n{self.unit(self.DUP)}\n{self.unit(self.PART)}"), [("SM020", 2, "SW.Blocks.FB B")]) + + def test_networks_of_one_compile_unit_are_reported_in_document_order(self): + dup = '' + got = self.check(f"<{self.UNIT}>\n{dup}\n{dup}") + self.assertEqual([line for _, line, _ in got], [2, 3]) + + def test_unresolved_references_and_nested_graph_payloads_keep_their_scopes(self): + graph = ('' + '') + self.assertEqual(self.check(self.unit(graph)), [("SM021", 1, "SW.Blocks.FB B")]) + + def test_payloads_of_a_nested_compile_unit_belong_to_it(self): + # Not a valid structure; pinned so the scope is a decision, not an accident of the walk. The + # outer unit's payload reuses UId 1 with the inner one's and neither sees the other. + inner = self.unit(self.DUP) + self.assertEqual(self.check(f"<{self.UNIT}>{self.PART}{inner}"), + [("SM020", 1, "SW.Blocks.FB B")]) + + def test_a_compile_unit_inside_a_payload_does_not_lend_its_parts_to_it(self): + part_in_unit = f'<{self.UNIT}>' + self.assertEqual(self.check(self.unit(f"{part_in_unit}")), []) + + if __name__ == "__main__": unittest.main() From 325d87e8ca14e2ec15313aeaf41e6a99b69d1616 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Smail=20Galija=C5=A1evi=C4=87?= <32690613+SmailG@users.noreply.github.com> Date: Tue, 10 Nov 2026 16:00:36 +0100 Subject: [PATCH 14/29] fix: parse a file linked from many AutomationML interfaces once Each PLCopenXMLInterface link parsed the target file and walked it again, so n links to one file (including the AutomationML file itself) cost n parses. The globalId and uuid values are now collected once per file, keyed by the resolved path; a malformed file is cached as such. --- scripts/plccheck/aml.py | 25 +++++++++++--- tests/test_linear_time.py | 73 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 93 insertions(+), 5 deletions(-) diff --git a/scripts/plccheck/aml.py b/scripts/plccheck/aml.py index c3f6ecc..4a53d2c 100644 --- a/scripts/plccheck/aml.py +++ b/scripts/plccheck/aml.py @@ -21,6 +21,23 @@ def _resolve(ref: str, base: Path) -> tuple[Path | None, str]: return (base / p) if p else None, u.fragment +def _global_ids(target: Path, cache: dict[Path, set[str] | None]) -> set[str] | None: + """The globalId and uuid values of a referenced PLCopen file, or None if it is not well-formed XML. + + Parsed once per file: every interface that links to the same file would otherwise parse and walk + it again, so a file of n links to itself costs n parses of an n-link document. + """ + key = target.resolve() # `x.xml`, `./x.xml` and `a/../x.xml` are one file + if key not in cache: + try: + doc = parse_file(target) + except XmlError: + cache[key] = None + else: + cache[key] = {v for n in doc.iter() for v in (n.get("globalId"), n.get("uuid")) if v is not None} + return cache[key] + + def check_aml(root: Node, path: Path | None = None) -> list[Finding]: out: list[Finding] = [] if root.tag != "CAEXFile": @@ -56,6 +73,7 @@ def check_aml(root: Node, path: Path | None = None) -> list[Finding]: out.append(warning("AM005", f"InternalLink {side} '{ref}' matches no interface ID in this file", link.line)) base = path.parent if path is not None else None + plcopen_ids: dict[Path, set[str] | None] = {} for iface in root.iter("ExternalInterface"): cls = iface.get("RefBaseClassPath") or "" if not any(cls.endswith(c) for c in DATA_CONNECTORS): @@ -74,11 +92,8 @@ def check_aml(root: Node, path: Path | None = None) -> list[Finding]: out.append(warning("AM011", f"refURI target {target.name} not found next to this file", value.line)) elif fragment and cls.endswith("PLCopenXMLInterface") and target.is_file(): # An absolute refURI can name a directory, FIFO or device; only a regular file is read. - try: - doc = parse_file(target) - except XmlError: - continue - if not any(n.get("globalId") == fragment or n.get("uuid") == fragment for n in doc.iter()): + ids = _global_ids(target, plcopen_ids) + if ids is not None and fragment not in ids: out.append(warning("AM012", f"refURI fragment #{fragment} matches no globalId in {target.name}", value.line)) for ext in root.findall("ExternalReference"): if base is not None and ext.get("Path") and not (base / ext.get("Path")).exists(): diff --git a/tests/test_linear_time.py b/tests/test_linear_time.py index 1b70729..14183bc 100644 --- a/tests/test_linear_time.py +++ b/tests/test_linear_time.py @@ -8,13 +8,16 @@ from __future__ import annotations import sys +import tempfile import time import unittest from pathlib import Path +from unittest import mock HERE = Path(__file__).resolve().parent sys.path.insert(0, str(HERE.parent / "scripts")) +from plccheck import aml # noqa: E402 from plccheck.logix import check_l5k, check_l5x # noqa: E402 from plccheck.plcopen import check_iec61131_10 # noqa: E402 from plccheck.schneider import check_control_expert # noqa: E402 @@ -436,5 +439,75 @@ def test_a_compile_unit_inside_a_payload_does_not_lend_its_parts_to_it(self): self.assertEqual(self.check(self.unit(f"{part_in_unit}")), []) +class AmlLinkedFileParsing(LinearTime): + """A PLCopen file linked from many interfaces is parsed and walked once, not once per link.""" + + PLCOPEN = '' + + def interface(self, i: int, ref: str, cls: str = "AMLBase/PLCopenXMLInterface") -> str: + return (f'' + f'{ref}') + + def aml_text(self, interfaces: list[str]) -> str: + return ('' + + "".join(interfaces) + "") + + def check(self, d: str, refs: list[str], files: dict[str, str] | None = None) -> list[tuple[str, str]]: + base = Path(d) + for name, text in {"plc.xml": self.PLCOPEN, **(files or {})}.items(): + (base / name).parent.mkdir(exist_ok=True) + (base / name).write_text(text, encoding="utf-8") + path = base / "plant.aml" + path.write_text(self.aml_text([self.interface(i, ref) for i, ref in enumerate(refs)]), encoding="utf-8") + return [(f.code, f.message) for f in aml.check_aml(parse_bytes(path.read_bytes()), path)] + + def test_check_time_grows_linearly_with_the_links_to_one_file(self): + # Every link to the same file parsed it again, so n links to the file itself (n links + # in it) cost n parses of an n-link document: quadratic (about 16x for 4x). + with tempfile.TemporaryDirectory() as d: + def make(n): # one file per size: the thunks run after both are built + path = Path(d) / f"plant{n}.aml" + path.write_text(self.aml_text([self.interface(i, f"{path.name}#zzz") for i in range(n)]), encoding="utf-8") + root = parse_bytes(path.read_bytes()) + return lambda: aml.check_aml(root, path) + self.assert_linear(make, 80, 320) + + def test_a_file_linked_many_times_is_parsed_once_whatever_the_spelling_of_its_path(self): + refs = ["plc.xml#zz", "./plc.xml#zz", "sub/../plc.xml#zz", "plc.xml#yy"] + with tempfile.TemporaryDirectory() as d, mock.patch.object(aml, "parse_file", wraps=aml.parse_file) as parse: + got = self.check(d, refs, {"sub/x.xml": ""}) + self.assertEqual(parse.call_count, 1) + self.assertEqual([code for code, _ in got], ["AM012"] * 4) + + def test_a_malformed_file_is_parsed_once_and_gives_no_finding(self): + with tempfile.TemporaryDirectory() as d, mock.patch.object(aml, "parse_file", wraps=aml.parse_file) as parse: + got = self.check(d, ["bad.xml#a", "bad.xml#b", "bad.xml#c"], {"bad.xml": ""}) + self.assertEqual(parse.call_count, 1) + self.assertEqual(got, []) + + def test_each_file_is_checked_for_its_own_fragments(self): + files = {"other.xml": ''} + refs = ["plc.xml#g1", "plc.xml#u1", "plc.xml#g2", "other.xml#g2", "other.xml#g1", "missing.xml#g1"] + with tempfile.TemporaryDirectory() as d: + got = self.check(d, refs, files) + # plc.xml has globalId g1 and uuid u1 but not g2; other.xml has g2 only; missing.xml is AM011 + self.assertEqual(got, [ + ("AM012", "refURI fragment #g2 matches no globalId in plc.xml"), + ("AM012", "refURI fragment #g1 matches no globalId in other.xml"), + ("AM011", "refURI target missing.xml not found next to this file"), + ]) + + def test_only_plcopen_links_with_a_fragment_are_followed(self): + with tempfile.TemporaryDirectory() as d, mock.patch.object(aml, "parse_file", wraps=aml.parse_file) as parse: + base = Path(d) + (base / "plc.xml").write_text(self.PLCOPEN, encoding="utf-8") + (base / "plant.aml").write_text(self.aml_text([ + self.interface(0, "plc.xml", "AMLBase/PLCopenXMLInterface"), + self.interface(1, "plc.xml#zz", "AMLBase/COLLADAInterface"), + ]), encoding="utf-8") + got = aml.check_aml(parse_bytes((base / "plant.aml").read_bytes()), base / "plant.aml") + self.assertEqual((parse.call_count, got), (0, [])) + + if __name__ == "__main__": unittest.main() From af37c2176bdb67e071329102bb930785daf869a4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Smail=20Galija=C5=A1evi=C4=87?= <32690613+SmailG@users.noreply.github.com> Date: Tue, 10 Nov 2026 16:06:21 +0100 Subject: [PATCH 15/29] fix: read rung operands in one pass instead of rescanning per instruction Every `NAME(` scanned on to its matching parenthesis and rebuilt its operand text, so nested or unclosed instructions cost time quadratic in the rung length, and a long identifier rescanned itself from every character. Operands are now read from one pass over the rung. An OTE operand that holds a `(` is a nested instruction, not a tag, and is no longer recorded as a coil: copies of every enclosing operand took memory quadratic in the nesting depth (20 KB of `OTE(` held 40 MB). --- scripts/plccheck/logix.py | 82 +++++++++++++++++++++++++++------------ tests/test_linear_time.py | 63 +++++++++++++++++++++++++++++- 2 files changed, 120 insertions(+), 25 deletions(-) diff --git a/scripts/plccheck/logix.py b/scripts/plccheck/logix.py index 038b392..5110222 100644 --- a/scripts/plccheck/logix.py +++ b/scripts/plccheck/logix.py @@ -3,6 +3,7 @@ from __future__ import annotations import re +from bisect import bisect_left from collections import Counter from .findings import Finding, error, warning, with_context @@ -24,23 +25,62 @@ "JMP": 1, "LBL": 1, "AFI": 0, "NOP": 0, "TND": 0, "MCR": 0, "UID": 0, "UIE": 0, "GSV": 4, "SSV": 4, "MSG": 1, "PSC": 0, "PCMD": 3, } -_INSTR = re.compile(r"([A-Za-z_][A-Za-z0-9_]*)\(") +# `NAME(`, found at the start of an identifier run (leading digits skipped). Without the lookbehind every +# position of a long run would rescan it for a `(`. +_INSTR = re.compile(r"(? list[str]: - parts, depth, cur = [], 0, "" - for ch in s: - if ch in "([": +def _instructions(t: str) -> list[tuple[str, int, str | None]]: + """(mnemonic, operand count, first operand) for each `NAME(` in the neutral text, in order. + + The operands are the comma-separated parts, at bracket depth 0, between the instruction's + parentheses; a missing `)` runs to the end of the text less its last character. The count is 0 + when every part is blank. The first operand is "" when there is none, and None when it holds a + `(`: that is a nested instruction, not a tag, and keeping a copy of it for every instruction + around it would grow with the square of the nesting depth. + + One pass over the text: parentheses matched with a stack, commas kept by bracket depth, and the + operands of each instruction read from those, so no instruction rescans its own operand text. + """ + n = len(t) + depth_before = [0] * (n + 1) # bracket depth ( [ ) ] before each character + text_before = [0] * (n + 1) # non-whitespace characters before each character + close: dict[int, int] = {} # index of each `(` -> index of its `)` + commas: dict[int, list[int]] = {} # bracket depth -> indexes of the commas at that depth + opened: list[int] = [] + depth = chars = 0 + for k, ch in enumerate(t): + depth_before[k], text_before[k] = depth, chars + if ch == ",": + commas.setdefault(depth, []).append(k) + elif ch == "(": + opened.append(k) + depth += 1 + elif ch == ")": + if opened: + close[opened.pop()] = k + depth -= 1 + elif ch == "[": depth += 1 - elif ch in ")]": + elif ch == "]": depth -= 1 - if ch == "," and depth == 0: - parts.append(cur) - cur = "" - else: - cur += ch - parts.append(cur) - return [p.strip() for p in parts] if any(p.strip() for p in parts) else [] + if not ch.isspace(): + chars += 1 + depth_before[n], text_before[n] = depth, chars + found: list[tuple[str, int, str | None]] = [] + for m in _INSTR.finditer(t): + start = m.end() + end = close.get(start - 1, n - 1) + top = commas.get(depth_before[start], []) # a comma at the depth the operands start at is a separator + first, last = bisect_left(top, start), bisect_left(top, end) + separators = max(last - first, 0) + has_operand = end > start and text_before[end] - text_before[start] > separators + first_operand: str | None = "" + if has_operand: + stop = top[first] if separators else end + first_operand = None if t.find("(", start, stop) >= 0 else t[start:stop].strip() + found.append((m.group(1).upper(), separators + 1 if has_operand else 0, first_operand)) + return found def check_rung(text: str, line: int, ctx: str) -> tuple[list[Finding], list[str]]: @@ -70,17 +110,11 @@ def check_rung(text: str, line: int, ctx: str) -> tuple[list[Finding], list[str] if bracket: out.append(error("LX021", "unbalanced branch brackets", line, ctx)) otes: list[str] = [] - for m in _INSTR.finditer(t): - start, depth, i = m.end(), 1, m.end() - while i < len(t) and depth: - depth += {"(": 1, ")": -1}.get(t[i], 0) - i += 1 - ops = _split_operands(t[start:i - 1]) - mnem = m.group(1).upper() - if mnem in ARITY and len(ops) != ARITY[mnem]: - out.append(warning("LX022", f"{mnem} takes {ARITY[mnem]} operand(s), found {len(ops)}", line, ctx)) - if mnem == "OTE" and ops: - otes.append(ops[0]) + for mnem, count, first_operand in _instructions(t): + if mnem in ARITY and count != ARITY[mnem]: + out.append(warning("LX022", f"{mnem} takes {ARITY[mnem]} operand(s), found {count}", line, ctx)) + if mnem == "OTE" and count and first_operand is not None: + otes.append(first_operand) return out, otes diff --git a/tests/test_linear_time.py b/tests/test_linear_time.py index 14183bc..8a220ed 100644 --- a/tests/test_linear_time.py +++ b/tests/test_linear_time.py @@ -18,7 +18,7 @@ sys.path.insert(0, str(HERE.parent / "scripts")) from plccheck import aml # noqa: E402 -from plccheck.logix import check_l5k, check_l5x # noqa: E402 +from plccheck.logix import check_l5k, check_l5x, check_rung # noqa: E402 from plccheck.plcopen import check_iec61131_10 # noqa: E402 from plccheck.schneider import check_control_expert # noqa: E402 from plccheck.simaticml import check_simaticml, check_simatic_sd # noqa: E402 @@ -509,5 +509,66 @@ def test_only_plcopen_links_with_a_fragment_are_followed(self): self.assertEqual((parse.call_count, got), (0, [])) +class LogixRungOperands(LinearTime): + """Operands of every instruction are read in one pass, not by rescanning from each `NAME(`.""" + + # (text for n, small n, big n). Nested and unclosed instructions rescanned the rest of the rung + # per `(`; a long identifier rescanned its run from every character; sequential instructions with + # commas must not search all commas at their depth. + INPUTS = { + "n nested instructions": (lambda n: "XIC(" * n + ")" * n + ";", 300, 1_200), + "n unclosed instructions": (lambda n: "XIC(" * n + ";", 300, 1_200), + "one identifier of n characters": (lambda n: "a" * n + ";", 2_500, 10_000), + "n sequential instructions with commas": (lambda n: "XIC(a,b)" * n + ";", 3_000, 12_000), + } + + def warnings(self, text: str) -> list[str]: + return [f.message for f in check_rung(text, 1, "r")[0] if f.code == "LX022"] + + def test_check_time_grows_linearly_with_the_rung_length(self): + for name, (make, small, big) in self.INPUTS.items(): + with self.subTest(name): + self.assert_linear(lambda n, make=make: (lambda text=make(n): check_rung(text, 1, "r")), small, big) + + def test_operands_are_counted_at_the_instruction_s_own_bracket_depth(self): + self.assertEqual(self.warnings("XIC(a);XIO(b);"), []) + self.assertEqual(self.warnings("XIC(a,b);"), ["XIC takes 1 operand(s), found 2"]) + self.assertEqual(self.warnings("MOV(a,,b);"), ["MOV takes 2 operand(s), found 3"]) + self.assertEqual(self.warnings("COP([a,b],c,d);"), []) # a comma inside [ ] separates nothing + self.assertEqual(self.warnings("XIC(MOV(a,b));"), []) # nor does one inside a nested instruction + self.assertEqual(self.warnings("XIC(MOV(a,b,c));"), ["MOV takes 2 operand(s), found 3"]) + self.assertEqual(self.warnings("XIC(a));"), []) # a stray ) closes nothing + + def test_blank_operands_count_as_none(self): + self.assertEqual(self.warnings("XIC();"), ["XIC takes 1 operand(s), found 0"]) + self.assertEqual(self.warnings("XIC( , );"), ["XIC takes 1 operand(s), found 0"]) + self.assertEqual(self.warnings("MOV(a, );"), []) # one blank part among others still counts + + def test_an_instruction_without_its_closing_parenthesis_drops_the_last_character(self): + # How the checker has always read `XIC(a,b` (operands "a,"): pinned, not endorsed. + self.assertEqual(self.warnings("XIC(a,b"), ["XIC takes 1 operand(s), found 2"]) + self.assertEqual(self.warnings("XIC(a"), ["XIC takes 1 operand(s), found 0"]) + self.assertEqual(self.warnings("XIC("), ["XIC takes 1 operand(s), found 0"]) + + def test_the_mnemonic_skips_leading_digits(self): + self.assertEqual(self.warnings("12XIC(a,b);"), ["XIC takes 1 operand(s), found 2"]) + self.assertEqual(self.warnings("9(a,b);"), []) + + def test_output_coil_operands_are_the_first_operand_stripped(self): + self.assertEqual(check_rung("XIC(a)OTE(b);", 1, "r")[1], ["b"]) + self.assertEqual(check_rung("OTE( x [1] , y );", 1, "r")[1], ["x [1]"]) + self.assertEqual(check_rung("ote(a);", 1, "r")[1], ["a"]) + self.assertEqual(check_rung("OTE( , a);", 1, "r")[1], [""]) # as before: an empty first operand is kept + self.assertEqual(check_rung("OTE( );", 1, "r")[1], []) + + def test_an_operand_holding_an_instruction_is_not_a_coil(self): + # A copy of every enclosing operand grew with the square of the nesting depth (20 KB of + # `OTE(` held 40 MB of operands). An operand with a `(` is a nested call, not a tag. + self.assertEqual(check_rung("OTE(OTE(a));", 1, "r")[1], ["a"]) + self.assertEqual(check_rung("OTE((a));", 1, "r")[1], []) + self.assertEqual(check_rung("OTE(a, OTE(b));", 1, "r")[1], ["a", "b"]) # a first operand ends at its comma + self.assertEqual(check_rung("OTE(" * 400 + ")" * 400 + ";", 1, "r")[1], []) + + if __name__ == "__main__": unittest.main() From 4dd63fcf043be33feb9c056e57475df6f8824bb8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Smail=20Galija=C5=A1evi=C4=87?= <32690613+SmailG@users.noreply.github.com> Date: Tue, 10 Nov 2026 16:57:26 +0100 Subject: [PATCH 16/29] test: use the real XEF shape and kill the surviving ordering mutants The Control Expert scaling tests put STSource directly under FBSource; Control Expert writes FBSource > FBProgram > STSource, so dropping the child walk in the nested-source finder went unnoticed. Build the real shape, add a DFB body with an unclosed IF, two sources in document order and a source under two wrappers. Add tests for the operand-count depth and comma arithmetic in the Logix rung reader, the document order of rungs and SimaticML payloads, and loosen the TwinCAT search-count spy to "at most 3". Reference the private TwinCAT helpers as module attributes so reverting one module fails its own tests instead of the import of the whole test module. tokenize() takes first_line explicitly: its only caller always passes it. --- scripts/plccheck/st.py | 2 +- tests/test_linear_time.py | 106 +++++++++++++++++++++++++++----------- 2 files changed, 77 insertions(+), 31 deletions(-) diff --git a/scripts/plccheck/st.py b/scripts/plccheck/st.py index ea4e6fa..a73c09f 100644 --- a/scripts/plccheck/st.py +++ b/scripts/plccheck/st.py @@ -110,7 +110,7 @@ def shifted(self, offset: int) -> "StAnalysis": ) -def tokenize(src: str, first_line: int = 1) -> tuple[list[Tok], list[Finding]]: +def tokenize(src: str, first_line: int) -> tuple[list[Tok], list[Finding]]: toks: list[Tok] = [] findings: list[Finding] = [] i, n, line = 0, len(src), first_line diff --git a/tests/test_linear_time.py b/tests/test_linear_time.py index 8a220ed..f05ab0f 100644 --- a/tests/test_linear_time.py +++ b/tests/test_linear_time.py @@ -17,13 +17,12 @@ HERE = Path(__file__).resolve().parent sys.path.insert(0, str(HERE.parent / "scripts")) -from plccheck import aml # noqa: E402 +from plccheck import aml, twincat # noqa: E402 from plccheck.logix import check_l5k, check_l5x, check_rung # noqa: E402 from plccheck.plcopen import check_iec61131_10 # noqa: E402 from plccheck.schneider import check_control_expert # noqa: E402 from plccheck.simaticml import check_simaticml, check_simatic_sd # noqa: E402 from plccheck.st import lint_st # noqa: E402 -from plccheck.twincat import _blank_spans, _strip_comments # noqa: E402 from plccheck.xmlutil import parse_bytes # noqa: E402 @@ -64,10 +63,22 @@ def xef(body: str) -> str: class ControlExpertNestedDfb(LinearTime): - """An STSource is linted once, for the nearest FBSource around it, not once per FBSource around it.""" + """An STSource is linted once, for the nearest FBSource around it, not once per FBSource around it. + + The shape is the one Control Expert writes: FBSource > FBProgram > STSource. + """ + + OPEN = '' + CLOSE = "" + + def dfb(self, name: str, *sources: str) -> str: + return self.OPEN.format(name) + "".join(f"{s}" for s in sources) + self.CLOSE def nested(self, depth: int, st: str) -> str: - return xef('' * depth + f"{st}" + "" * depth) + return xef(self.OPEN.format("D") * depth + f"{st}" + self.CLOSE * depth) + + def check(self, body: str) -> list[tuple[str, int, str | None]]: + return [(f.code, f.line, f.context) for f in check_control_expert(xml_root(xef(body)))] def test_lint_time_grows_linearly_with_depth_and_source_size_together(self): # Depth and ST length grow together: relinting the ST once per enclosing FBSource is @@ -77,25 +88,39 @@ def make(n): return lambda: check_control_expert(root) self.assert_linear(make, 15, 60) + def test_a_dfb_body_reports_its_unclosed_block_once_with_the_dfb_context(self): + self.assertEqual(self.check(self.dfb("Pump", "IF a THEN ")), [("ST002", 1, "DFB Pump (ST)")]) + self.assertEqual(self.check(self.dfb("Pump", "IF a THEN x := 1; END_IF;")), []) + def test_deeply_nested_dfb_reports_the_findings_of_a_single_lint(self): st = "IF a THEN " single = check_control_expert(xml_root(self.nested(1, st))) self.assertEqual(len(single), 1, single) self.assertEqual(check_control_expert(xml_root(self.nested(50, st))), single) + def test_an_st_source_under_two_wrappers_is_found(self): + got = self.check('
IF a THEN
' + "") + self.assertEqual(got, [("ST002", 1, "DFB Deep (ST)")]) + + def test_two_st_sources_of_one_dfb_are_reported_in_document_order(self): + got = self.check(self.OPEN.format("Pump") + "\nIF a THEN \nFOR i := 1 TO 2 DO " + + self.CLOSE) + self.assertEqual(got, [("ST002", 2, "DFB Pump (ST)"), ("ST002", 3, "DFB Pump (ST)")]) + + def test_st_sources_of_sibling_wrappers_are_reported_in_document_order(self): + got = self.check('\nIF a THEN \n' + 'FOR i := 1 TO 2 DO ') + self.assertEqual(got, [("ST002", 2, "DFB Pump (ST)"), ("ST002", 3, "DFB Pump (ST)")]) + def test_nested_st_source_belongs_to_the_nearest_dfb(self): - root = xml_root(xef( - 'IF a THEN ' - 'FOR i := 1 TO 2 DO ')) - got = sorted((f.code, f.context) for f in check_control_expert(root)) - self.assertEqual(got, [("ST002", "DFB Inner (ST)"), ("ST002", "DFB Outer (ST)")]) + got = self.check(self.OPEN.format("Outer") + "IF a THEN " + + self.dfb("Inner", "FOR i := 1 TO 2 DO ") + self.CLOSE) + self.assertEqual(sorted((code, ctx) for code, _, ctx in got), [("ST002", "DFB Inner (ST)"), ("ST002", "DFB Outer (ST)")]) def test_sibling_dfbs_keep_their_own_context_and_line(self): - root = xml_root(xef( - '\nIF a THEN \n' - '\nFOR i := 1 TO 2 DO ')) - got = [(f.code, f.line, f.context) for f in check_control_expert(root)] - self.assertEqual(got, [("ST002", 2, "DFB A (ST)"), ("ST002", 4, "DFB B (ST)")]) + got = self.check("\n" + self.dfb("A", "IF a THEN ") + "\n" + self.dfb("B", "FOR i := 1 TO 2 DO ")) + self.assertEqual(got, [("ST002", 2, "DFB A (ST)"), ("ST002", 3, "DFB B (ST)")]) class Iec61131_10NestedBodies(LinearTime): @@ -163,7 +188,7 @@ class TwinCatCommentStripping(LinearTime): def test_strip_time_grows_linearly_with_the_unclosed_openers(self): for name, (unit, small, big) in self.INPUTS.items(): with self.subTest(name): - self.assert_linear(lambda n, unit=unit: (lambda text=unit * n: _strip_comments(text)), small, big) + self.assert_linear(lambda n, unit=unit: (lambda text=unit * n: twincat._strip_comments(text)), small, big) def test_an_opener_without_a_closer_ends_the_scan(self): # The timing test cannot see a retry loop built on str.find (about 10x cheaper than a regex @@ -176,10 +201,10 @@ def find(self, *args): return super().find(*args) text = CountingStr("(* " * 1000) - self.assertEqual(_blank_spans(text, "(*", "*)"), text) - self.assertEqual(CountingStr.calls, 2) # one opener found, one failed closer search + self.assertEqual(twincat._blank_spans(text, "(*", "*)"), text) + self.assertLessEqual(CountingStr.calls, 3) # one opener found, one failed closer search; a retry per opener is 1000 CountingStr.calls = 0 - _blank_spans(CountingStr("(* a *) " * 1000), "(*", "*)") + twincat._blank_spans(CountingStr("(* a *) " * 1000), "(*", "*)") self.assertLessEqual(CountingStr.calls, 2 * 1000 + 1) # an opener and a closer search per span, one last opener search def test_terminated_unterminated_and_adjacent_comments(self): @@ -198,24 +223,24 @@ def test_terminated_unterminated_and_adjacent_comments(self): } for text, want in cases.items(): with self.subTest(text=text): - self.assertEqual(_strip_comments(text), want) + self.assertEqual(twincat._strip_comments(text), want) def test_an_opener_is_not_closed_by_its_own_characters(self): for text in ("(*)", "a (*) b", "/*/", "a /*/ b"): with self.subTest(text=text): - self.assertEqual(_strip_comments(text), text) - self.assertEqual(_strip_comments("(*)*)"), " ") - self.assertEqual(_strip_comments("/*/*/"), " ") + self.assertEqual(twincat._strip_comments(text), text) + self.assertEqual(twincat._strip_comments("(*)*)"), " ") + self.assertEqual(twincat._strip_comments("/*/*/"), " ") def test_openers_do_not_nest_and_passes_run_in_order(self): - self.assertEqual(_strip_comments("(* (* *) *)"), " *)") - self.assertEqual(_strip_comments("(* // *)\nx"), " \nx") # the (* *) pass runs before // - self.assertEqual(_strip_comments("(* /* *) */"), " */") # (* *) first: it takes the /* inside - self.assertEqual(_strip_comments("/* (* */ *)"), "/* ") # then /* */ finds nothing left to pair with - self.assertEqual(_strip_comments("// (* \n *)"), " ") # so the (* *) span crosses the line end - self.assertEqual(_strip_comments("// x (* y\nz"), " \nz") # an unclosed (* is left for the // pass - self.assertEqual(_strip_comments("{ // }\nz"), "{ \nz") # { } runs last: // already took the } - self.assertEqual(_strip_comments("/* { */ }"), " }") + self.assertEqual(twincat._strip_comments("(* (* *) *)"), " *)") + self.assertEqual(twincat._strip_comments("(* // *)\nx"), " \nx") # the (* *) pass runs before // + self.assertEqual(twincat._strip_comments("(* /* *) */"), " */") # (* *) first: it takes the /* inside + self.assertEqual(twincat._strip_comments("/* (* */ *)"), "/* ") # then /* */ finds nothing left to pair with + self.assertEqual(twincat._strip_comments("// (* \n *)"), " ") # so the (* *) span crosses the line end + self.assertEqual(twincat._strip_comments("// x (* y\nz"), " \nz") # an unclosed (* is left for the // pass + self.assertEqual(twincat._strip_comments("{ // }\nz"), "{ \nz") # { } runs last: // already took the } + self.assertEqual(twincat._strip_comments("/* { */ }"), " }") class LogixL5kBlockStack(LinearTime): @@ -362,6 +387,15 @@ def test_sibling_routines_keep_their_own_rungs_and_context(self): + self.routine("b", self.rung(0, "XIC(b)") + self.rung(1, "OTE(c)")) + "") self.assertEqual(got, [("LX020", 2, "p/a rung 0"), ("LX020", 3, "p/b rung 0"), ("LX020", 3, "p/b rung 1")]) + def test_rungs_are_reported_in_document_order(self): + # Two children of one routine, each holding a rung, and two rungs in one child. + rungs = lambda *texts: "".join(f'{t}' for i, t in enumerate(texts)) + routine = ('\n' + rungs("XIC(a)", "XIC(b)") + "\n" + "" + rungs("XIC(c)", "XIC(d)") + "") + got = self.check('' + routine + "") + self.assertEqual([(line, ctx) for _, line, ctx in got], + [(2, "p/a rung 0"), (2, "p/a rung 1"), (3, "p/a rung 0"), (3, "p/a rung 1")]) + def test_coils_are_counted_per_routines_scope(self): got = self.check('' + self.routine("a", self.rung(0, "OTE(x);") + self.rung(1, "OTE(x);")) + self.routine("b", self.rung(0, "OTE(x);")) + "") @@ -422,6 +456,11 @@ def test_networks_of_one_compile_unit_are_reported_in_document_order(self): got = self.check(f"<{self.UNIT}>\n{dup}\n{dup}") self.assertEqual([line for _, line, _ in got], [2, 3]) + def test_payloads_of_one_wrapper_are_reported_in_document_order(self): + dup = '' + got = self.check(f"<{self.UNIT}>\n{dup}\n{dup}") + self.assertEqual([line for _, line, _ in got], [2, 3]) + def test_unresolved_references_and_nested_graph_payloads_keep_their_scopes(self): graph = ('' '') @@ -538,16 +577,23 @@ def test_operands_are_counted_at_the_instruction_s_own_bracket_depth(self): self.assertEqual(self.warnings("XIC(MOV(a,b));"), []) # nor does one inside a nested instruction self.assertEqual(self.warnings("XIC(MOV(a,b,c));"), ["MOV takes 2 operand(s), found 3"]) self.assertEqual(self.warnings("XIC(a));"), []) # a stray ) closes nothing + self.assertEqual(self.warnings("MOV(ABS(x),y);"), []) # the comma after a nested call is at the outer depth + self.assertEqual(self.warnings("MOV(ABS(x),y,z);"), ["MOV takes 2 operand(s), found 3"]) + self.assertEqual(self.warnings("MOV(ABS(ABS(x)),y);"), []) + self.assertEqual(self.warnings("MOV(x,ABS(y));XIC(a);"), []) # depth returns to 0 after each instruction def test_blank_operands_count_as_none(self): self.assertEqual(self.warnings("XIC();"), ["XIC takes 1 operand(s), found 0"]) self.assertEqual(self.warnings("XIC( , );"), ["XIC takes 1 operand(s), found 0"]) self.assertEqual(self.warnings("MOV(a, );"), []) # one blank part among others still counts + self.assertEqual(self.warnings("XIC(,a);"), ["XIC takes 1 operand(s), found 2"]) # a comma right after the `(` def test_an_instruction_without_its_closing_parenthesis_drops_the_last_character(self): # How the checker has always read `XIC(a,b` (operands "a,"): pinned, not endorsed. self.assertEqual(self.warnings("XIC(a,b"), ["XIC takes 1 operand(s), found 2"]) self.assertEqual(self.warnings("XIC(a"), ["XIC takes 1 operand(s), found 0"]) + self.assertEqual(self.warnings("XIC(a,"), []) # the dropped last character is the comma: one operand, `a` + self.assertEqual(self.warnings("XIC(a,,"), ["XIC takes 1 operand(s), found 2"]) self.assertEqual(self.warnings("XIC("), ["XIC takes 1 operand(s), found 0"]) def test_the_mnemonic_skips_leading_digits(self): From 1fa18189d6e13e0cb86ded0d7df30fb2def28d38 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Smail=20Galija=C5=A1evi=C4=87?= <32690613+SmailG@users.noreply.github.com> Date: Tue, 10 Nov 2026 16:58:07 +0100 Subject: [PATCH 17/29] test: split the scaling tests by input kind and list them in CONTRIBUTING test_linear_time.py had reached 574 lines. Move the ratio helper and the XML builders to linear_support.py (not a test module, like codesys_synth.py) and the tests to test_linear_xml.py (Control Expert, IEC 61131-10, L5X, SimaticML), test_linear_aml.py and test_linear_text.py (TwinCAT, L5K, SIMATIC SD, rungs). AutomationML gets its own file so that none of them passes about 300 lines. The tests are unchanged. The tests table in CONTRIBUTING.md lists the new files and test_codesys_hardening.py, which it omitted. --- CONTRIBUTING.md | 2 +- tests/linear_support.py | 61 ++++ tests/test_linear_aml.py | 95 ++++++ tests/test_linear_text.py | 267 ++++++++++++++++ tests/test_linear_time.py | 620 -------------------------------------- tests/test_linear_xml.py | 269 +++++++++++++++++ 6 files changed, 693 insertions(+), 621 deletions(-) create mode 100644 tests/linear_support.py create mode 100644 tests/test_linear_aml.py create mode 100644 tests/test_linear_text.py delete mode 100644 tests/test_linear_time.py create mode 100644 tests/test_linear_xml.py diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 517b174..7e1abd8 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -14,7 +14,7 @@ privately, as described in [SECURITY.md](SECURITY.md). Pull requests use the tem | `scripts/plc_validate.py` | Validator CLI and hook entry point | | `scripts/codesys_project.py` | Offline reader for CODESYS 3.5 / Machine Expert `.project` files (`list`, `st`, `dump-st`, `graph`, `info`, `diff`, `xref`); `scripts/codesysproj/` holds `binary.py` (varint, string table), `project.py` (objects, kinds, text), `graph.py` (LD/FBD records) and `graphview.py` (their text and JSON), `export.py` (`dump-st`), `buildinfo.py` (`info`), `diff.py` (`diff`), `xref.py` and `stscan.py` (`xref`) | | `scripts/plccheck/` | One module per format (`st.py`, `plcopen.py`, `simaticml.py`, `twincat.py`, `logix.py`, `schneider.py`, `iec61499.py`, `aml.py`, `dexpi.py`, `tabular.py`); `findings.py` holds the finding type, `xmlutil.py` the line-numbered XML parser | -| `tests/` | `test_st.py`, `test_formats.py` (validator rules), `test_codesys_project.py` and `test_codesys_tools.py` (the `.project` reader and its `info`, `diff` and `xref`, on synthetic projects only), `test_skills.py` (frontmatter, templates, every `iecst` block lints clean) | +| `tests/` | `test_st.py`, `test_formats.py` (validator rules), `test_codesys_project.py` and `test_codesys_tools.py` (the `.project` reader and its `info`, `diff` and `xref`, on synthetic projects only), `test_codesys_hardening.py` (hostile and damaged `.project` input and the reader's CLI contracts, synthetic projects only), `test_linear_xml.py`, `test_linear_aml.py` and `test_linear_text.py` (CPU-time scaling and ordering of the format checkers on crafted input; `linear_support.py` holds their ratio helper and XML builders), `test_skills.py` (frontmatter, templates, every `iecst` block lints clean) | | `evals/` | `claude plugin eval` cases comparing the plugin against a no-plugin baseline | ## Rules diff --git a/tests/linear_support.py b/tests/linear_support.py new file mode 100644 index 0000000..5f12c39 --- /dev/null +++ b/tests/linear_support.py @@ -0,0 +1,61 @@ +"""Shared pieces of the CPU-time scaling tests: the ratio helper and two XML builders. + +Not a test module (no `test_` prefix), like codesys_synth.py; test_linear_xml.py, test_linear_aml.py +and test_linear_text.py import it. +""" + +from __future__ import annotations + +import sys +import time +import unittest +from pathlib import Path + +HERE = Path(__file__).resolve().parent +sys.path.insert(0, str(HERE.parent / "scripts")) + +from plccheck.xmlutil import Node, parse_bytes # noqa: E402 + + +class LinearTime(unittest.TestCase): + """Base class: the ratio helper.""" + + RUNS = 3 + MAX_RATIO = 8 + TIMER_FLOOR = 0.01 # seconds; keeps the ratio meaningful if the small run is below timer noise + + def best_of_runs(self, run) -> float: + best = float("inf") + for _ in range(self.RUNS): + start = time.process_time() + run() + best = min(best, time.process_time() - start) + return best + + def assert_linear(self, make, small, big): + """`make(n)` builds the input outside the timer and returns the thunk to time. + + A ratio, not an absolute bound, so a slow CI runner does not flake: both sizes slow down + together. Process (CPU) time drops scheduler noise, and the best of three drops a run a + hiccup inflated. Linear work lands near big/small, a rescan per element near its square. + """ + run_small, run_big = make(small), make(big) + t_small, t_big = self.best_of_runs(run_small), self.best_of_runs(run_big) + self.assertLess(t_big, self.MAX_RATIO * max(t_small, self.TIMER_FLOOR), + f"size {small} took {t_small:.3f}s, size {big} took {t_big:.3f}s") + + +def xml_root(text: str): + return parse_bytes(text.encode()) + + +def xef(body: str) -> str: + return f"{body}" + + +def xml_root(text: str) -> Node: + return parse_bytes(text.encode()) + + +def xef(body: str) -> str: + return f"{body}" diff --git a/tests/test_linear_aml.py b/tests/test_linear_aml.py new file mode 100644 index 0000000..202f750 --- /dev/null +++ b/tests/test_linear_aml.py @@ -0,0 +1,95 @@ +"""A crafted AutomationML file must not make the checker spin: a PLCopen file linked from many interfaces. + +Each scaling test feeds a checker an input whose size grows 4x and asserts the CPU time grows +about 4x, not 16x (a rescan per element). Each semantics test beside it pins what the linear +version must keep returning on legitimate input. +""" + +from __future__ import annotations + +import sys +import tempfile +import unittest +from pathlib import Path +from unittest import mock + +HERE = Path(__file__).resolve().parent +sys.path.insert(0, str(HERE.parent / "scripts")) +sys.path.insert(0, str(HERE)) + +from linear_support import LinearTime # noqa: E402 +from plccheck import aml # noqa: E402 +from plccheck.xmlutil import parse_bytes # noqa: E402 + +class AmlLinkedFileParsing(LinearTime): + """A PLCopen file linked from many interfaces is parsed and walked once, not once per link.""" + + PLCOPEN = '' + + def interface(self, i: int, ref: str, cls: str = "AMLBase/PLCopenXMLInterface") -> str: + return (f'' + f'{ref}') + + def aml_text(self, interfaces: list[str]) -> str: + return ('' + + "".join(interfaces) + "") + + def check(self, d: str, refs: list[str], files: dict[str, str] | None = None) -> list[tuple[str, str]]: + base = Path(d) + for name, text in {"plc.xml": self.PLCOPEN, **(files or {})}.items(): + (base / name).parent.mkdir(exist_ok=True) + (base / name).write_text(text, encoding="utf-8") + path = base / "plant.aml" + path.write_text(self.aml_text([self.interface(i, ref) for i, ref in enumerate(refs)]), encoding="utf-8") + return [(f.code, f.message) for f in aml.check_aml(parse_bytes(path.read_bytes()), path)] + + def test_check_time_grows_linearly_with_the_links_to_one_file(self): + # Every link to the same file parsed it again, so n links to the file itself (n links + # in it) cost n parses of an n-link document: quadratic (about 16x for 4x). + with tempfile.TemporaryDirectory() as d: + def make(n): # one file per size: the thunks run after both are built + path = Path(d) / f"plant{n}.aml" + path.write_text(self.aml_text([self.interface(i, f"{path.name}#zzz") for i in range(n)]), encoding="utf-8") + root = parse_bytes(path.read_bytes()) + return lambda: aml.check_aml(root, path) + self.assert_linear(make, 80, 320) + + def test_a_file_linked_many_times_is_parsed_once_whatever_the_spelling_of_its_path(self): + refs = ["plc.xml#zz", "./plc.xml#zz", "sub/../plc.xml#zz", "plc.xml#yy"] + with tempfile.TemporaryDirectory() as d, mock.patch.object(aml, "parse_file", wraps=aml.parse_file) as parse: + got = self.check(d, refs, {"sub/x.xml": ""}) + self.assertEqual(parse.call_count, 1) + self.assertEqual([code for code, _ in got], ["AM012"] * 4) + + def test_a_malformed_file_is_parsed_once_and_gives_no_finding(self): + with tempfile.TemporaryDirectory() as d, mock.patch.object(aml, "parse_file", wraps=aml.parse_file) as parse: + got = self.check(d, ["bad.xml#a", "bad.xml#b", "bad.xml#c"], {"bad.xml": ""}) + self.assertEqual(parse.call_count, 1) + self.assertEqual(got, []) + + def test_each_file_is_checked_for_its_own_fragments(self): + files = {"other.xml": ''} + refs = ["plc.xml#g1", "plc.xml#u1", "plc.xml#g2", "other.xml#g2", "other.xml#g1", "missing.xml#g1"] + with tempfile.TemporaryDirectory() as d: + got = self.check(d, refs, files) + # plc.xml has globalId g1 and uuid u1 but not g2; other.xml has g2 only; missing.xml is AM011 + self.assertEqual(got, [ + ("AM012", "refURI fragment #g2 matches no globalId in plc.xml"), + ("AM012", "refURI fragment #g1 matches no globalId in other.xml"), + ("AM011", "refURI target missing.xml not found next to this file"), + ]) + + def test_only_plcopen_links_with_a_fragment_are_followed(self): + with tempfile.TemporaryDirectory() as d, mock.patch.object(aml, "parse_file", wraps=aml.parse_file) as parse: + base = Path(d) + (base / "plc.xml").write_text(self.PLCOPEN, encoding="utf-8") + (base / "plant.aml").write_text(self.aml_text([ + self.interface(0, "plc.xml", "AMLBase/PLCopenXMLInterface"), + self.interface(1, "plc.xml#zz", "AMLBase/COLLADAInterface"), + ]), encoding="utf-8") + got = aml.check_aml(parse_bytes((base / "plant.aml").read_bytes()), base / "plant.aml") + self.assertEqual((parse.call_count, got), (0, [])) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_linear_text.py b/tests/test_linear_text.py new file mode 100644 index 0000000..3ae82df --- /dev/null +++ b/tests/test_linear_text.py @@ -0,0 +1,267 @@ +"""A crafted text file must not make a checker spin: TwinCAT comments, L5K and SIMATIC SD blocks, Logix +rungs and ST routines in L5K. + +Each scaling test feeds a checker an input whose size grows 4x and asserts the CPU time grows +about 4x, not 16x (a rescan per element). Each semantics test beside it pins what the linear +version must keep returning on legitimate input. +""" + +from __future__ import annotations + +import sys +import unittest +from pathlib import Path + +HERE = Path(__file__).resolve().parent +sys.path.insert(0, str(HERE.parent / "scripts")) +sys.path.insert(0, str(HERE)) + +from linear_support import LinearTime # noqa: E402 +from plccheck import twincat # noqa: E402 +from plccheck.logix import check_l5k, check_rung # noqa: E402 +from plccheck.simaticml import check_simatic_sd # noqa: E402 +from plccheck.st import lint_st # noqa: E402 + +class TwinCatCommentStripping(LinearTime): + """Comment stripping scans once; an opener that never closes must not rescan the rest of the text.""" + + # (opener repeated, small, big). Every opener is unclosed, so a retry from each one walks to the + # end of the text. The pragma case needs a bigger text: its scan is a single character class. + INPUTS = { + "(* never closed": ("(* ", 1_500, 6_000), + "/* never closed": ("/* ", 1_500, 6_000), + "{ never closed": ("{ ", 5_000, 20_000), + } + + def test_strip_time_grows_linearly_with_the_unclosed_openers(self): + for name, (unit, small, big) in self.INPUTS.items(): + with self.subTest(name): + self.assert_linear(lambda n, unit=unit: (lambda text=unit * n: twincat._strip_comments(text)), small, big) + + def test_an_opener_without_a_closer_ends_the_scan(self): + # The timing test cannot see a retry loop built on str.find (about 10x cheaper than a regex + # retry, so below timer noise at a test-sized input), so count the searches directly. + class CountingStr(str): + calls = 0 + + def find(self, *args): + CountingStr.calls += 1 + return super().find(*args) + + text = CountingStr("(* " * 1000) + self.assertEqual(twincat._blank_spans(text, "(*", "*)"), text) + self.assertLessEqual(CountingStr.calls, 3) # one opener found, one failed closer search; a retry per opener is 1000 + CountingStr.calls = 0 + twincat._blank_spans(CountingStr("(* a *) " * 1000), "(*", "*)") + self.assertLessEqual(CountingStr.calls, 2 * 1000 + 1) # an opener and a closer search per span, one last opener search + + def test_terminated_unterminated_and_adjacent_comments(self): + cases = { + "a (* c *) b": "a b", + "a (* c": "a (* c", + "(* x *)(* y *)": " ", + "a /* c */ b /* d": "a b /* d", + "/* x *//* y */z": " z", + "a (* x\ny *) b": "a b", + "{ pragma } a { open": " a { open", + "{a\n}b": " b", + "x // c\ny": "x \ny", + "x // c": "x ", + "//\n//\n": " \n \n", + } + for text, want in cases.items(): + with self.subTest(text=text): + self.assertEqual(twincat._strip_comments(text), want) + + def test_an_opener_is_not_closed_by_its_own_characters(self): + for text in ("(*)", "a (*) b", "/*/", "a /*/ b"): + with self.subTest(text=text): + self.assertEqual(twincat._strip_comments(text), text) + self.assertEqual(twincat._strip_comments("(*)*)"), " ") + self.assertEqual(twincat._strip_comments("/*/*/"), " ") + + def test_openers_do_not_nest_and_passes_run_in_order(self): + self.assertEqual(twincat._strip_comments("(* (* *) *)"), " *)") + self.assertEqual(twincat._strip_comments("(* // *)\nx"), " \nx") # the (* *) pass runs before // + self.assertEqual(twincat._strip_comments("(* /* *) */"), " */") # (* *) first: it takes the /* inside + self.assertEqual(twincat._strip_comments("/* (* */ *)"), "/* ") # then /* */ finds nothing left to pair with + self.assertEqual(twincat._strip_comments("// (* \n *)"), " ") # so the (* *) span crosses the line end + self.assertEqual(twincat._strip_comments("// x (* y\nz"), " \nz") # an unclosed (* is left for the // pass + self.assertEqual(twincat._strip_comments("{ // }\nz"), "{ \nz") # { } runs last: // already took the } + self.assertEqual(twincat._strip_comments("/* { */ }"), " }") + + +class LogixL5kBlockStack(LinearTime): + """A closer that matches no open block is found without scanning the stack.""" + + def errors(self, text: str) -> list[tuple[int | None, str]]: + found = check_l5k(text) + self.assertEqual({f.code for f in found}, {"LX050"} if found else set()) + return [(f.line, f.message) for f in found] + + def test_check_time_grows_linearly_with_the_open_blocks(self): + # Every END_ZZ matches nothing, so each one used to walk all the open PROGRAM frames: + # quadratic in the depth (about 16x for 4x), a count lookup is linear (about 4x). + def make(n): + text = "PROGRAM " * n + "END_ZZ " * n + return lambda: check_l5k(text) + self.assert_linear(make, 1_000, 4_000) + + def test_a_closer_matching_nothing_reports_its_error(self): + self.assertEqual(self.errors("END_ROUTINE"), [(1, "END_ROUTINE without an open ROUTINE")]) + self.assertEqual(self.errors("PROGRAM p\nEND_ROUTINE\nEND_PROGRAM"), [(2, "END_ROUTINE without an open ROUTINE")]) + + def test_a_matched_closer_closes_its_block(self): + self.assertEqual(self.errors("PROGRAM p\nROUTINE r\nEND_ROUTINE\nEND_PROGRAM"), []) + self.assertEqual(self.errors("PROGRAM p\nEND_PROGRAM\nEND_PROGRAM"), [(3, "END_PROGRAM without an open PROGRAM")]) + self.assertEqual(self.errors("PROGRAM a PROGRAM b END_PROGRAM END_PROGRAM END_PROGRAM"), + [(1, "END_PROGRAM without an open PROGRAM")]) + + def test_a_closer_closes_the_blocks_inside_its_own_and_none_more(self): + got = self.errors("PROGRAM p\nROUTINE r\nTAG t\nEND_PROGRAM\nEND_ROUTINE\nEND_TAG\nEND_PROGRAM") + self.assertEqual(got, [ + (2, "ROUTINE opened at line 2 is not closed before END_PROGRAM"), + (3, "TAG opened at line 3 is not closed before END_PROGRAM"), + (5, "END_ROUTINE without an open ROUTINE"), + (6, "END_TAG without an open TAG"), + (7, "END_PROGRAM without an open PROGRAM"), + ]) + + def test_blocks_left_open_are_reported_at_the_end(self): + self.assertEqual(self.errors("PROGRAM p\nROUTINE r\nEND_ROUTINE"), [(1, "PROGRAM opened here is never closed")]) + + +class LogixL5kStRoutineLines(LinearTime): + """An ST routine is linted from its own line, not behind a padding of one newline per file line before it.""" + + def test_lint_time_grows_linearly_with_the_number_of_routines(self): + # Routine k used to be padded with about 3k newlines, so n routines cost n^2 characters: + # quadratic in the file size (about 16x for 4x). + def make(n): + text = "ST_ROUTINE r\n'a := 1;\nEND_ST_ROUTINE\n" * n + return lambda: check_l5k(text) + self.assert_linear(make, 700, 2_800) + + def test_findings_name_file_lines_in_their_position_and_their_message(self): + text = "PROGRAM p\nROUTINE r\nEND_ROUTINE\nST_ROUTINE s\n'IF a THEN\n'x := 1;\nEND_ST_ROUTINE\n" \ + "ST_ROUTINE t\n'(* open\nEND_ST_ROUTINE\nEND_PROGRAM" + got = [(f.code, f.line, f.message, f.context) for f in check_l5k(text)] + self.assertEqual(got, [ + ("ST002", 5, "IF opened at line 5 is never closed", "L5K ST routine"), + ("ST001", 9, "unterminated comment opened with '(*'", "L5K ST routine"), + ]) + + def test_lint_st_numbers_lines_from_first_line(self): + findings, _ = lint_st("x := 1;\nIF a THEN", "body", 10) + self.assertEqual([(f.line, f.message) for f in findings], [(11, "IF opened at line 11 is never closed")]) + findings, _ = lint_st("x := 1;\nIF a THEN", "body") + self.assertEqual([(f.line, f.message) for f in findings], [(2, "IF opened at line 2 is never closed")]) + + +class SimaticSdBlockStack(LinearTime): + """SIMATIC SD: a closer that matches no open NETWORK/RUNG is found without scanning the stack.""" + + def errors(self, text: str) -> list[tuple[int | None, str]]: + found = check_simatic_sd(text) + self.assertEqual({f.code for f in found}, {"SD001"} if found else set()) + return [(f.line, f.message) for f in found] + + def test_check_time_grows_linearly_with_the_open_blocks(self): + # Every END_RUNG matches nothing, so each one used to walk all the open NETWORK frames: + # quadratic in the depth (about 16x for 4x), a count lookup is linear (about 4x). + def make(n): + text = "NETWORK " * n + "END_RUNG " * n + return lambda: check_simatic_sd(text) + self.assert_linear(make, 1_000, 4_000) + + def test_a_closer_matching_nothing_reports_its_error(self): + self.assertEqual(self.errors("END_RUNG"), [(1, "END_RUNG without matching RUNG")]) + self.assertEqual(self.errors("NETWORK\nEND_RUNG\nEND_NETWORK"), [(2, "END_RUNG without matching RUNG")]) + + def test_a_matched_closer_closes_its_block(self): + self.assertEqual(self.errors("NETWORK\nRUNG\nEND_RUNG\nEND_NETWORK"), []) + self.assertEqual(self.errors("NETWORK\nEND_NETWORK\nEND_NETWORK"), [(3, "END_NETWORK without matching NETWORK")]) + self.assertEqual(self.errors("RUNG RUNG END_RUNG END_RUNG END_RUNG"), [(1, "END_RUNG without matching RUNG")]) + + def test_a_closer_closes_the_blocks_inside_its_own_and_none_more(self): + got = self.errors("NETWORK\nRUNG\nEND_NETWORK\nEND_RUNG\nEND_NETWORK") + self.assertEqual(got, [ + (2, "RUNG opened at line 2 is not closed before END_NETWORK"), + (4, "END_RUNG without matching RUNG"), + (5, "END_NETWORK without matching NETWORK"), + ]) + + def test_blocks_left_open_are_reported_at_the_end(self): + self.assertEqual(self.errors("NETWORK\nRUNG\nEND_RUNG"), [(1, "NETWORK opened here is never closed")]) + + +class LogixRungOperands(LinearTime): + """Operands of every instruction are read in one pass, not by rescanning from each `NAME(`.""" + + # (text for n, small n, big n). Nested and unclosed instructions rescanned the rest of the rung + # per `(`; a long identifier rescanned its run from every character; sequential instructions with + # commas must not search all commas at their depth. + INPUTS = { + "n nested instructions": (lambda n: "XIC(" * n + ")" * n + ";", 300, 1_200), + "n unclosed instructions": (lambda n: "XIC(" * n + ";", 300, 1_200), + "one identifier of n characters": (lambda n: "a" * n + ";", 2_500, 10_000), + "n sequential instructions with commas": (lambda n: "XIC(a,b)" * n + ";", 3_000, 12_000), + } + + def warnings(self, text: str) -> list[str]: + return [f.message for f in check_rung(text, 1, "r")[0] if f.code == "LX022"] + + def test_check_time_grows_linearly_with_the_rung_length(self): + for name, (make, small, big) in self.INPUTS.items(): + with self.subTest(name): + self.assert_linear(lambda n, make=make: (lambda text=make(n): check_rung(text, 1, "r")), small, big) + + def test_operands_are_counted_at_the_instruction_s_own_bracket_depth(self): + self.assertEqual(self.warnings("XIC(a);XIO(b);"), []) + self.assertEqual(self.warnings("XIC(a,b);"), ["XIC takes 1 operand(s), found 2"]) + self.assertEqual(self.warnings("MOV(a,,b);"), ["MOV takes 2 operand(s), found 3"]) + self.assertEqual(self.warnings("COP([a,b],c,d);"), []) # a comma inside [ ] separates nothing + self.assertEqual(self.warnings("XIC(MOV(a,b));"), []) # nor does one inside a nested instruction + self.assertEqual(self.warnings("XIC(MOV(a,b,c));"), ["MOV takes 2 operand(s), found 3"]) + self.assertEqual(self.warnings("XIC(a));"), []) # a stray ) closes nothing + self.assertEqual(self.warnings("MOV(ABS(x),y);"), []) # the comma after a nested call is at the outer depth + self.assertEqual(self.warnings("MOV(ABS(x),y,z);"), ["MOV takes 2 operand(s), found 3"]) + self.assertEqual(self.warnings("MOV(ABS(ABS(x)),y);"), []) + self.assertEqual(self.warnings("MOV(x,ABS(y));XIC(a);"), []) # depth returns to 0 after each instruction + + def test_blank_operands_count_as_none(self): + self.assertEqual(self.warnings("XIC();"), ["XIC takes 1 operand(s), found 0"]) + self.assertEqual(self.warnings("XIC( , );"), ["XIC takes 1 operand(s), found 0"]) + self.assertEqual(self.warnings("MOV(a, );"), []) # one blank part among others still counts + self.assertEqual(self.warnings("XIC(,a);"), ["XIC takes 1 operand(s), found 2"]) # a comma right after the `(` + + def test_an_instruction_without_its_closing_parenthesis_drops_the_last_character(self): + # How the checker has always read `XIC(a,b` (operands "a,"): pinned, not endorsed. + self.assertEqual(self.warnings("XIC(a,b"), ["XIC takes 1 operand(s), found 2"]) + self.assertEqual(self.warnings("XIC(a"), ["XIC takes 1 operand(s), found 0"]) + self.assertEqual(self.warnings("XIC(a,"), []) # the dropped last character is the comma: one operand, `a` + self.assertEqual(self.warnings("XIC(a,,"), ["XIC takes 1 operand(s), found 2"]) + self.assertEqual(self.warnings("XIC("), ["XIC takes 1 operand(s), found 0"]) + + def test_the_mnemonic_skips_leading_digits(self): + self.assertEqual(self.warnings("12XIC(a,b);"), ["XIC takes 1 operand(s), found 2"]) + self.assertEqual(self.warnings("9(a,b);"), []) + + def test_output_coil_operands_are_the_first_operand_stripped(self): + self.assertEqual(check_rung("XIC(a)OTE(b);", 1, "r")[1], ["b"]) + self.assertEqual(check_rung("OTE( x [1] , y );", 1, "r")[1], ["x [1]"]) + self.assertEqual(check_rung("ote(a);", 1, "r")[1], ["a"]) + self.assertEqual(check_rung("OTE( , a);", 1, "r")[1], [""]) # as before: an empty first operand is kept + self.assertEqual(check_rung("OTE( );", 1, "r")[1], []) + + def test_an_operand_holding_an_instruction_is_not_a_coil(self): + # A copy of every enclosing operand grew with the square of the nesting depth (20 KB of + # `OTE(` held 40 MB of operands). An operand with a `(` is a nested call, not a tag. + self.assertEqual(check_rung("OTE(OTE(a));", 1, "r")[1], ["a"]) + self.assertEqual(check_rung("OTE((a));", 1, "r")[1], []) + self.assertEqual(check_rung("OTE(a, OTE(b));", 1, "r")[1], ["a", "b"]) # a first operand ends at its comma + self.assertEqual(check_rung("OTE(" * 400 + ")" * 400 + ";", 1, "r")[1], []) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_linear_time.py b/tests/test_linear_time.py deleted file mode 100644 index f05ab0f..0000000 --- a/tests/test_linear_time.py +++ /dev/null @@ -1,620 +0,0 @@ -"""A crafted file must not make a checker spin or multiply its memory. - -Each scaling test feeds a checker an input whose size grows 4x and asserts the CPU time grows -about 4x, not 16x (a rescan per element). Each semantics test beside it pins what the linear -version must keep returning on legitimate input. -""" - -from __future__ import annotations - -import sys -import tempfile -import time -import unittest -from pathlib import Path -from unittest import mock - -HERE = Path(__file__).resolve().parent -sys.path.insert(0, str(HERE.parent / "scripts")) - -from plccheck import aml, twincat # noqa: E402 -from plccheck.logix import check_l5k, check_l5x, check_rung # noqa: E402 -from plccheck.plcopen import check_iec61131_10 # noqa: E402 -from plccheck.schneider import check_control_expert # noqa: E402 -from plccheck.simaticml import check_simaticml, check_simatic_sd # noqa: E402 -from plccheck.st import lint_st # noqa: E402 -from plccheck.xmlutil import parse_bytes # noqa: E402 - - -class LinearTime(unittest.TestCase): - """Base class: the ratio helper.""" - - RUNS = 3 - MAX_RATIO = 8 - TIMER_FLOOR = 0.01 # seconds; keeps the ratio meaningful if the small run is below timer noise - - def best_of_runs(self, run) -> float: - best = float("inf") - for _ in range(self.RUNS): - start = time.process_time() - run() - best = min(best, time.process_time() - start) - return best - - def assert_linear(self, make, small, big): - """`make(n)` builds the input outside the timer and returns the thunk to time. - - A ratio, not an absolute bound, so a slow CI runner does not flake: both sizes slow down - together. Process (CPU) time drops scheduler noise, and the best of three drops a run a - hiccup inflated. Linear work lands near big/small, a rescan per element near its square. - """ - run_small, run_big = make(small), make(big) - t_small, t_big = self.best_of_runs(run_small), self.best_of_runs(run_big) - self.assertLess(t_big, self.MAX_RATIO * max(t_small, self.TIMER_FLOOR), - f"size {small} took {t_small:.3f}s, size {big} took {t_big:.3f}s") - - -def xml_root(text: str): - return parse_bytes(text.encode()) - - -def xef(body: str) -> str: - return f"{body}" - - -class ControlExpertNestedDfb(LinearTime): - """An STSource is linted once, for the nearest FBSource around it, not once per FBSource around it. - - The shape is the one Control Expert writes: FBSource > FBProgram > STSource. - """ - - OPEN = '' - CLOSE = "" - - def dfb(self, name: str, *sources: str) -> str: - return self.OPEN.format(name) + "".join(f"{s}" for s in sources) + self.CLOSE - - def nested(self, depth: int, st: str) -> str: - return xef(self.OPEN.format("D") * depth + f"{st}" + self.CLOSE * depth) - - def check(self, body: str) -> list[tuple[str, int, str | None]]: - return [(f.code, f.line, f.context) for f in check_control_expert(xml_root(xef(body)))] - - def test_lint_time_grows_linearly_with_depth_and_source_size_together(self): - # Depth and ST length grow together: relinting the ST once per enclosing FBSource is - # quadratic in the file size (about 16x for 4x), one lint per STSource is linear (about 4x). - def make(n): - root = xml_root(self.nested(n, "IF a THEN " * (10 * n))) - return lambda: check_control_expert(root) - self.assert_linear(make, 15, 60) - - def test_a_dfb_body_reports_its_unclosed_block_once_with_the_dfb_context(self): - self.assertEqual(self.check(self.dfb("Pump", "IF a THEN ")), [("ST002", 1, "DFB Pump (ST)")]) - self.assertEqual(self.check(self.dfb("Pump", "IF a THEN x := 1; END_IF;")), []) - - def test_deeply_nested_dfb_reports_the_findings_of_a_single_lint(self): - st = "IF a THEN " - single = check_control_expert(xml_root(self.nested(1, st))) - self.assertEqual(len(single), 1, single) - self.assertEqual(check_control_expert(xml_root(self.nested(50, st))), single) - - def test_an_st_source_under_two_wrappers_is_found(self): - got = self.check('
IF a THEN
' - "") - self.assertEqual(got, [("ST002", 1, "DFB Deep (ST)")]) - - def test_two_st_sources_of_one_dfb_are_reported_in_document_order(self): - got = self.check(self.OPEN.format("Pump") + "\nIF a THEN \nFOR i := 1 TO 2 DO " - + self.CLOSE) - self.assertEqual(got, [("ST002", 2, "DFB Pump (ST)"), ("ST002", 3, "DFB Pump (ST)")]) - - def test_st_sources_of_sibling_wrappers_are_reported_in_document_order(self): - got = self.check('\nIF a THEN \n' - 'FOR i := 1 TO 2 DO ') - self.assertEqual(got, [("ST002", 2, "DFB Pump (ST)"), ("ST002", 3, "DFB Pump (ST)")]) - - def test_nested_st_source_belongs_to_the_nearest_dfb(self): - got = self.check(self.OPEN.format("Outer") + "IF a THEN " - + self.dfb("Inner", "FOR i := 1 TO 2 DO ") + self.CLOSE) - self.assertEqual(sorted((code, ctx) for code, _, ctx in got), [("ST002", "DFB Inner (ST)"), ("ST002", "DFB Outer (ST)")]) - - def test_sibling_dfbs_keep_their_own_context_and_line(self): - got = self.check("\n" + self.dfb("A", "IF a THEN ") + "\n" + self.dfb("B", "FOR i := 1 TO 2 DO ")) - self.assertEqual(got, [("ST002", 2, "DFB A (ST)"), ("ST002", 3, "DFB B (ST)")]) - - -class Iec61131_10NestedBodies(LinearTime): - """Connections and ids are collected in one pass, each for its nearest BodyContent.""" - - def project(self, body: str) -> str: - return f'{body}' - - def px121(self, body: str) -> list[tuple[str, int]]: - found = check_iec61131_10(xml_root(self.project(body))) - self.assertEqual({f.code for f in found}, {"PX121"} if found else set()) - return [(f.message.split()[4], f.line) for f in found] - - def test_check_time_grows_linearly_with_the_nesting_depth(self): - # Scanning the whole subtree of every BodyContent for ids and Connection elements is - # quadratic in the depth (about 16x for 4x); one walk is linear (about 4x). - def make(n): - root = xml_root(self.project('' * n + "" * n)) - return lambda: check_iec61131_10(root) - self.assert_linear(make, 500, 2000) - - def test_a_missing_ref_warns_and_a_resolving_one_does_not(self): - got = self.px121( - '\n\n' - '\n') - self.assertEqual(got, [("9,", 4)]) - - def test_ids_resolve_only_inside_their_own_body(self): - got = self.px121( - '\n' - '') - self.assertEqual(got, [("2,", 1), ("1,", 2)]) - - def test_the_body_element_itself_and_deeper_elements_supply_ids(self): - got = self.px121( - '' - '') - self.assertEqual(got, []) - - def test_elements_outside_any_body_are_ignored(self): - got = self.px121('' - '') - self.assertEqual(got, [("1,", 1)]) - - def test_nested_body_is_its_own_scope(self): - # Not a valid structure; pinned so the scope is a decision, not an accident of the walk. - got = self.px121( - '' - '' - '') - self.assertEqual(got, [("1,", 1), ("3,", 1), ("2,", 1)]) - - -class TwinCatCommentStripping(LinearTime): - """Comment stripping scans once; an opener that never closes must not rescan the rest of the text.""" - - # (opener repeated, small, big). Every opener is unclosed, so a retry from each one walks to the - # end of the text. The pragma case needs a bigger text: its scan is a single character class. - INPUTS = { - "(* never closed": ("(* ", 1_500, 6_000), - "/* never closed": ("/* ", 1_500, 6_000), - "{ never closed": ("{ ", 5_000, 20_000), - } - - def test_strip_time_grows_linearly_with_the_unclosed_openers(self): - for name, (unit, small, big) in self.INPUTS.items(): - with self.subTest(name): - self.assert_linear(lambda n, unit=unit: (lambda text=unit * n: twincat._strip_comments(text)), small, big) - - def test_an_opener_without_a_closer_ends_the_scan(self): - # The timing test cannot see a retry loop built on str.find (about 10x cheaper than a regex - # retry, so below timer noise at a test-sized input), so count the searches directly. - class CountingStr(str): - calls = 0 - - def find(self, *args): - CountingStr.calls += 1 - return super().find(*args) - - text = CountingStr("(* " * 1000) - self.assertEqual(twincat._blank_spans(text, "(*", "*)"), text) - self.assertLessEqual(CountingStr.calls, 3) # one opener found, one failed closer search; a retry per opener is 1000 - CountingStr.calls = 0 - twincat._blank_spans(CountingStr("(* a *) " * 1000), "(*", "*)") - self.assertLessEqual(CountingStr.calls, 2 * 1000 + 1) # an opener and a closer search per span, one last opener search - - def test_terminated_unterminated_and_adjacent_comments(self): - cases = { - "a (* c *) b": "a b", - "a (* c": "a (* c", - "(* x *)(* y *)": " ", - "a /* c */ b /* d": "a b /* d", - "/* x *//* y */z": " z", - "a (* x\ny *) b": "a b", - "{ pragma } a { open": " a { open", - "{a\n}b": " b", - "x // c\ny": "x \ny", - "x // c": "x ", - "//\n//\n": " \n \n", - } - for text, want in cases.items(): - with self.subTest(text=text): - self.assertEqual(twincat._strip_comments(text), want) - - def test_an_opener_is_not_closed_by_its_own_characters(self): - for text in ("(*)", "a (*) b", "/*/", "a /*/ b"): - with self.subTest(text=text): - self.assertEqual(twincat._strip_comments(text), text) - self.assertEqual(twincat._strip_comments("(*)*)"), " ") - self.assertEqual(twincat._strip_comments("/*/*/"), " ") - - def test_openers_do_not_nest_and_passes_run_in_order(self): - self.assertEqual(twincat._strip_comments("(* (* *) *)"), " *)") - self.assertEqual(twincat._strip_comments("(* // *)\nx"), " \nx") # the (* *) pass runs before // - self.assertEqual(twincat._strip_comments("(* /* *) */"), " */") # (* *) first: it takes the /* inside - self.assertEqual(twincat._strip_comments("/* (* */ *)"), "/* ") # then /* */ finds nothing left to pair with - self.assertEqual(twincat._strip_comments("// (* \n *)"), " ") # so the (* *) span crosses the line end - self.assertEqual(twincat._strip_comments("// x (* y\nz"), " \nz") # an unclosed (* is left for the // pass - self.assertEqual(twincat._strip_comments("{ // }\nz"), "{ \nz") # { } runs last: // already took the } - self.assertEqual(twincat._strip_comments("/* { */ }"), " }") - - -class LogixL5kBlockStack(LinearTime): - """A closer that matches no open block is found without scanning the stack.""" - - def errors(self, text: str) -> list[tuple[int | None, str]]: - found = check_l5k(text) - self.assertEqual({f.code for f in found}, {"LX050"} if found else set()) - return [(f.line, f.message) for f in found] - - def test_check_time_grows_linearly_with_the_open_blocks(self): - # Every END_ZZ matches nothing, so each one used to walk all the open PROGRAM frames: - # quadratic in the depth (about 16x for 4x), a count lookup is linear (about 4x). - def make(n): - text = "PROGRAM " * n + "END_ZZ " * n - return lambda: check_l5k(text) - self.assert_linear(make, 1_000, 4_000) - - def test_a_closer_matching_nothing_reports_its_error(self): - self.assertEqual(self.errors("END_ROUTINE"), [(1, "END_ROUTINE without an open ROUTINE")]) - self.assertEqual(self.errors("PROGRAM p\nEND_ROUTINE\nEND_PROGRAM"), [(2, "END_ROUTINE without an open ROUTINE")]) - - def test_a_matched_closer_closes_its_block(self): - self.assertEqual(self.errors("PROGRAM p\nROUTINE r\nEND_ROUTINE\nEND_PROGRAM"), []) - self.assertEqual(self.errors("PROGRAM p\nEND_PROGRAM\nEND_PROGRAM"), [(3, "END_PROGRAM without an open PROGRAM")]) - self.assertEqual(self.errors("PROGRAM a PROGRAM b END_PROGRAM END_PROGRAM END_PROGRAM"), - [(1, "END_PROGRAM without an open PROGRAM")]) - - def test_a_closer_closes_the_blocks_inside_its_own_and_none_more(self): - got = self.errors("PROGRAM p\nROUTINE r\nTAG t\nEND_PROGRAM\nEND_ROUTINE\nEND_TAG\nEND_PROGRAM") - self.assertEqual(got, [ - (2, "ROUTINE opened at line 2 is not closed before END_PROGRAM"), - (3, "TAG opened at line 3 is not closed before END_PROGRAM"), - (5, "END_ROUTINE without an open ROUTINE"), - (6, "END_TAG without an open TAG"), - (7, "END_PROGRAM without an open PROGRAM"), - ]) - - def test_blocks_left_open_are_reported_at_the_end(self): - self.assertEqual(self.errors("PROGRAM p\nROUTINE r\nEND_ROUTINE"), [(1, "PROGRAM opened here is never closed")]) - - -class LogixL5kStRoutineLines(LinearTime): - """An ST routine is linted from its own line, not behind a padding of one newline per file line before it.""" - - def test_lint_time_grows_linearly_with_the_number_of_routines(self): - # Routine k used to be padded with about 3k newlines, so n routines cost n^2 characters: - # quadratic in the file size (about 16x for 4x). - def make(n): - text = "ST_ROUTINE r\n'a := 1;\nEND_ST_ROUTINE\n" * n - return lambda: check_l5k(text) - self.assert_linear(make, 700, 2_800) - - def test_findings_name_file_lines_in_their_position_and_their_message(self): - text = "PROGRAM p\nROUTINE r\nEND_ROUTINE\nST_ROUTINE s\n'IF a THEN\n'x := 1;\nEND_ST_ROUTINE\n" \ - "ST_ROUTINE t\n'(* open\nEND_ST_ROUTINE\nEND_PROGRAM" - got = [(f.code, f.line, f.message, f.context) for f in check_l5k(text)] - self.assertEqual(got, [ - ("ST002", 5, "IF opened at line 5 is never closed", "L5K ST routine"), - ("ST001", 9, "unterminated comment opened with '(*'", "L5K ST routine"), - ]) - - def test_lint_st_numbers_lines_from_first_line(self): - findings, _ = lint_st("x := 1;\nIF a THEN", "body", 10) - self.assertEqual([(f.line, f.message) for f in findings], [(11, "IF opened at line 11 is never closed")]) - findings, _ = lint_st("x := 1;\nIF a THEN", "body") - self.assertEqual([(f.line, f.message) for f in findings], [(2, "IF opened at line 2 is never closed")]) - - -class SimaticSdBlockStack(LinearTime): - """SIMATIC SD: a closer that matches no open NETWORK/RUNG is found without scanning the stack.""" - - def errors(self, text: str) -> list[tuple[int | None, str]]: - found = check_simatic_sd(text) - self.assertEqual({f.code for f in found}, {"SD001"} if found else set()) - return [(f.line, f.message) for f in found] - - def test_check_time_grows_linearly_with_the_open_blocks(self): - # Every END_RUNG matches nothing, so each one used to walk all the open NETWORK frames: - # quadratic in the depth (about 16x for 4x), a count lookup is linear (about 4x). - def make(n): - text = "NETWORK " * n + "END_RUNG " * n - return lambda: check_simatic_sd(text) - self.assert_linear(make, 1_000, 4_000) - - def test_a_closer_matching_nothing_reports_its_error(self): - self.assertEqual(self.errors("END_RUNG"), [(1, "END_RUNG without matching RUNG")]) - self.assertEqual(self.errors("NETWORK\nEND_RUNG\nEND_NETWORK"), [(2, "END_RUNG without matching RUNG")]) - - def test_a_matched_closer_closes_its_block(self): - self.assertEqual(self.errors("NETWORK\nRUNG\nEND_RUNG\nEND_NETWORK"), []) - self.assertEqual(self.errors("NETWORK\nEND_NETWORK\nEND_NETWORK"), [(3, "END_NETWORK without matching NETWORK")]) - self.assertEqual(self.errors("RUNG RUNG END_RUNG END_RUNG END_RUNG"), [(1, "END_RUNG without matching RUNG")]) - - def test_a_closer_closes_the_blocks_inside_its_own_and_none_more(self): - got = self.errors("NETWORK\nRUNG\nEND_NETWORK\nEND_RUNG\nEND_NETWORK") - self.assertEqual(got, [ - (2, "RUNG opened at line 2 is not closed before END_NETWORK"), - (4, "END_RUNG without matching RUNG"), - (5, "END_NETWORK without matching NETWORK"), - ]) - - def test_blocks_left_open_are_reported_at_the_end(self): - self.assertEqual(self.errors("NETWORK\nRUNG\nEND_RUNG"), [(1, "NETWORK opened here is never closed")]) - - -class LogixL5xNestedRoutines(LinearTime): - """A rung is checked for the routine it is in, not once for every routine around it.""" - - def l5x(self, programs: str) -> str: - return ('' - f'{programs}') - - def routine(self, name: str, rungs: str, inner: str = "") -> str: - return f'{rungs}{inner}' - - def rung(self, number: int, text: str) -> str: - return f'{text}' - - def nested(self, depth: int, text: str) -> str: - """`depth` routines, each in the of the one before and each with one rung.""" - out = "" - for _ in range(depth): - out = self.routine("r", self.rung(0, text), f"{out}" if out else "") - return self.l5x(f'{out}') - - def check(self, programs: str) -> list[tuple[str, int, str | None]]: - return [(f.code, f.line, f.context) for f in check_l5x(xml_root(self.l5x(programs)))] - - def test_check_time_grows_linearly_with_the_nesting_depth(self): - # Rescanning the whole subtree of every routine for rungs is quadratic in the depth - # (about 16x for 4x); one visit per rung is linear (about 4x). - def make(n): - root = xml_root(self.nested(n, "XIC(a);")) - return lambda: check_l5x(root) - self.assert_linear(make, 150, 600) - - def test_deeply_nested_routines_report_each_rung_once(self): - found = check_l5x(xml_root(self.nested(50, "XIC(a)"))) # no ';': one LX020 per rung - self.assertEqual([f.code for f in found], ["LX020"] * 50) - - def test_sibling_routines_keep_their_own_rungs_and_context(self): - got = self.check('\n' + self.routine("a", self.rung(0, "XIC(a)")) + "\n" - + self.routine("b", self.rung(0, "XIC(b)") + self.rung(1, "OTE(c)")) + "") - self.assertEqual(got, [("LX020", 2, "p/a rung 0"), ("LX020", 3, "p/b rung 0"), ("LX020", 3, "p/b rung 1")]) - - def test_rungs_are_reported_in_document_order(self): - # Two children of one routine, each holding a rung, and two rungs in one child. - rungs = lambda *texts: "".join(f'{t}' for i, t in enumerate(texts)) - routine = ('\n' + rungs("XIC(a)", "XIC(b)") + "\n" - "" + rungs("XIC(c)", "XIC(d)") + "") - got = self.check('' + routine + "") - self.assertEqual([(line, ctx) for _, line, ctx in got], - [(2, "p/a rung 0"), (2, "p/a rung 1"), (3, "p/a rung 0"), (3, "p/a rung 1")]) - - def test_coils_are_counted_per_routines_scope(self): - got = self.check('' + self.routine("a", self.rung(0, "OTE(x);") + self.rung(1, "OTE(x);")) - + self.routine("b", self.rung(0, "OTE(x);")) + "") - self.assertEqual(got, [("LX040", 1, "p")]) - - def test_a_routine_nested_without_a_routines_wrapper_keeps_its_rungs_with_the_routine_around_it(self): - # Only a is visited as a scope of its own; a bare inside a routine is not. - got = self.check('' + self.routine("a", self.rung(0, "XIC(a);") - + self.routine("bare", self.rung(1, "XIC(b)"))) + "") - self.assertEqual(got, [("LX020", 1, "p/a rung 1")]) - - def test_rungs_of_a_nested_routines_scope_belong_to_it(self): - # Not a valid structure; pinned so the scope is a decision, not an accident of the walk. - got = self.check('' + self.routine("a", self.rung(0, "XIC(a)"), - "" + self.routine("in", self.rung(0, "XIC(b)")) + "") + "") - self.assertEqual(got, [("LX020", 1, "p/a rung 0"), ("LX020", 1, "a/in rung 0")]) - - -class SimaticMlNestedCompileUnits(LinearTime): - """A network payload is checked for the CompileUnit it is in, not once for every CompileUnit around it.""" - - UNIT = "SW.Blocks.CompileUnit" - PART = '' - DUP = '' # one SM020 - - def unit(self, *payloads: str) -> str: - """A CompileUnit with each payload under its NetworkSource, as TIA writes it.""" - return f"<{self.UNIT}>" + "".join(f"{p}" for p in payloads) + f"" - - def document(self, units: str) -> str: - return ('B' - f"{units}") - - def nested(self, depth: int, payload: str) -> str: - return self.document(f"<{self.UNIT}>{payload}" * depth + f"" * depth) - - def check(self, units: str) -> list[tuple[str, int, str | None]]: - return [(f.code, f.line, f.context) for f in check_simaticml(xml_root(self.document(units)))] - - def test_check_time_grows_linearly_with_the_nesting_depth(self): - # Collecting the payloads of the whole subtree for every CompileUnit is quadratic in the - # depth (about 16x for 4x); one visit per payload is linear (about 4x). - def make(n): - root = xml_root(self.nested(n, self.PART)) - return lambda: check_simaticml(root) - self.assert_linear(make, 150, 600) - - def test_deeply_nested_compile_units_report_each_network_once(self): - found = check_simaticml(xml_root(self.nested(50, self.DUP))) - self.assertEqual([f.code for f in found], ["SM020"] * 50) - - def test_sibling_compile_units_are_separate_scopes(self): - self.assertEqual(self.check(self.unit(self.PART) + self.unit(self.PART)), []) - self.assertEqual(self.check(f"\n{self.unit(self.DUP)}\n{self.unit(self.PART)}"), [("SM020", 2, "SW.Blocks.FB B")]) - - def test_networks_of_one_compile_unit_are_reported_in_document_order(self): - dup = '' - got = self.check(f"<{self.UNIT}>\n{dup}\n{dup}") - self.assertEqual([line for _, line, _ in got], [2, 3]) - - def test_payloads_of_one_wrapper_are_reported_in_document_order(self): - dup = '' - got = self.check(f"<{self.UNIT}>\n{dup}\n{dup}") - self.assertEqual([line for _, line, _ in got], [2, 3]) - - def test_unresolved_references_and_nested_graph_payloads_keep_their_scopes(self): - graph = ('' - '') - self.assertEqual(self.check(self.unit(graph)), [("SM021", 1, "SW.Blocks.FB B")]) - - def test_payloads_of_a_nested_compile_unit_belong_to_it(self): - # Not a valid structure; pinned so the scope is a decision, not an accident of the walk. The - # outer unit's payload reuses UId 1 with the inner one's and neither sees the other. - inner = self.unit(self.DUP) - self.assertEqual(self.check(f"<{self.UNIT}>{self.PART}{inner}"), - [("SM020", 1, "SW.Blocks.FB B")]) - - def test_a_compile_unit_inside_a_payload_does_not_lend_its_parts_to_it(self): - part_in_unit = f'<{self.UNIT}>' - self.assertEqual(self.check(self.unit(f"{part_in_unit}")), []) - - -class AmlLinkedFileParsing(LinearTime): - """A PLCopen file linked from many interfaces is parsed and walked once, not once per link.""" - - PLCOPEN = '' - - def interface(self, i: int, ref: str, cls: str = "AMLBase/PLCopenXMLInterface") -> str: - return (f'' - f'{ref}') - - def aml_text(self, interfaces: list[str]) -> str: - return ('' - + "".join(interfaces) + "") - - def check(self, d: str, refs: list[str], files: dict[str, str] | None = None) -> list[tuple[str, str]]: - base = Path(d) - for name, text in {"plc.xml": self.PLCOPEN, **(files or {})}.items(): - (base / name).parent.mkdir(exist_ok=True) - (base / name).write_text(text, encoding="utf-8") - path = base / "plant.aml" - path.write_text(self.aml_text([self.interface(i, ref) for i, ref in enumerate(refs)]), encoding="utf-8") - return [(f.code, f.message) for f in aml.check_aml(parse_bytes(path.read_bytes()), path)] - - def test_check_time_grows_linearly_with_the_links_to_one_file(self): - # Every link to the same file parsed it again, so n links to the file itself (n links - # in it) cost n parses of an n-link document: quadratic (about 16x for 4x). - with tempfile.TemporaryDirectory() as d: - def make(n): # one file per size: the thunks run after both are built - path = Path(d) / f"plant{n}.aml" - path.write_text(self.aml_text([self.interface(i, f"{path.name}#zzz") for i in range(n)]), encoding="utf-8") - root = parse_bytes(path.read_bytes()) - return lambda: aml.check_aml(root, path) - self.assert_linear(make, 80, 320) - - def test_a_file_linked_many_times_is_parsed_once_whatever_the_spelling_of_its_path(self): - refs = ["plc.xml#zz", "./plc.xml#zz", "sub/../plc.xml#zz", "plc.xml#yy"] - with tempfile.TemporaryDirectory() as d, mock.patch.object(aml, "parse_file", wraps=aml.parse_file) as parse: - got = self.check(d, refs, {"sub/x.xml": ""}) - self.assertEqual(parse.call_count, 1) - self.assertEqual([code for code, _ in got], ["AM012"] * 4) - - def test_a_malformed_file_is_parsed_once_and_gives_no_finding(self): - with tempfile.TemporaryDirectory() as d, mock.patch.object(aml, "parse_file", wraps=aml.parse_file) as parse: - got = self.check(d, ["bad.xml#a", "bad.xml#b", "bad.xml#c"], {"bad.xml": ""}) - self.assertEqual(parse.call_count, 1) - self.assertEqual(got, []) - - def test_each_file_is_checked_for_its_own_fragments(self): - files = {"other.xml": ''} - refs = ["plc.xml#g1", "plc.xml#u1", "plc.xml#g2", "other.xml#g2", "other.xml#g1", "missing.xml#g1"] - with tempfile.TemporaryDirectory() as d: - got = self.check(d, refs, files) - # plc.xml has globalId g1 and uuid u1 but not g2; other.xml has g2 only; missing.xml is AM011 - self.assertEqual(got, [ - ("AM012", "refURI fragment #g2 matches no globalId in plc.xml"), - ("AM012", "refURI fragment #g1 matches no globalId in other.xml"), - ("AM011", "refURI target missing.xml not found next to this file"), - ]) - - def test_only_plcopen_links_with_a_fragment_are_followed(self): - with tempfile.TemporaryDirectory() as d, mock.patch.object(aml, "parse_file", wraps=aml.parse_file) as parse: - base = Path(d) - (base / "plc.xml").write_text(self.PLCOPEN, encoding="utf-8") - (base / "plant.aml").write_text(self.aml_text([ - self.interface(0, "plc.xml", "AMLBase/PLCopenXMLInterface"), - self.interface(1, "plc.xml#zz", "AMLBase/COLLADAInterface"), - ]), encoding="utf-8") - got = aml.check_aml(parse_bytes((base / "plant.aml").read_bytes()), base / "plant.aml") - self.assertEqual((parse.call_count, got), (0, [])) - - -class LogixRungOperands(LinearTime): - """Operands of every instruction are read in one pass, not by rescanning from each `NAME(`.""" - - # (text for n, small n, big n). Nested and unclosed instructions rescanned the rest of the rung - # per `(`; a long identifier rescanned its run from every character; sequential instructions with - # commas must not search all commas at their depth. - INPUTS = { - "n nested instructions": (lambda n: "XIC(" * n + ")" * n + ";", 300, 1_200), - "n unclosed instructions": (lambda n: "XIC(" * n + ";", 300, 1_200), - "one identifier of n characters": (lambda n: "a" * n + ";", 2_500, 10_000), - "n sequential instructions with commas": (lambda n: "XIC(a,b)" * n + ";", 3_000, 12_000), - } - - def warnings(self, text: str) -> list[str]: - return [f.message for f in check_rung(text, 1, "r")[0] if f.code == "LX022"] - - def test_check_time_grows_linearly_with_the_rung_length(self): - for name, (make, small, big) in self.INPUTS.items(): - with self.subTest(name): - self.assert_linear(lambda n, make=make: (lambda text=make(n): check_rung(text, 1, "r")), small, big) - - def test_operands_are_counted_at_the_instruction_s_own_bracket_depth(self): - self.assertEqual(self.warnings("XIC(a);XIO(b);"), []) - self.assertEqual(self.warnings("XIC(a,b);"), ["XIC takes 1 operand(s), found 2"]) - self.assertEqual(self.warnings("MOV(a,,b);"), ["MOV takes 2 operand(s), found 3"]) - self.assertEqual(self.warnings("COP([a,b],c,d);"), []) # a comma inside [ ] separates nothing - self.assertEqual(self.warnings("XIC(MOV(a,b));"), []) # nor does one inside a nested instruction - self.assertEqual(self.warnings("XIC(MOV(a,b,c));"), ["MOV takes 2 operand(s), found 3"]) - self.assertEqual(self.warnings("XIC(a));"), []) # a stray ) closes nothing - self.assertEqual(self.warnings("MOV(ABS(x),y);"), []) # the comma after a nested call is at the outer depth - self.assertEqual(self.warnings("MOV(ABS(x),y,z);"), ["MOV takes 2 operand(s), found 3"]) - self.assertEqual(self.warnings("MOV(ABS(ABS(x)),y);"), []) - self.assertEqual(self.warnings("MOV(x,ABS(y));XIC(a);"), []) # depth returns to 0 after each instruction - - def test_blank_operands_count_as_none(self): - self.assertEqual(self.warnings("XIC();"), ["XIC takes 1 operand(s), found 0"]) - self.assertEqual(self.warnings("XIC( , );"), ["XIC takes 1 operand(s), found 0"]) - self.assertEqual(self.warnings("MOV(a, );"), []) # one blank part among others still counts - self.assertEqual(self.warnings("XIC(,a);"), ["XIC takes 1 operand(s), found 2"]) # a comma right after the `(` - - def test_an_instruction_without_its_closing_parenthesis_drops_the_last_character(self): - # How the checker has always read `XIC(a,b` (operands "a,"): pinned, not endorsed. - self.assertEqual(self.warnings("XIC(a,b"), ["XIC takes 1 operand(s), found 2"]) - self.assertEqual(self.warnings("XIC(a"), ["XIC takes 1 operand(s), found 0"]) - self.assertEqual(self.warnings("XIC(a,"), []) # the dropped last character is the comma: one operand, `a` - self.assertEqual(self.warnings("XIC(a,,"), ["XIC takes 1 operand(s), found 2"]) - self.assertEqual(self.warnings("XIC("), ["XIC takes 1 operand(s), found 0"]) - - def test_the_mnemonic_skips_leading_digits(self): - self.assertEqual(self.warnings("12XIC(a,b);"), ["XIC takes 1 operand(s), found 2"]) - self.assertEqual(self.warnings("9(a,b);"), []) - - def test_output_coil_operands_are_the_first_operand_stripped(self): - self.assertEqual(check_rung("XIC(a)OTE(b);", 1, "r")[1], ["b"]) - self.assertEqual(check_rung("OTE( x [1] , y );", 1, "r")[1], ["x [1]"]) - self.assertEqual(check_rung("ote(a);", 1, "r")[1], ["a"]) - self.assertEqual(check_rung("OTE( , a);", 1, "r")[1], [""]) # as before: an empty first operand is kept - self.assertEqual(check_rung("OTE( );", 1, "r")[1], []) - - def test_an_operand_holding_an_instruction_is_not_a_coil(self): - # A copy of every enclosing operand grew with the square of the nesting depth (20 KB of - # `OTE(` held 40 MB of operands). An operand with a `(` is a nested call, not a tag. - self.assertEqual(check_rung("OTE(OTE(a));", 1, "r")[1], ["a"]) - self.assertEqual(check_rung("OTE((a));", 1, "r")[1], []) - self.assertEqual(check_rung("OTE(a, OTE(b));", 1, "r")[1], ["a", "b"]) # a first operand ends at its comma - self.assertEqual(check_rung("OTE(" * 400 + ")" * 400 + ";", 1, "r")[1], []) - - -if __name__ == "__main__": - unittest.main() diff --git a/tests/test_linear_xml.py b/tests/test_linear_xml.py new file mode 100644 index 0000000..bf38847 --- /dev/null +++ b/tests/test_linear_xml.py @@ -0,0 +1,269 @@ +"""A crafted XML document must not make a checker spin or multiply its memory: Control Expert, +IEC 61131-10, L5X and SimaticML. + +Each scaling test feeds a checker an input whose size grows 4x and asserts the CPU time grows +about 4x, not 16x (a rescan per element). Each semantics test beside it pins what the linear +version must keep returning on legitimate input. +""" + +from __future__ import annotations + +import sys +import unittest +from pathlib import Path + +HERE = Path(__file__).resolve().parent +sys.path.insert(0, str(HERE.parent / "scripts")) +sys.path.insert(0, str(HERE)) + +from linear_support import LinearTime, xef, xml_root # noqa: E402 +from plccheck.logix import check_l5x # noqa: E402 +from plccheck.plcopen import check_iec61131_10 # noqa: E402 +from plccheck.schneider import check_control_expert # noqa: E402 +from plccheck.simaticml import check_simaticml # noqa: E402 + +class ControlExpertNestedDfb(LinearTime): + """An STSource is linted once, for the nearest FBSource around it, not once per FBSource around it. + + The shape is the one Control Expert writes: FBSource > FBProgram > STSource. + """ + + OPEN = '' + CLOSE = "" + + def dfb(self, name: str, *sources: str) -> str: + return self.OPEN.format(name) + "".join(f"{s}" for s in sources) + self.CLOSE + + def nested(self, depth: int, st: str) -> str: + return xef(self.OPEN.format("D") * depth + f"{st}" + self.CLOSE * depth) + + def check(self, body: str) -> list[tuple[str, int, str | None]]: + return [(f.code, f.line, f.context) for f in check_control_expert(xml_root(xef(body)))] + + def test_lint_time_grows_linearly_with_depth_and_source_size_together(self): + # Depth and ST length grow together: relinting the ST once per enclosing FBSource is + # quadratic in the file size (about 16x for 4x), one lint per STSource is linear (about 4x). + def make(n): + root = xml_root(self.nested(n, "IF a THEN " * (10 * n))) + return lambda: check_control_expert(root) + self.assert_linear(make, 15, 60) + + def test_a_dfb_body_reports_its_unclosed_block_once_with_the_dfb_context(self): + self.assertEqual(self.check(self.dfb("Pump", "IF a THEN ")), [("ST002", 1, "DFB Pump (ST)")]) + self.assertEqual(self.check(self.dfb("Pump", "IF a THEN x := 1; END_IF;")), []) + + def test_deeply_nested_dfb_reports_the_findings_of_a_single_lint(self): + st = "IF a THEN " + single = check_control_expert(xml_root(self.nested(1, st))) + self.assertEqual(len(single), 1, single) + self.assertEqual(check_control_expert(xml_root(self.nested(50, st))), single) + + def test_an_st_source_under_two_wrappers_is_found(self): + got = self.check('
IF a THEN
' + "") + self.assertEqual(got, [("ST002", 1, "DFB Deep (ST)")]) + + def test_two_st_sources_of_one_dfb_are_reported_in_document_order(self): + got = self.check(self.OPEN.format("Pump") + "\nIF a THEN \nFOR i := 1 TO 2 DO " + + self.CLOSE) + self.assertEqual(got, [("ST002", 2, "DFB Pump (ST)"), ("ST002", 3, "DFB Pump (ST)")]) + + def test_st_sources_of_sibling_wrappers_are_reported_in_document_order(self): + got = self.check('\nIF a THEN \n' + 'FOR i := 1 TO 2 DO ') + self.assertEqual(got, [("ST002", 2, "DFB Pump (ST)"), ("ST002", 3, "DFB Pump (ST)")]) + + def test_nested_st_source_belongs_to_the_nearest_dfb(self): + got = self.check(self.OPEN.format("Outer") + "IF a THEN " + + self.dfb("Inner", "FOR i := 1 TO 2 DO ") + self.CLOSE) + self.assertEqual(sorted((code, ctx) for code, _, ctx in got), [("ST002", "DFB Inner (ST)"), ("ST002", "DFB Outer (ST)")]) + + def test_sibling_dfbs_keep_their_own_context_and_line(self): + got = self.check("\n" + self.dfb("A", "IF a THEN ") + "\n" + self.dfb("B", "FOR i := 1 TO 2 DO ")) + self.assertEqual(got, [("ST002", 2, "DFB A (ST)"), ("ST002", 3, "DFB B (ST)")]) + + +class Iec61131_10NestedBodies(LinearTime): + """Connections and ids are collected in one pass, each for its nearest BodyContent.""" + + def project(self, body: str) -> str: + return f'{body}' + + def px121(self, body: str) -> list[tuple[str, int]]: + found = check_iec61131_10(xml_root(self.project(body))) + self.assertEqual({f.code for f in found}, {"PX121"} if found else set()) + return [(f.message.split()[4], f.line) for f in found] + + def test_check_time_grows_linearly_with_the_nesting_depth(self): + # Scanning the whole subtree of every BodyContent for ids and Connection elements is + # quadratic in the depth (about 16x for 4x); one walk is linear (about 4x). + def make(n): + root = xml_root(self.project('' * n + "" * n)) + return lambda: check_iec61131_10(root) + self.assert_linear(make, 500, 2000) + + def test_a_missing_ref_warns_and_a_resolving_one_does_not(self): + got = self.px121( + '\n\n' + '\n') + self.assertEqual(got, [("9,", 4)]) + + def test_ids_resolve_only_inside_their_own_body(self): + got = self.px121( + '\n' + '') + self.assertEqual(got, [("2,", 1), ("1,", 2)]) + + def test_the_body_element_itself_and_deeper_elements_supply_ids(self): + got = self.px121( + '' + '') + self.assertEqual(got, []) + + def test_elements_outside_any_body_are_ignored(self): + got = self.px121('' + '') + self.assertEqual(got, [("1,", 1)]) + + def test_nested_body_is_its_own_scope(self): + # Not a valid structure; pinned so the scope is a decision, not an accident of the walk. + got = self.px121( + '' + '' + '') + self.assertEqual(got, [("1,", 1), ("3,", 1), ("2,", 1)]) + + +class LogixL5xNestedRoutines(LinearTime): + """A rung is checked for the routine it is in, not once for every routine around it.""" + + def l5x(self, programs: str) -> str: + return ('' + f'{programs}') + + def routine(self, name: str, rungs: str, inner: str = "") -> str: + return f'{rungs}{inner}' + + def rung(self, number: int, text: str) -> str: + return f'{text}' + + def nested(self, depth: int, text: str) -> str: + """`depth` routines, each in the of the one before and each with one rung.""" + out = "" + for _ in range(depth): + out = self.routine("r", self.rung(0, text), f"{out}" if out else "") + return self.l5x(f'{out}') + + def check(self, programs: str) -> list[tuple[str, int, str | None]]: + return [(f.code, f.line, f.context) for f in check_l5x(xml_root(self.l5x(programs)))] + + def test_check_time_grows_linearly_with_the_nesting_depth(self): + # Rescanning the whole subtree of every routine for rungs is quadratic in the depth + # (about 16x for 4x); one visit per rung is linear (about 4x). + def make(n): + root = xml_root(self.nested(n, "XIC(a);")) + return lambda: check_l5x(root) + self.assert_linear(make, 150, 600) + + def test_deeply_nested_routines_report_each_rung_once(self): + found = check_l5x(xml_root(self.nested(50, "XIC(a)"))) # no ';': one LX020 per rung + self.assertEqual([f.code for f in found], ["LX020"] * 50) + + def test_sibling_routines_keep_their_own_rungs_and_context(self): + got = self.check('\n' + self.routine("a", self.rung(0, "XIC(a)")) + "\n" + + self.routine("b", self.rung(0, "XIC(b)") + self.rung(1, "OTE(c)")) + "") + self.assertEqual(got, [("LX020", 2, "p/a rung 0"), ("LX020", 3, "p/b rung 0"), ("LX020", 3, "p/b rung 1")]) + + def test_rungs_are_reported_in_document_order(self): + # Two children of one routine, each holding a rung, and two rungs in one child. + rungs = lambda *texts: "".join(f'{t}' for i, t in enumerate(texts)) + routine = ('\n' + rungs("XIC(a)", "XIC(b)") + "\n" + "" + rungs("XIC(c)", "XIC(d)") + "") + got = self.check('' + routine + "") + self.assertEqual([(line, ctx) for _, line, ctx in got], + [(2, "p/a rung 0"), (2, "p/a rung 1"), (3, "p/a rung 0"), (3, "p/a rung 1")]) + + def test_coils_are_counted_per_routines_scope(self): + got = self.check('' + self.routine("a", self.rung(0, "OTE(x);") + self.rung(1, "OTE(x);")) + + self.routine("b", self.rung(0, "OTE(x);")) + "") + self.assertEqual(got, [("LX040", 1, "p")]) + + def test_a_routine_nested_without_a_routines_wrapper_keeps_its_rungs_with_the_routine_around_it(self): + # Only a is visited as a scope of its own; a bare inside a routine is not. + got = self.check('' + self.routine("a", self.rung(0, "XIC(a);") + + self.routine("bare", self.rung(1, "XIC(b)"))) + "") + self.assertEqual(got, [("LX020", 1, "p/a rung 1")]) + + def test_rungs_of_a_nested_routines_scope_belong_to_it(self): + # Not a valid structure; pinned so the scope is a decision, not an accident of the walk. + got = self.check('' + self.routine("a", self.rung(0, "XIC(a)"), + "" + self.routine("in", self.rung(0, "XIC(b)")) + "") + "") + self.assertEqual(got, [("LX020", 1, "p/a rung 0"), ("LX020", 1, "a/in rung 0")]) + + +class SimaticMlNestedCompileUnits(LinearTime): + """A network payload is checked for the CompileUnit it is in, not once for every CompileUnit around it.""" + + UNIT = "SW.Blocks.CompileUnit" + PART = '' + DUP = '' # one SM020 + + def unit(self, *payloads: str) -> str: + """A CompileUnit with each payload under its NetworkSource, as TIA writes it.""" + return f"<{self.UNIT}>" + "".join(f"{p}" for p in payloads) + f"" + + def document(self, units: str) -> str: + return ('B' + f"{units}") + + def nested(self, depth: int, payload: str) -> str: + return self.document(f"<{self.UNIT}>{payload}" * depth + f"" * depth) + + def check(self, units: str) -> list[tuple[str, int, str | None]]: + return [(f.code, f.line, f.context) for f in check_simaticml(xml_root(self.document(units)))] + + def test_check_time_grows_linearly_with_the_nesting_depth(self): + # Collecting the payloads of the whole subtree for every CompileUnit is quadratic in the + # depth (about 16x for 4x); one visit per payload is linear (about 4x). + def make(n): + root = xml_root(self.nested(n, self.PART)) + return lambda: check_simaticml(root) + self.assert_linear(make, 150, 600) + + def test_deeply_nested_compile_units_report_each_network_once(self): + found = check_simaticml(xml_root(self.nested(50, self.DUP))) + self.assertEqual([f.code for f in found], ["SM020"] * 50) + + def test_sibling_compile_units_are_separate_scopes(self): + self.assertEqual(self.check(self.unit(self.PART) + self.unit(self.PART)), []) + self.assertEqual(self.check(f"\n{self.unit(self.DUP)}\n{self.unit(self.PART)}"), [("SM020", 2, "SW.Blocks.FB B")]) + + def test_networks_of_one_compile_unit_are_reported_in_document_order(self): + dup = '' + got = self.check(f"<{self.UNIT}>\n{dup}\n{dup}") + self.assertEqual([line for _, line, _ in got], [2, 3]) + + def test_payloads_of_one_wrapper_are_reported_in_document_order(self): + dup = '' + got = self.check(f"<{self.UNIT}>\n{dup}\n{dup}") + self.assertEqual([line for _, line, _ in got], [2, 3]) + + def test_unresolved_references_and_nested_graph_payloads_keep_their_scopes(self): + graph = ('' + '') + self.assertEqual(self.check(self.unit(graph)), [("SM021", 1, "SW.Blocks.FB B")]) + + def test_payloads_of_a_nested_compile_unit_belong_to_it(self): + # Not a valid structure; pinned so the scope is a decision, not an accident of the walk. The + # outer unit's payload reuses UId 1 with the inner one's and neither sees the other. + inner = self.unit(self.DUP) + self.assertEqual(self.check(f"<{self.UNIT}>{self.PART}{inner}"), + [("SM020", 1, "SW.Blocks.FB B")]) + + def test_a_compile_unit_inside_a_payload_does_not_lend_its_parts_to_it(self): + part_in_unit = f'<{self.UNIT}>' + self.assertEqual(self.check(self.unit(f"{part_in_unit}")), []) + + +if __name__ == "__main__": + unittest.main() From e68c65990834ba1578107f9e145a1fc53a2e67af Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Smail=20Galija=C5=A1evi=C4=87?= <32690613+SmailG@users.noreply.github.com> Date: Tue, 10 Nov 2026 17:01:20 +0100 Subject: [PATCH 18/29] fix: drop an OTE operand only when it holds another instruction The one-pass operand reader dropped every OTE operand that held a `(`, so OTE(Arr[(i+1)]) written twice lost its LX040 double-coil warning. Drop an operand only when a `NAME(` starts inside it: that is a nested call, and keeping a copy of it per enclosing instruction is what grew with the square of the nesting depth. Matches do not overlap, so only the next one can lie inside an operand; the memory stays linear (5000 nested OTE( peak about 2 MB, where the rescan held 50 MB). --- scripts/plccheck/logix.py | 15 ++++++++++----- tests/test_linear_text.py | 27 +++++++++++++++++++++++++-- tests/test_linear_xml.py | 4 ++++ 3 files changed, 39 insertions(+), 7 deletions(-) diff --git a/scripts/plccheck/logix.py b/scripts/plccheck/logix.py index 5110222..dcb37eb 100644 --- a/scripts/plccheck/logix.py +++ b/scripts/plccheck/logix.py @@ -5,6 +5,7 @@ import re from bisect import bisect_left from collections import Counter +from itertools import tee, zip_longest from .findings import Finding, error, warning, with_context from .st import lint_st @@ -35,9 +36,10 @@ def _instructions(t: str) -> list[tuple[str, int, str | None]]: The operands are the comma-separated parts, at bracket depth 0, between the instruction's parentheses; a missing `)` runs to the end of the text less its last character. The count is 0 - when every part is blank. The first operand is "" when there is none, and None when it holds a - `(`: that is a nested instruction, not a tag, and keeping a copy of it for every instruction - around it would grow with the square of the nesting depth. + when every part is blank. The first operand is "" when there is none, and None when it holds + another instruction (a `NAME(`): that is a call, not a tag, and keeping a copy of it for every + instruction around it would grow with the square of the nesting depth. A bare `(` does not + count, so `Arr[(i+1)]` stays an operand. One pass over the text: parentheses matched with a stack, commas kept by bracket depth, and the operands of each instruction read from those, so no instruction rescans its own operand text. @@ -68,7 +70,10 @@ def _instructions(t: str) -> list[tuple[str, int, str | None]]: chars += 1 depth_before[n], text_before[n] = depth, chars found: list[tuple[str, int, str | None]] = [] - for m in _INSTR.finditer(t): + # Each match is paired with the next. Matches do not overlap, so only the next one can lie inside this one's operands. + current, following = tee(_INSTR.finditer(t)) + next(following, None) + for m, after in zip_longest(current, following): start = m.end() end = close.get(start - 1, n - 1) top = commas.get(depth_before[start], []) # a comma at the depth the operands start at is a separator @@ -78,7 +83,7 @@ def _instructions(t: str) -> list[tuple[str, int, str | None]]: first_operand: str | None = "" if has_operand: stop = top[first] if separators else end - first_operand = None if t.find("(", start, stop) >= 0 else t[start:stop].strip() + first_operand = None if after is not None and after.end() <= stop else t[start:stop].strip() found.append((m.group(1).upper(), separators + 1 if has_operand else 0, first_operand)) return found diff --git a/tests/test_linear_text.py b/tests/test_linear_text.py index 3ae82df..11320dc 100644 --- a/tests/test_linear_text.py +++ b/tests/test_linear_text.py @@ -9,6 +9,7 @@ from __future__ import annotations import sys +import tracemalloc import unittest from pathlib import Path @@ -256,12 +257,34 @@ def test_output_coil_operands_are_the_first_operand_stripped(self): def test_an_operand_holding_an_instruction_is_not_a_coil(self): # A copy of every enclosing operand grew with the square of the nesting depth (20 KB of - # `OTE(` held 40 MB of operands). An operand with a `(` is a nested call, not a tag. + # `OTE(` held 50 MB of operands). An operand that holds another `NAME(` is a call, not a tag. self.assertEqual(check_rung("OTE(OTE(a));", 1, "r")[1], ["a"]) - self.assertEqual(check_rung("OTE((a));", 1, "r")[1], []) + self.assertEqual(check_rung("OTE(a(b),c);", 1, "r")[1], []) + self.assertEqual(check_rung("OTE(12XIC(a));", 1, "r")[1], []) self.assertEqual(check_rung("OTE(a, OTE(b));", 1, "r")[1], ["a", "b"]) # a first operand ends at its comma + self.assertEqual(check_rung("OTE(a(", 1, "r")[1], ["a"]) # the unclosed text drops its last character, the `(` + self.assertEqual(check_rung("OTE(a()", 1, "r")[1], []) # here the dropped last character is a `)`: `a(` is inside self.assertEqual(check_rung("OTE(" * 400 + ")" * 400 + ";", 1, "r")[1], []) + def test_a_parenthesis_that_starts_no_instruction_keeps_the_operand(self): + # Whether Logix accepts a parenthesised subscript is not verified; the checker must not + # lose the double-coil warning for it (v0.2.1 kept these operands). + self.assertEqual(check_rung("OTE((a));", 1, "r")[1], ["(a)"]) + self.assertEqual(check_rung("OTE(Arr[(i+1)]);", 1, "r")[1], ["Arr[(i+1)]"]) + self.assertEqual(check_rung("OTE(Arr[(i+1)],b);", 1, "r")[1], ["Arr[(i+1)]"]) + + def test_memory_grows_linearly_with_the_nesting_depth(self): + # About 2 MB for 5000 nested `OTE(` (the lists indexed by character); the operand copies + # the rescan kept were 50 MB. + for text in ("OTE(" * 5_000, "OTE(" * 5_000 + ")" * 5_000 + ";"): + tracemalloc.start() + try: + check_rung(text, 1, "r") + peak = tracemalloc.get_traced_memory()[1] + finally: + tracemalloc.stop() + self.assertLess(peak, 10_000_000) + if __name__ == "__main__": unittest.main() diff --git a/tests/test_linear_xml.py b/tests/test_linear_xml.py index bf38847..d10c4a3 100644 --- a/tests/test_linear_xml.py +++ b/tests/test_linear_xml.py @@ -188,6 +188,10 @@ def test_coils_are_counted_per_routines_scope(self): + self.routine("b", self.rung(0, "OTE(x);")) + "") self.assertEqual(got, [("LX040", 1, "p")]) + def test_a_double_coil_on_a_parenthesised_subscript_is_found(self): + two = self.routine("a", self.rung(0, "OTE(Arr[(i+1)]);") + self.rung(1, "OTE(Arr[(i+1)]);")) + self.assertEqual(self.check('' + two + ""), [("LX040", 1, "p")]) + def test_a_routine_nested_without_a_routines_wrapper_keeps_its_rungs_with_the_routine_around_it(self): # Only a is visited as a scope of its own; a bare inside a routine is not. got = self.check('' + self.routine("a", self.rung(0, "XIC(a);") From cdc6d23a4b60a7f4adce687fb2fdedee496b6316 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Smail=20Galija=C5=A1evi=C4=87?= <32690613+SmailG@users.noreply.github.com> Date: Tue, 10 Nov 2026 17:05:32 +0100 Subject: [PATCH 19/29] fix: refuse a namespace URI over 256 characters before parsing Expat copies a namespace URI into every element name and namespaced attribute name under its declaration, so one long URI costs its length times the element count: a 36 KB file with a 20,000-character default namespace and 4000 empty children peaked at 81 MB, nested elements at 62 MB. A cap in a namespace-declaration handler is too late, because the declaring element's own attributes are already expanded. Run a first pass without namespace processing that rejects an xmlns or xmlns:* attribute longer than 256 characters (the longest URI the checkers know is 77) with XML001, before the real parse. That pass still expands entities in attribute values, so it is also where entity declarations are refused: they precede the root element, and the real parse never sees one. The pass costs about 15 percent more on a 4 MB file. --- CONTRIBUTING.md | 2 +- scripts/plccheck/xmlutil.py | 42 +++++++++++-- tests/test_xml_limits.py | 121 ++++++++++++++++++++++++++++++++++++ 3 files changed, 159 insertions(+), 6 deletions(-) create mode 100644 tests/test_xml_limits.py diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 7e1abd8..ce01dde 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -14,7 +14,7 @@ privately, as described in [SECURITY.md](SECURITY.md). Pull requests use the tem | `scripts/plc_validate.py` | Validator CLI and hook entry point | | `scripts/codesys_project.py` | Offline reader for CODESYS 3.5 / Machine Expert `.project` files (`list`, `st`, `dump-st`, `graph`, `info`, `diff`, `xref`); `scripts/codesysproj/` holds `binary.py` (varint, string table), `project.py` (objects, kinds, text), `graph.py` (LD/FBD records) and `graphview.py` (their text and JSON), `export.py` (`dump-st`), `buildinfo.py` (`info`), `diff.py` (`diff`), `xref.py` and `stscan.py` (`xref`) | | `scripts/plccheck/` | One module per format (`st.py`, `plcopen.py`, `simaticml.py`, `twincat.py`, `logix.py`, `schneider.py`, `iec61499.py`, `aml.py`, `dexpi.py`, `tabular.py`); `findings.py` holds the finding type, `xmlutil.py` the line-numbered XML parser | -| `tests/` | `test_st.py`, `test_formats.py` (validator rules), `test_codesys_project.py` and `test_codesys_tools.py` (the `.project` reader and its `info`, `diff` and `xref`, on synthetic projects only), `test_codesys_hardening.py` (hostile and damaged `.project` input and the reader's CLI contracts, synthetic projects only), `test_linear_xml.py`, `test_linear_aml.py` and `test_linear_text.py` (CPU-time scaling and ordering of the format checkers on crafted input; `linear_support.py` holds their ratio helper and XML builders), `test_skills.py` (frontmatter, templates, every `iecst` block lints clean) | +| `tests/` | `test_st.py`, `test_formats.py` (validator rules), `test_codesys_project.py` and `test_codesys_tools.py` (the `.project` reader and its `info`, `diff` and `xref`, on synthetic projects only), `test_codesys_hardening.py` (hostile and damaged `.project` input and the reader's CLI contracts, synthetic projects only), `test_linear_xml.py`, `test_linear_aml.py` and `test_linear_text.py` (CPU-time scaling and ordering of the format checkers on crafted input; `linear_support.py` holds their ratio helper and XML builders), `test_xml_limits.py` (the namespace URI cap of the shared XML parser), `test_skills.py` (frontmatter, templates, every `iecst` block lints clean) | | `evals/` | `claude plugin eval` cases comparing the plugin against a no-plugin baseline | ## Rules diff --git a/scripts/plccheck/xmlutil.py b/scripts/plccheck/xmlutil.py index 0c55b02..5f5f779 100644 --- a/scripts/plccheck/xmlutil.py +++ b/scripts/plccheck/xmlutil.py @@ -5,6 +5,10 @@ entity declarations precede the root element, the format sniffer stops at the first one too: a file that declares an entity is not detected as any format, so the edit-time hook skips it, and validating it with an explicit format reports XML001. + +A namespace URI longer than MAX_NS_URI is refused as well: expat copies the URI into every element +and namespaced attribute name under its declaration, so one long URI multiplies the memory by the +element count (a 35 KB file reached 82 MB). The longest URI the checkers know is 77 characters. """ from __future__ import annotations @@ -68,7 +72,40 @@ def __init__(self, message: str, line: int | None): self.line = line +MAX_NS_URI = 256 + + +def _refuse_long_namespaces(data: bytes) -> None: + """Raise XmlError for a declaration of a namespace URI over MAX_NS_URI, before the real parse. + + A parser without namespace processing keeps `xmlns` attributes as written and expands no URI + into names, so this pass costs about one more parse of the file. A handler for namespace + declarations in the real parser would be too late: the declaring element's own attributes are + already expanded by then. It still expands entities in attribute values, so it is where entity + declarations are refused: they precede the root element, so none is expanded before it raises, + and the real parse never sees one. What it cannot parse is left for the real parse to report. + """ + parser = xml.parsers.expat.ParserCreate() + + def start(_name, attrs): + for name, value in attrs.items(): + if (name == "xmlns" or name.startswith("xmlns:")) and len(value) > MAX_NS_URI: + raise XmlError(f"namespace URI longer than {MAX_NS_URI} characters", parser.CurrentLineNumber) + + def entity_decl(*_args): + # Expanding entities makes the work grow with the nesting, not the file size. + raise XmlError("XML entity declarations are not supported", parser.CurrentLineNumber) + + parser.StartElementHandler = start + parser.EntityDeclHandler = entity_decl + try: + parser.Parse(data, True) + except xml.parsers.expat.ExpatError: + pass + + def parse_bytes(data: bytes) -> Node: + _refuse_long_namespaces(data) parser = xml.parsers.expat.ParserCreate(namespace_separator="}") parser.SetParamEntityParsing(xml.parsers.expat.XML_PARAM_ENTITY_PARSING_NEVER) root: list[Node] = [] @@ -101,14 +138,9 @@ def chars(data): stack[-1].text_line = parser.CurrentLineNumber chunks[-1].append(data) - def entity_decl(*_args): - # Expanding entities makes the work grow with the nesting, not the file size. - raise XmlError("XML entity declarations are not supported", parser.CurrentLineNumber) - parser.StartElementHandler = start parser.EndElementHandler = end parser.CharacterDataHandler = chars - parser.EntityDeclHandler = entity_decl try: parser.Parse(data, True) except xml.parsers.expat.ExpatError as exc: diff --git a/tests/test_xml_limits.py b/tests/test_xml_limits.py new file mode 100644 index 0000000..7dcad60 --- /dev/null +++ b/tests/test_xml_limits.py @@ -0,0 +1,121 @@ +"""The shared XML parser refuses what multiplies its memory: a long namespace URI. + +Expat copies a namespace URI into every element name and namespaced attribute name, so one +declaration of a long URI costs its length times the number of elements or attributes under it +(a 35 KB file reached 82 MB). Real URIs are short (the longest one the checkers know is 77 +characters, a Siemens StructuredText namespace), so a declaration over MAX_NS_URI is refused. +""" + +from __future__ import annotations + +import sys +import tempfile +import tracemalloc +import unittest +from pathlib import Path + +HERE = Path(__file__).resolve().parent +sys.path.insert(0, str(HERE.parent / "scripts")) + +import plc_validate # noqa: E402 +from plccheck import simaticml, xmlutil # noqa: E402 + +SIEMENS_URI = simaticml.NS_BASE + "NetworkSource/StructuredText/v4" +CAP = 256 +PEAK_LIMIT = 5_000_000 # bytes; the unfixed parser peaks at tens of MB on the shapes below +LONG_URI_ERROR = "namespace URI longer than 256 characters" +ENTITY_ERROR = "XML entity declarations are not supported" + + +def uri(length: int) -> str: + return "http://x/" + "a" * (length - len("http://x/")) + + +class NamespaceUriCap(unittest.TestCase): + def refused(self, data: bytes) -> xmlutil.XmlError: + with self.assertRaises(xmlutil.XmlError) as ctx: + xmlutil.parse_bytes(data) + return ctx.exception + + def peak(self, data: bytes) -> int: + tracemalloc.start() + try: + try: + xmlutil.parse_bytes(data) + except xmlutil.XmlError: + pass + return tracemalloc.get_traced_memory()[1] + finally: + tracemalloc.stop() + + def test_the_cap_is_well_above_the_longest_namespace_the_checkers_know(self): + known = [simaticml.NS_BASE + f"{family}/{ver}" for exp in simaticml.EXPECTED_NS.values() for family, ver in exp.items()] + self.assertEqual(max(map(len, known)), 77) + self.assertEqual(xmlutil.MAX_NS_URI, CAP) + + def test_uris_up_to_the_cap_parse_and_longer_ones_are_refused(self): + for length in (len(SIEMENS_URI), CAP): + with self.subTest(length=length): + root = xmlutil.parse_bytes(f''.encode()) + self.assertEqual((root.ns, root.children[0].ns), (uri(length), uri(length))) + exc = self.refused(f''.encode()) + self.assertEqual(str(exc), LONG_URI_ERROR) + + def test_a_prefixed_declaration_is_capped_too(self): + self.assertEqual(xmlutil.parse_bytes(f''.encode()).children[0].ns, uri(CAP)) + self.assertEqual(str(self.refused(f''.encode())), LONG_URI_ERROR) + + def test_a_long_value_of_an_attribute_that_only_starts_with_xmlns_is_not_a_declaration(self): + for name in ("xmlnsx", "xmlns_p", "p:xmlns"): + with self.subTest(attribute=name): + self.assertEqual(xmlutil.parse_bytes(f''.encode()).tag, "r") + + def test_the_error_names_the_line_of_the_declaring_element(self): + exc = self.refused(f'\n\n'.encode()) + self.assertEqual((str(exc), exc.line), (LONG_URI_ERROR, 3)) + + def test_a_long_default_namespace_over_many_elements_is_refused_in_little_memory(self): + # 36 KB: one 20,000-character URI, 4000 empty children (82 MB unfixed). + data = f'{"" * 4000}'.encode() + self.assertLess(len(data), 40_000) + self.assertEqual(str(self.refused(data)), LONG_URI_ERROR) + self.assertLess(self.peak(data), PEAK_LIMIT) + + def test_a_long_prefix_over_many_distinct_attributes_is_refused_in_little_memory(self): + # The declaring element's own attributes each carry the URI too, so a cap in a namespace + # declaration handler would come too late. 33 KB: one 16,000-character URI, 1200 attributes. + attrs = " ".join(f'p:a{i}="1"' for i in range(1200)) + data = f''.encode() + self.assertLess(len(data), 40_000) + self.assertEqual(str(self.refused(data)), LONG_URI_ERROR) + self.assertLess(self.peak(data), PEAK_LIMIT) + + def test_the_cap_holds_in_utf16(self): + # The check reads attribute values from the parser, so the encoding does not matter. + data = f''.encode("utf-16") + self.assertEqual(str(self.refused(data)), LONG_URI_ERROR) + ok = f''.encode("utf-16") + self.assertEqual(xmlutil.parse_bytes(ok).ns, uri(CAP)) + + def test_an_entity_in_a_namespace_value_is_refused_as_an_entity(self): + # The first pass does not process namespaces but still expands entities in attribute values, + # so it refuses entity declarations itself, before the entity can be expanded: one entity + # whose value is over the cap would otherwise be reported as a long URI. + data = f']>\n'.encode() + exc = self.refused(data) + self.assertEqual((str(exc), exc.line), (ENTITY_ERROR, 1)) + + def test_a_malformed_document_is_still_reported_by_the_parse(self): + self.assertTrue(str(self.refused(b"")).startswith("XML is not well-formed")) + self.assertTrue(str(self.refused(b'')).startswith("XML is not well-formed")) # unbound prefix + + def test_a_file_with_a_long_uri_is_an_xml001_finding(self): + with tempfile.TemporaryDirectory() as d: + path = Path(d) / "a.xml" + path.write_text(f'', encoding="utf-8") + fmt, findings = plc_validate.check_file(path, "plcopen") + self.assertEqual((fmt, [(f.code, f.line, f.message) for f in findings]), ("plcopen", [("XML001", 1, LONG_URI_ERROR)])) + + +if __name__ == "__main__": + unittest.main() From 900bbe3ca7737fd878078c71a88ca2700c040bad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Smail=20Galija=C5=A1evi=C4=87?= <32690613+SmailG@users.noreply.github.com> Date: Tue, 10 Nov 2026 17:07:52 +0100 Subject: [PATCH 20/29] fix: cut a finding's message and context at 400 characters A name from the file is quoted in the context of every finding under it (a Logix program name in each rung finding, a POU name in each body finding), so the printed output grew with the square of the file size: a 2,000-character program name over 40 rungs printed 85 KB, a 20,000-character one over 400 rungs 8 MB. Finding.__post_init__ cuts message and context to MAX_FINDING_TEXT, with an ellipsis inside the limit. Every finding is built through it, including those re-anchored by with_context(). The longest message the tests produce is 195 characters, the longest context 26, and the longest message template 183 plus its two names, so ordinary output is unchanged. The same files now print 21 KB and 206 KB. --- CONTRIBUTING.md | 2 +- scripts/plccheck/findings.py | 19 +++++++ tests/test_finding_clip.py | 102 +++++++++++++++++++++++++++++++++++ 3 files changed, 122 insertions(+), 1 deletion(-) create mode 100644 tests/test_finding_clip.py diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index ce01dde..84e8257 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -14,7 +14,7 @@ privately, as described in [SECURITY.md](SECURITY.md). Pull requests use the tem | `scripts/plc_validate.py` | Validator CLI and hook entry point | | `scripts/codesys_project.py` | Offline reader for CODESYS 3.5 / Machine Expert `.project` files (`list`, `st`, `dump-st`, `graph`, `info`, `diff`, `xref`); `scripts/codesysproj/` holds `binary.py` (varint, string table), `project.py` (objects, kinds, text), `graph.py` (LD/FBD records) and `graphview.py` (their text and JSON), `export.py` (`dump-st`), `buildinfo.py` (`info`), `diff.py` (`diff`), `xref.py` and `stscan.py` (`xref`) | | `scripts/plccheck/` | One module per format (`st.py`, `plcopen.py`, `simaticml.py`, `twincat.py`, `logix.py`, `schneider.py`, `iec61499.py`, `aml.py`, `dexpi.py`, `tabular.py`); `findings.py` holds the finding type, `xmlutil.py` the line-numbered XML parser | -| `tests/` | `test_st.py`, `test_formats.py` (validator rules), `test_codesys_project.py` and `test_codesys_tools.py` (the `.project` reader and its `info`, `diff` and `xref`, on synthetic projects only), `test_codesys_hardening.py` (hostile and damaged `.project` input and the reader's CLI contracts, synthetic projects only), `test_linear_xml.py`, `test_linear_aml.py` and `test_linear_text.py` (CPU-time scaling and ordering of the format checkers on crafted input; `linear_support.py` holds their ratio helper and XML builders), `test_xml_limits.py` (the namespace URI cap of the shared XML parser), `test_skills.py` (frontmatter, templates, every `iecst` block lints clean) | +| `tests/` | `test_st.py`, `test_formats.py` (validator rules), `test_codesys_project.py` and `test_codesys_tools.py` (the `.project` reader and its `info`, `diff` and `xref`, on synthetic projects only), `test_codesys_hardening.py` (hostile and damaged `.project` input and the reader's CLI contracts, synthetic projects only), `test_linear_xml.py`, `test_linear_aml.py` and `test_linear_text.py` (CPU-time scaling and ordering of the format checkers on crafted input; `linear_support.py` holds their ratio helper and XML builders), `test_xml_limits.py` (the namespace URI cap of the shared XML parser), `test_finding_clip.py` (the length cap on a finding's message and context), `test_skills.py` (frontmatter, templates, every `iecst` block lints clean) | | `evals/` | `claude plugin eval` cases comparing the plugin against a no-plugin baseline | ## Rules diff --git a/scripts/plccheck/findings.py b/scripts/plccheck/findings.py index aec446c..1f4cf56 100644 --- a/scripts/plccheck/findings.py +++ b/scripts/plccheck/findings.py @@ -4,6 +4,19 @@ from dataclasses import dataclass +# A message or context quotes names from the file, and a context is repeated on every finding +# under it, so one long name times many findings is quadratic in the file size. The longest +# message in the tests is 195 characters and the longest context 26. +MAX_FINDING_TEXT = 400 +ELLIPSIS = "…" + + +def _clip(text: str) -> str: + """`text` as is, or cut to MAX_FINDING_TEXT characters, the last being an ellipsis.""" + if len(text) <= MAX_FINDING_TEXT: + return text + return text[: MAX_FINDING_TEXT - len(ELLIPSIS)] + ELLIPSIS + @dataclass(frozen=True) class Finding: @@ -13,6 +26,12 @@ class Finding: line: int | None = None context: str | None = None # e.g. "POU FB_Motor body (ST)" + def __post_init__(self) -> None: + # Every finding is built through here (error(), warning() and with_context() end in Finding(...)). + object.__setattr__(self, "message", _clip(self.message)) + if self.context is not None: + object.__setattr__(self, "context", _clip(self.context)) + def format(self, path: str) -> str: where = f"{path}:{self.line}" if self.line else path ctx = f" [{self.context}]" if self.context else "" diff --git a/tests/test_finding_clip.py b/tests/test_finding_clip.py new file mode 100644 index 0000000..278b6f6 --- /dev/null +++ b/tests/test_finding_clip.py @@ -0,0 +1,102 @@ +"""A finding's message and context are cut to MAX_FINDING_TEXT, in the one place every finding is built. + +A name from the file is quoted in the context of every finding under it (a Logix program name in +every rung finding), so a 2,000-character name over 40 rungs printed 85 KB and a 20,000-character +one over 400 rungs printed 8 MB, growing with the square of the file size. +""" + +from __future__ import annotations + +import sys +import unittest +from pathlib import Path + +HERE = Path(__file__).resolve().parent +sys.path.insert(0, str(HERE.parent / "scripts")) +sys.path.insert(0, str(HERE)) + +from linear_support import xml_root # noqa: E402 +from plccheck import findings # noqa: E402 +from plccheck.logix import check_l5x # noqa: E402 +from plccheck.st import check_st_file # noqa: E402 + + +def l5x(program: str, rungs: int) -> str: + body = "".join(f'XIC(a)' for i in range(rungs)) + return ('' + f'{body}' + "") + + +def formatted(program: str, rungs: int) -> str: + return "\n".join(f.format("a.L5X") for f in check_l5x(xml_root(l5x(program, rungs)))) + + +class FindingClip(unittest.TestCase): + MAX = findings.MAX_FINDING_TEXT + + def assert_clipped(self, text: str): + self.assertEqual(len(text), self.MAX) + self.assertTrue(text.endswith(findings.ELLIPSIS)) + self.assertEqual(text[:-1], "x" * (self.MAX - 1)) + + def test_a_text_up_to_the_limit_is_kept_and_a_longer_one_is_cut_to_it(self): + exact, over = "x" * self.MAX, "x" * (self.MAX + 1) + f = findings.error("ST002", exact, 3, exact) + self.assertEqual((f.message, f.context), (exact, exact)) + f = findings.error("ST002", over, 3, over) + self.assert_clipped(f.message) + self.assert_clipped(f.context) + + def test_message_and_context_are_cut_on_their_own(self): + over = "x" * (self.MAX + 1) + only_message = findings.warning("ST002", over, 3, "short") + self.assert_clipped(only_message.message) + self.assertEqual(only_message.context, "short") + only_context = findings.warning("ST002", "short", 3, over) + self.assertEqual(only_context.message, "short") + self.assert_clipped(only_context.context) + self.assertIsNone(findings.error("ST002", "short").context) + + def test_a_real_long_message_is_kept_whole(self): + src = ("PROGRAM P\nVAR\n tonWaitForTheBelt : TON;\n x : BOOL;\nEND_VAR\n" + "IF x THEN\n tonWaitForTheBelt(IN := TRUE, PT := T#5S);\nEND_IF;\nEND_PROGRAM\n") + [f] = check_st_file(src) + self.assertEqual((f.code, len(f.message)), ("ST030", 203)) + self.assertTrue(f.message.endswith("gate its input instead")) + + def test_the_limit_is_well_above_the_messages_the_checkers_write(self): + # The longest message the tests produce is 195 characters (ST030 with two names), the longest + # template 183 before its two names; a limit near them would cut real findings. + self.assertGreaterEqual(self.MAX, 300) + + def test_a_context_added_by_with_context_is_cut(self): + inner = findings.error("ST002", "m", 4, "y" * 300) + [out] = findings.with_context([inner], "x" * 300, 10) + self.assertEqual((out.line, len(out.context)), (14, self.MAX)) + self.assertTrue(out.context.endswith(findings.ELLIPSIS)) + + def test_the_formatted_and_dict_forms_carry_the_cut_text(self): + f = findings.error("ST002", "x" * 1000, 3, "y" * 1000) + self.assertEqual(len(f.format("p")), len("p:3: error ST002: []") + 2 * self.MAX) + d = f.as_dict("p") + self.assertEqual((len(d["message"]), len(d["context"])), (self.MAX, self.MAX)) + + def test_a_short_name_gives_the_same_output_as_before(self): + self.assertEqual(formatted("p", 2), "\n".join( + f"a.L5X:1: error LX020: rung text must end with ';' [p/r rung {i}]" for i in range(2))) + + def test_a_long_program_name_does_not_make_the_output_grow_with_it(self): + short, long_, longer = formatted("p" * 10, 40), formatted("p" * 2_000, 40), formatted("p" * 4_000, 40) + self.assertEqual(len(long_), len(longer)) # the same output for any name over the limit + self.assertLess(len(long_), 40 * (self.MAX + 100)) # was 85 KB for 40 rungs + self.assertGreater(len(long_), len(short)) + self.assertEqual(long_.splitlines()[0].count(findings.ELLIPSIS), 1) + + def test_the_output_grows_linearly_with_the_rung_count_for_a_long_name(self): + small, big = len(formatted("p" * 2_000, 20)), len(formatted("p" * 2_000, 80)) + self.assertLess(big, 4.2 * small) # not exactly 4x: the rung numbers have more digits + + +if __name__ == "__main__": + unittest.main() From 68b46326762ee67d88f9c58feb3c3ae196d92718 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Smail=20Galija=C5=A1evi=C4=87?= <32690613+SmailG@users.noreply.github.com> Date: Tue, 10 Nov 2026 17:09:07 +0100 Subject: [PATCH 21/29] fix: collect SimaticML namespaces in a dict in document order The check rebuilt a set of the (family, version) pairs it had seen for every namespaced element: k namespaces over m elements cost k times m (1000 over 10,000 elements, 158 KB, took 0.35 s; it now takes 0.006 s). Keep a dict from pair to the line of its first element instead. The SM003 findings were sorted by line over a set, so two namespaces on one line came out in an order that changed with the hash seed. They now follow document order. --- scripts/plccheck/simaticml.py | 8 ++++---- tests/test_linear_xml.py | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 4 deletions(-) diff --git a/scripts/plccheck/simaticml.py b/scripts/plccheck/simaticml.py index db73294..fd9141f 100644 --- a/scripts/plccheck/simaticml.py +++ b/scripts/plccheck/simaticml.py @@ -52,12 +52,12 @@ def check_simaticml(root: Node) -> list[Finding]: if not tia: out.append(warning("SM002", "no ; TIA cannot tell which schema generation this is", root.line)) expected = EXPECTED_NS.get(tia, {}) - seen_ns: set[tuple[str, str, int]] = set() + seen_ns: dict[tuple[str, str], int] = {} # (family, version) -> line of its first element, in document order for node in root.iter(): m = _NS_RE.match(node.ns) if node.ns else None - if m and (m.group("family"), m.group("ver")) not in {(f, v) for f, v, _ in seen_ns}: - seen_ns.add((m.group("family"), m.group("ver"), node.line)) - for family, ver, line in sorted(seen_ns, key=lambda x: x[2]): + if m: + seen_ns.setdefault((m.group("family"), m.group("ver")), node.line) + for (family, ver), line in seen_ns.items(): want = expected.get(family) if want and want != ver: out.append(warning("SM003", f"{family} namespace is {ver} but TIA {tia} exports use {want}; import may be rejected", line)) diff --git a/tests/test_linear_xml.py b/tests/test_linear_xml.py index d10c4a3..f0c519c 100644 --- a/tests/test_linear_xml.py +++ b/tests/test_linear_xml.py @@ -205,6 +205,40 @@ def test_rungs_of_a_nested_routines_scope_belong_to_it(self): self.assertEqual(got, [("LX020", 1, "p/a rung 0"), ("LX020", 1, "a/in rung 0")]) +class SimaticMlNamespaces(LinearTime): + """Each (family, version) namespace is reported once, at its first element, in document order.""" + + BASE = "http://www.siemens.com/automation/Openness/SW/" + + def check(self, body: str, declarations: str = "") -> list[tuple[str, int, str]]: + doc = f'{body}' + return [(f.code, f.line, f.message) for f in check_simaticml(xml_root(doc)) if f.code == "SM003"] + + def test_check_time_grows_linearly_with_the_namespaces_and_elements_together(self): + # k distinct namespaces over 10k elements: rebuilding the set of seen (family, version) + # pairs for every element is k times m, quadratic when both grow (about 16x for 4x). + def make(n): + declarations = " ".join(f'xmlns:p{i}="{self.BASE}Interface/v{i + 10}"' for i in range(n)) + root = xml_root(f'' + + "".join(f"" for i in range(10 * n)) + "") + return lambda: check_simaticml(root) + self.assert_linear(make, 250, 1_000) + + def test_a_namespace_is_reported_once_at_its_first_element(self): + ns = f'xmlns="{self.BASE}NetworkSource/FlgNet/v3"' + got = self.check(f"\n\n\n") + self.assertEqual(got, [("SM003", 2, "NetworkSource/FlgNet namespace is v3 but TIA V18 exports use v4; import may be rejected")]) + + def test_a_matching_namespace_is_not_reported(self): + self.assertEqual(self.check(f''), []) + + def test_namespaces_on_one_line_are_reported_in_document_order(self): + # Ties on the line used to fall to the iteration order of a set, which changes with the hash seed. + versions = [9, 1, 8, 2, 7, 3, 6, 4] # none is v5, the version V18 expects of Interface + got = self.check("".join(f'' for v in versions)) + self.assertEqual([(line, msg.split()[3]) for _, line, msg in got], [(1, f"v{v}") for v in versions]) + + class SimaticMlNestedCompileUnits(LinearTime): """A network payload is checked for the CompileUnit it is in, not once for every CompileUnit around it.""" From 0619029c7d0fdb68ba01d60355b228eba9757dd2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Smail=20Galija=C5=A1evi=C4=87?= <32690613+SmailG@users.noreply.github.com> Date: Tue, 10 Nov 2026 17:11:53 +0100 Subject: [PATCH 22/29] fix: identify an AutomationML link target by inode and keep long names safe The linked-file cache was keyed by the resolved path, which misses another spelling of the same file on a case-insensitive file system and a hard link: 800 links to a file under 32 case spellings parsed it 32 times (0.17 s), and under 800 spellings once each. Key it by (st_dev, st_ino) and fall back to the resolved path when the file system reports no inode (0); the same links now parse once (0.02 s). Check refURI and ExternalReference targets with os.path.exists(): on Python 3.9 Path.exists() raises ENAMETOOLONG for a path component over the file system's limit, which crashed the validator instead of giving AM011/AM013. Rename the local `ids` in check_aml's link loop to `target_ids`; it shadowed the dict of IDs above. --- scripts/plccheck/aml.py | 28 +++++++++++---- tests/test_linear_aml.py | 75 +++++++++++++++++++++++++++++++++++++--- 2 files changed, 92 insertions(+), 11 deletions(-) diff --git a/scripts/plccheck/aml.py b/scripts/plccheck/aml.py index 4a53d2c..442e2d7 100644 --- a/scripts/plccheck/aml.py +++ b/scripts/plccheck/aml.py @@ -2,6 +2,7 @@ from __future__ import annotations +import os from pathlib import Path from urllib.parse import unquote, urlparse @@ -21,13 +22,23 @@ def _resolve(ref: str, base: Path) -> tuple[Path | None, str]: return (base / p) if p else None, u.fragment -def _global_ids(target: Path, cache: dict[Path, set[str] | None]) -> set[str] | None: +def _file_key(target: Path, st: os.stat_result) -> tuple[int, int] | Path: + """What identifies the file behind `target`: its device and inode, else its resolved path. + + The resolved path misses one file under another spelling of its name (`X.XML` for `x.xml` on a + case-insensitive file system) and a hard link; the inode does not. A file system that reports + no inode (0) falls back to the resolved path, which still joins `./x.xml` and `a/../x.xml`. + """ + return (st.st_dev, st.st_ino) if st.st_ino else target.resolve() + + +def _global_ids(target: Path, cache: dict[tuple[int, int] | Path, set[str] | None]) -> set[str] | None: """The globalId and uuid values of a referenced PLCopen file, or None if it is not well-formed XML. Parsed once per file: every interface that links to the same file would otherwise parse and walk it again, so a file of n links to itself costs n parses of an n-link document. """ - key = target.resolve() # `x.xml`, `./x.xml` and `a/../x.xml` are one file + key = _file_key(target, target.stat()) if key not in cache: try: doc = parse_file(target) @@ -73,7 +84,7 @@ def check_aml(root: Node, path: Path | None = None) -> list[Finding]: out.append(warning("AM005", f"InternalLink {side} '{ref}' matches no interface ID in this file", link.line)) base = path.parent if path is not None else None - plcopen_ids: dict[Path, set[str] | None] = {} + plcopen_ids: dict[tuple[int, int] | Path, set[str] | None] = {} for iface in root.iter("ExternalInterface"): cls = iface.get("RefBaseClassPath") or "" if not any(cls.endswith(c) for c in DATA_CONNECTORS): @@ -88,14 +99,17 @@ def check_aml(root: Node, path: Path | None = None) -> list[Finding]: target, fragment = _resolve(value.text.strip(), base) if target is None: continue - if not target.exists(): + # os.path, not Path: on Python 3.9 Path.exists() raises ENAMETOOLONG for a name over the + # file system's limit, where os.path.exists() answers False. is_file() below only runs + # on a name that exists, so it cannot meet that error. + if not os.path.exists(target): out.append(warning("AM011", f"refURI target {target.name} not found next to this file", value.line)) elif fragment and cls.endswith("PLCopenXMLInterface") and target.is_file(): # An absolute refURI can name a directory, FIFO or device; only a regular file is read. - ids = _global_ids(target, plcopen_ids) - if ids is not None and fragment not in ids: + target_ids = _global_ids(target, plcopen_ids) + if target_ids is not None and fragment not in target_ids: out.append(warning("AM012", f"refURI fragment #{fragment} matches no globalId in {target.name}", value.line)) for ext in root.findall("ExternalReference"): - if base is not None and ext.get("Path") and not (base / ext.get("Path")).exists(): + if base is not None and ext.get("Path") and not os.path.exists(base / ext.get("Path")): out.append(warning("AM013", f"ExternalReference '{ext.get('Path')}' not found next to this file", ext.line)) return out diff --git a/tests/test_linear_aml.py b/tests/test_linear_aml.py index 202f750..b00e04f 100644 --- a/tests/test_linear_aml.py +++ b/tests/test_linear_aml.py @@ -7,6 +7,7 @@ from __future__ import annotations +import os import sys import tempfile import unittest @@ -21,6 +22,32 @@ from plccheck import aml # noqa: E402 from plccheck.xmlutil import parse_bytes # noqa: E402 + +class AmlFileKey(unittest.TestCase): + """What identifies a linked file: device and inode, else the resolved path.""" + + def stat(self, dev: int, ino: int) -> os.stat_result: + return os.stat_result((0o100644, ino, dev, 1, 0, 0, 10, 0, 0, 0)) + + def test_one_inode_on_one_device_is_one_file_whatever_the_path(self): + self.assertEqual(aml._file_key(Path("a.xml"), self.stat(1, 5)), aml._file_key(Path("b.xml"), self.stat(1, 5))) + + def test_the_same_inode_number_on_another_device_is_another_file(self): + self.assertNotEqual(aml._file_key(Path("a.xml"), self.stat(1, 5)), aml._file_key(Path("a.xml"), self.stat(2, 5))) + + def test_another_inode_on_one_device_is_another_file(self): + self.assertNotEqual(aml._file_key(Path("a.xml"), self.stat(1, 5)), aml._file_key(Path("a.xml"), self.stat(1, 6))) + + def test_a_file_system_without_inodes_falls_back_to_the_resolved_path(self): + with tempfile.TemporaryDirectory() as d: + base = Path(d) + (base / "sub").mkdir() + plain, dotted = base / "x.xml", base / "sub" / ".." / "x.xml" + self.assertEqual(aml._file_key(plain, self.stat(1, 0)), aml._file_key(dotted, self.stat(1, 0))) + self.assertEqual(aml._file_key(plain, self.stat(1, 0)), plain.resolve()) + self.assertNotEqual(aml._file_key(plain, self.stat(1, 0)), aml._file_key(base / "y.xml", self.stat(1, 0))) + + class AmlLinkedFileParsing(LinearTime): """A PLCopen file linked from many interfaces is parsed and walked once, not once per link.""" @@ -30,19 +57,25 @@ def interface(self, i: int, ref: str, cls: str = "AMLBase/PLCopenXMLInterface") return (f'' f'{ref}') - def aml_text(self, interfaces: list[str]) -> str: + def aml_text(self, interfaces: list[str], tail: str = "") -> str: return ('' - + "".join(interfaces) + "") + + "".join(interfaces) + "" + tail + "") - def check(self, d: str, refs: list[str], files: dict[str, str] | None = None) -> list[tuple[str, str]]: + def write(self, d: str, refs: list[str], files: dict[str, str] | None = None, tail: str = "") -> Path: base = Path(d) for name, text in {"plc.xml": self.PLCOPEN, **(files or {})}.items(): (base / name).parent.mkdir(exist_ok=True) (base / name).write_text(text, encoding="utf-8") path = base / "plant.aml" - path.write_text(self.aml_text([self.interface(i, ref) for i, ref in enumerate(refs)]), encoding="utf-8") + path.write_text(self.aml_text([self.interface(i, ref) for i, ref in enumerate(refs)], tail), encoding="utf-8") + return path + + def run_check(self, path: Path) -> list[tuple[str, str]]: return [(f.code, f.message) for f in aml.check_aml(parse_bytes(path.read_bytes()), path)] + def check(self, d: str, refs: list[str], files: dict[str, str] | None = None) -> list[tuple[str, str]]: + return self.run_check(self.write(d, refs, files)) + def test_check_time_grows_linearly_with_the_links_to_one_file(self): # Every link to the same file parsed it again, so n links to the file itself (n links # in it) cost n parses of an n-link document: quadratic (about 16x for 4x). @@ -61,6 +94,40 @@ def test_a_file_linked_many_times_is_parsed_once_whatever_the_spelling_of_its_pa self.assertEqual(parse.call_count, 1) self.assertEqual([code for code, _ in got], ["AM012"] * 4) + def test_a_hard_link_to_the_file_is_the_same_file(self): + with tempfile.TemporaryDirectory() as d, mock.patch.object(aml, "parse_file", wraps=aml.parse_file) as parse: + path = self.write(d, ["plc.xml#zz", "link.xml#zz"]) + try: + os.link(path.parent / "plc.xml", path.parent / "link.xml") + except (OSError, AttributeError, NotImplementedError) as exc: + self.skipTest(f"no hard links here: {exc}") + got = self.run_check(path) + self.assertEqual(parse.call_count, 1) + self.assertEqual([code for code, _ in got], ["AM012"] * 2) + + def test_another_spelling_of_the_name_on_a_case_insensitive_file_system_is_the_same_file(self): + with tempfile.TemporaryDirectory() as d, mock.patch.object(aml, "parse_file", wraps=aml.parse_file) as parse: + path = self.write(d, ["plc.xml#zz", "PLC.xml#zz", "pLc.XML#zz"]) + if not os.path.exists(path.parent / "PLC.XML"): + self.skipTest("case-sensitive file system") + got = self.run_check(path) + self.assertEqual(parse.call_count, 1) + self.assertEqual([code for code, _ in got], ["AM012"] * 3) + + def test_two_different_files_are_two_parses(self): + with tempfile.TemporaryDirectory() as d, mock.patch.object(aml, "parse_file", wraps=aml.parse_file) as parse: + self.check(d, ["plc.xml#zz", "copy.xml#zz"], {"copy.xml": self.PLCOPEN}) + self.assertEqual(parse.call_count, 2) + + def test_a_name_over_the_file_system_limit_is_a_missing_target_not_a_crash(self): + # Python 3.9's Path.exists() raises OSError (ENAMETOOLONG) for it, where 3.10 and later + # answer False: on a newer Python this passes with or without the os.path calls. + long_name = "a" * 300 + ".xml" + with tempfile.TemporaryDirectory() as d: + path = self.write(d, [f"{long_name}#zz"], tail=f'') + got = self.run_check(path) + self.assertEqual([code for code, _ in got], ["AM011", "AM013"]) + def test_a_malformed_file_is_parsed_once_and_gives_no_finding(self): with tempfile.TemporaryDirectory() as d, mock.patch.object(aml, "parse_file", wraps=aml.parse_file) as parse: got = self.check(d, ["bad.xml#a", "bad.xml#b", "bad.xml#c"], {"bad.xml": ""}) From 485ff7402f6fbd9276154ea97303d3f7c50ce1d7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Smail=20Galija=C5=A1evi=C4=87?= <32690613+SmailG@users.noreply.github.com> Date: Tue, 10 Nov 2026 17:13:02 +0100 Subject: [PATCH 23/29] test: two blank lines after the imports of the split scaling tests --- tests/test_linear_text.py | 1 + tests/test_linear_xml.py | 1 + 2 files changed, 2 insertions(+) diff --git a/tests/test_linear_text.py b/tests/test_linear_text.py index 11320dc..1584db2 100644 --- a/tests/test_linear_text.py +++ b/tests/test_linear_text.py @@ -23,6 +23,7 @@ from plccheck.simaticml import check_simatic_sd # noqa: E402 from plccheck.st import lint_st # noqa: E402 + class TwinCatCommentStripping(LinearTime): """Comment stripping scans once; an opener that never closes must not rescan the rest of the text.""" diff --git a/tests/test_linear_xml.py b/tests/test_linear_xml.py index f0c519c..054d9b3 100644 --- a/tests/test_linear_xml.py +++ b/tests/test_linear_xml.py @@ -22,6 +22,7 @@ from plccheck.schneider import check_control_expert # noqa: E402 from plccheck.simaticml import check_simaticml # noqa: E402 + class ControlExpertNestedDfb(LinearTime): """An STSource is linted once, for the nearest FBSource around it, not once per FBSource around it. From 40e095848d7025047bacc4b15639ea3f892e7ab1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Smail=20Galija=C5=A1evi=C4=87?= <32690613+SmailG@users.noreply.github.com> Date: Tue, 10 Nov 2026 17:19:06 +0100 Subject: [PATCH 24/29] test: cover an ATTLIST xmlns and correct figures in comments Add a test that a namespace declaration defaulted by an ATTLIST is capped too (no entity involved; expat reports a defaulted attribute to the start handler). Correct three comments to what was measured: the longest message from an ordinary name in the tests is 203 characters (not 195); the distinct-attribute input is 29 KB (not 33 KB); Path.exists() raises ENAMETOOLONG on Python 3.9.2 and 3.12.3 and answers False on 3.14.8. --- scripts/plccheck/findings.py | 3 ++- tests/test_finding_clip.py | 5 +++-- tests/test_linear_aml.py | 4 ++-- tests/test_xml_limits.py | 10 +++++++++- 4 files changed, 16 insertions(+), 6 deletions(-) diff --git a/scripts/plccheck/findings.py b/scripts/plccheck/findings.py index 1f4cf56..ce6362e 100644 --- a/scripts/plccheck/findings.py +++ b/scripts/plccheck/findings.py @@ -6,7 +6,8 @@ # A message or context quotes names from the file, and a context is repeated on every finding # under it, so one long name times many findings is quadratic in the file size. The longest -# message in the tests is 195 characters and the longest context 26. +# message the tests produce from ordinary names is 203 characters (ST030, with a 17-character +# instance name) and the longest context 26; messages that quote a 300-character file name reach 352. MAX_FINDING_TEXT = 400 ELLIPSIS = "…" diff --git a/tests/test_finding_clip.py b/tests/test_finding_clip.py index 278b6f6..edc3e7d 100644 --- a/tests/test_finding_clip.py +++ b/tests/test_finding_clip.py @@ -66,8 +66,9 @@ def test_a_real_long_message_is_kept_whole(self): self.assertTrue(f.message.endswith("gate its input instead")) def test_the_limit_is_well_above_the_messages_the_checkers_write(self): - # The longest message the tests produce is 195 characters (ST030 with two names), the longest - # template 183 before its two names; a limit near them would cut real findings. + # The longest message from an ordinary name is 203 characters (ST030 with a 17-character + # instance name, pinned above); the longest template is 183 before its two names. A limit + # near them would cut real findings. self.assertGreaterEqual(self.MAX, 300) def test_a_context_added_by_with_context_is_cut(self): diff --git a/tests/test_linear_aml.py b/tests/test_linear_aml.py index b00e04f..ac30f6a 100644 --- a/tests/test_linear_aml.py +++ b/tests/test_linear_aml.py @@ -120,8 +120,8 @@ def test_two_different_files_are_two_parses(self): self.assertEqual(parse.call_count, 2) def test_a_name_over_the_file_system_limit_is_a_missing_target_not_a_crash(self): - # Python 3.9's Path.exists() raises OSError (ENAMETOOLONG) for it, where 3.10 and later - # answer False: on a newer Python this passes with or without the os.path calls. + # Path.exists() raises OSError (ENAMETOOLONG) for it on Python 3.9.2 and 3.12.3 and answers + # False on 3.14.8 (the versions measured): on 3.14 this passes with or without os.path. long_name = "a" * 300 + ".xml" with tempfile.TemporaryDirectory() as d: path = self.write(d, [f"{long_name}#zz"], tail=f'') diff --git a/tests/test_xml_limits.py b/tests/test_xml_limits.py index 7dcad60..1b460b0 100644 --- a/tests/test_xml_limits.py +++ b/tests/test_xml_limits.py @@ -83,13 +83,21 @@ def test_a_long_default_namespace_over_many_elements_is_refused_in_little_memory def test_a_long_prefix_over_many_distinct_attributes_is_refused_in_little_memory(self): # The declaring element's own attributes each carry the URI too, so a cap in a namespace - # declaration handler would come too late. 33 KB: one 16,000-character URI, 1200 attributes. + # declaration handler would come too late. 29 KB: one 16,000-character URI, 1200 attributes. attrs = " ".join(f'p:a{i}="1"' for i in range(1200)) data = f''.encode() self.assertLess(len(data), 40_000) self.assertEqual(str(self.refused(data)), LONG_URI_ERROR) self.assertLess(self.peak(data), PEAK_LIMIT) + def test_a_declaration_defaulted_by_an_attlist_is_capped_too(self): + # No entity is involved: the DTD gives every a default xmlns, and expat reports a + # defaulted attribute to the start handler like a written one. + long_ = f']>' + self.assertEqual(str(self.refused(long_.encode())), LONG_URI_ERROR) + short = f']>' + self.assertEqual(xmlutil.parse_bytes(short.encode()).children[0].ns, uri(CAP)) + def test_the_cap_holds_in_utf16(self): # The check reads attribute values from the parser, so the encoding does not matter. data = f''.encode("utf-16") From 54bab21b956056f2254016c874a58667aaadac6b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Smail=20Galija=C5=A1evi=C4=87?= <32690613+SmailG@users.noreply.github.com> Date: Tue, 10 Nov 2026 17:20:30 +0100 Subject: [PATCH 25/29] Revert "test: cover an ATTLIST xmlns and correct figures in comments" This reverts commit 40e095848d7025047bacc4b15639ea3f892e7ab1. --- scripts/plccheck/findings.py | 3 +-- tests/test_finding_clip.py | 5 ++--- tests/test_linear_aml.py | 4 ++-- tests/test_xml_limits.py | 10 +--------- 4 files changed, 6 insertions(+), 16 deletions(-) diff --git a/scripts/plccheck/findings.py b/scripts/plccheck/findings.py index ce6362e..1f4cf56 100644 --- a/scripts/plccheck/findings.py +++ b/scripts/plccheck/findings.py @@ -6,8 +6,7 @@ # A message or context quotes names from the file, and a context is repeated on every finding # under it, so one long name times many findings is quadratic in the file size. The longest -# message the tests produce from ordinary names is 203 characters (ST030, with a 17-character -# instance name) and the longest context 26; messages that quote a 300-character file name reach 352. +# message in the tests is 195 characters and the longest context 26. MAX_FINDING_TEXT = 400 ELLIPSIS = "…" diff --git a/tests/test_finding_clip.py b/tests/test_finding_clip.py index edc3e7d..278b6f6 100644 --- a/tests/test_finding_clip.py +++ b/tests/test_finding_clip.py @@ -66,9 +66,8 @@ def test_a_real_long_message_is_kept_whole(self): self.assertTrue(f.message.endswith("gate its input instead")) def test_the_limit_is_well_above_the_messages_the_checkers_write(self): - # The longest message from an ordinary name is 203 characters (ST030 with a 17-character - # instance name, pinned above); the longest template is 183 before its two names. A limit - # near them would cut real findings. + # The longest message the tests produce is 195 characters (ST030 with two names), the longest + # template 183 before its two names; a limit near them would cut real findings. self.assertGreaterEqual(self.MAX, 300) def test_a_context_added_by_with_context_is_cut(self): diff --git a/tests/test_linear_aml.py b/tests/test_linear_aml.py index ac30f6a..b00e04f 100644 --- a/tests/test_linear_aml.py +++ b/tests/test_linear_aml.py @@ -120,8 +120,8 @@ def test_two_different_files_are_two_parses(self): self.assertEqual(parse.call_count, 2) def test_a_name_over_the_file_system_limit_is_a_missing_target_not_a_crash(self): - # Path.exists() raises OSError (ENAMETOOLONG) for it on Python 3.9.2 and 3.12.3 and answers - # False on 3.14.8 (the versions measured): on 3.14 this passes with or without os.path. + # Python 3.9's Path.exists() raises OSError (ENAMETOOLONG) for it, where 3.10 and later + # answer False: on a newer Python this passes with or without the os.path calls. long_name = "a" * 300 + ".xml" with tempfile.TemporaryDirectory() as d: path = self.write(d, [f"{long_name}#zz"], tail=f'') diff --git a/tests/test_xml_limits.py b/tests/test_xml_limits.py index 1b460b0..7dcad60 100644 --- a/tests/test_xml_limits.py +++ b/tests/test_xml_limits.py @@ -83,21 +83,13 @@ def test_a_long_default_namespace_over_many_elements_is_refused_in_little_memory def test_a_long_prefix_over_many_distinct_attributes_is_refused_in_little_memory(self): # The declaring element's own attributes each carry the URI too, so a cap in a namespace - # declaration handler would come too late. 29 KB: one 16,000-character URI, 1200 attributes. + # declaration handler would come too late. 33 KB: one 16,000-character URI, 1200 attributes. attrs = " ".join(f'p:a{i}="1"' for i in range(1200)) data = f''.encode() self.assertLess(len(data), 40_000) self.assertEqual(str(self.refused(data)), LONG_URI_ERROR) self.assertLess(self.peak(data), PEAK_LIMIT) - def test_a_declaration_defaulted_by_an_attlist_is_capped_too(self): - # No entity is involved: the DTD gives every a default xmlns, and expat reports a - # defaulted attribute to the start handler like a written one. - long_ = f']>' - self.assertEqual(str(self.refused(long_.encode())), LONG_URI_ERROR) - short = f']>' - self.assertEqual(xmlutil.parse_bytes(short.encode()).children[0].ns, uri(CAP)) - def test_the_cap_holds_in_utf16(self): # The check reads attribute values from the parser, so the encoding does not matter. data = f''.encode("utf-16") From 1a4a0669e7dc8d2c3aa7e4a54c06549069950553 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Smail=20Galija=C5=A1evi=C4=87?= <32690613+SmailG@users.noreply.github.com> Date: Tue, 10 Nov 2026 17:20:30 +0100 Subject: [PATCH 26/29] Revert "fix: identify an AutomationML link target by inode and keep long names safe" This reverts commit 0619029c7d0fdb68ba01d60355b228eba9757dd2. --- scripts/plccheck/aml.py | 28 ++++----------- tests/test_linear_aml.py | 75 +++------------------------------------- 2 files changed, 11 insertions(+), 92 deletions(-) diff --git a/scripts/plccheck/aml.py b/scripts/plccheck/aml.py index 442e2d7..4a53d2c 100644 --- a/scripts/plccheck/aml.py +++ b/scripts/plccheck/aml.py @@ -2,7 +2,6 @@ from __future__ import annotations -import os from pathlib import Path from urllib.parse import unquote, urlparse @@ -22,23 +21,13 @@ def _resolve(ref: str, base: Path) -> tuple[Path | None, str]: return (base / p) if p else None, u.fragment -def _file_key(target: Path, st: os.stat_result) -> tuple[int, int] | Path: - """What identifies the file behind `target`: its device and inode, else its resolved path. - - The resolved path misses one file under another spelling of its name (`X.XML` for `x.xml` on a - case-insensitive file system) and a hard link; the inode does not. A file system that reports - no inode (0) falls back to the resolved path, which still joins `./x.xml` and `a/../x.xml`. - """ - return (st.st_dev, st.st_ino) if st.st_ino else target.resolve() - - -def _global_ids(target: Path, cache: dict[tuple[int, int] | Path, set[str] | None]) -> set[str] | None: +def _global_ids(target: Path, cache: dict[Path, set[str] | None]) -> set[str] | None: """The globalId and uuid values of a referenced PLCopen file, or None if it is not well-formed XML. Parsed once per file: every interface that links to the same file would otherwise parse and walk it again, so a file of n links to itself costs n parses of an n-link document. """ - key = _file_key(target, target.stat()) + key = target.resolve() # `x.xml`, `./x.xml` and `a/../x.xml` are one file if key not in cache: try: doc = parse_file(target) @@ -84,7 +73,7 @@ def check_aml(root: Node, path: Path | None = None) -> list[Finding]: out.append(warning("AM005", f"InternalLink {side} '{ref}' matches no interface ID in this file", link.line)) base = path.parent if path is not None else None - plcopen_ids: dict[tuple[int, int] | Path, set[str] | None] = {} + plcopen_ids: dict[Path, set[str] | None] = {} for iface in root.iter("ExternalInterface"): cls = iface.get("RefBaseClassPath") or "" if not any(cls.endswith(c) for c in DATA_CONNECTORS): @@ -99,17 +88,14 @@ def check_aml(root: Node, path: Path | None = None) -> list[Finding]: target, fragment = _resolve(value.text.strip(), base) if target is None: continue - # os.path, not Path: on Python 3.9 Path.exists() raises ENAMETOOLONG for a name over the - # file system's limit, where os.path.exists() answers False. is_file() below only runs - # on a name that exists, so it cannot meet that error. - if not os.path.exists(target): + if not target.exists(): out.append(warning("AM011", f"refURI target {target.name} not found next to this file", value.line)) elif fragment and cls.endswith("PLCopenXMLInterface") and target.is_file(): # An absolute refURI can name a directory, FIFO or device; only a regular file is read. - target_ids = _global_ids(target, plcopen_ids) - if target_ids is not None and fragment not in target_ids: + ids = _global_ids(target, plcopen_ids) + if ids is not None and fragment not in ids: out.append(warning("AM012", f"refURI fragment #{fragment} matches no globalId in {target.name}", value.line)) for ext in root.findall("ExternalReference"): - if base is not None and ext.get("Path") and not os.path.exists(base / ext.get("Path")): + if base is not None and ext.get("Path") and not (base / ext.get("Path")).exists(): out.append(warning("AM013", f"ExternalReference '{ext.get('Path')}' not found next to this file", ext.line)) return out diff --git a/tests/test_linear_aml.py b/tests/test_linear_aml.py index b00e04f..202f750 100644 --- a/tests/test_linear_aml.py +++ b/tests/test_linear_aml.py @@ -7,7 +7,6 @@ from __future__ import annotations -import os import sys import tempfile import unittest @@ -22,32 +21,6 @@ from plccheck import aml # noqa: E402 from plccheck.xmlutil import parse_bytes # noqa: E402 - -class AmlFileKey(unittest.TestCase): - """What identifies a linked file: device and inode, else the resolved path.""" - - def stat(self, dev: int, ino: int) -> os.stat_result: - return os.stat_result((0o100644, ino, dev, 1, 0, 0, 10, 0, 0, 0)) - - def test_one_inode_on_one_device_is_one_file_whatever_the_path(self): - self.assertEqual(aml._file_key(Path("a.xml"), self.stat(1, 5)), aml._file_key(Path("b.xml"), self.stat(1, 5))) - - def test_the_same_inode_number_on_another_device_is_another_file(self): - self.assertNotEqual(aml._file_key(Path("a.xml"), self.stat(1, 5)), aml._file_key(Path("a.xml"), self.stat(2, 5))) - - def test_another_inode_on_one_device_is_another_file(self): - self.assertNotEqual(aml._file_key(Path("a.xml"), self.stat(1, 5)), aml._file_key(Path("a.xml"), self.stat(1, 6))) - - def test_a_file_system_without_inodes_falls_back_to_the_resolved_path(self): - with tempfile.TemporaryDirectory() as d: - base = Path(d) - (base / "sub").mkdir() - plain, dotted = base / "x.xml", base / "sub" / ".." / "x.xml" - self.assertEqual(aml._file_key(plain, self.stat(1, 0)), aml._file_key(dotted, self.stat(1, 0))) - self.assertEqual(aml._file_key(plain, self.stat(1, 0)), plain.resolve()) - self.assertNotEqual(aml._file_key(plain, self.stat(1, 0)), aml._file_key(base / "y.xml", self.stat(1, 0))) - - class AmlLinkedFileParsing(LinearTime): """A PLCopen file linked from many interfaces is parsed and walked once, not once per link.""" @@ -57,25 +30,19 @@ def interface(self, i: int, ref: str, cls: str = "AMLBase/PLCopenXMLInterface") return (f'' f'{ref}') - def aml_text(self, interfaces: list[str], tail: str = "") -> str: + def aml_text(self, interfaces: list[str]) -> str: return ('' - + "".join(interfaces) + "" + tail + "") + + "".join(interfaces) + "") - def write(self, d: str, refs: list[str], files: dict[str, str] | None = None, tail: str = "") -> Path: + def check(self, d: str, refs: list[str], files: dict[str, str] | None = None) -> list[tuple[str, str]]: base = Path(d) for name, text in {"plc.xml": self.PLCOPEN, **(files or {})}.items(): (base / name).parent.mkdir(exist_ok=True) (base / name).write_text(text, encoding="utf-8") path = base / "plant.aml" - path.write_text(self.aml_text([self.interface(i, ref) for i, ref in enumerate(refs)], tail), encoding="utf-8") - return path - - def run_check(self, path: Path) -> list[tuple[str, str]]: + path.write_text(self.aml_text([self.interface(i, ref) for i, ref in enumerate(refs)]), encoding="utf-8") return [(f.code, f.message) for f in aml.check_aml(parse_bytes(path.read_bytes()), path)] - def check(self, d: str, refs: list[str], files: dict[str, str] | None = None) -> list[tuple[str, str]]: - return self.run_check(self.write(d, refs, files)) - def test_check_time_grows_linearly_with_the_links_to_one_file(self): # Every link to the same file parsed it again, so n links to the file itself (n links # in it) cost n parses of an n-link document: quadratic (about 16x for 4x). @@ -94,40 +61,6 @@ def test_a_file_linked_many_times_is_parsed_once_whatever_the_spelling_of_its_pa self.assertEqual(parse.call_count, 1) self.assertEqual([code for code, _ in got], ["AM012"] * 4) - def test_a_hard_link_to_the_file_is_the_same_file(self): - with tempfile.TemporaryDirectory() as d, mock.patch.object(aml, "parse_file", wraps=aml.parse_file) as parse: - path = self.write(d, ["plc.xml#zz", "link.xml#zz"]) - try: - os.link(path.parent / "plc.xml", path.parent / "link.xml") - except (OSError, AttributeError, NotImplementedError) as exc: - self.skipTest(f"no hard links here: {exc}") - got = self.run_check(path) - self.assertEqual(parse.call_count, 1) - self.assertEqual([code for code, _ in got], ["AM012"] * 2) - - def test_another_spelling_of_the_name_on_a_case_insensitive_file_system_is_the_same_file(self): - with tempfile.TemporaryDirectory() as d, mock.patch.object(aml, "parse_file", wraps=aml.parse_file) as parse: - path = self.write(d, ["plc.xml#zz", "PLC.xml#zz", "pLc.XML#zz"]) - if not os.path.exists(path.parent / "PLC.XML"): - self.skipTest("case-sensitive file system") - got = self.run_check(path) - self.assertEqual(parse.call_count, 1) - self.assertEqual([code for code, _ in got], ["AM012"] * 3) - - def test_two_different_files_are_two_parses(self): - with tempfile.TemporaryDirectory() as d, mock.patch.object(aml, "parse_file", wraps=aml.parse_file) as parse: - self.check(d, ["plc.xml#zz", "copy.xml#zz"], {"copy.xml": self.PLCOPEN}) - self.assertEqual(parse.call_count, 2) - - def test_a_name_over_the_file_system_limit_is_a_missing_target_not_a_crash(self): - # Python 3.9's Path.exists() raises OSError (ENAMETOOLONG) for it, where 3.10 and later - # answer False: on a newer Python this passes with or without the os.path calls. - long_name = "a" * 300 + ".xml" - with tempfile.TemporaryDirectory() as d: - path = self.write(d, [f"{long_name}#zz"], tail=f'') - got = self.run_check(path) - self.assertEqual([code for code, _ in got], ["AM011", "AM013"]) - def test_a_malformed_file_is_parsed_once_and_gives_no_finding(self): with tempfile.TemporaryDirectory() as d, mock.patch.object(aml, "parse_file", wraps=aml.parse_file) as parse: got = self.check(d, ["bad.xml#a", "bad.xml#b", "bad.xml#c"], {"bad.xml": ""}) From ff8957da628e0e4f49461c34249acdf7ac49376b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Smail=20Galija=C5=A1evi=C4=87?= <32690613+SmailG@users.noreply.github.com> Date: Tue, 10 Nov 2026 17:20:30 +0100 Subject: [PATCH 27/29] Revert "fix: collect SimaticML namespaces in a dict in document order" This reverts commit 68b46326762ee67d88f9c58feb3c3ae196d92718. --- scripts/plccheck/simaticml.py | 8 ++++---- tests/test_linear_xml.py | 34 ---------------------------------- 2 files changed, 4 insertions(+), 38 deletions(-) diff --git a/scripts/plccheck/simaticml.py b/scripts/plccheck/simaticml.py index fd9141f..db73294 100644 --- a/scripts/plccheck/simaticml.py +++ b/scripts/plccheck/simaticml.py @@ -52,12 +52,12 @@ def check_simaticml(root: Node) -> list[Finding]: if not tia: out.append(warning("SM002", "no ; TIA cannot tell which schema generation this is", root.line)) expected = EXPECTED_NS.get(tia, {}) - seen_ns: dict[tuple[str, str], int] = {} # (family, version) -> line of its first element, in document order + seen_ns: set[tuple[str, str, int]] = set() for node in root.iter(): m = _NS_RE.match(node.ns) if node.ns else None - if m: - seen_ns.setdefault((m.group("family"), m.group("ver")), node.line) - for (family, ver), line in seen_ns.items(): + if m and (m.group("family"), m.group("ver")) not in {(f, v) for f, v, _ in seen_ns}: + seen_ns.add((m.group("family"), m.group("ver"), node.line)) + for family, ver, line in sorted(seen_ns, key=lambda x: x[2]): want = expected.get(family) if want and want != ver: out.append(warning("SM003", f"{family} namespace is {ver} but TIA {tia} exports use {want}; import may be rejected", line)) diff --git a/tests/test_linear_xml.py b/tests/test_linear_xml.py index 054d9b3..e134cc7 100644 --- a/tests/test_linear_xml.py +++ b/tests/test_linear_xml.py @@ -206,40 +206,6 @@ def test_rungs_of_a_nested_routines_scope_belong_to_it(self): self.assertEqual(got, [("LX020", 1, "p/a rung 0"), ("LX020", 1, "a/in rung 0")]) -class SimaticMlNamespaces(LinearTime): - """Each (family, version) namespace is reported once, at its first element, in document order.""" - - BASE = "http://www.siemens.com/automation/Openness/SW/" - - def check(self, body: str, declarations: str = "") -> list[tuple[str, int, str]]: - doc = f'{body}' - return [(f.code, f.line, f.message) for f in check_simaticml(xml_root(doc)) if f.code == "SM003"] - - def test_check_time_grows_linearly_with_the_namespaces_and_elements_together(self): - # k distinct namespaces over 10k elements: rebuilding the set of seen (family, version) - # pairs for every element is k times m, quadratic when both grow (about 16x for 4x). - def make(n): - declarations = " ".join(f'xmlns:p{i}="{self.BASE}Interface/v{i + 10}"' for i in range(n)) - root = xml_root(f'' - + "".join(f"" for i in range(10 * n)) + "") - return lambda: check_simaticml(root) - self.assert_linear(make, 250, 1_000) - - def test_a_namespace_is_reported_once_at_its_first_element(self): - ns = f'xmlns="{self.BASE}NetworkSource/FlgNet/v3"' - got = self.check(f"\n\n\n") - self.assertEqual(got, [("SM003", 2, "NetworkSource/FlgNet namespace is v3 but TIA V18 exports use v4; import may be rejected")]) - - def test_a_matching_namespace_is_not_reported(self): - self.assertEqual(self.check(f''), []) - - def test_namespaces_on_one_line_are_reported_in_document_order(self): - # Ties on the line used to fall to the iteration order of a set, which changes with the hash seed. - versions = [9, 1, 8, 2, 7, 3, 6, 4] # none is v5, the version V18 expects of Interface - got = self.check("".join(f'' for v in versions)) - self.assertEqual([(line, msg.split()[3]) for _, line, msg in got], [(1, f"v{v}") for v in versions]) - - class SimaticMlNestedCompileUnits(LinearTime): """A network payload is checked for the CompileUnit it is in, not once for every CompileUnit around it.""" From 32270100f4d4b485133c2cbee3a45d6c51c51174 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Smail=20Galija=C5=A1evi=C4=87?= <32690613+SmailG@users.noreply.github.com> Date: Tue, 10 Nov 2026 17:20:30 +0100 Subject: [PATCH 28/29] Revert "fix: cut a finding's message and context at 400 characters" This reverts commit 900bbe3ca7737fd878078c71a88ca2700c040bad. --- CONTRIBUTING.md | 2 +- scripts/plccheck/findings.py | 19 ------- tests/test_finding_clip.py | 102 ----------------------------------- 3 files changed, 1 insertion(+), 122 deletions(-) delete mode 100644 tests/test_finding_clip.py diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 84e8257..ce01dde 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -14,7 +14,7 @@ privately, as described in [SECURITY.md](SECURITY.md). Pull requests use the tem | `scripts/plc_validate.py` | Validator CLI and hook entry point | | `scripts/codesys_project.py` | Offline reader for CODESYS 3.5 / Machine Expert `.project` files (`list`, `st`, `dump-st`, `graph`, `info`, `diff`, `xref`); `scripts/codesysproj/` holds `binary.py` (varint, string table), `project.py` (objects, kinds, text), `graph.py` (LD/FBD records) and `graphview.py` (their text and JSON), `export.py` (`dump-st`), `buildinfo.py` (`info`), `diff.py` (`diff`), `xref.py` and `stscan.py` (`xref`) | | `scripts/plccheck/` | One module per format (`st.py`, `plcopen.py`, `simaticml.py`, `twincat.py`, `logix.py`, `schneider.py`, `iec61499.py`, `aml.py`, `dexpi.py`, `tabular.py`); `findings.py` holds the finding type, `xmlutil.py` the line-numbered XML parser | -| `tests/` | `test_st.py`, `test_formats.py` (validator rules), `test_codesys_project.py` and `test_codesys_tools.py` (the `.project` reader and its `info`, `diff` and `xref`, on synthetic projects only), `test_codesys_hardening.py` (hostile and damaged `.project` input and the reader's CLI contracts, synthetic projects only), `test_linear_xml.py`, `test_linear_aml.py` and `test_linear_text.py` (CPU-time scaling and ordering of the format checkers on crafted input; `linear_support.py` holds their ratio helper and XML builders), `test_xml_limits.py` (the namespace URI cap of the shared XML parser), `test_finding_clip.py` (the length cap on a finding's message and context), `test_skills.py` (frontmatter, templates, every `iecst` block lints clean) | +| `tests/` | `test_st.py`, `test_formats.py` (validator rules), `test_codesys_project.py` and `test_codesys_tools.py` (the `.project` reader and its `info`, `diff` and `xref`, on synthetic projects only), `test_codesys_hardening.py` (hostile and damaged `.project` input and the reader's CLI contracts, synthetic projects only), `test_linear_xml.py`, `test_linear_aml.py` and `test_linear_text.py` (CPU-time scaling and ordering of the format checkers on crafted input; `linear_support.py` holds their ratio helper and XML builders), `test_xml_limits.py` (the namespace URI cap of the shared XML parser), `test_skills.py` (frontmatter, templates, every `iecst` block lints clean) | | `evals/` | `claude plugin eval` cases comparing the plugin against a no-plugin baseline | ## Rules diff --git a/scripts/plccheck/findings.py b/scripts/plccheck/findings.py index 1f4cf56..aec446c 100644 --- a/scripts/plccheck/findings.py +++ b/scripts/plccheck/findings.py @@ -4,19 +4,6 @@ from dataclasses import dataclass -# A message or context quotes names from the file, and a context is repeated on every finding -# under it, so one long name times many findings is quadratic in the file size. The longest -# message in the tests is 195 characters and the longest context 26. -MAX_FINDING_TEXT = 400 -ELLIPSIS = "…" - - -def _clip(text: str) -> str: - """`text` as is, or cut to MAX_FINDING_TEXT characters, the last being an ellipsis.""" - if len(text) <= MAX_FINDING_TEXT: - return text - return text[: MAX_FINDING_TEXT - len(ELLIPSIS)] + ELLIPSIS - @dataclass(frozen=True) class Finding: @@ -26,12 +13,6 @@ class Finding: line: int | None = None context: str | None = None # e.g. "POU FB_Motor body (ST)" - def __post_init__(self) -> None: - # Every finding is built through here (error(), warning() and with_context() end in Finding(...)). - object.__setattr__(self, "message", _clip(self.message)) - if self.context is not None: - object.__setattr__(self, "context", _clip(self.context)) - def format(self, path: str) -> str: where = f"{path}:{self.line}" if self.line else path ctx = f" [{self.context}]" if self.context else "" diff --git a/tests/test_finding_clip.py b/tests/test_finding_clip.py deleted file mode 100644 index 278b6f6..0000000 --- a/tests/test_finding_clip.py +++ /dev/null @@ -1,102 +0,0 @@ -"""A finding's message and context are cut to MAX_FINDING_TEXT, in the one place every finding is built. - -A name from the file is quoted in the context of every finding under it (a Logix program name in -every rung finding), so a 2,000-character name over 40 rungs printed 85 KB and a 20,000-character -one over 400 rungs printed 8 MB, growing with the square of the file size. -""" - -from __future__ import annotations - -import sys -import unittest -from pathlib import Path - -HERE = Path(__file__).resolve().parent -sys.path.insert(0, str(HERE.parent / "scripts")) -sys.path.insert(0, str(HERE)) - -from linear_support import xml_root # noqa: E402 -from plccheck import findings # noqa: E402 -from plccheck.logix import check_l5x # noqa: E402 -from plccheck.st import check_st_file # noqa: E402 - - -def l5x(program: str, rungs: int) -> str: - body = "".join(f'XIC(a)' for i in range(rungs)) - return ('' - f'{body}' - "") - - -def formatted(program: str, rungs: int) -> str: - return "\n".join(f.format("a.L5X") for f in check_l5x(xml_root(l5x(program, rungs)))) - - -class FindingClip(unittest.TestCase): - MAX = findings.MAX_FINDING_TEXT - - def assert_clipped(self, text: str): - self.assertEqual(len(text), self.MAX) - self.assertTrue(text.endswith(findings.ELLIPSIS)) - self.assertEqual(text[:-1], "x" * (self.MAX - 1)) - - def test_a_text_up_to_the_limit_is_kept_and_a_longer_one_is_cut_to_it(self): - exact, over = "x" * self.MAX, "x" * (self.MAX + 1) - f = findings.error("ST002", exact, 3, exact) - self.assertEqual((f.message, f.context), (exact, exact)) - f = findings.error("ST002", over, 3, over) - self.assert_clipped(f.message) - self.assert_clipped(f.context) - - def test_message_and_context_are_cut_on_their_own(self): - over = "x" * (self.MAX + 1) - only_message = findings.warning("ST002", over, 3, "short") - self.assert_clipped(only_message.message) - self.assertEqual(only_message.context, "short") - only_context = findings.warning("ST002", "short", 3, over) - self.assertEqual(only_context.message, "short") - self.assert_clipped(only_context.context) - self.assertIsNone(findings.error("ST002", "short").context) - - def test_a_real_long_message_is_kept_whole(self): - src = ("PROGRAM P\nVAR\n tonWaitForTheBelt : TON;\n x : BOOL;\nEND_VAR\n" - "IF x THEN\n tonWaitForTheBelt(IN := TRUE, PT := T#5S);\nEND_IF;\nEND_PROGRAM\n") - [f] = check_st_file(src) - self.assertEqual((f.code, len(f.message)), ("ST030", 203)) - self.assertTrue(f.message.endswith("gate its input instead")) - - def test_the_limit_is_well_above_the_messages_the_checkers_write(self): - # The longest message the tests produce is 195 characters (ST030 with two names), the longest - # template 183 before its two names; a limit near them would cut real findings. - self.assertGreaterEqual(self.MAX, 300) - - def test_a_context_added_by_with_context_is_cut(self): - inner = findings.error("ST002", "m", 4, "y" * 300) - [out] = findings.with_context([inner], "x" * 300, 10) - self.assertEqual((out.line, len(out.context)), (14, self.MAX)) - self.assertTrue(out.context.endswith(findings.ELLIPSIS)) - - def test_the_formatted_and_dict_forms_carry_the_cut_text(self): - f = findings.error("ST002", "x" * 1000, 3, "y" * 1000) - self.assertEqual(len(f.format("p")), len("p:3: error ST002: []") + 2 * self.MAX) - d = f.as_dict("p") - self.assertEqual((len(d["message"]), len(d["context"])), (self.MAX, self.MAX)) - - def test_a_short_name_gives_the_same_output_as_before(self): - self.assertEqual(formatted("p", 2), "\n".join( - f"a.L5X:1: error LX020: rung text must end with ';' [p/r rung {i}]" for i in range(2))) - - def test_a_long_program_name_does_not_make_the_output_grow_with_it(self): - short, long_, longer = formatted("p" * 10, 40), formatted("p" * 2_000, 40), formatted("p" * 4_000, 40) - self.assertEqual(len(long_), len(longer)) # the same output for any name over the limit - self.assertLess(len(long_), 40 * (self.MAX + 100)) # was 85 KB for 40 rungs - self.assertGreater(len(long_), len(short)) - self.assertEqual(long_.splitlines()[0].count(findings.ELLIPSIS), 1) - - def test_the_output_grows_linearly_with_the_rung_count_for_a_long_name(self): - small, big = len(formatted("p" * 2_000, 20)), len(formatted("p" * 2_000, 80)) - self.assertLess(big, 4.2 * small) # not exactly 4x: the rung numbers have more digits - - -if __name__ == "__main__": - unittest.main() From 68b530feb0946876e9c115f06ba05bf03aa6eb28 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Smail=20Galija=C5=A1evi=C4=87?= <32690613+SmailG@users.noreply.github.com> Date: Tue, 10 Nov 2026 17:20:30 +0100 Subject: [PATCH 29/29] Revert "fix: refuse a namespace URI over 256 characters before parsing" This reverts commit cdc6d23a4b60a7f4adce687fb2fdedee496b6316. --- CONTRIBUTING.md | 2 +- scripts/plccheck/xmlutil.py | 42 ++----------- tests/test_xml_limits.py | 121 ------------------------------------ 3 files changed, 6 insertions(+), 159 deletions(-) delete mode 100644 tests/test_xml_limits.py diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index ce01dde..7e1abd8 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -14,7 +14,7 @@ privately, as described in [SECURITY.md](SECURITY.md). Pull requests use the tem | `scripts/plc_validate.py` | Validator CLI and hook entry point | | `scripts/codesys_project.py` | Offline reader for CODESYS 3.5 / Machine Expert `.project` files (`list`, `st`, `dump-st`, `graph`, `info`, `diff`, `xref`); `scripts/codesysproj/` holds `binary.py` (varint, string table), `project.py` (objects, kinds, text), `graph.py` (LD/FBD records) and `graphview.py` (their text and JSON), `export.py` (`dump-st`), `buildinfo.py` (`info`), `diff.py` (`diff`), `xref.py` and `stscan.py` (`xref`) | | `scripts/plccheck/` | One module per format (`st.py`, `plcopen.py`, `simaticml.py`, `twincat.py`, `logix.py`, `schneider.py`, `iec61499.py`, `aml.py`, `dexpi.py`, `tabular.py`); `findings.py` holds the finding type, `xmlutil.py` the line-numbered XML parser | -| `tests/` | `test_st.py`, `test_formats.py` (validator rules), `test_codesys_project.py` and `test_codesys_tools.py` (the `.project` reader and its `info`, `diff` and `xref`, on synthetic projects only), `test_codesys_hardening.py` (hostile and damaged `.project` input and the reader's CLI contracts, synthetic projects only), `test_linear_xml.py`, `test_linear_aml.py` and `test_linear_text.py` (CPU-time scaling and ordering of the format checkers on crafted input; `linear_support.py` holds their ratio helper and XML builders), `test_xml_limits.py` (the namespace URI cap of the shared XML parser), `test_skills.py` (frontmatter, templates, every `iecst` block lints clean) | +| `tests/` | `test_st.py`, `test_formats.py` (validator rules), `test_codesys_project.py` and `test_codesys_tools.py` (the `.project` reader and its `info`, `diff` and `xref`, on synthetic projects only), `test_codesys_hardening.py` (hostile and damaged `.project` input and the reader's CLI contracts, synthetic projects only), `test_linear_xml.py`, `test_linear_aml.py` and `test_linear_text.py` (CPU-time scaling and ordering of the format checkers on crafted input; `linear_support.py` holds their ratio helper and XML builders), `test_skills.py` (frontmatter, templates, every `iecst` block lints clean) | | `evals/` | `claude plugin eval` cases comparing the plugin against a no-plugin baseline | ## Rules diff --git a/scripts/plccheck/xmlutil.py b/scripts/plccheck/xmlutil.py index 5f5f779..0c55b02 100644 --- a/scripts/plccheck/xmlutil.py +++ b/scripts/plccheck/xmlutil.py @@ -5,10 +5,6 @@ entity declarations precede the root element, the format sniffer stops at the first one too: a file that declares an entity is not detected as any format, so the edit-time hook skips it, and validating it with an explicit format reports XML001. - -A namespace URI longer than MAX_NS_URI is refused as well: expat copies the URI into every element -and namespaced attribute name under its declaration, so one long URI multiplies the memory by the -element count (a 35 KB file reached 82 MB). The longest URI the checkers know is 77 characters. """ from __future__ import annotations @@ -72,40 +68,7 @@ def __init__(self, message: str, line: int | None): self.line = line -MAX_NS_URI = 256 - - -def _refuse_long_namespaces(data: bytes) -> None: - """Raise XmlError for a declaration of a namespace URI over MAX_NS_URI, before the real parse. - - A parser without namespace processing keeps `xmlns` attributes as written and expands no URI - into names, so this pass costs about one more parse of the file. A handler for namespace - declarations in the real parser would be too late: the declaring element's own attributes are - already expanded by then. It still expands entities in attribute values, so it is where entity - declarations are refused: they precede the root element, so none is expanded before it raises, - and the real parse never sees one. What it cannot parse is left for the real parse to report. - """ - parser = xml.parsers.expat.ParserCreate() - - def start(_name, attrs): - for name, value in attrs.items(): - if (name == "xmlns" or name.startswith("xmlns:")) and len(value) > MAX_NS_URI: - raise XmlError(f"namespace URI longer than {MAX_NS_URI} characters", parser.CurrentLineNumber) - - def entity_decl(*_args): - # Expanding entities makes the work grow with the nesting, not the file size. - raise XmlError("XML entity declarations are not supported", parser.CurrentLineNumber) - - parser.StartElementHandler = start - parser.EntityDeclHandler = entity_decl - try: - parser.Parse(data, True) - except xml.parsers.expat.ExpatError: - pass - - def parse_bytes(data: bytes) -> Node: - _refuse_long_namespaces(data) parser = xml.parsers.expat.ParserCreate(namespace_separator="}") parser.SetParamEntityParsing(xml.parsers.expat.XML_PARAM_ENTITY_PARSING_NEVER) root: list[Node] = [] @@ -138,9 +101,14 @@ def chars(data): stack[-1].text_line = parser.CurrentLineNumber chunks[-1].append(data) + def entity_decl(*_args): + # Expanding entities makes the work grow with the nesting, not the file size. + raise XmlError("XML entity declarations are not supported", parser.CurrentLineNumber) + parser.StartElementHandler = start parser.EndElementHandler = end parser.CharacterDataHandler = chars + parser.EntityDeclHandler = entity_decl try: parser.Parse(data, True) except xml.parsers.expat.ExpatError as exc: diff --git a/tests/test_xml_limits.py b/tests/test_xml_limits.py deleted file mode 100644 index 7dcad60..0000000 --- a/tests/test_xml_limits.py +++ /dev/null @@ -1,121 +0,0 @@ -"""The shared XML parser refuses what multiplies its memory: a long namespace URI. - -Expat copies a namespace URI into every element name and namespaced attribute name, so one -declaration of a long URI costs its length times the number of elements or attributes under it -(a 35 KB file reached 82 MB). Real URIs are short (the longest one the checkers know is 77 -characters, a Siemens StructuredText namespace), so a declaration over MAX_NS_URI is refused. -""" - -from __future__ import annotations - -import sys -import tempfile -import tracemalloc -import unittest -from pathlib import Path - -HERE = Path(__file__).resolve().parent -sys.path.insert(0, str(HERE.parent / "scripts")) - -import plc_validate # noqa: E402 -from plccheck import simaticml, xmlutil # noqa: E402 - -SIEMENS_URI = simaticml.NS_BASE + "NetworkSource/StructuredText/v4" -CAP = 256 -PEAK_LIMIT = 5_000_000 # bytes; the unfixed parser peaks at tens of MB on the shapes below -LONG_URI_ERROR = "namespace URI longer than 256 characters" -ENTITY_ERROR = "XML entity declarations are not supported" - - -def uri(length: int) -> str: - return "http://x/" + "a" * (length - len("http://x/")) - - -class NamespaceUriCap(unittest.TestCase): - def refused(self, data: bytes) -> xmlutil.XmlError: - with self.assertRaises(xmlutil.XmlError) as ctx: - xmlutil.parse_bytes(data) - return ctx.exception - - def peak(self, data: bytes) -> int: - tracemalloc.start() - try: - try: - xmlutil.parse_bytes(data) - except xmlutil.XmlError: - pass - return tracemalloc.get_traced_memory()[1] - finally: - tracemalloc.stop() - - def test_the_cap_is_well_above_the_longest_namespace_the_checkers_know(self): - known = [simaticml.NS_BASE + f"{family}/{ver}" for exp in simaticml.EXPECTED_NS.values() for family, ver in exp.items()] - self.assertEqual(max(map(len, known)), 77) - self.assertEqual(xmlutil.MAX_NS_URI, CAP) - - def test_uris_up_to_the_cap_parse_and_longer_ones_are_refused(self): - for length in (len(SIEMENS_URI), CAP): - with self.subTest(length=length): - root = xmlutil.parse_bytes(f''.encode()) - self.assertEqual((root.ns, root.children[0].ns), (uri(length), uri(length))) - exc = self.refused(f''.encode()) - self.assertEqual(str(exc), LONG_URI_ERROR) - - def test_a_prefixed_declaration_is_capped_too(self): - self.assertEqual(xmlutil.parse_bytes(f''.encode()).children[0].ns, uri(CAP)) - self.assertEqual(str(self.refused(f''.encode())), LONG_URI_ERROR) - - def test_a_long_value_of_an_attribute_that_only_starts_with_xmlns_is_not_a_declaration(self): - for name in ("xmlnsx", "xmlns_p", "p:xmlns"): - with self.subTest(attribute=name): - self.assertEqual(xmlutil.parse_bytes(f''.encode()).tag, "r") - - def test_the_error_names_the_line_of_the_declaring_element(self): - exc = self.refused(f'\n\n'.encode()) - self.assertEqual((str(exc), exc.line), (LONG_URI_ERROR, 3)) - - def test_a_long_default_namespace_over_many_elements_is_refused_in_little_memory(self): - # 36 KB: one 20,000-character URI, 4000 empty children (82 MB unfixed). - data = f'{"" * 4000}'.encode() - self.assertLess(len(data), 40_000) - self.assertEqual(str(self.refused(data)), LONG_URI_ERROR) - self.assertLess(self.peak(data), PEAK_LIMIT) - - def test_a_long_prefix_over_many_distinct_attributes_is_refused_in_little_memory(self): - # The declaring element's own attributes each carry the URI too, so a cap in a namespace - # declaration handler would come too late. 33 KB: one 16,000-character URI, 1200 attributes. - attrs = " ".join(f'p:a{i}="1"' for i in range(1200)) - data = f''.encode() - self.assertLess(len(data), 40_000) - self.assertEqual(str(self.refused(data)), LONG_URI_ERROR) - self.assertLess(self.peak(data), PEAK_LIMIT) - - def test_the_cap_holds_in_utf16(self): - # The check reads attribute values from the parser, so the encoding does not matter. - data = f''.encode("utf-16") - self.assertEqual(str(self.refused(data)), LONG_URI_ERROR) - ok = f''.encode("utf-16") - self.assertEqual(xmlutil.parse_bytes(ok).ns, uri(CAP)) - - def test_an_entity_in_a_namespace_value_is_refused_as_an_entity(self): - # The first pass does not process namespaces but still expands entities in attribute values, - # so it refuses entity declarations itself, before the entity can be expanded: one entity - # whose value is over the cap would otherwise be reported as a long URI. - data = f']>\n'.encode() - exc = self.refused(data) - self.assertEqual((str(exc), exc.line), (ENTITY_ERROR, 1)) - - def test_a_malformed_document_is_still_reported_by_the_parse(self): - self.assertTrue(str(self.refused(b"")).startswith("XML is not well-formed")) - self.assertTrue(str(self.refused(b'')).startswith("XML is not well-formed")) # unbound prefix - - def test_a_file_with_a_long_uri_is_an_xml001_finding(self): - with tempfile.TemporaryDirectory() as d: - path = Path(d) / "a.xml" - path.write_text(f'', encoding="utf-8") - fmt, findings = plc_validate.check_file(path, "plcopen") - self.assertEqual((fmt, [(f.code, f.line, f.message) for f in findings]), ("plcopen", [("XML001", 1, LONG_URI_ERROR)])) - - -if __name__ == "__main__": - unittest.main()