From a4f97cd038ed4da676d6ead2f48d85f17d5eac8a Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 10 Oct 2026 08:24:18 +0000 Subject: [PATCH 1/3] ruff_python_spo odoo_regions: resolve extension views through inherit_id An Odoo extension view patches its parent with locators (an element, or any element carrying `position`, e.g. ). The harvester docked every control in an extension view at the fallback token "root", and the shorthand form was worse: the locator itself was emitted as a control, and the controls it added were skipped as comodel fields because they sat under a element. Now a two-pass join: - views are collected per record with their inherit_id (joined on the local part of the xml id, the odoo_nav convention); - each view is walked after its parent; it records anchors (fields, buttons, containers, named elements) with the region each sits in; - a locator resolves its target against the parent chain's anchors: after/before/replace -> the target's region, inside a container -> that container, inside a field -> comodel subview (not docked), attributes -> no controls. A container opened inside the locator block still wins for its contents. A locator is never a control. - only bare-tag or @name-filtered xpath steps resolve; [hasclass(..)], [@string=..], positional or * steps stay unresolved (-> "root") rather than matching the first element of that tag. Also fixes stack balance: a self-closing non-container element (, ) pushed a frame no close ever popped, so the arch wrapper's popped it instead of ending the arch. Measured on Odoo 19 addons/ (old = origin/main, same harness): facts 17,154 -> 17,298; screens 3,171 -> 3,092; docked at root 3,513 (20.5%) -> 2,170 (12.5%), -38%. Remaining root: non-@name xpaths and parents outside the scanned tree. Tests: 7 new on an invented fixture (each position, both locator forms, inside-a-field comodel, unresolvable predicate, missing parent, extension of an extension, stack balance, xpath step parsing). ruff_python_spo 91/91; clippy -D warnings clean on 1.98.1. Disable runs: no join -> 3 red; self-closing frame push -> 1 red. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01NSfzujtWL1BnHgnJGAhg9N --- crates/ruff_python_spo/src/odoo_regions.rs | 766 ++++++++++++++++++--- 1 file changed, 685 insertions(+), 81 deletions(-) diff --git a/crates/ruff_python_spo/src/odoo_regions.rs b/crates/ruff_python_spo/src/odoo_regions.rs index 361ac92f3f246..cbf8ccee0d791 100644 --- a/crates/ruff_python_spo/src/odoo_regions.rs +++ b/crates/ruff_python_spo/src/odoo_regions.rs @@ -33,12 +33,38 @@ //! `act_window` join that [`super::odoo_nav`] already parses; that refinement //! is the named follow-up, not fabricated here. //! -//! A control with NO region-bearing ancestor docks at the honest fallback -//! token `"root"` — this is the shape of an **extension view** (`` patch) whose true region lives in the parent view resolved -//! via `inherit_id`. That cross-record join (the mirror of `odoo_nav`'s -//! cross-file Shape B) is the named follow-up; until then the consumer -//! `region=` config maps `root` to the main content region (`root:center`). +//! # Extension views: the `inherit_id` join +//! +//! An **extension view** (``) does not carry +//! a full arch. It patches its parent with **locators**: an `` element, or the shorthand form, any element that +//! carries a `position` attribute (``). +//! A locator is never a control itself. The controls it brings dock in the +//! parent's region at the locator's TARGET: +//! +//! - `after` / `before` / `replace` → the region the target sits in; +//! - `inside` a region container → that container's token; +//! - `inside` a `` → the field's comodel subview, so not docked (the +//! depth-0 rule); +//! - `attributes` → no controls. +//! +//! A container opened INSIDE the locator block still wins for the controls it +//! wraps (a new `` docks its fields at `group`). Targets resolve against +//! the parent's anchors, then its own parent's, up the `inherit_id` chain, so +//! an extension of an extension resolves through what the first one added. +//! References join on the LOCAL part of an xml id (`analytic.view_x` → +//! `view_x`), the convention [`super::odoo_nav`] uses for cross-file refs. +//! +//! Only `name`-addressed or bare-tag targets resolve (`//field[@name='x']`, +//! `//sheet`); an xpath whose last step filters on anything else +//! (`[hasclass(…)]`, `[@string=…]`, positional) is left unresolved rather than +//! matched to the first element of that tag. An unresolved locator (unknown +//! target, or a parent outside the scanned root) docks its controls at the +//! honest fallback token `"root"`, which the consumer `region=` config maps to +//! the main content region (`root:center`). +//! +//! A control with NO region-bearing ancestor in a base view docks at `"root"` +//! for the same reason. use std::fs; use std::path::Path; @@ -86,19 +112,21 @@ pub const REGION_CONTAINERS: &[&str] = &[ // dot — the dotless control is always the final segment.) /// Scan `` for Odoo view XML and extract, per `ir.ui.view` record, the -/// region placement of every depth-0 control in its arch. +/// region placement of every depth-0 control in its arch. Extension views are +/// resolved against their `inherit_id` parent across every scanned file. #[must_use] pub fn extract_odoo_view_regions(root: &Path) -> Vec { let mut files = Vec::new(); collect_xml_files(root, &mut files); - let mut facts = Vec::new(); + let mut views = Vec::new(); for path in &files { let Ok(content) = fs::read_to_string(path) else { continue; }; let rel = relative_path(root, path); - scan_file_regions(&content, &rel, &mut facts); + collect_views(&content, &rel, &mut views); } + let mut facts = regions_of_views(&views); facts.sort_by(|a, b| { a.screen .cmp(&b.screen) @@ -108,20 +136,23 @@ pub fn extract_odoo_view_regions(root: &Path) -> Vec { } /// Extract region facts straight from an XML string (fixture/testing entry; -/// `rel` is the synthetic screen-file label). +/// `rel` is the synthetic screen-file label). Extension views resolve against +/// parents in the same string only. #[must_use] pub fn extract_regions_from_source(content: &str, rel: &str) -> Vec { - let mut facts = Vec::new(); - scan_file_regions(content, rel, &mut facts); - facts + let mut views = Vec::new(); + collect_views(content, rel, &mut views); + regions_of_views(&views) } -/// A classified arch tag in document order. +/// A classified tag in document order. enum ArchTag { /// `` opens a view record. ViewRecordOpen { id: Option }, /// `` closes any record. RecordClose, + /// `` — the view's parent. + InheritRef(String), /// The arch wrapper `` opens the arch body. ArchOpen { self_closing: bool }, /// A `` control inside the arch. `name` is its field name. @@ -135,57 +166,246 @@ enum ArchTag { opens_popup: Option, self_closing: bool, }, - /// A region-bearing container open (its normalized dock token). - ContainerOpen { token: String, self_closing: bool }, - /// Any element close `` — pops one arch stack entry. + /// A region-bearing container open: its raw tag, its `name` attribute and + /// its normalized dock token. + ContainerOpen { + tag: String, + name: Option, + token: String, + self_closing: bool, + }, + /// An extension-view locator: an `` or any element carrying a + /// `position` attribute. `target` is `(tag, name)` when it is resolvable. + LocatorOpen { + target: Option<(String, Option)>, + position: String, + self_closing: bool, + }, + /// Any element close ``. ElementClose, - /// Any other non-self-closing element open (kept on the stack so open/close - /// balance holds; not a region container). - OtherOpen { token: String }, + /// Any other element open (kept so open/close balance holds; not a region + /// container, but addressable as a locator target by tag and name). + OtherOpen { + tag: String, + name: Option, + self_closing: bool, + }, } -/// Position-ordered region scan of one XML file. Maintains, per view record, a -/// full arch element stack; a control docks at the innermost container on it. -fn scan_file_regions(content: &str, rel: &str, out: &mut Vec) { - let mut screen: Option = None; - let mut in_arch = false; - // The arch element stack (dock tokens for containers, `field`/other for the - // rest) — used both for innermost-container lookup and the depth-0 rule. - let mut stack: Vec = Vec::new(); - let mut order = 0usize; +impl ArchTag { + /// Whether this open pushes a frame that a later `` pops. + fn opens_frame(&self) -> bool { + match self { + Self::FieldOpen { self_closing, .. } + | Self::ButtonOpen { self_closing, .. } + | Self::ContainerOpen { self_closing, .. } + | Self::LocatorOpen { self_closing, .. } + | Self::OtherOpen { self_closing, .. } => !self_closing, + _ => false, + } + } +} + +/// One `ir.ui.view` record: its screen label, the local part of its xml id, +/// its parent's local id (extension views), and its arch-body tags. +struct ViewRec { + screen: String, + local_id: String, + inherit: Option, + arch: Vec, +} +/// The local part of an xml id (`module.name` → `name`). +fn local_id(id: &str) -> String { + id.rsplit_once('.').map_or(id, |(_, l)| l).to_string() +} + +/// Split one file into view records, each with its arch-body tags. +fn collect_views(content: &str, rel: &str, out: &mut Vec) { + let mut cur: Option = None; + let mut in_arch = false; + let mut depth = 0usize; for tag in arch_tags(content) { match tag { ArchTag::ViewRecordOpen { id } => { - screen = id.map(|i| format!("{rel}#{i}")); + out.extend(cur.take()); + cur = id.map(|i| ViewRec { + screen: format!("{rel}#{i}"), + local_id: local_id(&i), + inherit: None, + arch: Vec::new(), + }); in_arch = false; - stack.clear(); - order = 0; } ArchTag::RecordClose => { - screen = None; + out.extend(cur.take()); in_arch = false; - stack.clear(); + } + ArchTag::InheritRef(r) => { + if !in_arch && let Some(v) = cur.as_mut() { + v.inherit = Some(local_id(&r)); + } } ArchTag::ArchOpen { self_closing } => { - if screen.is_some() && !self_closing { + if cur.is_some() && !self_closing { in_arch = true; - stack.clear(); + depth = 0; } } - ArchTag::FieldOpen { name, self_closing } => { + ArchTag::ElementClose => { if !in_arch { continue; } - // Depth-0 rule: a field with a `field` ancestor is a comodel - // field — do not dock it. - let is_comodel = stack.iter().any(|t| t == "field"); - if !is_comodel && let Some(n) = &name { - push_fact(out, screen.as_deref(), n, &stack, order, None); + if depth == 0 { + // The arch wrapper's own `` — leave the arch body. + in_arch = false; + } else { + depth -= 1; + if let Some(v) = cur.as_mut() { + v.arch.push(ArchTag::ElementClose); + } + } + } + other => { + if !in_arch { + continue; + } + if other.opens_frame() { + depth += 1; + } + if let Some(v) = cur.as_mut() { + v.arch.push(other); + } + } + } + } + out.extend(cur); +} + +/// An addressable element of a resolved view: what an extension's locator can +/// target. `self_token` is set for region containers; `enclosing` is the region +/// the element itself sits in. +#[derive(Clone)] +struct Anchor { + tag: String, + name: Option, + self_token: Option, + enclosing: String, +} + +/// One frame of the arch element stack. +enum Frame { + Container(String), + Field, + Other, + /// A locator block: where its controls dock (`None` = unresolved → root), + /// and whether it sits inside a field target (comodel subview). + Locator { + dock: Option, + comodel: bool, + }, +} + +/// The region a control at this stack position docks in. +fn dock_of(stack: &[Frame]) -> String { + for f in stack.iter().rev() { + match f { + Frame::Container(t) => return t.clone(), + Frame::Locator { dock, .. } => { + return dock.clone().unwrap_or_else(|| "root".to_string()); + } + Frame::Field | Frame::Other => {} + } + } + "root".to_string() +} + +/// Whether a control at this stack position is a comodel subview field: it has +/// a `` ancestor before the nearest locator, or the locator itself +/// targets the inside of a field. +fn is_comodel(stack: &[Frame]) -> bool { + for f in stack.iter().rev() { + match f { + Frame::Field => return true, + Frame::Locator { comodel, .. } => return *comodel, + Frame::Container(_) | Frame::Other => {} + } + } + false +} + +/// Resolve a locator's target and position against the parent's anchors. +fn resolve_locator( + target: Option<&(String, Option)>, + position: &str, + parent: &[Anchor], +) -> Frame { + let unresolved = Frame::Locator { + dock: None, + comodel: false, + }; + let Some((tag, name)) = target else { + return unresolved; + }; + let Some(anchor) = parent + .iter() + .find(|a| &a.tag == tag && (name.is_none() || &a.name == name)) + else { + return unresolved; + }; + match position { + "attributes" => Frame::Locator { + dock: None, + comodel: true, + }, + "inside" => match &anchor.self_token { + Some(t) => Frame::Locator { + dock: Some(t.clone()), + comodel: false, + }, + None if anchor.tag == "field" => Frame::Locator { + dock: None, + comodel: true, + }, + None => Frame::Locator { + dock: Some(anchor.enclosing.clone()), + comodel: false, + }, + }, + // after / before / replace (and any future sibling position): the + // region the target itself sits in. + _ => Frame::Locator { + dock: Some(anchor.enclosing.clone()), + comodel: false, + }, + } +} + +/// Walk one view's arch: its region facts, and the anchors it contributes for +/// views that extend it. `parent` are the resolved anchors of its parent chain. +fn walk(view: &ViewRec, parent: &[Anchor]) -> (Vec, Vec) { + let mut facts = Vec::new(); + let mut anchors = Vec::new(); + let mut stack: Vec = Vec::new(); + let mut order = 0usize; + for tag in &view.arch { + match tag { + ArchTag::FieldOpen { name, self_closing } => { + if !is_comodel(&stack) + && let Some(n) = name + { + let dock = dock_of(&stack); + push_fact(&mut facts, &view.screen, n, &dock, order, None); order += 1; + anchors.push(Anchor { + tag: "field".to_string(), + name: Some(n.clone()), + self_token: None, + enclosing: dock, + }); } if !self_closing { - stack.push("field".to_string()); + stack.push(Frame::Field); } } ArchTag::ButtonOpen { @@ -193,69 +413,150 @@ fn scan_file_regions(content: &str, rel: &str, out: &mut Vec) { opens_popup, self_closing, } => { - if !in_arch { - continue; - } - let is_comodel = stack.iter().any(|t| t == "field"); - if !is_comodel { - push_fact(out, screen.as_deref(), &control, &stack, order, opens_popup); + if !is_comodel(&stack) { + let dock = dock_of(&stack); + push_fact( + &mut facts, + &view.screen, + control, + &dock, + order, + opens_popup.clone(), + ); order += 1; + anchors.push(Anchor { + tag: "button".to_string(), + name: Some(control.clone()), + self_token: None, + enclosing: dock, + }); } if !self_closing { - stack.push("button".to_string()); + stack.push(Frame::Other); } } ArchTag::ContainerOpen { + tag, + name, token, self_closing, } => { - if in_arch && !self_closing { - stack.push(token); + if !is_comodel(&stack) { + anchors.push(Anchor { + tag: tag.clone(), + name: name.clone(), + self_token: Some(token.clone()), + enclosing: dock_of(&stack), + }); } - } - ArchTag::OtherOpen { token } => { - if in_arch { - stack.push(token); + if !self_closing { + stack.push(Frame::Container(token.clone())); } } - ArchTag::ElementClose => { - if !in_arch { - continue; + ArchTag::OtherOpen { + tag, + name, + self_closing, + } => { + if !is_comodel(&stack) { + anchors.push(Anchor { + tag: tag.clone(), + name: name.clone(), + self_token: None, + enclosing: dock_of(&stack), + }); } - if stack.is_empty() { - // The arch wrapper's own `` — leave the arch body. - in_arch = false; - } else { - stack.pop(); + if !self_closing { + stack.push(Frame::Other); } } + ArchTag::LocatorOpen { + target, + position, + self_closing, + } => { + if !self_closing { + stack.push(resolve_locator(target.as_ref(), position, parent)); + } + } + ArchTag::ElementClose => { + stack.pop(); + } + ArchTag::ViewRecordOpen { .. } + | ArchTag::RecordClose + | ArchTag::InheritRef(_) + | ArchTag::ArchOpen { .. } => {} } } + (facts, anchors) +} + +/// Resolve every view (parents first, through `inherit_id`) and return the +/// region facts in view order. +fn regions_of_views(views: &[ViewRec]) -> Vec { + let mut index: std::collections::HashMap<&str, usize> = std::collections::HashMap::new(); + for (i, v) in views.iter().enumerate() { + index.entry(v.local_id.as_str()).or_insert(i); + } + let mut resolved: Vec, Vec)>> = vec![None; views.len()]; + let mut visiting = vec![false; views.len()]; + for i in 0..views.len() { + resolve_view(i, views, &index, &mut resolved, &mut visiting); + } + resolved + .into_iter() + .flat_map(|r| r.map(|(facts, _)| facts).unwrap_or_default()) + .collect() +} + +/// Resolve view `i` after its parent; its anchors are its own followed by the +/// parent chain's. A cycle in `inherit_id` resolves the repeated view with no +/// parent anchors instead of looping. +fn resolve_view( + i: usize, + views: &[ViewRec], + index: &std::collections::HashMap<&str, usize>, + resolved: &mut Vec, Vec)>>, + visiting: &mut Vec, +) { + if resolved[i].is_some() || visiting[i] { + return; + } + visiting[i] = true; + let parent = views[i] + .inherit + .as_deref() + .and_then(|p| index.get(p).copied()) + .filter(|&p| p != i); + let parent_anchors = match parent { + Some(p) => { + resolve_view(p, views, index, resolved, visiting); + resolved[p] + .as_ref() + .map(|(_, a)| a.clone()) + .unwrap_or_default() + } + None => Vec::new(), + }; + let (facts, mut anchors) = walk(&views[i], &parent_anchors); + anchors.extend(parent_anchors); + resolved[i] = Some((facts, anchors)); + visiting[i] = false; } -/// Emit one [`RegionFact`], docking `control` at the innermost region container -/// on `stack` (or `"root"` if none — an arch with no recognized container). +/// Emit one [`RegionFact`] docked at `dock_token`. fn push_fact( out: &mut Vec, - screen: Option<&str>, + screen: &str, control: &str, - stack: &[String], + dock_token: &str, order: usize, opens_popup: Option, ) { - let Some(screen) = screen else { - return; - }; - let dock_token = stack - .iter() - .rev() - .find(|t| REGION_CONTAINERS.contains(&t.as_str())) - .cloned() - .unwrap_or_else(|| "root".to_string()); out.push(RegionFact { screen: screen.to_string(), control: control.to_string(), - dock_token, + dock_token: dock_token.to_string(), // DOM document position is the faithful ordinal; the shared field is // `Option` (a harvester that cannot assign an ordinal leaves it // `None`). Odoo always has one — the non-panicking cast mirrors @@ -268,6 +569,52 @@ fn push_fact( }); } +/// The `(tag, name)` an xpath's LAST step addresses, if it is resolvable: a +/// bare tag, or a tag filtered by `@name` equality. Anything else +/// (`[hasclass(…)]`, `[@string=…]`, positional, `*`, functions) is `None` +/// rather than a guess. +fn xpath_target(expr: &str) -> Option<(String, Option)> { + let first = expr.split('|').next()?.trim(); + let mut depth = 0i32; + let mut cut = 0usize; + for (i, c) in first.char_indices() { + match c { + '[' => depth += 1, + ']' => depth -= 1, + '/' if depth == 0 => cut = i + 1, + _ => {} + } + } + let step = &first[cut..]; + let (tag, pred) = match step.find('[') { + Some(b) => (&step[..b], Some(&step[b..])), + None => (step, None), + }; + let tag = tag.trim(); + if tag.is_empty() + || !tag + .chars() + .all(|c| c.is_ascii_alphanumeric() || c == '_' || c == '-') + { + return None; + } + let name = match pred { + None => None, + Some(p) => Some(name_in_predicate(p)?), + }; + Some((tag.to_string(), name)) +} + +/// The value of an `@name='…'` / `@name="…"` equality inside an xpath +/// predicate. +fn name_in_predicate(pred: &str) -> Option { + let rest = &pred[pred.find("@name")? + "@name".len()..]; + let rest = rest.trim_start().strip_prefix('=')?.trim_start(); + let quote = rest.chars().next().filter(|q| *q == '\'' || *q == '"')?; + let body = &rest[1..]; + Some(body[..body.find(quote)?].to_string()) +} + /// All arch-relevant tags in `content`, in byte-position order. Every `<…>` is /// classified; unknown element opens become [`ArchTag::OtherOpen`] so the /// stack stays balanced. @@ -308,6 +655,25 @@ fn arch_tags(content: &str) -> Vec { let tag = tag_slice(content, idx); let self_closing = tag.trim_end().ends_with('/'); + if name != "record" + && let Some(position) = attr_value(tag, "position") + { + let target = if name == "xpath" { + attr_value(tag, "expr").and_then(|e| xpath_target(&e)) + } else { + Some((name.clone(), attr_value(tag, "name"))) + }; + toks.push(( + idx, + ArchTag::LocatorOpen { + target, + position, + self_closing, + }, + )); + continue; + } + match name.as_str() { "record" => { if attr_value(tag, "model").as_deref() == Some("ir.ui.view") { @@ -318,12 +684,34 @@ fn arch_tags(content: &str) -> Vec { }, )); } else { - toks.push((idx, ArchTag::OtherOpen { token: name })); + toks.push(( + idx, + ArchTag::OtherOpen { + tag: name, + name: None, + self_closing, + }, + )); } } "field" if attr_value(tag, "name").as_deref() == Some("arch") => { toks.push((idx, ArchTag::ArchOpen { self_closing })); } + "field" if attr_value(tag, "name").as_deref() == Some("inherit_id") => { + if let Some(r) = attr_value(tag, "ref") { + toks.push((idx, ArchTag::InheritRef(r))); + } + if !self_closing { + toks.push(( + idx, + ArchTag::OtherOpen { + tag: name, + name: None, + self_closing, + }, + )); + } + } "field" => { toks.push(( idx, @@ -357,29 +745,49 @@ fn arch_tags(content: &str) -> Vec { let is_chatter = attr_value(tag, "class") .map(|c| c.split_whitespace().any(|w| w == "oe_chatter")) .unwrap_or(false); + let el_name = attr_value(tag, "name"); if is_chatter { toks.push(( idx, ArchTag::ContainerOpen { + tag: name, + name: el_name, token: "chatter".to_string(), self_closing, }, )); } else { - toks.push((idx, ArchTag::OtherOpen { token: name })); + toks.push(( + idx, + ArchTag::OtherOpen { + tag: name, + name: el_name, + self_closing, + }, + )); } } n if REGION_CONTAINERS.contains(&n) => { toks.push(( idx, ArchTag::ContainerOpen { + tag: name.clone(), + name: attr_value(tag, "name"), token: name.clone(), self_closing, }, )); } _ => { - toks.push((idx, ArchTag::OtherOpen { token: name })); + let el_name = attr_value(tag, "name"); + toks.push(( + idx, + ArchTag::OtherOpen { + tag: name, + name: el_name, + self_closing, + }, + )); } } } @@ -518,6 +926,202 @@ mod tests { } } + /// A neutral base view and extensions of it (all names invented). The base + /// docks `amount` in `group`, has a named `notebook` page, a named `group`, + /// a `line_ids` one2many, and a self-closing `` before the end of + /// its arch (the stack-balance case). + const EXT_FIXTURE: &str = r#" + + + a.gadget + +
+
+
+ + + + + + + + + + + + + + + + +
+ + 20 + + + a.gadget + + + + + + + + + + + + + + + + + +