fix(sql): recover T-SQL bracket-named, CREATE OR ALTER, and PROC routines

graphify/extractors/sql.py

@@ -6,6 +6,75 @@
 from pathlib import Path
 from graphify.extractors.base import _file_stem, _make_id
 
+# Recovers CREATE FUNCTION/PROCEDURE statements the grammar could not parse
+# structurally. Used by BOTH recovery sites — the walk-time ERROR-node scan and
+# the whole-file has_error fallback (#2180). They MUST share one pattern: when
+# they disagreed, the same statement produced two nodes with different names
+# (the ERROR scan captured `dbo.[usp_Mixed]`, the fallback stopped at `dbo`,
+# and _add_node's id-dedupe never fired because the ids differed).
+#
+# Each name part is a bare identifier, a double-quoted (delimited) one, or a
+# T-SQL bracket-delimited one, so CREATE OR REPLACE FUNCTION "public"."fn"(...)
+# and CREATE PROCEDURE [dbo].[usp_Load] ... are both recovered. A bare [\w$.]+
+# stops dead at the leading delimiter, which silently dropped every quoted
+# PL/pgSQL routine (#2180) and every bracket-named T-SQL procedure. T-SQL's
+# AS BEGIN...END body idiom always lands in recovery — the grammar has no
+# create_procedure parse for it — and T-SQL spells re-creation CREATE OR ALTER
+# (it has no OR REPLACE), so accept that form too, mirroring fb_proc_or_trigger.
+# Inside a bracket-delimited part, a literal ] is escaped by doubling
+# ([a]]b] names the identifier a]b), so consume ]] before treating a
+# lone ] as the closing delimiter — stopping at the first ] truncated
+# the name and minted a phantom that could collide with a real [a].
+# PROC is T-SQL's official shorthand for PROCEDURE and equally common in the
+# wild; the optional (?:EDURE)? still requires trailing whitespace, so a word
+# that merely starts with PROC cannot match.
+_ROUTINE_RECOVERY_RX = re.compile(
+    r"CREATE\s+(?:OR\s+(?:REPLACE|ALTER)\s+)?(?:FUNCTION|PROC(?:EDURE)?)\s+"
+    r"(?:IF\s+NOT\s+EXISTS\s+)?"
+    r"((?:\"[^\"\n]+\"|\[(?:[^\]\n]|\]\])+\]|[\w$]+)"
+    r"(?:\s*\.\s*(?:\"[^\"\n]+\"|\[(?:[^\]\n]|\]\])+\]|[\w$]+))*)",
+    re.IGNORECASE,
+)
+
+# Matches string/identifier literals (preserved) or comments (blanked) for
+# _mask_sql_comments. Literals are matched first so a comment opener INSIDE a
+# literal ('-- not a comment', [a--b]) is not treated as a comment. Literal
+# patterns are deliberately single-line: this mask only runs on files that
+# already failed to parse, where an unclosed quote is likely, and a multi-line
+# literal match would let one unclosed delimiter swallow real DDL below it —
+# a masked-away routine (false negative) is recoverable by fixing the file, a
+# swallowed one is silent. A comment opener inside a multi-line string
+# therefore still masks to end-of-line, which can only hide a routine, never
+# fabricate one. DOTALL so a block comment may span lines.
+_SQL_COMMENT_OR_LITERAL_RX = re.compile(
+    r"'(?:[^'\n]|'')*'"          # single-quoted string, '' escape (one line)
+    r"|\"[^\"\n]*\""             # double-quoted identifier (one line)
+    r"|\[(?:[^\]\n]|\]\])*\]"   # bracket-delimited identifier, ]] escape
+    r"|(--[^\n]*|/\*.*?\*/)",    # group 1: the comment span to blank
+    re.DOTALL,
+)
+
+
+def _mask_sql_comments(text: str) -> str:
+    """Blank out comment spans, preserving every character offset.
+
+    Non-newline characters inside a comment become spaces and newlines are
+    kept, so positions and line numbers computed against the masked text are
+    valid against the original. String and identifier literals are preserved
+    verbatim, so a `--` or `/*` inside one does not start a comment. Used by
+    the whole-file routine recovery so commented-out CREATE PROCEDURE/FUNCTION
+    DDL in a file that has an unrelated parse error cannot fabricate a routine
+    node.
+    """
+    return _SQL_COMMENT_OR_LITERAL_RX.sub(
+        lambda m: (
+            "".join("\n" if c == "\n" else " " for c in m.group(0))
+            if m.group(1)
+            else m.group(0)
+        ),
+        text,
+    )
+
 
 def _norm_ident(name: str) -> str:
     """Normalize a SQL identifier for name-based reference resolution.
@@ -254,18 +323,14 @@ def walk(node) -> None:
             # do not scan the body for FROM/JOIN references: PL/pgSQL loop
             # variables and locals would produce junk reads_from targets.
             #
-            # Each name part is either a bare identifier or a double-quoted
-            # (delimited) one, so schema-qualified generated DDL such as
-            # CREATE OR REPLACE FUNCTION "public"."fn"(...) is recovered too.
-            # A bare [\w$.]+ stops dead at the leading quote, which silently
-            # dropped every quoted PL/pgSQL routine (#2180).
-            text = _read(node)
-            for m in re.finditer(
-                r"CREATE\s+(?:OR\s+REPLACE\s+)?(?:FUNCTION|PROCEDURE)\s+"
-                r"(?:IF\s+NOT\s+EXISTS\s+)?"
-                r"((?:\"[^\"\n]+\"|[\w$]+)(?:\s*\.\s*(?:\"[^\"\n]+\"|[\w$]+))*)",
-                text, re.IGNORECASE,
-            ):
+            # Name and keyword shapes accepted here are defined once in
+            # _ROUTINE_RECOVERY_RX, shared with the whole-file fallback below —
+            # see the comment on the constant. Comments are masked the same
+            # way too: an ERROR blob can swallow commented-out DDL along with
+            # the statements around it, and unmasked text would fabricate a
+            # routine node from it exactly as the whole-file scan once did.
+            text = _mask_sql_comments(_read(node))
+            for m in _ROUTINE_RECOVERY_RX.finditer(text):
                 name = m.group(1)
                 m_line = line + text[: m.start()].count("\n")
                 nid = _make_id(stem, name)
@@ -429,8 +494,9 @@ def _collect_defined_names(node) -> None:
     #      (keyword_create/keyword_function/object_reference/... ) and the ERROR
     #      node holds only the offending body line, e.g. `PERFORM x();` or
     #      `x := 1;` -- so no CREATE text is inside any ERROR node at all
-    #   3. the name is a quoted identifier ("public"."fn"), which a bare
-    #      [\w$.]+ pattern cannot match
+    #   3. the name is a delimited identifier — quoted ("public"."fn") or
+    #      T-SQL-bracketed ([dbo].[usp_Load]) — which a bare [\w$.]+ pattern
+    #      cannot match
     # Shapes 2 and 3 silently dropped the routine: no node, no warning, exit 0.
     # Scanning the raw source catches all three, and _add_node dedupes by id so
     # routines already recovered from the tree are not emitted twice.
@@ -441,12 +507,10 @@ def _collect_defined_names(node) -> None:
     # observed drop shape leaves an ERROR node in the tree, so has_error loses
     # nothing while protecting clean corpora (#2180 follow-up).
     if root.has_error:
-        for m in re.finditer(
-            r"CREATE\s+(?:OR\s+REPLACE\s+)?(?:FUNCTION|PROCEDURE)\s+"
-            r"(?:IF\s+NOT\s+EXISTS\s+)?"
-            r"((?:\"[^\"\n]+\"|[\w$]+)(?:\s*\.\s*(?:\"[^\"\n]+\"|[\w$]+))*)",
-            src_text, re.IGNORECASE,
-        ):
+        # Comments are masked (offset-preserving) so commented-out DDL cannot
+        # fabricate a routine when an unrelated error arms this scan; DDL
+        # inside string bodies remains covered only by the has_error gate.
+        for m in _ROUTINE_RECOVERY_RX.finditer(_mask_sql_comments(src_text)):
             fn_name = m.group(1)
             fn_line = src_text[: m.start()].count("\n") + 1
             _add_node(_make_id(stem, fn_name), f"{fn_name}()", fn_line)

tests/test_multilang.py

@@ -487,6 +487,165 @@ def test_sql_no_dangling_edges():
     for e in r["edges"]:
         assert e["source"] in node_ids, f"dangling source: {e['source']}"
 
+def test_sql_tsql_bracketed_procedure_is_recovered(tmp_path):
+    """T-SQL CREATE PROCEDURE [Schema].[Name] ... AS BEGIN...END.
+
+    The grammar has no create_procedure parse for T-SQL's AS BEGIN...END body
+    idiom, so the statement lands in ERROR recovery — where a name pattern
+    without a bracket-delimited alternative recovered nothing. In a T-SQL
+    codebase that brackets every identifier (a common house standard), that
+    made every stored procedure invisible.
+    """
+    pytest.importorskip("tree_sitter_sql")
+    p = tmp_path / "proc.sql"
+    p.write_text(
+        "CREATE PROCEDURE [dbo].[usp_LoadDebtors]\n"
+        "    @BatchId INT\n"
+        "AS\n"
+        "BEGIN\n"
+        "    SET NOCOUNT ON;\n"
+        "    INSERT INTO dbo.Debtors (Id) SELECT Id FROM staging.Debtors;\n"
+        "END;\n"
+    )
+    r = extract_sql(p)
+    routine = [n["label"] for n in r["nodes"] if n["label"] != "proc.sql"]
+    assert routine == ["[dbo].[usp_LoadDebtors]()"], routine
+
+
+def test_sql_tsql_create_or_alter_procedure_is_recovered(tmp_path):
+    """T-SQL spells idempotent re-creation CREATE OR ALTER (no OR REPLACE)."""
+    pytest.importorskip("tree_sitter_sql")
+    p = tmp_path / "proc.sql"
+    p.write_text(
+        "CREATE OR ALTER PROCEDURE [Utils].[ValidateSourceView]\n"
+        "AS\nBEGIN\n    SELECT 1;\nEND;\n"
+        "CREATE OR ALTER PROCEDURE usp_Bare AS\nBEGIN\n SELECT 1;\nEND;\n"
+    )
+    r = extract_sql(p)
+    labels = sorted(n["label"] for n in r["nodes"] if n["label"] != "proc.sql")
+    assert labels == ["[Utils].[ValidateSourceView]()", "usp_Bare()"], labels
+
+
+def test_sql_escaped_closing_bracket_in_routine_name_is_consumed(tmp_path):
+    """T-SQL escapes a literal ] inside a bracketed identifier by doubling it:
+    [a]]b] names the identifier a]b. A pattern that stops at the first ]
+    truncated the name to [dbo].[a] — a phantom that could collide with a
+    genuinely-named [dbo].[a]."""
+    import tree_sitter_sql  # noqa: F401 — required by the recovery path under test
+    p = tmp_path / "proc.sql"
+    p.write_text("CREATE PROCEDURE [dbo].[a]]b]\nAS\nBEGIN\n    SELECT 1;\nEND;\n")
+    r = extract_sql(p)
+    routine = [n["label"] for n in r["nodes"] if n["label"] != "proc.sql"]
+    assert routine == ["[dbo].[a]]b]()"], routine
+
+
+def test_sql_recovery_sites_agree_on_the_captured_name(tmp_path):
+    """A mixed-delimiter name (dbo.[usp_Mixed]) must yield exactly ONE node.
+
+    The walk-time ERROR scan and the whole-file has_error fallback recover the
+    same statement; when their patterns drifted, they captured different names
+    (`dbo.[usp_Mixed]` vs `dbo`), minted different ids, and _add_node's
+    id-dedupe never fired — one procedure became two nodes, one of them a
+    phantom named after the schema. Sharing _ROUTINE_RECOVERY_RX makes the
+    drift impossible; this pins the observable behaviour.
+    """
+    pytest.importorskip("tree_sitter_sql")
+    p = tmp_path / "proc.sql"
+    p.write_text("CREATE PROCEDURE dbo.[usp_Mixed] AS\nBEGIN\n SELECT 1;\nEND;\n")
+    r = extract_sql(p)
+    routine = [n["label"] for n in r["nodes"] if n["label"] != "proc.sql"]
+    assert routine == ["dbo.[usp_Mixed]()"], routine
+
+
+def test_sql_tsql_proc_shorthand_is_recovered(tmp_path):
+    """T-SQL's official PROC shorthand, including CREATE OR ALTER PROC with a
+    qualified bracket-delimited name, must recover like the long form."""
+    pytest.importorskip("tree_sitter_sql")
+    p = tmp_path / "proc.sql"
+    p.write_text(
+        "CREATE OR ALTER PROC [dbo].[usp_Short]\n"
+        "AS\nBEGIN\n    SELECT 1;\nEND;\n"
+        "CREATE PROC usp_BareShort AS\nBEGIN\n SELECT 1;\nEND;\n"
+    )
+    r = extract_sql(p)
+    labels = sorted(n["label"] for n in r["nodes"] if n["label"] != "proc.sql")
+    assert labels == ["[dbo].[usp_Short]()", "usp_BareShort()"], labels
+
+
+def test_sql_commented_ddl_is_not_fabricated_by_error_recovery(tmp_path):
+    """An unrelated syntax error arms the whole-file recovery scan; DDL that
+    exists only inside comments must not fabricate routine nodes from it."""
+    pytest.importorskip("tree_sitter_sql")
+    p = tmp_path / "broken.sql"
+    p.write_text(
+        "-- CREATE PROCEDURE [dbo].[usp_LineComment] AS BEGIN SELECT 1; END;\n"
+        "/*\nCREATE OR ALTER PROC dbo.usp_BlockComment AS\nBEGIN SELECT 1; END;\n*/\n"
+        "CREATE PROCEDURE [dbo].[usp_Real]\nAS\nBEGIN\n    SELECT 1;\nEND;\n"
+        "THIS IS NOT SQL AT ALL %%%;\n"
+        # Sandwiched between broken segments so one ERROR blob's byte span
+        # covers the comment: the walk-time ERROR scan sees it too (the
+        # leading comments above land in proper comment nodes instead, and a
+        # trailing comment falls outside the ERROR span):
+        "%%% BROKEN JUNK\n"
+        "-- CREATE PROC [dbo].[usp_ErrComment] AS BEGIN SELECT 1; END;\n"
+        "%%% MORE BROKEN JUNK\n"
+    )
+    r = extract_sql(p)
+    labels = [n["label"] for n in r["nodes"] if n["label"] != "broken.sql"]
+    assert "[dbo].[usp_Real]()" in labels, labels
+    assert not any("Comment" in l for l in labels), (
+        f"commented-out DDL fabricated a node: {labels}"
+    )
+
+
+def test_sql_comment_openers_inside_string_literals_do_not_hide_ddl(tmp_path):
+    """A `--` or `/*` inside a string literal is data, not a comment opener.
+
+    Both recovery scans mask comments before matching, and a mask that reads
+    `'-- note'` as a line comment blanks to end-of-line — hiding a routine
+    declared after the literal — while a `/*` inside a string blanks
+    everything through the next real `*/`, swallowing whole statements
+    between. Both must recover normally. This is behavior coverage: the
+    grammar's own fb_proc_or_trigger recovery can rescue these shapes even
+    under a literal-blind mask, so the mutation-killing pin for the mask
+    itself is test_mask_sql_comments_preserves_literals_and_blanks_comments.
+    """
+    pytest.importorskip("tree_sitter_sql")
+    p = tmp_path / "strings.sql"
+    # The lines carrying the literals start with junk so they cannot parse as
+    # proper statements: literal and routine land in the SAME ERROR blob, and
+    # only the mask decides whether the routine survives. (A literal in a
+    # statement that parses never reaches the masked scans at all.)
+    p.write_text(
+        "%%% INSERT INTO log VALUES ('-- note'); "
+        "CREATE PROCEDURE [dbo].[usp_AfterLineString] AS BEGIN SELECT 1; END;\n"
+        "%%% SELECT 'open /* here' AS x; "
+        "CREATE PROCEDURE [dbo].[usp_AfterBlockString] AS BEGIN SELECT 1; END;\n"
+        "/* a real comment, giving the false opener a closer to reach */\n"
+    )
+    r = extract_sql(p)
+    labels = [n["label"] for n in r["nodes"] if n["label"] != "strings.sql"]
+    assert "[dbo].[usp_AfterLineString]()" in labels, labels
+    assert "[dbo].[usp_AfterBlockString]()" in labels, labels
+
+
+def test_mask_sql_comments_preserves_literals_and_blanks_comments():
+    """Unit pin for _mask_sql_comments: comment openers inside string or
+    identifier literals are data; real comments blank to spaces with newlines
+    and offsets preserved."""
+    from graphify.extractors.sql import _mask_sql_comments as mask
+
+    assert mask("select '-- x' from t") == "select '-- x' from t"
+    assert mask("select 'it''s -- ok'") == "select 'it''s -- ok'"
+    assert mask('select "a--b" from t') == 'select "a--b" from t'
+    assert mask("select [a--b] from t") == "select [a--b] from t"
+    assert mask("select 'a /* b' as x") == "select 'a /* b' as x"
+    assert mask("a -- b") == "a     "
+    assert mask("x /* y */ z") == "x         z"
+    masked = mask("a /* m\nl */ b")
+    assert masked == "a     \n     b" and len(masked) == len("a /* m\nl */ b")
+
+
 def test_sql_cte_is_not_read_as_a_table():
     """#2577: a name bound by WITH ... AS (...) is scoped to its statement, not a table.