diff --git a/.github/ISSUE_TEMPLATE/contentious-construct.yml b/.github/ISSUE_TEMPLATE/contentious-construct.yml new file mode 100644 index 0000000..7559f91 --- /dev/null +++ b/.github/ISSUE_TEMPLATE/contentious-construct.yml @@ -0,0 +1,63 @@ +name: Contentious construct (a parser may reasonably reject) +description: Propose a statement that the engine accepts but that a parser may reasonably decline to support. +title: "[contentious] " +labels: ["contentious"] +body: + - type: markdown + attributes: + value: | + Use this form when the reference engine accepts a statement but you think a parser may reasonably reject it anyway (for example an engine-specific quirk, a non-standard extension, or a construct with surprising semantics). This feeds the contentious-construct rule registry described in [docs/contentious-constructs.md](https://github.com/LucaCappelletti94/sql_ast_benchmark/blob/main/docs/contentious-constructs.md). The strict, engine-graded recall is never changed by this. Most fields are prefilled when you arrive here from the failures view. + - type: input + id: dialect + attributes: + label: Dialect + description: The reference dialect whose engine accepts the statement. + validations: + required: true + - type: input + id: parser + attributes: + label: Parser + description: The parser whose failures view you came from (optional context). + validations: + required: false + - type: textarea + id: statement + attributes: + label: Statement + description: The SQL the engine accepts but a parser may reasonably reject. Prefilled from the failures view. + validations: + required: true + - type: textarea + id: parser_error + attributes: + label: Parser error + description: The error the parser returned (prefilled from the failures view). + validations: + required: false + - type: dropdown + id: category + attributes: + label: Category + description: Why is this construct contentious? See the design doc for the meaning of each category. + options: + - engine-specific + - non-standard + - lossy-or-ambiguous + - deprecated + validations: + required: true + - type: textarea + id: rationale + attributes: + label: Why a parser may reasonably reject it + description: Explain the divergence and why declining to support it is defensible. + validations: + required: true + - type: textarea + id: references + attributes: + label: References + description: Links to the SQL standard, engine documentation, or peer-parser behavior that support the category. + validations: + required: false diff --git a/.github/ISSUE_TEMPLATE/not-contentious.yml b/.github/ISSUE_TEMPLATE/not-contentious.yml new file mode 100644 index 0000000..be73eb2 --- /dev/null +++ b/.github/ISSUE_TEMPLATE/not-contentious.yml @@ -0,0 +1,51 @@ +name: Dispute a contentious classification +description: Argue that a statement currently marked as a contentious construct is not actually contentious and a parser should be expected to accept it. +title: "[not-contentious] " +labels: ["contentious-dispute"] +body: + - type: markdown + attributes: + value: | + Use this form to push back on a statement the site marks as a contentious construct (an intentional divergence). If you think it is a normal statement that a parser should accept, not a quirk a parser may reasonably reject, say so here. This challenges the rule, not any parser. See [docs/contentious-constructs.md](https://github.com/LucaCappelletti94/sql_ast_benchmark/blob/main/docs/contentious-constructs.md). Most fields are prefilled when you arrive here from the failures view. + - type: input + id: rule + attributes: + label: Contentious rule being disputed + description: The rule that currently tags this statement (prefilled from the failures view). + validations: + required: true + - type: input + id: dialect + attributes: + label: Dialect + description: The reference dialect the statement is from. + validations: + required: true + - type: input + id: parser + attributes: + label: Parser + description: The parser whose failures view you came from (optional context). + validations: + required: false + - type: textarea + id: statement + attributes: + label: Statement + description: The statement tagged contentious. Prefilled from the failures view. + validations: + required: true + - type: textarea + id: parser_error + attributes: + label: Parser error + description: The error the parser returned (prefilled from the failures view). + validations: + required: false + - type: textarea + id: rationale + attributes: + label: Why it is not contentious + description: Explain why this is a normal statement a parser should accept, not a quirk a parser may reasonably reject. + validations: + required: true diff --git a/.github/ISSUE_TEMPLATE/should-be-rejected.yml b/.github/ISSUE_TEMPLATE/should-be-rejected.yml new file mode 100644 index 0000000..f19a2ef --- /dev/null +++ b/.github/ISSUE_TEMPLATE/should-be-rejected.yml @@ -0,0 +1,51 @@ +name: Statement should be rejected (invalid syntax) +description: Report a statement the reference engine accepted but that is actually invalid, so its valid label is wrong. +title: "[invalid-label] " +labels: ["oracle-label"] +body: + - type: markdown + attributes: + value: | + Use this form when a statement is shown as a parser miss but you believe the statement is actually invalid and should be rejected. The benchmark labels a statement valid only when the reference database engine accepts it, so this report is about the engine's label, not about any parser. Most fields are prefilled when you arrive here from the failures view. + - type: input + id: dialect + attributes: + label: Dialect + description: The reference dialect whose engine labeled the statement valid. + validations: + required: true + - type: input + id: parser + attributes: + label: Parser + description: The parser whose failures view you came from (optional context). + validations: + required: false + - type: textarea + id: statement + attributes: + label: Statement + description: The SQL the engine accepted. Prefilled from the failures view. + validations: + required: true + - type: textarea + id: parser_error + attributes: + label: Parser error + description: The error the parser returned (prefilled from the failures view). + validations: + required: false + - type: textarea + id: rationale + attributes: + label: Why it should be rejected + description: Explain why this statement is invalid SQL despite the engine accepting it. + validations: + required: true + - type: textarea + id: references + attributes: + label: References + description: Links to the SQL standard, the engine's own documentation, or anything showing the statement should be rejected. + validations: + required: false diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 33c3423..4ebaf19 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -34,6 +34,24 @@ cargo run --bin sqlbench -- export # read all of the above, write web/asse The charts are rendered in the browser from the JSON by the shared `viz` crate (plotters, SVG backend), so no chart images are committed. +## Contentious constructs + +A contentious construct is one the reference engine accepts but a parser may reasonably decline to support (a niche engine quirk, a non-standard extension, a lossy or deprecated form). The benchmark keeps strict, oracle-graded recall as the headline number and adds a secondary "recall excluding contentious" beside it, plus a per-statement badge on the failures view. The design is written up in [docs/contentious-constructs.md](docs/contentious-constructs.md). + +Rules are data: one TOML file per rule under `contentious/`. A regex rule needs no Rust (the pattern is matched against a masked form of the statement, so string literals and comments cannot trigger it). A structural rule (for a property a regex cannot express, like a repeated identifier) names a built-in predicate added in `src/contentious.rs`. Each file declares `id`, `title`, `category` (`engine-specific`, `non-standard`, `lossy-or-ambiguous`, or `deprecated`), the `dialects` it may fire in, `description`, `references`, and `matches` / `non_matches` example statements. See the two existing files for the shape. + +To add a rule: + +```bash +# 1. write contentious/.toml +cargo test -p sql_ast_benchmark --lib contentious # guards: examples match/non-match, + # ids unique, each rule covers >=1 + # engine-valid corpus statement +cargo run --release --bin sqlbench -- export # refresh web/assets/bench.json.zst +``` + +Then open a PR. A regex rule is a data-only change. Review is a data review: is the construct genuinely engine-valid, is the category honest, are the references real. The classifier only ever runs on engine-valid statements, so a rule can never change strict recall or excuse genuinely-invalid SQL. + ## Time machine (per-version history) The `timemachine` crate benchmarks several historical versions of each pure-Rust parser and writes `web/assets/history.json.zst` (committed, embedded and decompressed in wasm with `ruzstd`, so the site still does no runtime fetch). It hosts many versions of one crate at once with `package`-rename aliases, which works because different `0.x` minors are semver-incompatible. The FFI parsers (`pg_query`) are excluded: two libpg_query builds export the same C symbols and collide at link. diff --git a/Cargo.toml b/Cargo.toml index 5943d3a..2d7ada6 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -39,6 +39,7 @@ sqlite3-parser = "0.16.0" turso_parser = "0.6.1" fallible-iterator = "0.3.0" viz = { path = "viz" } +serde = { version = "1", features = ["derive"] } serde_json = "1" # Compress the per-(dialect, parser) rejected-statement TSV downloads. zstd = "0.13" @@ -46,6 +47,10 @@ zstd = "0.13" # time, so the web viewer ships no runtime syntax highlighter. `default-fancy` # uses the pure-Rust regex engine, avoiding the oniguruma C dependency. syntect = { version = "5", default-features = false, features = ["default-fancy"] } +# Contentious-construct rule registry: regex rules matched in guaranteed linear +# time (no backreferences) and one TOML file per rule under `contentious/`. +regex = "1" +toml = "0.8" [[bench]] name = "parsing" diff --git a/contentious/duplicate-target-columns.toml b/contentious/duplicate-target-columns.toml new file mode 100644 index 0000000..24b8831 --- /dev/null +++ b/contentious/duplicate-target-columns.toml @@ -0,0 +1,23 @@ +id = "duplicate-target-columns" +title = "Duplicate columns in a target list" +category = "non-standard" +dialects = ["sqlite"] +# A repeated identifier inside one group cannot be a regex (Rust's regex has no +# backreferences), so this is a structural rule backed by a named predicate. +kind = "structural" +predicate = "duplicate_columns" +description = "A column named more than once in an INSERT target list, an UPDATE SET column group, or a USING clause. SQLite accepts these (INSERT keeps the first value for a repeated column, UPDATE ... SET keeps the last), but PostgreSQL rejects them and a parser that does the same is arguably safer." +references = [ + "https://github.com/gwenn/lemon-rs/issues/89", + "https://www.postgresql.org/docs/current/sql-insert.html", +] +matches = [ + "INSERT INTO dup1(a,b,c,a,b,c) VALUES(1,2,3,4,5,6)", + "UPDATE t1 SET (a,a,a,b)=(SELECT 99,100,101,102)", + "SELECT y FROM t1 JOIN t1 USING (y,y)", +] +non_matches = [ + "INSERT INTO t(a,b,c) VALUES(1,2,3)", + "UPDATE t1 SET (a,b)=(SELECT 1,2)", + "SELECT y FROM t1 JOIN t2 USING (y)", +] diff --git a/contentious/sqlite-nonstandard-alter.toml b/contentious/sqlite-nonstandard-alter.toml new file mode 100644 index 0000000..dbfdd73 --- /dev/null +++ b/contentious/sqlite-nonstandard-alter.toml @@ -0,0 +1,27 @@ +id = "sqlite-nonstandard-alter" +title = "Non-standard SQLite ALTER TABLE operations" +category = "non-standard" +dialects = ["sqlite"] +kind = "regex" +# SQLite's documented ALTER TABLE supports only RENAME, ADD COLUMN, RENAME COLUMN, +# and DROP COLUMN. Its grammar nonetheless tolerates ADD/DROP CONSTRAINT, ADD +# CHECK, and ALTER [COLUMN] SET/DROP NOT NULL without a syntax error, so the +# engine labels them valid. The alternation is anchored on ALTER TABLE and targets +# only the unsupported operations, so the supported forms never match. +pattern = '(?i)\balter\s+table\b.*(\b(add|drop)\s+constraint\b|\badd\s+check\b|\balter\s+(column\s+)?\S+\s+(set|drop)\s+not\s+null\b)' +description = "ALTER TABLE operations the SQLite grammar accepts without a syntax error but that are not part of supported SQLite (ADD/DROP CONSTRAINT, ADD CHECK, ALTER COLUMN SET/DROP NOT NULL). A parser modeling real SQLite may reasonably reject them." +references = [ + "https://www.sqlite.org/lang_altertable.html", + "https://github.com/tursodatabase/turso", +] +matches = [ + "ALTER TABLE t1 ADD CONSTRAINT c1 CHECK(a=b)", + "ALTER TABLE t2 ALTER x SET NOT NULL", + "ALTER TABLE abc DROP CONSTRAINT two", + "ALTER TABLE abc ADD CHECK (z>=0)", +] +non_matches = [ + "ALTER TABLE t ADD COLUMN c INTEGER", + "ALTER TABLE t RENAME TO t2", + "ALTER TABLE t DROP COLUMN c", +] diff --git a/contentious/tcl-variables.toml b/contentious/tcl-variables.toml new file mode 100644 index 0000000..446ee22 --- /dev/null +++ b/contentious/tcl-variables.toml @@ -0,0 +1,24 @@ +id = "tcl-variables" +title = "TCL bind variables" +category = "engine-specific" +dialects = ["sqlite"] +kind = "regex" +# Matched against the masked form (string and blob literals and comments +# replaced), so `$::x` inside a string literal does not trigger. Covers `$::name` +# and `$namespace::name`. +pattern = '\$\w*::' +description = "Bind variables using TCL :: namespaces, valid only because SQLite began as a TCL extension and meaningless outside a TCL interpreter." +references = [ + "https://sqlite.org/lang_expr.html#parameters", + "https://github.com/gwenn/lemon-rs/issues/102", +] +matches = [ + "INSERT INTO t1 VALUES($::w,$::x,$::y,$::z)", + "select $testnamespace::xyz", + "SELECT strftime($::FMT,$::TS,'unixepoch')", +] +non_matches = [ + "SELECT :w, :x", + "SELECT '$::x' AS lit", + "SELECT x::int", +] diff --git a/src/contentious.rs b/src/contentious.rs new file mode 100644 index 0000000..ddc2bbf --- /dev/null +++ b/src/contentious.rs @@ -0,0 +1,473 @@ +//! Contentious-construct classifier and rule registry. +//! +//! A *contentious* construct is one the reference engine accepts but a parser may +//! reasonably decline to support (a niche engine quirk, a non-standard extension, +//! a lossy or deprecated form). Strict, oracle-graded recall is never affected by +//! this layer: it only tags statements for presentation and the secondary metric. +//! +//! Rules are data, one TOML file per rule under [`RULES_DIR`]. A rule is either a +//! `regex` rule (a [`regex`] pattern matched against a masked form of the +//! statement) or a `structural` rule (a named built-in predicate for properties a +//! regex cannot express, such as a repeated identifier). The `regex` crate matches +//! in guaranteed linear time with no backreferences, so a contributed pattern +//! cannot run arbitrary code or cause catastrophic backtracking. + +use crate::datasets::Dialect; +use regex::Regex; +use serde::Deserialize; +use std::collections::HashSet; +use std::path::Path; +use std::sync::OnceLock; + +/// Directory holding the committed rule files, relative to the working directory. +pub const RULES_DIR: &str = "contentious"; + +/// Why a construct is contentious. A small fixed enum so meaning stays stable and +/// new categories are a deliberate change, not a contributor free-for-all. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Deserialize)] +#[serde(rename_all = "kebab-case")] +pub enum Category { + /// A quirk tied to one engine's history or embedding (e.g. TCL variables). + EngineSpecific, + /// Accepted by the engine but outside the SQL standard and rejected by peers. + NonStandard, + /// Accepted but with surprising or implementation-defined semantics. + LossyOrAmbiguous, + /// Accepted for backward compatibility but discouraged by the engine's docs. + Deprecated, +} + +impl Category { + /// The kebab-case wire form, also used in the rule files and the export. + #[must_use] + pub const fn as_str(self) -> &'static str { + match self { + Self::EngineSpecific => "engine-specific", + Self::NonStandard => "non-standard", + Self::LossyOrAmbiguous => "lossy-or-ambiguous", + Self::Deprecated => "deprecated", + } + } +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq, Deserialize)] +#[serde(rename_all = "lowercase")] +enum Kind { + Regex, + Structural, +} + +/// One rule file under [`RULES_DIR`], deserialized from TOML. +#[derive(Debug, Clone, Deserialize)] +pub struct RuleFile { + pub id: String, + pub title: String, + pub category: Category, + /// Dir names of the dialects this rule may fire in, or a single `"all"`. + pub dialects: Vec, + kind: Kind, + /// The regex pattern (regex rules only). + #[serde(default)] + pub pattern: Option, + /// The built-in predicate name (structural rules only). + #[serde(default)] + pub predicate: Option, + #[serde(default)] + pub description: String, + #[serde(default)] + pub references: Vec, + /// Statements the rule must match (verified in tests). + #[serde(default)] + pub matches: Vec, + /// Statements the rule must not match (verified in tests). + #[serde(default)] + pub non_matches: Vec, +} + +/// A built-in structural predicate, run against the original statement. +type Predicate = fn(&str) -> bool; + +enum Matcher { + Regex(Regex), + Structural(Predicate), +} + +/// A loaded, compiled rule: its metadata plus its matcher. +pub struct Rule { + pub meta: RuleFile, + matcher: Matcher, +} + +impl Rule { + /// Whether this rule is allowed to fire in `dialect`. + #[must_use] + pub fn applies_to(&self, dialect: Dialect) -> bool { + self.meta + .dialects + .iter() + .any(|d| d == "all" || d == dialect.dir_name()) + } + + /// Whether this rule matches. Regex rules see the masked form, structural + /// rules see the original statement. + fn is_match(&self, masked: &str, original: &str) -> bool { + match &self.matcher { + Matcher::Regex(re) => re.is_match(masked), + Matcher::Structural(p) => p(original), + } + } +} + +/// Map a structural predicate name to its built-in implementation. Structural +/// rules are a closed set, extended only by a PR that touches this file. +fn structural_predicate(name: &str) -> Option { + match name { + "duplicate_columns" => Some(duplicate_columns), + _ => None, + } +} + +/// The loaded rule registry. +pub struct Registry { + pub rules: Vec, +} + +impl Registry { + /// Load and compile every `*.toml` rule under `dir`, in id order. + /// + /// # Errors + /// + /// Returns an error (rather than panicking) on a bad file, a bad regex, an + /// unknown predicate, a missing `pattern`/`predicate`, or a duplicate id, so + /// callers can fail the build or a test with a precise message. + pub fn load(dir: &Path) -> Result { + let mut paths: Vec<_> = std::fs::read_dir(dir) + .map_err(|e| format!("reading {}: {e}", dir.display()))? + .filter_map(Result::ok) + .map(|e| e.path()) + .filter(|p| p.extension().is_some_and(|x| x == "toml")) + .collect(); + paths.sort(); + + let mut rules = Vec::new(); + let mut ids = HashSet::new(); + for path in paths { + let text = std::fs::read_to_string(&path) + .map_err(|e| format!("reading {}: {e}", path.display()))?; + let meta: RuleFile = + toml::from_str(&text).map_err(|e| format!("parsing {}: {e}", path.display()))?; + if !ids.insert(meta.id.clone()) { + return Err(format!("duplicate rule id `{}`", meta.id)); + } + let matcher = + match meta.kind { + Kind::Regex => { + let pat = meta.pattern.as_ref().ok_or_else(|| { + format!("rule `{}`: regex rule needs `pattern`", meta.id) + })?; + Matcher::Regex( + Regex::new(pat) + .map_err(|e| format!("rule `{}`: bad regex: {e}", meta.id))?, + ) + } + Kind::Structural => { + let name = meta.predicate.as_ref().ok_or_else(|| { + format!("rule `{}`: structural rule needs `predicate`", meta.id) + })?; + Matcher::Structural(structural_predicate(name).ok_or_else(|| { + format!("rule `{}`: unknown predicate `{name}`", meta.id) + })?) + } + }; + rules.push(Rule { meta, matcher }); + } + Ok(Self { rules }) + } + + /// The first rule that fires for `sql` in `dialect`, if any. A statement is + /// contentious when this returns `Some`. + #[must_use] + pub fn classify(&self, sql: &str, dialect: Dialect) -> Option<&Rule> { + let masked = mask(sql); + self.rules + .iter() + .find(|r| r.applies_to(dialect) && r.is_match(&masked, sql)) + } +} + +/// The process-wide registry, loaded once from [`RULES_DIR`]. +/// +/// # Panics +/// +/// Panics with the load error if the rule files are malformed, which fails the +/// export build. +#[must_use] +pub fn registry() -> &'static Registry { + static REGISTRY: OnceLock = OnceLock::new(); + REGISTRY.get_or_init(|| { + Registry::load(Path::new(RULES_DIR)).unwrap_or_else(|e| panic!("contentious registry: {e}")) + }) +} + +/// Produce a masked form of `sql` for matching. +/// +/// String and blob literals and comments are replaced by a single space and runs +/// of whitespace are collapsed, so a regex rule matches a predictable token stream +/// and a literal like `'$::x'` or a comment cannot trigger a false match. +/// Double-quoted, backtick, and bracket identifiers are left intact, since an +/// identifier is not a literal. +#[must_use] +pub fn mask(sql: &str) -> String { + let b = sql.as_bytes(); + // Build raw bytes: only whole literal/comment regions (delimited by ASCII + // bytes that never occur mid-UTF-8) are dropped, so the result stays valid + // UTF-8 and multibyte identifiers pass through unchanged. + let mut out: Vec = Vec::with_capacity(b.len()); + let mut i = 0; + while i < b.len() { + let c = b[i]; + // Line comment: -- ... end of line. + if c == b'-' && i + 1 < b.len() && b[i + 1] == b'-' { + out.push(b' '); + i += 2; + while i < b.len() && b[i] != b'\n' { + i += 1; + } + continue; + } + // Block comment: /* ... */. + if c == b'/' && i + 1 < b.len() && b[i + 1] == b'*' { + out.push(b' '); + i += 2; + while i + 1 < b.len() && !(b[i] == b'*' && b[i + 1] == b'/') { + i += 1; + } + i = (i + 2).min(b.len()); + continue; + } + // Blob literal: x'...' or X'...', but only when `x` starts a token. After + // an identifier character it is concatenation like `max` + `'foo'`, not a + // blob, so the following string is masked by the single-quote branch. + let x_starts_token = i == 0 || !(b[i - 1].is_ascii_alphanumeric() || b[i - 1] == b'_'); + if (c == b'x' || c == b'X') && x_starts_token && i + 1 < b.len() && b[i + 1] == b'\'' { + out.push(b' '); + i += 2; + while i < b.len() && b[i] != b'\'' { + i += 1; + } + i = (i + 1).min(b.len()); + continue; + } + // Single-quoted string with '' escape. + if c == b'\'' { + out.push(b' '); + i += 1; + while i < b.len() { + if b[i] == b'\'' { + if i + 1 < b.len() && b[i + 1] == b'\'' { + i += 2; + continue; + } + i += 1; + break; + } + i += 1; + } + continue; + } + out.push(c); + i += 1; + } + // Collapse whitespace runs to single spaces. + String::from_utf8_lossy(&out) + .split_whitespace() + .collect::>() + .join(" ") +} + +/// Structural predicate: a column named more than once in an `INSERT` target +/// list, an `UPDATE ... SET (cols) = ...` group, or a `USING (cols)` clause. +/// Operates on the masked form so a `)` inside a string literal cannot break the +/// list capture. Identifier comparison is case-insensitive. +fn duplicate_columns(sql: &str) -> bool { + static LISTS: OnceLock> = OnceLock::new(); + let lists = LISTS.get_or_init(|| { + vec![ + // INSERT INTO ( ) + Regex::new(r"(?i)\binsert\s+into\s+[^\s(]+\s*\(([^)]*)\)").unwrap(), + // SET ( ) = + Regex::new(r"(?i)\bset\s*\(([^)]*)\)\s*=").unwrap(), + // USING ( ) + Regex::new(r"(?i)\busing\s*\(([^)]*)\)").unwrap(), + ] + }); + let masked = mask(sql); + for re in lists { + for caps in re.captures_iter(&masked) { + if let Some(list) = caps.get(1) { + if has_duplicate_identifier(list.as_str()) { + return true; + } + } + } + } + false +} + +/// Whether a comma-separated identifier list names any identifier twice +/// (case-insensitive, surrounding quotes or backticks stripped). Case-insensitive +/// comparison is correct for `SQLite` (the only dialect declaring this rule +/// today). A case-sensitive dialect would want quoted identifiers compared as-is. +fn has_duplicate_identifier(list: &str) -> bool { + let mut seen = HashSet::new(); + for part in list.split(',') { + let id = part + .trim() + .trim_matches(|c| c == '"' || c == '`' || c == '[' || c == ']') + .to_ascii_lowercase(); + if id.is_empty() { + continue; + } + if !seen.insert(id) { + return true; + } + } + false +} + +#[cfg(test)] +mod tests { + use super::*; + + fn reg() -> Registry { + Registry::load(Path::new(RULES_DIR)).expect("load registry") + } + + #[test] + fn registry_loads_and_ids_are_unique() { + let r = reg(); + assert!(!r.rules.is_empty(), "expected at least one rule"); + let mut ids: Vec<_> = r.rules.iter().map(|x| x.meta.id.as_str()).collect(); + ids.sort_unstable(); + let n = ids.len(); + ids.dedup(); + assert_eq!(n, ids.len(), "rule ids must be unique"); + } + + #[test] + fn every_rule_matches_its_examples_and_skips_non_matches() { + let r = reg(); + for rule in &r.rules { + // Examples are checked against the dialects the rule declares (the + // first one is enough, a rule fires identically in any allowed one). + let dialect = rule + .meta + .dialects + .iter() + .find_map(|d| Dialect::from_dir_name(d)) + .unwrap_or(Dialect::Sqlite); + for ex in &rule.meta.matches { + let masked = mask(ex); + assert!( + rule.is_match(&masked, ex), + "rule `{}` should match `{ex}`", + rule.meta.id + ); + } + for ex in &rule.meta.non_matches { + let masked = mask(ex); + assert!( + !rule.is_match(&masked, ex), + "rule `{}` should not match `{ex}`", + rule.meta.id + ); + } + // Sanity: the rule applies to at least one real dialect. + assert!( + rule.applies_to(dialect) || rule.meta.dialects.iter().any(|d| d == "all"), + "rule `{}` declares no resolvable dialect", + rule.meta.id + ); + } + } + + #[test] + fn mask_hides_literals_and_comments() { + assert_eq!(mask("SELECT '$::x' AS lit"), "SELECT AS lit"); + assert_eq!(mask("SELECT 1 -- $::x\n, 2"), "SELECT 1 , 2"); + assert_eq!(mask("SELECT /* $::y */ 1"), "SELECT 1"); + assert_eq!(mask("SELECT x'4869' "), "SELECT"); + // `x` ending an identifier is concatenation, not a blob: the identifier + // survives and only the adjacent string is masked. + assert_eq!(mask("SELECT max'foo'"), "SELECT max"); + // Doubled '' escape stays inside one string. + assert_eq!(mask("SELECT 'a''b' x"), "SELECT x"); + // Outside a literal, the token survives. + assert_eq!(mask("select $::xyz"), "select $::xyz"); + } + + #[test] + fn every_category_string_roundtrips() { + for (c, s) in [ + (Category::EngineSpecific, "engine-specific"), + (Category::NonStandard, "non-standard"), + (Category::LossyOrAmbiguous, "lossy-or-ambiguous"), + (Category::Deprecated, "deprecated"), + ] { + assert_eq!(c.as_str(), s); + } + } + + /// Coverage guard against the real corpus: every rule must match at least one + /// engine-valid statement, so a dead or misscoped rule cannot land silently. + /// + /// Scope (a rule never excusing genuinely-invalid SQL) is enforced at the call + /// sites, not here: the classifier is only ever consulted on engine-valid + /// statements (`contentious_valid` filters to valid first, and failure tags + /// come from the expected-valid set), so a rule matching an invalid statement + /// has no effect. A construct like duplicate columns legitimately appears in + /// otherwise-invalid statements too, so we count coverage among valid ones. + #[test] + fn every_rule_covers_at_least_one_valid_corpus_statement() { + crate::datasets::ensure_corpus().expect("prepare datasets"); + let registry = reg(); + for rule in ®istry.rules { + let mut hits = 0usize; + for d in Dialect::ALL + .iter() + .copied() + .filter(|&d| rule.applies_to(d) && crate::has_reference(d)) + { + for s in crate::report::load_dialect(d) { + if crate::reference_accepts(&s, d) != Some(true) { + continue; + } + if rule.is_match(&mask(&s), &s) { + hits += 1; + } + } + } + assert!( + hits > 0, + "rule `{}` matched no engine-valid corpus statement (dead rule)", + rule.meta.id + ); + } + } + + #[test] + fn duplicate_columns_predicate() { + assert!(duplicate_columns( + "INSERT INTO dup1(a,b,c,a,b,c) VALUES(1,2,3,4,5,6)" + )); + assert!(duplicate_columns( + "UPDATE t1 SET (a,a,a,b)=(SELECT 99,100,101,102)" + )); + assert!(duplicate_columns("SELECT y FROM t1 JOIN t1 USING (y,y)")); + assert!(!duplicate_columns("INSERT INTO t(a,b,c) VALUES(1,2,3)")); + assert!(!duplicate_columns("UPDATE t1 SET (a,b)=(SELECT 1,2)")); + assert!(!duplicate_columns("SELECT y FROM t1 JOIN t2 USING (y)")); + // A repeated name inside a string literal must not count. + assert!(!duplicate_columns("INSERT INTO t(a,b) VALUES('x,x,x', 1)")); + } +} diff --git a/src/export.rs b/src/export.rs index f9fff06..60d9acb 100644 --- a/src/export.rs +++ b/src/export.rs @@ -10,12 +10,12 @@ use crate::datasets::Dialect; use crate::report::{self, DialectReport}; -use crate::{bench_dist, stats, BenchParser, Parser}; +use crate::{bench_dist, contentious, stats, BenchParser, Parser}; use std::cmp::Ordering; use std::path::Path; use viz::{ Bundle, CoverageFile, CoverageMatrix, DialectData, MemDist, ParserBatch, ParserFailures, - ParserMem, ParserMetrics, ParserPerf, + ParserMem, ParserMetrics, ParserPerf, RuleMeta, }; /// Output path (relative to repo root, where `cargo run` runs from). @@ -115,11 +115,22 @@ fn metrics(report: &DialectReport) -> Vec { version: p.version.to_string(), accepted_valid: s.accepted_valid, accepted_invalid: s.accepted_invalid, + accepted_valid_contentious: s.accepted_valid_contentious, recall_pct: if reference { pct(s.accepted_valid, report.valid_total) } else { None }, + recall_excl_contentious_pct: if reference { + // Drop contentious valid statements from both numerator and + // denominator. `pct` returns None when V == Cv (zero base). + pct( + s.accepted_valid - s.accepted_valid_contentious, + report.valid_total - report.contentious_valid, + ) + } else { + None + }, false_positive_pct: if reference && report.invalid_total > 0 { pct(s.accepted_invalid, report.invalid_total) } else { @@ -288,7 +299,7 @@ fn read_batch_mem() -> Vec { /// Merge batch time and batch memory rows for one dialect into per-parser /// `ParserBatch`. Every batch-capable parser has a time row carrying its accuracy -/// (the time bench runs them all), so it appears here; memory is added where the +/// (the time bench runs them all), so it appears here. Memory is added where the /// parser's allocations are Rust-visible. Pure, so the merge is testable. fn batch_for(dir: &str, perf: &[BatchPerfRow], mem: &[BatchMemRow]) -> Vec { use std::collections::BTreeMap; @@ -407,6 +418,8 @@ fn failures_for(dir: &str, parsers: &[&dyn Parser]) -> Vec { expected_total: f.total, preview_html: Vec::new(), preview_reasons: Vec::new(), + preview_sql: Vec::new(), + preview_tags: Vec::new(), download: None, }); continue; @@ -423,6 +436,25 @@ fn failures_for(dir: &str, parsers: &[&dyn Parser]) -> Vec { .take(FAIL_PREVIEW) .cloned() .collect::>(); + // Raw statement text for each previewed row, aligned with `preview`, so the + // failures-view feedback buttons can prefill an issue with the statement. + let preview_sql = f + .rejected + .iter() + .take(FAIL_PREVIEW) + .cloned() + .collect::>(); + // Tag each previewed statement with the contentious rule it matches (if + // any), so the viewer can mark it an intentional divergence. Rules are + // dialect-scoped, so this is a no-op where no rule applies. + let preview_tags = preview_sql + .iter() + .map(|s| { + contentious::registry() + .classify(s, dialect) + .map(|r| r.meta.id.clone()) + }) + .collect::>(); let file = format!("{dir}__{}.tsv.zst", stats::slug(name)); match write_failure_tsv(&file, &f.rejected, &f.reasons) { Ok(()) => out.push(ParserFailures { @@ -431,6 +463,8 @@ fn failures_for(dir: &str, parsers: &[&dyn Parser]) -> Vec { expected_total: f.total, preview_html: preview, preview_reasons, + preview_sql, + preview_tags, download: Some(format!("failures/{file}")), }), Err(e) => { @@ -441,6 +475,8 @@ fn failures_for(dir: &str, parsers: &[&dyn Parser]) -> Vec { expected_total: f.total, preview_html: preview, preview_reasons, + preview_sql, + preview_tags, download: None, }); } @@ -570,6 +606,7 @@ pub fn run() -> Result<(), Box> { has_reference: report.has_reference, valid_total: report.valid_total, invalid_total: report.invalid_total, + contentious_valid: report.contentious_valid, correctness: metrics(&report), perf: perf_for(d.dir_name(), &summary), coverage: coverage_for(d, &dyn_parsers), @@ -584,6 +621,17 @@ pub fn run() -> Result<(), Box> { git_commit: git_short(), parsers: parsers.iter().map(|p| p.name().to_string()).collect(), dialects, + contentious_rules: contentious::registry() + .rules + .iter() + .map(|r| RuleMeta { + id: r.meta.id.clone(), + title: r.meta.title.clone(), + category: r.meta.category.as_str().to_string(), + description: r.meta.description.clone(), + references: r.meta.references.clone(), + }) + .collect(), }; // Compact JSON, zstd-compressed: the viewer embeds and decompresses it in diff --git a/src/lib.rs b/src/lib.rs index d3f31b4..d264b14 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -914,6 +914,7 @@ impl Parser for BenchParser { pub mod batch; pub mod bench_dist; +pub mod contentious; pub mod datasets; pub mod export; pub mod mem; diff --git a/src/report.rs b/src/report.rs index f2a4f16..21bd1db 100644 --- a/src/report.rs +++ b/src/report.rs @@ -9,7 +9,7 @@ //! for speed. use crate::datasets::Dialect; -use crate::{has_reference, reference_accepts, Parser, ParserId}; +use crate::{contentious, has_reference, reference_accepts, Parser, ParserId}; use std::fs; use std::path::{Path, PathBuf}; @@ -27,6 +27,10 @@ pub struct ParserStat { /// provenance dialect (no reference) every statement is treated as valid, so /// this is the plain acceptance count. pub accepted_valid: usize, + /// Accepted among reference-valid statements that are also contentious (the + /// `Avc` subtracted from the secondary "recall excluding contentious" metric). + /// Always a subset of `accepted_valid`. Reference dialects only. + pub accepted_valid_contentious: usize, /// Accepted among reference-invalid statements (false-positive numerator). pub accepted_invalid: usize, /// Round-trip-stable among accepted-valid. @@ -43,6 +47,7 @@ pub struct ParserStat { impl ParserStat { const fn merge(&mut self, other: &Self) { self.accepted_valid += other.accepted_valid; + self.accepted_valid_contentious += other.accepted_valid_contentious; self.accepted_invalid += other.accepted_invalid; self.roundtrip_ok += other.roundtrip_ok; self.attempted += other.attempted; @@ -55,6 +60,10 @@ pub struct DialectReport { pub dialect: Dialect, pub has_reference: bool, pub valid_total: usize, + /// Reference dialects: valid statements matched by at least one contentious + /// rule (`Cv`, the denominator adjustment for the secondary metric). 0 on + /// provenance dialects. + pub contentious_valid: usize, pub invalid_total: usize, /// Identity (family + version) of each graded parser, aligned with `stats`. pub parsers: Vec, @@ -69,6 +78,7 @@ impl DialectReport { dialect, has_reference: has_reference(dialect), valid_total: 0, + contentious_valid: 0, invalid_total: 0, parsers: parsers.iter().map(|p| p.id()).collect(), stats: parsers @@ -84,6 +94,7 @@ impl DialectReport { /// Add another report's tallies (same dialect and parser order assumed). pub fn merge(&mut self, other: &Self) { self.valid_total += other.valid_total; + self.contentious_valid += other.contentious_valid; self.invalid_total += other.invalid_total; for (a, b) in self.stats.iter_mut().zip(other.stats.iter()) { a.merge(b); @@ -115,6 +126,14 @@ pub fn grade_chunk(stmts: &[String], dialect: Dialect, parsers: &[&dyn Parser]) } else { report.invalid_total += 1; } + // Classify once per statement (a property of the SQL, not the parser). + // Only reference dialects carry a contentious layer, and the rules are + // dialect-scoped, so this is a no-op elsewhere. + let is_contentious = + reference && is_valid && contentious::registry().classify(sql, dialect).is_some(); + if is_contentious { + report.contentious_valid += 1; + } for (i, &p) in parsers.iter().enumerate() { // A panic is still a non-acceptance (it does not enter the accepted @@ -134,6 +153,9 @@ pub fn grade_chunk(stmts: &[String], dialect: Dialect, parsers: &[&dyn Parser]) } if is_valid { report.stats[i].accepted_valid += 1; + if is_contentious { + report.stats[i].accepted_valid_contentious += 1; + } if report.stats[i].can_reprint && p.roundtrips(sql, dialect) == Some(true) { report.stats[i].roundtrip_ok += 1; } diff --git a/viz/src/chart.rs b/viz/src/chart.rs index 71cc492..3a3f4b6 100644 --- a/viz/src/chart.rs +++ b/viz/src/chart.rs @@ -650,6 +650,7 @@ mod tests { has_reference: true, valid_total: 90, invalid_total: 10, + contentious_valid: 0, correctness: vec![], perf: vec![perf], coverage: CoverageMatrix { diff --git a/viz/src/lib.rs b/viz/src/lib.rs index 1c9cc8e..3569316 100644 --- a/viz/src/lib.rs +++ b/viz/src/lib.rs @@ -21,5 +21,5 @@ pub use marker::{marker_for, Marker}; pub use schema::{ Bundle, CoverageFile, CoverageMatrix, DepthReport, DepthScan, DialectData, DialectRun, FamilyHistory, FeatureCounts, FeatureScan, LintPolicy, MemDist, ParserBatch, ParserFailures, - ParserFeatures, ParserMem, ParserMetrics, ParserPerf, VersionRun, + ParserFeatures, ParserMem, ParserMetrics, ParserPerf, RuleMeta, VersionRun, }; diff --git a/viz/src/schema.rs b/viz/src/schema.rs index 7d30f4c..dc39b63 100644 --- a/viz/src/schema.rs +++ b/viz/src/schema.rs @@ -129,6 +129,23 @@ pub struct Bundle { pub parsers: Vec, /// One entry per dialect, in display order. pub dialects: Vec, + /// Display metadata for every contentious-construct rule, so the viewer's + /// legend and per-row badges are data-driven (empty in older snapshots). + #[serde(default)] + pub contentious_rules: Vec, +} + +/// Display metadata for one contentious-construct rule, copied into the export so +/// the web viewer needs no code change when a rule is added. The matching logic +/// stays in the native crate. +#[derive(Serialize, Deserialize, Clone, Debug)] +pub struct RuleMeta { + pub id: String, + pub title: String, + /// Kebab-case category (`engine-specific`, `non-standard`, ...). + pub category: String, + pub description: String, + pub references: Vec, } /// Everything the viewer shows for one dialect. @@ -139,6 +156,10 @@ pub struct DialectData { pub has_reference: bool, pub valid_total: usize, pub invalid_total: usize, + /// Reference dialects: number of valid statements matched by at least one + /// contentious rule (`Cv`). 0 in older snapshots and on provenance dialects. + #[serde(default)] + pub contentious_valid: usize, /// Per-parser correctness metrics (reference dialects) or acceptance /// (provenance dialects). pub correctness: Vec, @@ -247,6 +268,18 @@ pub struct ParserFailures { /// `preview_html` (same index). Plain text, escaped by the viewer at render. #[serde(default)] pub preview_reasons: Vec, + /// The raw SQL of each previewed statement, aligned with `preview_html` (same + /// index). The highlighted `preview_html` is not usable as plain text, so the + /// failures-view feedback buttons read the statement from here to prefill a + /// GitHub issue. Empty in older snapshots. + #[serde(default)] + pub preview_sql: Vec, + /// The contentious-rule id matched by each previewed statement, aligned with + /// `preview_html` (same index), or `None` for a genuine gap. Drives the + /// per-row "intentional divergence" badge and the dispute button. Empty in + /// older snapshots. + #[serde(default)] + pub preview_tags: Vec>, /// Path (relative to the site root) of the full `.tsv.zst` download, or /// `None` when there were no failures to ship. pub download: Option, @@ -264,8 +297,19 @@ pub struct ParserMetrics { pub version: String, pub accepted_valid: usize, pub accepted_invalid: usize, + /// Reference dialects: accepted-valid statements that are also contentious + /// (`Avc`), the numerator adjustment for `recall_excl_contentious_pct`. 0 in + /// older snapshots. + #[serde(default)] + pub accepted_valid_contentious: usize, /// Reference dialects: accepted among reference-valid. pub recall_pct: Option, + /// Reference dialects: recall excluding contentious constructs, + /// `(accepted_valid - accepted_valid_contentious) / (valid_total - + /// contentious_valid)`. `None` on provenance dialects, in older snapshots, or + /// when every valid statement is contentious. + #[serde(default)] + pub recall_excl_contentious_pct: Option, /// Reference dialects: accepted among reference-invalid (lower is better). pub false_positive_pct: Option, /// Display round-trip rate among accepted (None without a printer). diff --git a/web/assets/bench.json.zst b/web/assets/bench.json.zst index 1715f30..189ed13 100644 Binary files a/web/assets/bench.json.zst and b/web/assets/bench.json.zst differ diff --git a/web/assets/main.css b/web/assets/main.css index 0aea08d..229328b 100644 --- a/web/assets/main.css +++ b/web/assets/main.css @@ -86,7 +86,7 @@ main { display: block; } .intro { padding: 2rem 1.2rem 0.5rem; } .intro h1 { font-size: 1.9rem; margin: 0 0 0.3em; letter-spacing: -0.01em; } -/* Title row with the snapshot/commit pills; wraps under the title on mobile. */ +/* Title row with the snapshot/commit pills, wraps under the title on mobile. */ .intro-head { display: flex; flex-wrap: wrap; align-items: baseline; gap: 0.4rem 0.8rem; } .intro-head h1 { margin-bottom: 0; } .stamp { display: flex; flex-wrap: wrap; gap: 0.4rem; } @@ -270,7 +270,7 @@ main { display: block; } border-radius: 999px; font-size: 0.78rem; } -/* The source-repository pill is a link; keep the pill look, add a hover cue. */ +/* The source-repository pill is a link. Keep the pill look, add a hover cue. */ .meta-link { text-decoration: none; color: inherit; cursor: pointer; transition: border-color 0.12s ease, background 0.12s ease; } .meta-link:hover { border-color: var(--accent); background: var(--card-hover, #eef2f7); } .meta-val { font-weight: 700; color: var(--fg); } @@ -280,7 +280,7 @@ main { display: block; } letter-spacing: 0.03em; font-size: 0.7rem; } -/* Constrain the icon so it never balloons; size is set on the SVG by the icon. */ +/* Constrain the icon so it never balloons. Size is set on the SVG by the icon. */ .meta-ico { flex: none; display: inline-flex; color: var(--accent); } .meta-ico svg { width: 12px; height: 12px; display: block; } /* A problematic fact (not fuzzed, no test suite, no benchmarks) flagged red. */ @@ -475,11 +475,21 @@ main { display: block; } background: #fbfbfb; color: #1f2328; } -/* Copy-to-clipboard button, revealed on hover over a preview. */ -.copy-btn { +/* Action buttons (copy, and the two feedback reports) over a preview, revealed + on hover or when one is focused. */ +.fail-actions { position: absolute; top: 6px; right: 6px; + display: inline-flex; + gap: 4px; + opacity: 0; + transition: opacity 0.12s ease; +} +.fail-code-wrap:hover .fail-actions, +.fail-actions:focus-within { opacity: 1; } +.copy-btn, +.report-btn { display: inline-flex; align-items: center; justify-content: center; @@ -491,12 +501,11 @@ main { display: block; } background: #fff; color: var(--muted); cursor: pointer; - opacity: 0; - transition: opacity 0.12s ease, color 0.12s ease, border-color 0.12s ease; + text-decoration: none; + transition: color 0.12s ease, border-color 0.12s ease; } -.fail-code-wrap:hover .copy-btn, -.copy-btn:focus-visible { opacity: 1; } -.copy-btn:hover { color: #1f2328; border-color: #b9b9b9; } +.copy-btn:hover, +.report-btn:hover { color: #1f2328; border-color: #b9b9b9; } /* The parser's error message for a rejected statement, under its preview. */ .fail-reason { margin: 0.2rem 0 0; @@ -509,6 +518,47 @@ main { display: block; } white-space: pre-wrap; overflow-wrap: anywhere; } +/* Marks a previewed statement as a known contentious construct (an intentional + divergence the engine accepts but a parser may reasonably reject). Neutral and + non-red on purpose: red is reserved for genuine rejections, and the category is + carried as text rather than a color, per the contentious-constructs design. */ +.contentious-badge { + margin: 0.2rem 0 0; + padding: 0.3rem 0.5rem 0.3rem 0.7rem; + border-left: 3px solid #c4c8d4; + background: #eef0f4; + color: #45506b; + font-size: 0.78rem; + line-height: 1.35; + overflow-wrap: anywhere; +} +/* Legend explaining the intentional-divergence badges, shown on the failures + view when a parser has at least one tagged statement. */ +.contentious-legend { + margin: 0.2rem 0 0.9rem; + padding: 0.5rem 0.7rem; + border: 1px solid var(--line); + border-radius: 8px; + background: #f7f8fa; + font-size: 0.8rem; + color: var(--muted); +} +.contentious-legend h3 { + font-size: 0.82rem; + margin: 0 0 0.3rem; + color: var(--fg); +} +.contentious-legend ul { margin: 0; padding-left: 1.1rem; } +.contentious-legend li { margin: 0.15rem 0; } +.contentious-legend .cat { color: var(--fg); font-weight: 600; } +/* Secondary value under a table cell (e.g. recall excluding contentious). */ +.cell-sub { + margin-top: 0.1rem; + font-size: 0.7rem; + font-weight: 400; + color: var(--muted); + white-space: nowrap; +} /* Tables */ .scroll { overflow-x: auto; } diff --git a/web/src/components.rs b/web/src/components.rs index 95348fe..86d3c27 100644 --- a/web/src/components.rs +++ b/web/src/components.rs @@ -700,13 +700,14 @@ pub fn ParserView(name: String) -> Element { name: d.display_name.clone(), }, cells: vec![ - Cell::pct(m.and_then(|m| { - if d.has_reference { - m.recall_pct - } else { - m.accept_pct - } - })), + if d.has_reference { + Cell::recall( + m.and_then(|m| m.recall_pct), + m.and_then(|m| m.recall_excl_contentious_pct), + ) + } else { + Cell::pct(m.and_then(|m| m.accept_pct)) + }, Cell::pct(m.and_then(|m| m.false_positive_pct)), Cell::pct(m.and_then(|m| m.roundtrip_pct)), Cell::ns(p.map(|p| p.median)), @@ -1189,23 +1190,124 @@ fn parser_memory_section(b: &viz::Bundle, parser: &str) -> Element { /// rejected-statement count, a short syntax-highlighted preview, and a download /// link to the full capped `.tsv.zst`. Renders nothing if the parser rejected /// nothing anywhere (or has no recorded failure data). +/// Render one failing-statement preview: the highlighted SQL, the copy button, +/// the per-row feedback buttons (reference dialects only), and, when the +/// statement matches a contentious rule, an intentional-divergence badge with a +/// dispute button in place of the "propose" button. +#[allow(clippy::too_many_arguments)] +fn fail_preview_row( + b: &viz::Bundle, + di: usize, + i: usize, + dialect: &str, + has_ref: bool, + parser: &str, + f: &viz::ParserFailures, + html: &str, +) -> Element { + let reason = f.preview_reasons.get(i).map(String::as_str).unwrap_or(""); + let sql = f.preview_sql.get(i).map(String::as_str); + // The contentious rule this statement matched, if any, resolved against the + // exported rule metadata. + let rule = f + .preview_tags + .get(i) + .and_then(|t| t.as_deref()) + .and_then(|id| b.contentious_rules.iter().find(|r| r.id == id)); + rsx! { + div { class: "fail-code-wrap", key: "{i}", + div { class: "fail-actions", + button { + class: "copy-btn", + r#type: "button", + aria_label: "Copy this statement to the clipboard", + title: "Copy statement", + onclick: move |_| { + document::eval(&format!( + "{{ const el = document.getElementById('fail-{di}-{i}'); if (el) {{ navigator.clipboard.writeText(el.textContent); }} }}" + )); + }, + Icon { width: 13, height: 13, fill: "currentColor".to_string(), icon: FaCopy } + } + // Feedback reports only where the engine assigned a validity label. + if has_ref { + if let Some(sql) = sql { + a { + class: "report-btn", + href: issue_url("should-be-rejected.yml", dialect, parser, sql, reason, &[]), + target: "_blank", + rel: "noopener noreferrer", + aria_label: "Report that this statement should be rejected as invalid", + title: "Should be rejected (invalid syntax)", + Icon { width: 13, height: 13, fill: "currentColor".to_string(), icon: FaCircleXmark } + } + if let Some(rule) = rule { + // Already tagged contentious: let people argue it is not. + a { + class: "report-btn", + href: issue_url("not-contentious.yml", dialect, parser, sql, reason, &[("rule", rule.title.as_str())]), + target: "_blank", + rel: "noopener noreferrer", + aria_label: "Dispute this: argue the statement is not actually contentious", + title: "Dispute: argue this is not contentious", + Icon { width: 13, height: 13, fill: "currentColor".to_string(), icon: FaBan } + } + } else { + a { + class: "report-btn", + href: issue_url("contentious-construct.yml", dialect, parser, sql, reason, &[]), + target: "_blank", + rel: "noopener noreferrer", + aria_label: "Propose this as a contentious construct a parser may reasonably reject", + title: "Parser may reject (contentious)", + Icon { width: 13, height: 13, fill: "currentColor".to_string(), icon: FaScaleBalanced } + } + } + } + } + } + pre { id: "fail-{di}-{i}", class: "fail-code", dangerous_inner_html: "{html}" } + if let Some(rule) = rule { + div { class: "contentious-badge", title: "{rule.description}", + "Intentional divergence: {rule.title} ({rule.category})" + } + } + if !reason.is_empty() { + div { class: "fail-reason", "{reason}" } + } + } + } +} + fn failures_section(b: &viz::Bundle, parser: &str) -> Element { // Gather (dialect display name, failures) for dialects where this parser // has at least one rejected statement. - let entries: Vec<(&str, &viz::ParserFailures)> = b + let entries: Vec<(&str, bool, &viz::ParserFailures)> = b .dialects .iter() .filter_map(|d| { d.failures .iter() .find(|f| f.parser == parser && f.rejected_total > 0) - .map(|f| (d.display_name.as_str(), f)) + .map(|f| (d.display_name.as_str(), d.has_reference, f)) }) .collect(); if entries.is_empty() { return rsx! {}; } + // Rules that tag at least one of this parser's previewed statements, used to + // build a legend explaining the badges below. + let tagged: std::collections::BTreeSet<&str> = entries + .iter() + .flat_map(|(_, _, f)| f.preview_tags.iter().filter_map(Option::as_deref)) + .collect(); + let legend_rules: Vec<&viz::RuleMeta> = b + .contentious_rules + .iter() + .filter(|r| tagged.contains(r.id.as_str())) + .collect(); + rsx! { section { class: "block", h2 { @@ -1215,7 +1317,29 @@ fn failures_section(b: &viz::Bundle, parser: &str) -> Element { p { class: "fail-intro", "Statements this parser was expected to accept but rejected. Each dialect links to the full set (capped at 1,000) as a compressed TSV." } - for (di , (dialect , f)) in entries.into_iter().enumerate() { + if !legend_rules.is_empty() { + div { class: "contentious-legend", + h3 { "Intentional divergences" } + p { + "Statements tagged below are engine-accepted constructs a parser may reasonably reject. They are flagged for context and do not change the strict recall above." + } + ul { + for r in legend_rules.iter() { + li { key: "{r.id}", + span { class: "cat", "{r.title}" } + " ({r.category}): {r.description}" + for (k , href) in r.references.iter().enumerate() { + span { key: "{k}", + " " + a { class: "inline-link", href: "{href}", target: "_blank", rel: "noopener noreferrer", "[ref]" } + } + } + } + } + } + } + } + for (di , (dialect , has_ref , f)) in entries.into_iter().enumerate() { div { class: "fail-dialect", key: "{dialect}", div { class: "fail-head", span { class: "fail-title", @@ -1233,24 +1357,7 @@ fn failures_section(b: &viz::Bundle, parser: &str) -> Element { } } for (i , html) in f.preview_html.iter().enumerate() { - div { class: "fail-code-wrap", key: "{i}", - button { - class: "copy-btn", - r#type: "button", - aria_label: "Copy this statement to the clipboard", - title: "Copy statement", - onclick: move |_| { - document::eval(&format!( - "{{ const el = document.getElementById('fail-{di}-{i}'); if (el) {{ navigator.clipboard.writeText(el.textContent); }} }}" - )); - }, - Icon { width: 13, height: 13, fill: "currentColor".to_string(), icon: FaCopy } - } - pre { id: "fail-{di}-{i}", class: "fail-code", dangerous_inner_html: "{html}" } - if let Some(reason) = f.preview_reasons.get(i).filter(|r| !r.is_empty()) { - div { class: "fail-reason", "{reason}" } - } - } + {fail_preview_row(b, di, i, dialect, has_ref, parser, f, html)} } } } @@ -1258,6 +1365,53 @@ fn failures_section(b: &viz::Bundle, parser: &str) -> Element { } } +// ---- feedback issue links ---- + +/// Percent-encode a value for safe inclusion in a URL query string. Only the +/// RFC 3986 unreserved set passes through unescaped, so newlines, spaces, and SQL +/// punctuation are all encoded. +fn url_encode(s: &str) -> String { + let mut out = String::with_capacity(s.len()); + for b in s.bytes() { + match b { + b'A'..=b'Z' | b'a'..=b'z' | b'0'..=b'9' | b'-' | b'_' | b'.' | b'~' => { + out.push(b as char); + } + _ => out.push_str(&format!("%{b:02X}")), + } + } + out +} + +/// Build a link to a prefilled GitHub issue form. `template` is the issue-form +/// file name under `.github/ISSUE_TEMPLATE/`, and the values prefill the form +/// fields with matching `id`s. `extra` carries any form-specific fields (e.g. the +/// disputed rule). Long statements may be truncated by GitHub's URL length limit, +/// in which case the reader pastes via the copy button. +fn issue_url( + template: &str, + dialect: &str, + parser: &str, + sql: &str, + reason: &str, + extra: &[(&str, &str)], +) -> String { + let mut url = format!( + "{REPO}/issues/new?template={template}&dialect={}&parser={}&statement={}&parser_error={}", + url_encode(dialect), + url_encode(parser), + url_encode(sql), + url_encode(reason), + ); + for (key, value) in extra { + url.push('&'); + url.push_str(key); + url.push('='); + url.push_str(&url_encode(value)); + } + url +} + // ---- formatting helpers ---- fn commas(n: usize) -> String { @@ -1743,7 +1897,7 @@ fn empirical_panic_pill(totals: Option<(usize, usize)>) -> Element { /// expected to accept, summed across every dialect. Neutral (informational): /// every parser misses some real-world SQL, so this is a coverage figure, not an /// alarm, and a red flag on every parser would dilute the genuine red flags. The -/// value is the absolute count the user asked for; the rate is in the tooltip. +/// value is the absolute count the user asked for, and the rate is in the tooltip. fn failures_pill(totals: Option<(usize, usize)>) -> Element { let (value, desc) = match totals { None => ( @@ -1950,6 +2104,9 @@ impl Head { struct Cell { text: String, num: Option, + /// Optional small grey second line under the value (e.g. recall excluding + /// contentious constructs). Display only, never a sort key. + sub: Option, } impl Cell { @@ -1958,6 +2115,23 @@ impl Cell { Cell { text: fmt_pct(v), num: v, + sub: None, + } + } + /// Recall cell with a grey sub-line showing recall excluding contentious + /// constructs, shown only when it differs from strict recall. The strict value + /// stays the text and the sort key, so the headline number is unchanged. + fn recall(strict: Option, excl: Option) -> Cell { + let sub = match (strict, excl) { + (Some(s), Some(e)) if (s - e).abs() >= 0.05 => { + Some(format!("{} excl. contentious", fmt_pct(Some(e)))) + } + _ => None, + }; + Cell { + text: fmt_pct(strict), + num: strict, + sub, } } /// Nanosecond cell from an optional value (comma-grouped, "N/A" if missing). @@ -1965,6 +2139,7 @@ impl Cell { Cell { text: v.map_or_else(|| "N/A".to_string(), |x| commas(x as usize)), num: v, + sub: None, } } /// Byte cell from an optional value, formatted as B / KB / MB ("N/A" if missing). @@ -1972,11 +2147,16 @@ impl Cell { Cell { text: v.map_or_else(|| "N/A".to_string(), fmt_bytes), num: v, + sub: None, } } /// Cell with explicit text and a numeric sort key. fn with(text: String, num: Option) -> Cell { - Cell { text, num } + Cell { + text, + num, + sub: None, + } } } @@ -2169,7 +2349,12 @@ fn SortTable( tr { key: "{row.key}", {render_head(&row.head)} for cell in row.cells.iter() { - td { "{cell.text}" } + td { + "{cell.text}" + if let Some(sub) = &cell.sub { + div { class: "cell-sub", "{sub}" } + } + } } } } @@ -2365,7 +2550,7 @@ fn correctness_table(d: &DialectData) -> Element { head: Head::Parser(m.parser.clone()), cells: if reference { vec![ - Cell::pct(m.recall_pct), + Cell::recall(m.recall_pct, m.recall_excl_contentious_pct), Cell::pct(m.false_positive_pct), Cell::pct(m.roundtrip_pct), ]