From 577c2ba25fa15816d17a9f74ab284a1ece1d5286 Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Sun, 15 Nov 2026 01:15:06 +0000 Subject: [PATCH 1/3] feat: add SQBRPYTHON node unit rules for Python nodes under python/ SQBRPYTHON003 to SQBRPYTHON012 check that each node lives in its own unit folder under its domain: node.py declares one node or factory that only returns main(...) with keyword arguments, main.py holds one main() with a result annotation that fits the node, checks.py holds the unit's checks, imports stay within the unit's boundaries, factories loop over the unit's specs, nodes use description= instead of docstrings, and nodes declared outside a unit's node.py are reported. The rules are on by default within SQBRPYTHON, so projects that select SQBRPYTHON may get new layout findings; selecting SQBR alone never runs them. --- .../fulfillment/shipment_export/main.py | 6 +- .../fulfillment/shipment_export/node.py | 12 +- .../python_rules/expected_findings.json | 8 + .../python/inventory/replenishment.py | 8 + .../python/inventory/stock_count/__init__.py | 0 .../python/inventory/stock_count/node.py | 11 + crates/sqlbuild-pysource/src/lib.rs | 1 + .../src/node_units/_helpers/functions.rs | 195 ++++++ .../src/node_units/_helpers/imports.rs | 100 +++ .../src/node_units/_helpers/mod.rs | 3 + .../src/node_units/_helpers/positions.rs | 29 + .../src/node_units/constants.rs | 12 + .../src/node_units/main/mod.rs | 1 + .../src/node_units/main/node_unit_module.rs | 43 ++ .../sqlbuild-pysource/src/node_units/mod.rs | 7 + .../src/node_units/models.rs | 90 +++ .../sqlbuild-pysource/src/node_units/tests.rs | 6 + .../src/node_units/tests/helpers.rs | 81 +++ .../node_units/tests/test_node_unit_module.rs | 166 +++++ .../src/node_units/tests/test_types.rs | 19 + .../engine/tests/test_incremental_rules.rs | 10 + .../src/python_rules/_helpers/faults.rs | 19 + .../src/python_rules/_helpers/layout.rs | 14 +- .../src/python_rules/_helpers/mod.rs | 1 + .../python_rules/_helpers/units/imports.rs | 79 +++ .../src/python_rules/_helpers/units/layout.rs | 157 +++++ .../src/python_rules/_helpers/units/mod.rs | 5 + .../python_rules/_helpers/units/modules.rs | 220 +++++++ .../src/python_rules/_helpers/units/nodes.rs | 195 ++++++ .../python_rules/_helpers/units/project.rs | 222 +++++++ .../src/python_rules/constants.rs | 210 +++++- .../main/evaluate_python_rules.rs | 6 + .../sqlbuild-rules/src/python_rules/models.rs | 23 + .../sqlbuild-rules/src/python_rules/tests.rs | 2 + .../src/python_rules/tests/test_node_units.rs | 614 ++++++++++++++++++ .../tests/test_python_catalogue.rs | 6 +- .../docs/concepts/python-nodes/overview.md | 2 +- .../docs/concepts/rules/python-rules.md | 81 ++- .../sqlbuild/cli/commands/main/test_rules.py | 2 +- .../docs/concepts/python-nodes/overview.mdx | 2 +- .../docs/docs/concepts/rules/python-rules.mdx | 81 ++- 41 files changed, 2720 insertions(+), 29 deletions(-) create mode 100644 benchmarks/duckshop/edge_cases/python_rules/violations/python/inventory/replenishment.py create mode 100644 benchmarks/duckshop/edge_cases/python_rules/violations/python/inventory/stock_count/__init__.py create mode 100644 benchmarks/duckshop/edge_cases/python_rules/violations/python/inventory/stock_count/node.py create mode 100644 crates/sqlbuild-pysource/src/node_units/_helpers/functions.rs create mode 100644 crates/sqlbuild-pysource/src/node_units/_helpers/imports.rs create mode 100644 crates/sqlbuild-pysource/src/node_units/_helpers/mod.rs create mode 100644 crates/sqlbuild-pysource/src/node_units/_helpers/positions.rs create mode 100644 crates/sqlbuild-pysource/src/node_units/constants.rs create mode 100644 crates/sqlbuild-pysource/src/node_units/main/mod.rs create mode 100644 crates/sqlbuild-pysource/src/node_units/main/node_unit_module.rs create mode 100644 crates/sqlbuild-pysource/src/node_units/mod.rs create mode 100644 crates/sqlbuild-pysource/src/node_units/models.rs create mode 100644 crates/sqlbuild-pysource/src/node_units/tests.rs create mode 100644 crates/sqlbuild-pysource/src/node_units/tests/helpers.rs create mode 100644 crates/sqlbuild-pysource/src/node_units/tests/test_node_unit_module.rs create mode 100644 crates/sqlbuild-pysource/src/node_units/tests/test_types.rs create mode 100644 crates/sqlbuild-rules/src/python_rules/_helpers/units/imports.rs create mode 100644 crates/sqlbuild-rules/src/python_rules/_helpers/units/layout.rs create mode 100644 crates/sqlbuild-rules/src/python_rules/_helpers/units/mod.rs create mode 100644 crates/sqlbuild-rules/src/python_rules/_helpers/units/modules.rs create mode 100644 crates/sqlbuild-rules/src/python_rules/_helpers/units/nodes.rs create mode 100644 crates/sqlbuild-rules/src/python_rules/_helpers/units/project.rs create mode 100644 crates/sqlbuild-rules/src/python_rules/tests/test_node_units.rs diff --git a/benchmarks/duckshop/edge_cases/python_rules/conforming/python/fulfillment/shipment_export/main.py b/benchmarks/duckshop/edge_cases/python_rules/conforming/python/fulfillment/shipment_export/main.py index 7ce3773fb..fb001a0a9 100644 --- a/benchmarks/duckshop/edge_cases/python_rules/conforming/python/fulfillment/shipment_export/main.py +++ b/benchmarks/duckshop/edge_cases/python_rules/conforming/python/fulfillment/shipment_export/main.py @@ -1,9 +1,11 @@ """Write each shipped order as one carrier line.""" from python.utils.currency import amounts +from sqlbuild.executor.python_nodes.models import PythonNodeResult +from sqlbuild.tasks import TaskContext -def export_shipments(ctx: object) -> None: +def main(ctx: TaskContext) -> PythonNodeResult: """Write each shipped order as one carrier line.""" label: str = "Expédition" - print(label, amounts.cents_text(1250), ctx) + return ctx.result(payload=f"{label} {amounts.cents_text(1250)}") diff --git a/benchmarks/duckshop/edge_cases/python_rules/conforming/python/fulfillment/shipment_export/node.py b/benchmarks/duckshop/edge_cases/python_rules/conforming/python/fulfillment/shipment_export/node.py index 35836cc7a..60fe8ac5c 100644 --- a/benchmarks/duckshop/edge_cases/python_rules/conforming/python/fulfillment/shipment_export/node.py +++ b/benchmarks/duckshop/edge_cases/python_rules/conforming/python/fulfillment/shipment_export/node.py @@ -1,10 +1,10 @@ """Export shipped orders to the carrier.""" -from python.fulfillment.shipment_export.main import export_shipments -from sqlbuild.tasks import task +from python.fulfillment.shipment_export.main import main +from sqlbuild.executor.python_nodes.models import PythonNodeResult +from sqlbuild.tasks import TaskContext, task -@task -def shipment_export(ctx: object) -> None: - """Export shipped orders to the carrier.""" - export_shipments(ctx) +@task(description="Export shipped orders to the carrier.") +def shipment_export(ctx: TaskContext) -> PythonNodeResult: + return main(ctx=ctx) diff --git a/benchmarks/duckshop/edge_cases/python_rules/expected_findings.json b/benchmarks/duckshop/edge_cases/python_rules/expected_findings.json index 772449218..c72b412c7 100644 --- a/benchmarks/duckshop/edge_cases/python_rules/expected_findings.json +++ b/benchmarks/duckshop/edge_cases/python_rules/expected_findings.json @@ -7,9 +7,17 @@ ["SQBRPYTHON306", "python/inventory/common/stock.py", 4, 1], ["SQBRPYTHON305", "python/inventory/main/publish.py", 1, 1], ["SQBRPYTHON306", "python/inventory/main/publish.py", 4, 1], + ["SQBRPYTHON614", "python/inventory/replenishment.py", 1, 1], + ["SQBRPYTHON012", "python/inventory/replenishment.py", 7, 5], ["SQBRPYTHON617", "python/inventory/restock/main.py", 1, 1], ["SQBRPYTHON306", "python/inventory/restock/main.py", 4, 1], ["SQBRPYTHON306", "python/inventory/restock/main.py", 9, 1], + ["SQBRPYTHON003", "python/inventory/stock_count/node.py", 1, 1], + ["SQBRPYTHON004", "python/inventory/stock_count/node.py", 5, 1], + ["SQBRPYTHON605", "python/inventory/stock_count/node.py", 5, 1], + ["SQBRPYTHON005", "python/inventory/stock_count/node.py", 9, 5], + ["SQBRPYTHON007", "python/inventory/stock_count/node.py", 9, 5], + ["SQBRPYTHON011", "python/inventory/stock_count/node.py", 9, 5], ["SQBRPYTHON001", "python/report.py", 1, 1], ["SQBRPYTHON614", "python/report.py", 1, 1] ] diff --git a/benchmarks/duckshop/edge_cases/python_rules/violations/python/inventory/replenishment.py b/benchmarks/duckshop/edge_cases/python_rules/violations/python/inventory/replenishment.py new file mode 100644 index 000000000..7accddcff --- /dev/null +++ b/benchmarks/duckshop/edge_cases/python_rules/violations/python/inventory/replenishment.py @@ -0,0 +1,8 @@ +"""Replenishment declared in a flat module.""" + +from sqlbuild.tasks import TaskContext, task + + +@task(description="Reorder products that ran out.") +def replenish(ctx: TaskContext) -> None: + return None diff --git a/benchmarks/duckshop/edge_cases/python_rules/violations/python/inventory/stock_count/__init__.py b/benchmarks/duckshop/edge_cases/python_rules/violations/python/inventory/stock_count/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/benchmarks/duckshop/edge_cases/python_rules/violations/python/inventory/stock_count/node.py b/benchmarks/duckshop/edge_cases/python_rules/violations/python/inventory/stock_count/node.py new file mode 100644 index 000000000..1e0c4bf07 --- /dev/null +++ b/benchmarks/duckshop/edge_cases/python_rules/violations/python/inventory/stock_count/node.py @@ -0,0 +1,11 @@ +"""Count stock in every warehouse.""" + +from sqlbuild.tasks import TaskContext, task + +WAREHOUSES: tuple[str, ...] = ("north", "south") + + +@task +def stock_count(context: TaskContext) -> int: + """Count stock in every warehouse.""" + return len(WAREHOUSES) diff --git a/crates/sqlbuild-pysource/src/lib.rs b/crates/sqlbuild-pysource/src/lib.rs index 0d5cbe413..849694215 100644 --- a/crates/sqlbuild-pysource/src/lib.rs +++ b/crates/sqlbuild-pysource/src/lib.rs @@ -8,6 +8,7 @@ pub mod import_rewrites; pub mod literal_sql; pub mod macro_modules; pub mod module_parse; +pub mod node_units; pub mod orchestration_modules; pub mod rule_evidence; pub mod string_literals; diff --git a/crates/sqlbuild-pysource/src/node_units/_helpers/functions.rs b/crates/sqlbuild-pysource/src/node_units/_helpers/functions.rs new file mode 100644 index 000000000..d063339fe --- /dev/null +++ b/crates/sqlbuild-pysource/src/node_units/_helpers/functions.rs @@ -0,0 +1,195 @@ +//! A function's decorators, signature and body shape, and the decorated functions inside it. + +use ruff_python_ast::{Decorator, Expr, Parameter, Stmt, StmtFunctionDef}; +use ruff_text_size::Ranged; + +use crate::cpython_tree::main::walk_breadth_first::walk_breadth_first; +use crate::cpython_tree::models::CpythonNode; +use crate::node_units::_helpers::positions::{UnitText, unit_position}; +use crate::node_units::constants::{ + DEPENDS_ON_KEYWORD, MAIN_FUNCTION, NONE_NAME, OPTIONAL_NAME, UNION_NAMES, +}; +use crate::node_units::models::{UnitDecorator, UnitFunction, UnitParameter}; + +/// The shape of `function` and of every decorated function inside it. +pub(crate) fn unit_function(function: &StmtFunctionDef, source: &UnitText<'_>) -> UnitFunction { + let parameters = &function.parameters; + let body: Vec> = + walk_breadth_first(function.body.iter().map(CpythonNode::Stmt).collect()); + UnitFunction { + name: function.name.to_string(), + position: unit_position(source, function.name.range().start()), + decorators: function + .decorator_list + .iter() + .filter_map(decorator) + .collect(), + positional: parameters + .posonlyargs + .iter() + .chain(¶meters.args) + .map(|with_default| parameter(&with_default.parameter)) + .collect(), + variadic: parameters.vararg.is_some(), + keyword_only: parameters + .kwonlyargs + .iter() + .map(|with_default| parameter(&with_default.parameter)) + .collect(), + keywords_variadic: parameters.kwarg.is_some(), + returns: function.returns.as_deref().map(union_names), + docstring: leading_docstring(&function.body), + returns_directly: matches!(without_docstring(&function.body), [Stmt::Return(_)]), + delegation: delegation(&function.body), + inner: body + .iter() + .filter_map(|node| match node { + CpythonNode::Stmt(Stmt::FunctionDef(inner)) if !inner.decorator_list.is_empty() => { + Some(unit_function(inner, source)) + } + _ => None, + }) + .collect(), + loops: body + .iter() + .filter_map(|node| match node { + CpythonNode::Stmt(Stmt::For(loop_statement)) => { + Some(match loop_statement.iter.as_ref() { + Expr::Name(name) => Some(name.id.to_string()), + _ => None, + }) + } + _ => None, + }) + .collect(), + } +} + +/// Whether `body` opens with a string expression statement. +pub(crate) fn leading_docstring(body: &[Stmt]) -> bool { + matches!(body.first(), Some(Stmt::Expr(statement)) if statement.value.is_string_literal_expr()) +} + +/// `body` without its leading docstring. +fn without_docstring(body: &[Stmt]) -> &[Stmt] { + if leading_docstring(body) { + &body[1..] + } else { + body + } +} + +/// For a body that is exactly `return main(...)` after any docstring, whether every argument is a named keyword. +fn delegation(body: &[Stmt]) -> Option { + let statements: &[Stmt] = if leading_docstring(body) { + &body[1..] + } else { + body + }; + let [Stmt::Return(returned)] = statements else { + return None; + }; + let Some(Expr::Call(call)) = returned.value.as_deref() else { + return None; + }; + let Expr::Name(function) = call.func.as_ref() else { + return None; + }; + (function.id.as_str() == MAIN_FUNCTION).then(|| { + call.arguments.args.is_empty() + && call + .arguments + .keywords + .iter() + .all(|keyword| keyword.arg.is_some()) + }) +} + +/// A decorator by its last name, with its keywords and the names its `depends_on` value reads. +fn decorator(decorator: &Decorator) -> Option { + let (target, call) = match &decorator.expression { + Expr::Call(call) => (call.func.as_ref(), Some(call)), + other => (other, None), + }; + let name: String = last_name(target)?; + let keywords = call + .map(|call| call.arguments.keywords.as_ref()) + .unwrap_or_default(); + Some(UnitDecorator { + name, + keywords: keywords + .iter() + .filter_map(|keyword| keyword.arg.as_ref().map(ToString::to_string)) + .collect(), + depends_on: keywords + .iter() + .filter(|keyword| { + keyword + .arg + .as_ref() + .is_some_and(|arg| arg.as_str() == DEPENDS_ON_KEYWORD) + }) + .flat_map(|keyword| walk_breadth_first(vec![CpythonNode::Expr(&keyword.value)])) + .filter_map(|node| match node { + CpythonNode::Expr(Expr::Name(name)) => Some(name.id.to_string()), + _ => None, + }) + .collect(), + }) +} + +fn parameter(parameter: &Parameter) -> UnitParameter { + UnitParameter { + name: parameter.name.to_string(), + annotation: parameter.annotation.as_deref().and_then(last_name), + } +} + +/// The last name of each member of an annotation written as `A | B`, `Optional[A]` or `Union[A, B]`. +fn union_names(annotation: &Expr) -> Vec { + match annotation { + Expr::BinOp(union) if union.op.is_bit_or() => { + let mut names: Vec = union_names(&union.left); + names.extend(union_names(&union.right)); + names + } + Expr::NoneLiteral(_) => vec![NONE_NAME.to_owned()], + Expr::Subscript(subscript) + if last_name(&subscript.value) + .is_some_and(|name| UNION_NAMES.contains(&name.as_str())) => + { + let mut names: Vec = match subscript.slice.as_ref() { + Expr::Tuple(members) => members.elts.iter().flat_map(union_names).collect(), + member => union_names(member), + }; + if last_name(&subscript.value).as_deref() == Some(OPTIONAL_NAME) { + names.push(NONE_NAME.to_owned()); + } + names + } + other => last_name(other).into_iter().collect(), + } +} + +/// The last name of a name, an attribute chain, or the subscripted base of either. +fn last_name(expression: &Expr) -> Option { + match expression { + Expr::Name(name) => Some(name.id.to_string()), + Expr::Attribute(attribute) => Some(attribute.attr.to_string()), + Expr::Subscript(subscript) => last_name(&subscript.value), + Expr::StringLiteral(literal) => Some( + literal + .value + .to_str() + .rsplit('.') + .next() + .unwrap_or_default() + .split('[') + .next() + .unwrap_or_default() + .trim() + .to_owned(), + ), + _ => None, + } +} diff --git a/crates/sqlbuild-pysource/src/node_units/_helpers/imports.rs b/crates/sqlbuild-pysource/src/node_units/_helpers/imports.rs new file mode 100644 index 000000000..6c7fe2184 --- /dev/null +++ b/crates/sqlbuild-pysource/src/node_units/_helpers/imports.rs @@ -0,0 +1,100 @@ +//! Module-level imports and whether each bound name is used only to name a dependency. + +use ruff_python_ast::{Expr, Stmt}; +use ruff_text_size::{Ranged, TextRange, TextSize}; + +use crate::cpython_tree::main::walk_breadth_first::walk_breadth_first; +use crate::cpython_tree::models::CpythonNode; +use crate::node_units::_helpers::positions::{UnitText, unit_position}; +use crate::node_units::constants::DEPENDS_ON_KEYWORD; +use crate::node_units::models::UnitImport; + +/// Every name the module's top-level imports bind, in source order. +pub(crate) fn module_imports(body: &[Stmt], source: &UnitText<'_>) -> Vec { + let nodes: Vec> = + walk_breadth_first(body.iter().map(CpythonNode::Stmt).collect()); + let dependency_ranges: Vec = depends_on_ranges(&nodes); + let mut imports: Vec = Vec::new(); + for statement in body { + match statement { + Stmt::Import(import) => imports.extend(import.names.iter().map(|alias| UnitImport { + module: alias.name.to_string(), + level: 0, + name: None, + local: alias.asname.as_ref().map_or_else( + || { + alias + .name + .as_str() + .split('.') + .next() + .unwrap_or_default() + .to_owned() + }, + ToString::to_string, + ), + position: unit_position(source, alias.range().start()), + only_in_depends_on: false, + })), + Stmt::ImportFrom(import) => imports.extend(import.names.iter().map(|alias| { + UnitImport { + module: import + .module + .as_ref() + .map(ToString::to_string) + .unwrap_or_default(), + level: import.level, + name: Some(alias.name.to_string()), + local: alias.asname.as_ref().unwrap_or(&alias.name).to_string(), + position: unit_position(source, alias.range().start()), + only_in_depends_on: false, + } + })), + _ => {} + } + } + for import in &mut imports { + import.only_in_depends_on = used_only_in(&nodes, &import.local, &dependency_ranges); + } + imports +} + +/// Whether every load of `local` sits inside one of `ranges`. +fn used_only_in(nodes: &[CpythonNode<'_>], local: &str, ranges: &[TextRange]) -> bool { + name_uses(nodes, local).all(|start| within_any(ranges, start)) +} + +fn within_any(ranges: &[TextRange], offset: TextSize) -> bool { + ranges.iter().any(|range| range.contains(offset)) +} + +/// The start of every load of `local` in the module. +fn name_uses<'a>( + nodes: &'a [CpythonNode<'a>], + local: &'a str, +) -> impl Iterator + 'a { + nodes.iter().filter_map(move |node| match node { + CpythonNode::Expr(Expr::Name(name)) if name.id.as_str() == local => { + Some(name.range().start()) + } + _ => None, + }) +} + +/// The value range of every `depends_on=` keyword in the module. +fn depends_on_ranges(nodes: &[CpythonNode<'_>]) -> Vec { + nodes + .iter() + .filter_map(|node| match node { + CpythonNode::Keyword(keyword) + if keyword + .arg + .as_ref() + .is_some_and(|arg| arg.as_str() == DEPENDS_ON_KEYWORD) => + { + Some(keyword.value.range()) + } + _ => None, + }) + .collect() +} diff --git a/crates/sqlbuild-pysource/src/node_units/_helpers/mod.rs b/crates/sqlbuild-pysource/src/node_units/_helpers/mod.rs new file mode 100644 index 000000000..0d018f6f9 --- /dev/null +++ b/crates/sqlbuild-pysource/src/node_units/_helpers/mod.rs @@ -0,0 +1,3 @@ +pub(crate) mod functions; +pub(crate) mod imports; +pub(crate) mod positions; diff --git a/crates/sqlbuild-pysource/src/node_units/_helpers/positions.rs b/crates/sqlbuild-pysource/src/node_units/_helpers/positions.rs new file mode 100644 index 000000000..8df49d508 --- /dev/null +++ b/crates/sqlbuild-pysource/src/node_units/_helpers/positions.rs @@ -0,0 +1,29 @@ +//! Character positions in the newline-translated source the tree was parsed from. + +use ruff_text_size::TextSize; + +use crate::cpython_tree::main::source_position::source_position; +use crate::cpython_tree::models::SourceLines; +use crate::node_units::models::UnitPosition; + +/// The parsed text and its line starts. +#[derive(Debug)] +pub(crate) struct UnitText<'a> { + pub(crate) text: &'a str, + pub(crate) lines: &'a SourceLines, +} + +/// The 1-based line and character column of `offset`. +pub(crate) fn unit_position(source: &UnitText<'_>, offset: TextSize) -> UnitPosition { + let (line, byte_column) = source_position(source.lines, offset); + let end: usize = offset.to_usize(); + let start: usize = end - byte_column; + UnitPosition { + line, + column: source + .text + .get(start..end) + .map_or(byte_column, |prefix| prefix.chars().count()) + + 1, + } +} diff --git a/crates/sqlbuild-pysource/src/node_units/constants.rs b/crates/sqlbuild-pysource/src/node_units/constants.rs new file mode 100644 index 000000000..98869cd03 --- /dev/null +++ b/crates/sqlbuild-pysource/src/node_units/constants.rs @@ -0,0 +1,12 @@ +//! The names a node unit module is read for. + +/// The function a node or check delegates to. +pub(crate) const MAIN_FUNCTION: &str = "main"; +/// The decorator keyword naming what a node or check depends on. +pub(crate) const DEPENDS_ON_KEYWORD: &str = "depends_on"; +/// How a `None` member of a return annotation is named. +pub(crate) const NONE_NAME: &str = "None"; +/// The typing form that adds `None` to its member. +pub(crate) const OPTIONAL_NAME: &str = "Optional"; +/// The typing forms whose members a return annotation unions. +pub(crate) const UNION_NAMES: [&str; 2] = ["Union", OPTIONAL_NAME]; diff --git a/crates/sqlbuild-pysource/src/node_units/main/mod.rs b/crates/sqlbuild-pysource/src/node_units/main/mod.rs new file mode 100644 index 000000000..77bf4695f --- /dev/null +++ b/crates/sqlbuild-pysource/src/node_units/main/mod.rs @@ -0,0 +1 @@ +pub mod node_unit_module; diff --git a/crates/sqlbuild-pysource/src/node_units/main/node_unit_module.rs b/crates/sqlbuild-pysource/src/node_units/main/node_unit_module.rs new file mode 100644 index 000000000..405a78069 --- /dev/null +++ b/crates/sqlbuild-pysource/src/node_units/main/node_unit_module.rs @@ -0,0 +1,43 @@ +//! Read a module under `python/` for the node unit rules, without importing it. + +use ruff_python_ast::Stmt; +use ruff_text_size::Ranged; + +use crate::cpython_tree::main::parse_cpython_module::parse_cpython_module; +use crate::cpython_tree::models::ParsedSource; +use crate::node_units::_helpers::functions::{leading_docstring, unit_function}; +use crate::node_units::_helpers::imports::module_imports; +use crate::node_units::_helpers::positions::{UnitText, unit_position}; +use crate::node_units::models::{UnitModule, UnitStatement, UnitStatementKind}; + +/// The top-level statements and imports of `contents`, or `None` when it does not parse. +#[must_use] +pub fn node_unit_module(contents: &str, python_version: (u8, u8)) -> Option { + let parsed: ParsedSource = parse_cpython_module(contents, python_version)?; + let text: String = contents.replace("\r\n", "\n").replace('\r', "\n"); + let source = UnitText { + text: &text, + lines: &parsed.lines, + }; + let body: &[Stmt] = &parsed.module.body; + let has_docstring: bool = leading_docstring(body); + let statements: Vec = body + .iter() + .enumerate() + .map(|(index, statement)| UnitStatement { + kind: match statement { + _ if index == 0 && has_docstring => UnitStatementKind::Docstring, + Stmt::Import(_) | Stmt::ImportFrom(_) => UnitStatementKind::Import, + Stmt::FunctionDef(function) => { + UnitStatementKind::Function(unit_function(function, &source)) + } + _ => UnitStatementKind::Other, + }, + position: unit_position(&source, statement.range().start()), + }) + .collect(); + Some(UnitModule { + statements, + imports: module_imports(body, &source), + }) +} diff --git a/crates/sqlbuild-pysource/src/node_units/mod.rs b/crates/sqlbuild-pysource/src/node_units/mod.rs new file mode 100644 index 000000000..1e64f3d11 --- /dev/null +++ b/crates/sqlbuild-pysource/src/node_units/mod.rs @@ -0,0 +1,7 @@ +pub(crate) mod _helpers; +pub(crate) mod constants; +pub mod main; +pub mod models; + +#[cfg(test)] +mod tests; diff --git a/crates/sqlbuild-pysource/src/node_units/models.rs b/crates/sqlbuild-pysource/src/node_units/models.rs new file mode 100644 index 000000000..dcf052101 --- /dev/null +++ b/crates/sqlbuild-pysource/src/node_units/models.rs @@ -0,0 +1,90 @@ +//! The shape of a Python module under `python/`, as the node unit rules read it. + +/// A 1-based line and character column. +#[derive(Clone, Copy, Debug, Default, PartialEq, Eq)] +pub struct UnitPosition { + pub line: usize, + pub column: usize, +} + +/// A module's top-level statements and every name it imports at module level. +#[derive(Clone, Debug, Default, PartialEq, Eq)] +pub struct UnitModule { + pub statements: Vec, + pub imports: Vec, +} + +/// One top-level statement and where it starts. +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct UnitStatement { + pub kind: UnitStatementKind, + pub position: UnitPosition, +} + +/// What a top-level statement is, as far as the unit rules care. +#[derive(Clone, Debug, PartialEq, Eq)] +pub enum UnitStatementKind { + /// The module docstring. + Docstring, + Import, + Function(UnitFunction), + /// Any other statement, such as a class, an assignment or a loop. + Other, +} + +/// One imported name of an `import` or `from ... import` at module level. +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct UnitImport { + /// The dotted module after `level` leading dots; for `import a.b`, `a.b`. + pub module: String, + pub level: u32, + /// The imported name of `from module import name`; `None` for `import module`. + pub name: Option, + /// The name the import binds in the module. + pub local: String, + pub position: UnitPosition, + /// Whether every use of the bound name sits inside a decorator's `depends_on` value. + pub only_in_depends_on: bool, +} + +/// A function, its decorators, signature and body shape, and the decorated functions inside it. +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct UnitFunction { + pub name: String, + /// Where the function's name is written. + pub position: UnitPosition, + pub decorators: Vec, + /// Positional-only and ordinary parameters, in order. + pub positional: Vec, + /// Whether it takes `*args`. + pub variadic: bool, + pub keyword_only: Vec, + /// Whether it takes `**kwargs`. + pub keywords_variadic: bool, + /// The last name of each member of the return annotation, or `None` without one. + pub returns: Option>, + pub docstring: bool, + /// Whether the body is exactly one `return` statement after any docstring. + pub returns_directly: bool, + /// For a body that is exactly `return main(...)` after any docstring, whether every argument is a named keyword. + pub delegation: Option, + /// The decorated functions defined anywhere in the body. + pub inner: Vec, + /// The iterable of each `for` loop in the body: its name, or `None` when it is not a plain name. + pub loops: Vec>, +} + +/// A decorator by its last name, the keywords it is called with and the names its `depends_on` reads. +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct UnitDecorator { + pub name: String, + pub keywords: Vec, + pub depends_on: Vec, +} + +/// A parameter and the last name of its annotation. +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct UnitParameter { + pub name: String, + pub annotation: Option, +} diff --git a/crates/sqlbuild-pysource/src/node_units/tests.rs b/crates/sqlbuild-pysource/src/node_units/tests.rs new file mode 100644 index 000000000..60fba9106 --- /dev/null +++ b/crates/sqlbuild-pysource/src/node_units/tests.rs @@ -0,0 +1,6 @@ +#[path = "tests/helpers.rs"] +mod helpers; +#[path = "tests/test_node_unit_module.rs"] +mod node_unit_module; +#[path = "tests/test_types.rs"] +mod test_types; diff --git a/crates/sqlbuild-pysource/src/node_units/tests/helpers.rs b/crates/sqlbuild-pysource/src/node_units/tests/helpers.rs new file mode 100644 index 000000000..cdd0da3f1 --- /dev/null +++ b/crates/sqlbuild-pysource/src/node_units/tests/helpers.rs @@ -0,0 +1,81 @@ +use crate::node_units::main::node_unit_module::node_unit_module; +use crate::node_units::models::{ + UnitDecorator, UnitFunction, UnitImport, UnitParameter, UnitPosition, UnitStatementKind, +}; + +const PYTHON_VERSION: (u8, u8) = (3, 12); + +/// The kind of each top-level statement of `contents`, or `None` when it does not parse. +pub(super) fn statement_kinds(contents: &str) -> Option> { + let module = node_unit_module(contents, PYTHON_VERSION)?; + Some( + module + .statements + .iter() + .map(|statement| match statement.kind { + UnitStatementKind::Docstring => "docstring", + UnitStatementKind::Import => "import", + UnitStatementKind::Function(_) => "function", + UnitStatementKind::Other => "other", + }) + .collect(), + ) +} + +/// The imports `contents` binds at module level. +pub(super) fn imports(contents: &str) -> Vec { + node_unit_module(contents, PYTHON_VERSION).map_or_else(Vec::new, |module| module.imports) +} + +/// The first top-level function of `contents`. +pub(super) fn first_function(contents: &str) -> Option { + node_unit_module(contents, PYTHON_VERSION)? + .statements + .into_iter() + .find_map(|statement| match statement.kind { + UnitStatementKind::Function(function) => Some(function), + _ => None, + }) +} + +pub(super) fn at(line: usize, column: usize) -> UnitPosition { + UnitPosition { line, column } +} + +pub(super) fn parameter(name: &str, annotation: Option<&str>) -> UnitParameter { + UnitParameter { + name: name.to_owned(), + annotation: annotation.map(str::to_owned), + } +} + +pub(super) fn decorator(name: &str, keywords: &[&str], depends_on: &[&str]) -> UnitDecorator { + UnitDecorator { + name: name.to_owned(), + keywords: keywords + .iter() + .map(|keyword| (*keyword).to_owned()) + .collect(), + depends_on: depends_on.iter().map(|name| (*name).to_owned()).collect(), + } +} + +pub(super) fn names(names: &[&str]) -> Vec { + names.iter().map(|name| (*name).to_owned()).collect() +} + +pub(super) fn import( + module: &str, + name: Option<&str>, + local: &str, + position: UnitPosition, +) -> UnitImport { + UnitImport { + module: module.to_owned(), + level: 0, + name: name.map(str::to_owned), + local: local.to_owned(), + position, + only_in_depends_on: false, + } +} diff --git a/crates/sqlbuild-pysource/src/node_units/tests/test_node_unit_module.rs b/crates/sqlbuild-pysource/src/node_units/tests/test_node_unit_module.rs new file mode 100644 index 000000000..a8f1a8f9b --- /dev/null +++ b/crates/sqlbuild-pysource/src/node_units/tests/test_node_unit_module.rs @@ -0,0 +1,166 @@ +use crate::node_units::models::{UnitFunction, UnitImport}; +use crate::node_units::tests::helpers::{ + at, decorator, first_function, import, imports, names, parameter, statement_kinds, +}; +use crate::node_units::tests::test_types::{ + FunctionTestCase, ImportsTestCase, StatementKindsTestCase, +}; + +const CRLF_NODE_MODULE: &str = "\"\"\"Doc.\"\"\"\r\nfrom .main import main\r\nfrom python.orders.daily.node import daily as upstream\r\nimport python.utils.text\r\n\r\nX = 'é'\r\n\r\n\r\n@sqlbuild.task(depends_on=[upstream], description=\"D.\")\r\ndef run(ctx: TaskContext, /, *, store: \"providers.Store\") -> PythonNodeResult | None:\r\n \"\"\"Doc.\"\"\"\r\n return main(ctx=ctx, store=store)\r\n"; + +const FACTORY_MODULE: &str = "@factory\ndef tables():\n for table in TABLES:\n for _ in specs():\n pass\n\n @task(name=table)\n def load(ctx, *args, **kwargs) -> Optional[Union[LoaderResult, list]]:\n rows = main(ctx=ctx)\n return rows\n\n def plain():\n return main(ctx, **extra)\n"; + +#[test] +fn given_module_when_reading_then_each_top_level_statement_has_its_kind() { + let test_cases = [ + StatementKindsTestCase { + description: "a CRLF node module with a docstring, imports, a value and a node", + contents: CRLF_NODE_MODULE, + expected_kinds: Some(vec![ + "docstring", + "import", + "import", + "import", + "other", + "function", + ]), + }, + StatementKindsTestCase { + description: "a module that does not parse", + contents: "def broken(:\n", + expected_kinds: None, + }, + ]; + for test_case in test_cases { + assert_eq!( + statement_kinds(test_case.contents), + test_case.expected_kinds, + "{}", + test_case.description + ); + } +} + +#[test] +fn given_imports_when_reading_then_each_bound_name_records_whether_only_depends_on_uses_it() { + let test_cases = [ImportsTestCase { + description: "a relative import, an aliased node used in depends_on and a dotted import", + contents: CRLF_NODE_MODULE, + expected_imports: vec![ + UnitImport { + level: 1, + ..import("main", Some("main"), "main", at(2, 19)) + }, + UnitImport { + only_in_depends_on: true, + ..import( + "python.orders.daily.node", + Some("daily"), + "upstream", + at(3, 38), + ) + }, + UnitImport { + only_in_depends_on: true, + ..import("python.utils.text", None, "python", at(4, 8)) + }, + ], + }]; + for test_case in test_cases { + assert_eq!( + imports(test_case.contents), + test_case.expected_imports, + "{}", + test_case.description + ); + } +} + +#[test] +fn given_declaration_when_reading_then_signature_body_and_inner_declarations_are_read() { + let test_cases = [ + FunctionTestCase { + description: "a node with positional-only ctx, a quoted provider and a union result", + contents: CRLF_NODE_MODULE, + expected_function: UnitFunction { + name: "run".to_owned(), + position: at(10, 5), + decorators: vec![decorator( + "task", + &["depends_on", "description"], + &["upstream"], + )], + positional: vec![parameter("ctx", Some("TaskContext"))], + variadic: false, + keyword_only: vec![parameter("store", Some("Store"))], + keywords_variadic: false, + returns: Some(names(&["PythonNodeResult", "None"])), + docstring: true, + returns_directly: true, + delegation: Some(true), + inner: vec![], + loops: vec![], + }, + }, + FunctionTestCase { + description: "a factory whose generated node computes before returning", + contents: FACTORY_MODULE, + expected_function: UnitFunction { + name: "tables".to_owned(), + position: at(2, 5), + decorators: vec![decorator("factory", &[], &[])], + positional: vec![], + variadic: false, + keyword_only: vec![], + keywords_variadic: false, + returns: None, + docstring: false, + returns_directly: false, + delegation: None, + inner: vec![UnitFunction { + name: "load".to_owned(), + position: at(8, 13), + decorators: vec![decorator("task", &["name"], &[])], + positional: vec![parameter("ctx", None)], + variadic: true, + keyword_only: vec![], + keywords_variadic: true, + returns: Some(names(&["LoaderResult", "list", "None"])), + docstring: false, + returns_directly: false, + delegation: None, + inner: vec![], + loops: vec![], + }], + loops: vec![Some("TABLES".to_owned()), None], + }, + }, + FunctionTestCase { + description: "a body that calls main() with a positional argument and unpacking", + contents: "def run(ctx):\n return main(ctx, **extra)\n", + expected_function: UnitFunction { + name: "run".to_owned(), + position: at(1, 5), + decorators: vec![], + positional: vec![parameter("ctx", None)], + variadic: false, + keyword_only: vec![], + keywords_variadic: false, + returns: None, + docstring: false, + returns_directly: true, + delegation: Some(false), + inner: vec![], + loops: vec![], + }, + }, + ]; + for test_case in test_cases { + assert_eq!( + first_function(test_case.contents), + Some(test_case.expected_function), + "{}", + test_case.description + ); + } +} diff --git a/crates/sqlbuild-pysource/src/node_units/tests/test_types.rs b/crates/sqlbuild-pysource/src/node_units/tests/test_types.rs new file mode 100644 index 000000000..66490f98e --- /dev/null +++ b/crates/sqlbuild-pysource/src/node_units/tests/test_types.rs @@ -0,0 +1,19 @@ +use crate::node_units::models::{UnitFunction, UnitImport}; + +pub(super) struct StatementKindsTestCase { + pub(super) description: &'static str, + pub(super) contents: &'static str, + pub(super) expected_kinds: Option>, +} + +pub(super) struct ImportsTestCase { + pub(super) description: &'static str, + pub(super) contents: &'static str, + pub(super) expected_imports: Vec, +} + +pub(super) struct FunctionTestCase { + pub(super) description: &'static str, + pub(super) contents: &'static str, + pub(super) expected_function: UnitFunction, +} diff --git a/crates/sqlbuild-rules/src/engine/tests/test_incremental_rules.rs b/crates/sqlbuild-rules/src/engine/tests/test_incremental_rules.rs index 17946eff4..6112085f7 100644 --- a/crates/sqlbuild-rules/src/engine/tests/test_incremental_rules.rs +++ b/crates/sqlbuild-rules/src/engine/tests/test_incremental_rules.rs @@ -48,6 +48,16 @@ const REVIEWED_SCOPES: &[(&str, RuleScope)] = &[ ("SQBRPROJECT204", RuleScope::Project), ("SQBRPYTHON001", RuleScope::Python), ("SQBRPYTHON002", RuleScope::Python), + ("SQBRPYTHON003", RuleScope::Python), + ("SQBRPYTHON004", RuleScope::Python), + ("SQBRPYTHON005", RuleScope::Python), + ("SQBRPYTHON006", RuleScope::Python), + ("SQBRPYTHON007", RuleScope::Python), + ("SQBRPYTHON008", RuleScope::Python), + ("SQBRPYTHON009", RuleScope::Python), + ("SQBRPYTHON010", RuleScope::Python), + ("SQBRPYTHON011", RuleScope::Python), + ("SQBRPYTHON012", RuleScope::Python), ("SQBRPYTHON101", RuleScope::Python), ("SQBRPYTHON102", RuleScope::Python), ("SQBRPYTHON103", RuleScope::Python), diff --git a/crates/sqlbuild-rules/src/python_rules/_helpers/faults.rs b/crates/sqlbuild-rules/src/python_rules/_helpers/faults.rs index 144b95f21..1f03073c1 100644 --- a/crates/sqlbuild-rules/src/python_rules/_helpers/faults.rs +++ b/crates/sqlbuild-rules/src/python_rules/_helpers/faults.rs @@ -77,3 +77,22 @@ fn line_start(text: &str, line: u32) -> Option { } Some(index) } + +/// A fault of the catalogue rule `code` on `path` at `(line, column)`, its message followed by `detail`. +pub(crate) fn catalogue_fault( + code: &str, + path: &str, + (line, column): (u64, u64), + detail: &str, +) -> Option { + let rule = PYTHON_RULES.iter().find(|rule| rule.code == code)?; + Some(Fault { + unevaluated: false, + code: code.to_owned(), + path: path.to_owned(), + line, + column, + message: format!("{}: {detail}", rule.message), + remediation: rule.remediation.to_owned(), + }) +} diff --git a/crates/sqlbuild-rules/src/python_rules/_helpers/layout.rs b/crates/sqlbuild-rules/src/python_rules/_helpers/layout.rs index f61e67846..cd0590b83 100644 --- a/crates/sqlbuild-rules/src/python_rules/_helpers/layout.rs +++ b/crates/sqlbuild-rules/src/python_rules/_helpers/layout.rs @@ -3,10 +3,11 @@ use std::collections::BTreeSet; use crate::models::{Fault, PythonSource}; +use crate::python_rules::_helpers::faults::catalogue_fault; use crate::python_rules::_helpers::placement::package_anchor; use crate::python_rules::constants::{ GENERIC_PACKAGE_NAMES, INIT_FILE, NODE_KIND_DIRECTORIES, PACKAGE_ROOT, - PROJECT_ROOT_ALLOWLIST_CODE, PYTHON_RULES, ROLE_DIRECTORIES, RUNTIME_PACKAGE_OWNER_NAMES_CODE, + PROJECT_ROOT_ALLOWLIST_CODE, ROLE_DIRECTORIES, RUNTIME_PACKAGE_OWNER_NAMES_CODE, SHARED_UTILITIES_PACKAGE, }; @@ -95,14 +96,5 @@ fn package_parts(sources: &[PythonSource]) -> impl Iterator> { /// A layout fault of rule `code` on the file at `path`. fn layout_fault(code: &str, path: &str, detail: &str) -> Option { - let rule = PYTHON_RULES.iter().find(|rule| rule.code == code)?; - Some(Fault { - unevaluated: false, - code: code.to_owned(), - path: path.to_owned(), - line: 1, - column: 1, - message: format!("{}: {detail}", rule.message), - remediation: rule.remediation.to_owned(), - }) + catalogue_fault(code, path, (1, 1), detail) } diff --git a/crates/sqlbuild-rules/src/python_rules/_helpers/mod.rs b/crates/sqlbuild-rules/src/python_rules/_helpers/mod.rs index 6685ab9f6..dd40c5313 100644 --- a/crates/sqlbuild-rules/src/python_rules/_helpers/mod.rs +++ b/crates/sqlbuild-rules/src/python_rules/_helpers/mod.rs @@ -7,3 +7,4 @@ pub(crate) mod observations; pub(crate) mod placement; pub(crate) mod programs; pub(crate) mod project_plane; +pub(crate) mod units; diff --git a/crates/sqlbuild-rules/src/python_rules/_helpers/units/imports.rs b/crates/sqlbuild-rules/src/python_rules/_helpers/units/imports.rs new file mode 100644 index 000000000..97e800af4 --- /dev/null +++ b/crates/sqlbuild-rules/src/python_rules/_helpers/units/imports.rs @@ -0,0 +1,79 @@ +//! Which project modules a unit's files may import. + +use sqlbuild_pysource::node_units::models::UnitImport; + +use crate::models::Fault; +use crate::python_rules::_helpers::units::project::{imported_module, unit_fault}; +use crate::python_rules::constants::{ + NODE_IMPORT_BOUNDARIES_CODE, NODE_MODULE, PACKAGE_ROOT, PROVIDER_ROLE_MODULES, PROVIDERS_ROOT, + SHARED_ROLE_MODULES, SHARED_UTILITIES_PACKAGE, UNIT_HELPERS_FOLDER, +}; +use crate::python_rules::models::UnitProject; + +/// Each import in a unit's files of a project module outside the unit's boundaries. +pub(crate) fn import_boundary_faults(project: &UnitProject<'_>) -> Vec { + let mut faults: Vec = Vec::new(); + for file in &project.files { + let Some(unit) = file.unit.as_deref() else { + continue; + }; + for import in &file.module.imports { + let Some(module) = imported_module(import, file, project) else { + continue; + }; + if !project.modules.contains(&module) || allowed(&module, import, unit, project) { + continue; + } + faults.extend(unit_fault( + NODE_IMPORT_BOUNDARIES_CODE, + file.path, + import.position, + &format!( + "{} imports {module}, which is outside its unit's boundaries", + file.path + ), + )); + } + } + faults +} + +/// Whether a file of `unit` may import the project module `module` through `import`. +fn allowed(module: &str, import: &UnitImport, unit: &str, project: &UnitProject<'_>) -> bool { + let unit_parts: Vec<&str> = unit.split('/').collect(); + let own: String = format!("{PACKAGE_ROOT}.{}", unit_parts.join(".")); + let mut shared: Vec = vec![own, format!("{PACKAGE_ROOT}.{SHARED_UTILITIES_PACKAGE}")]; + for depth in 1..unit_parts.len() { + let ancestor: String = format!("{PACKAGE_ROOT}.{}", unit_parts[..depth].join(".")); + shared.extend( + SHARED_ROLE_MODULES + .iter() + .chain([&UNIT_HELPERS_FOLDER]) + .map(|role| format!("{ancestor}.{role}")), + ); + } + if shared.iter().any(|prefix| within(module, prefix)) || provider_role(module) { + return true; + } + let other_node: bool = project + .units + .iter() + .any(|other| module == format!("{PACKAGE_ROOT}.{}.{NODE_MODULE}", other.replace('/', "."))); + other_node && import.name.is_some() && import.only_in_depends_on +} + +/// Whether `module` is a public role file of a provider capability, `providers..`. +fn provider_role(module: &str) -> bool { + let parts: Vec<&str> = module.split('.').collect(); + matches!( + parts.as_slice(), + [root, _, role, ..] if *root == PROVIDERS_ROOT && PROVIDER_ROLE_MODULES.contains(role) + ) +} + +/// Whether `module` is `package` or sits below it. +fn within(module: &str, package: &str) -> bool { + module + .strip_prefix(package) + .is_some_and(|rest| rest.is_empty() || rest.starts_with('.')) +} diff --git a/crates/sqlbuild-rules/src/python_rules/_helpers/units/layout.rs b/crates/sqlbuild-rules/src/python_rules/_helpers/units/layout.rs new file mode 100644 index 000000000..887547907 --- /dev/null +++ b/crates/sqlbuild-rules/src/python_rules/_helpers/units/layout.rs @@ -0,0 +1,157 @@ +//! What a unit folder holds, and nodes and checks declared outside their unit's files. + +use std::collections::BTreeSet; + +use sqlbuild_pysource::node_units::models::UnitFunction; + +use crate::models::{Fault, PythonSource}; +use crate::python_rules::_helpers::faults::catalogue_fault; +use crate::python_rules::_helpers::placement::package_anchor; +use crate::python_rules::_helpers::units::project::{ + Declared, declared, top_functions, unit_fault, unit_file_name, +}; +use crate::python_rules::constants::{ + NODE_DECLARED_IN_UNIT_CODE, NODE_KIND_DIRECTORIES, NODE_UNIT_LAYOUT_CODE, PACKAGE_ROOT, + ROLE_DIRECTORIES, SHARED_UTILITIES_PACKAGE, UNIT_CHECKS_FILE, UNIT_HELPERS_FOLDER, + UNIT_MAIN_FILE, UNIT_NODE_FILE, UNIT_ROLE_FILES, +}; +use crate::python_rules::models::{UnitFile, UnitProject}; + +/// A unit without `main.py`, and modules or folders a unit may not hold. +pub(crate) fn unit_layout_faults( + project: &UnitProject<'_>, + sources: &[PythonSource], +) -> Vec { + let mut faults: Vec = project + .units + .iter() + .filter(|unit| { + !project + .paths + .contains(format!("{PACKAGE_ROOT}/{unit}/{UNIT_MAIN_FILE}").as_str()) + }) + .filter_map(|unit| { + catalogue_fault( + NODE_UNIT_LAYOUT_CODE, + &format!("{PACKAGE_ROOT}/{unit}/{UNIT_NODE_FILE}"), + (1, 1), + &format!("{PACKAGE_ROOT}/{unit}/ has no {UNIT_MAIN_FILE}"), + ) + }) + .collect(); + let mut folders: BTreeSet = BTreeSet::new(); + for path in &project.paths { + let Some(rest) = path.strip_prefix(&format!("{PACKAGE_ROOT}/")) else { + continue; + }; + let parts: Vec<&str> = rest.split('/').collect(); + let Some(depth) = (1..parts.len()) + .rev() + .find(|depth| project.units.contains(&parts[..*depth].join("/"))) + else { + continue; + }; + let unit_path: String = format!("{PACKAGE_ROOT}/{}", parts[..depth].join("/")); + let entry: &str = parts[depth]; + if parts.len() == depth + 1 { + let allowed: bool = [UNIT_NODE_FILE, UNIT_MAIN_FILE].contains(&entry) + || UNIT_ROLE_FILES.contains(&entry); + if !allowed { + faults.extend(catalogue_fault( + NODE_UNIT_LAYOUT_CODE, + path, + (1, 1), + &format!("{unit_path}/{entry} is not a unit role file"), + )); + } + } else if entry != UNIT_HELPERS_FOLDER { + folders.insert(format!("{unit_path}/{entry}")); + } + } + faults.extend(folders.iter().filter_map(|folder| { + catalogue_fault( + NODE_UNIT_LAYOUT_CODE, + package_anchor(sources, folder)?, + (1, 1), + &format!("{folder}/ is not {UNIT_HELPERS_FOLDER}/"), + ) + })); + faults +} + +/// Each node or factory declared outside a unit's `node.py`, and each check outside its unit's files. +pub(crate) fn declared_outside_unit_faults(project: &UnitProject<'_>) -> Vec { + let mut faults: Vec = Vec::new(); + for file in &project.files { + let home: Option<&str> = unit_file_name(file); + for function in top_functions(file) { + let outer: Option = declared(function).map(|(what, _)| what); + if let Some(fault) = misplaced(file, home, function, false) { + faults.push(fault); + } + let in_factory: bool = outer == Some(Declared::Factory); + faults.extend( + function + .inner + .iter() + .filter_map(|inner| misplaced(file, home, inner, in_factory)), + ); + } + } + faults +} + +/// The fault for `function` when what it declares does not belong in a file named `home`. +fn misplaced( + file: &UnitFile<'_>, + home: Option<&str>, + function: &UnitFunction, + in_factory: bool, +) -> Option { + let (what, decorator) = declared(function)?; + let at_home: bool = match what { + Declared::Node(..) => home == Some(UNIT_NODE_FILE), + Declared::Factory => matches!(home, Some(UNIT_NODE_FILE | UNIT_CHECKS_FILE)), + Declared::Check => { + home == Some(UNIT_CHECKS_FILE) || (home == Some(UNIT_NODE_FILE) && in_factory) + } + }; + if at_home { + return None; + } + let destination: String = match what { + Declared::Check => { + format!("the {UNIT_CHECKS_FILE} of the unit folder whose node it checks") + } + _ => format!( + "{PACKAGE_ROOT}/{}/{}/{UNIT_NODE_FILE}", + domain(file), + function.name + ), + }; + unit_fault( + NODE_DECLARED_IN_UNIT_CODE, + file.path, + function.position, + &format!( + "@{} '{}' in {} belongs in {destination}", + decorator.name, function.name, file.path + ), + ) +} + +/// The domain folder `file` sits in, or `` when it sits in no domain. +fn domain(file: &UnitFile<'_>) -> String { + let [first, _, ..] = file.parts.as_slice() else { + return "".to_owned(); + }; + let first: &str = first; + let not_a_domain: bool = first == SHARED_UTILITIES_PACKAGE + || NODE_KIND_DIRECTORIES.contains(&first) + || ROLE_DIRECTORIES.iter().any(|(name, _)| *name == first); + if not_a_domain { + "".to_owned() + } else { + first.to_owned() + } +} diff --git a/crates/sqlbuild-rules/src/python_rules/_helpers/units/mod.rs b/crates/sqlbuild-rules/src/python_rules/_helpers/units/mod.rs new file mode 100644 index 000000000..86a8cf301 --- /dev/null +++ b/crates/sqlbuild-rules/src/python_rules/_helpers/units/mod.rs @@ -0,0 +1,5 @@ +pub(crate) mod imports; +pub(crate) mod layout; +pub(crate) mod modules; +pub(crate) mod nodes; +pub(crate) mod project; diff --git a/crates/sqlbuild-rules/src/python_rules/_helpers/units/modules.rs b/crates/sqlbuild-rules/src/python_rules/_helpers/units/modules.rs new file mode 100644 index 000000000..07a5d825b --- /dev/null +++ b/crates/sqlbuild-rules/src/python_rules/_helpers/units/modules.rs @@ -0,0 +1,220 @@ +//! The statements a unit's node.py, main.py and checks.py may hold. + +use sqlbuild_pysource::node_units::models::{UnitFunction, UnitPosition, UnitStatementKind}; + +use crate::models::Fault; +use crate::python_rules::_helpers::units::project::{ + Declared, declared, names_from_unit_modules, top_functions, unit_fault, unit_file_name, + unit_member, +}; +use crate::python_rules::constants::{ + CHECKS_MODULE_SHAPE_CODE, MAIN_MODULE_SHAPE_CODE, MAIN_NAME, MAIN_RESULTS, NODE_MODULE, + NODE_MODULE_SHAPE_CODE, UNIT_CHECKS_FILE, UNIT_MAIN_FILE, UNIT_NODE_FILE, +}; +use crate::python_rules::models::{UnitFile, UnitProject}; + +const FILE_START: UnitPosition = UnitPosition { line: 1, column: 1 }; +const OTHER_STATEMENT: &str = "a statement that is not an import or a function"; + +/// Statements and functions node.py, main.py and checks.py may not hold. +pub(crate) fn module_shape_faults(project: &UnitProject<'_>) -> Vec { + let mut faults: Vec = Vec::new(); + for file in &project.files { + match unit_file_name(file) { + Some(UNIT_NODE_FILE) => faults.extend(node_module_faults(file)), + Some(UNIT_MAIN_FILE) => faults.extend(main_module_faults(file, project)), + Some(UNIT_CHECKS_FILE) => faults.extend(checks_module_faults(file, project)), + _ => {} + } + } + faults +} + +/// node.py: imports and exactly one node or factory declaration. +fn node_module_faults(file: &UnitFile<'_>) -> Vec { + let mut faults: Vec = Vec::new(); + let mut declarations: usize = 0; + for statement in &file.module.statements { + let (detail, position): (String, UnitPosition) = match &statement.kind { + UnitStatementKind::Docstring | UnitStatementKind::Import => continue, + UnitStatementKind::Function(function) => match declared(function) { + Some((Declared::Node(..) | Declared::Factory, _)) => { + declarations += 1; + if declarations == 1 { + continue; + } + ( + format!("'{}' is a second declaration", function.name), + function.position, + ) + } + _ => ( + format!("function '{}' is not the node declaration", function.name), + function.position, + ), + }, + UnitStatementKind::Other => (OTHER_STATEMENT.to_owned(), statement.position), + }; + faults.extend(unit_fault( + NODE_MODULE_SHAPE_CODE, + file.path, + position, + &detail, + )); + } + if declarations == 0 { + faults.extend(unit_fault( + NODE_MODULE_SHAPE_CODE, + file.path, + FILE_START, + &format!("{} declares no node", file.path), + )); + } + faults +} + +/// main.py: imports and one public main() whose return annotation fits the unit's node kind. +fn main_module_faults(file: &UnitFile<'_>, project: &UnitProject<'_>) -> Vec { + let kind: Option<&str> = file + .unit + .as_deref() + .and_then(|unit| node_kind(project, unit)); + let mut faults: Vec = Vec::new(); + let mut found: bool = false; + for statement in &file.module.statements { + let (detail, position): (String, UnitPosition) = match &statement.kind { + UnitStatementKind::Docstring | UnitStatementKind::Import => continue, + UnitStatementKind::Function(function) if function.name == MAIN_NAME && !found => { + found = true; + match main_result_detail(function, kind) { + Some(detail) => (detail, function.position), + None => continue, + } + } + UnitStatementKind::Function(function) => ( + format!("function '{}' is not main()", function.name), + function.position, + ), + UnitStatementKind::Other => (OTHER_STATEMENT.to_owned(), statement.position), + }; + faults.extend(unit_fault( + MAIN_MODULE_SHAPE_CODE, + file.path, + position, + &detail, + )); + } + if !found { + faults.extend(unit_fault( + MAIN_MODULE_SHAPE_CODE, + file.path, + FILE_START, + &format!("{} defines no main()", file.path), + )); + } + faults +} + +/// Why main()'s return annotation does not fit a node of `kind`, if it does not. +fn main_result_detail(function: &UnitFunction, kind: Option<&str>) -> Option { + let Some(returns) = &function.returns else { + return Some("main() has no return annotation".to_owned()); + }; + let kind: &str = kind?; + let (_, allowed) = MAIN_RESULTS.iter().find(|(name, _)| *name == kind)?; + let wrong: Vec<&str> = returns + .iter() + .map(String::as_str) + .filter(|name| !allowed.contains(name)) + .collect(); + (!wrong.is_empty()).then(|| { + format!( + "main() returns {}, which is not a @{kind} result", + wrong.join(" | ") + ) + }) +} + +/// The kind of the node the unit's node.py declares, or of the first node its factory generates. +fn node_kind(project: &UnitProject<'_>, unit: &str) -> Option<&'static str> { + let node: &UnitFile<'_> = unit_member(project, unit, UNIT_NODE_FILE)?; + top_functions(node).find_map(|function| match declared(function)? { + (Declared::Node(kind, _), _) => Some(kind), + (Declared::Factory, _) => function.inner.iter().find_map(declared_node_kind), + _ => None, + }) +} + +/// The kind of node `function` declares, if it declares one. +fn declared_node_kind(function: &UnitFunction) -> Option<&'static str> { + match declared(function)? { + (Declared::Node(kind, _), _) => Some(kind), + _ => None, + } +} + +/// checks.py: imports and checks of the unit's own node that return directly, or one factory. +fn checks_module_faults(file: &UnitFile<'_>, project: &UnitProject<'_>) -> Vec { + let unit_node: Vec = file.unit.as_deref().map_or_else(Vec::new, |unit| { + let mut names: Vec = names_from_unit_modules(file, project, unit, &[NODE_MODULE]) + .into_iter() + .collect(); + if let Some(node) = unit_member(project, unit, UNIT_NODE_FILE) { + names.extend(top_functions(node).map(|function| function.name.clone())); + } + names + }); + let mut faults: Vec = Vec::new(); + let mut factories: usize = 0; + for statement in &file.module.statements { + let details: Vec<(String, UnitPosition)> = match &statement.kind { + UnitStatementKind::Docstring | UnitStatementKind::Import => continue, + UnitStatementKind::Function(function) => match declared(function) { + Some((Declared::Check, decorator)) => { + let mut details: Vec<(String, UnitPosition)> = Vec::new(); + if !decorator + .depends_on + .iter() + .any(|name| unit_node.contains(name)) + { + details.push(( + format!( + "check '{}' does not depend on its unit's node", + function.name + ), + function.position, + )); + } + if !function.returns_directly { + details.push(( + format!("check '{}' does more than return", function.name), + function.position, + )); + } + details + } + Some((Declared::Factory, _)) => { + factories += 1; + if factories == 1 { + continue; + } + vec![( + format!("'{}' is a second factory", function.name), + function.position, + )] + } + _ => vec![( + format!("function '{}' is not a check", function.name), + function.position, + )], + }, + UnitStatementKind::Other => { + vec![(OTHER_STATEMENT.to_owned(), statement.position)] + } + }; + faults.extend(details.iter().filter_map(|(detail, position)| { + unit_fault(CHECKS_MODULE_SHAPE_CODE, file.path, *position, detail) + })); + } + faults +} diff --git a/crates/sqlbuild-rules/src/python_rules/_helpers/units/nodes.rs b/crates/sqlbuild-rules/src/python_rules/_helpers/units/nodes.rs new file mode 100644 index 000000000..05ead161e --- /dev/null +++ b/crates/sqlbuild-rules/src/python_rules/_helpers/units/nodes.rs @@ -0,0 +1,195 @@ +//! How a unit's nodes, checks and factories are declared: delegation, signature, factory shape and description. + +use std::collections::BTreeSet; + +use sqlbuild_pysource::node_units::models::{UnitDecorator, UnitFunction}; + +use crate::models::Fault; +use crate::python_rules::_helpers::units::project::{ + Declared, declared, names_from_unit_modules, top_functions, unit_fault, unit_file_name, +}; +use crate::python_rules::constants::{ + CHECK_KIND, CONTEXT_PARAMETER, DESCRIPTION_KEYWORD, FACTORY_SHAPE_CODE, LOADER_DECORATOR, + MAIN_NAME, NAME_KEYWORD, NODE_DELEGATES_TO_MAIN_CODE, NODE_DESCRIPTION_CODE, + NODE_SIGNATURE_CODE, SPEC_MODULES, UNIT_CHECKS_FILE, UNIT_NODE_FILE, +}; +use crate::python_rules::models::{UnitFile, UnitProject}; + +/// The names a unit file imports from its unit's main.py and spec modules. +#[derive(Debug)] +struct UnitNames { + main: BTreeSet, + specs: BTreeSet, +} + +/// Declaration faults of every node, check and factory in a unit's node.py and checks.py. +pub(crate) fn declaration_faults(project: &UnitProject<'_>) -> Vec { + let mut faults: Vec = Vec::new(); + for file in &project.files { + let (Some(unit), Some(UNIT_NODE_FILE | UNIT_CHECKS_FILE)) = + (file.unit.as_deref(), unit_file_name(file)) + else { + continue; + }; + let names = UnitNames { + main: names_from_unit_modules(file, project, unit, &[MAIN_NAME]), + specs: names_from_unit_modules(file, project, unit, &SPEC_MODULES), + }; + for function in top_functions(file) { + match declared(function) { + Some((Declared::Factory, _)) => { + faults.extend(factory_faults(file, function, &names)); + } + Some((what, decorator)) => { + faults.extend(declaration_fault_list( + file, + function, + (what, decorator), + &names, + )); + } + None => {} + } + } + } + faults +} + +/// The faults of one node or check declaration, top-level or generated by a factory. +fn declaration_fault_list( + file: &UnitFile<'_>, + function: &UnitFunction, + (what, decorator): (Declared, &UnitDecorator), + names: &UnitNames, +) -> Vec { + let mut details: Vec<(&str, String)> = Vec::new(); + if let Declared::Node(..) = what { + details.extend( + delegation_detail(function, names).map(|detail| (NODE_DELEGATES_TO_MAIN_CODE, detail)), + ); + } + let context: &str = match what { + Declared::Node(_, context) => context, + _ => CHECK_KIND.1, + }; + details.extend( + signature_details(function, context) + .into_iter() + .map(|detail| (NODE_SIGNATURE_CODE, detail)), + ); + details.extend( + description_detail(function, decorator).map(|detail| (NODE_DESCRIPTION_CODE, detail)), + ); + details + .iter() + .filter_map(|(code, detail)| unit_fault(code, file.path, function.position, detail)) + .collect() +} + +/// Why `function`'s body is not `return main(...)` with keyword arguments and `main` from its unit, if it is not. +fn delegation_detail(function: &UnitFunction, names: &UnitNames) -> Option { + let name: &str = &function.name; + match function.delegation { + None => Some(format!("'{name}' does not return main(...)")), + Some(false) => Some(format!( + "'{name}' passes main() an argument that is not name=value" + )), + Some(true) if !names.main.contains(MAIN_NAME) => Some(format!( + "'{name}' calls a main() that is not imported from its unit's main.py" + )), + Some(true) => None, + } +} + +/// What is wrong with `function`'s parameters for a declaration whose `ctx` is a `context`. +fn signature_details(function: &UnitFunction, context: &str) -> Vec { + let name: &str = &function.name; + let mut details: Vec = Vec::new(); + match function.positional.as_slice() { + [ctx] if ctx.name == CONTEXT_PARAMETER => { + if ctx.annotation.as_deref() != Some(context) { + details.push(format!("'{name}' does not annotate ctx as {context}")); + } + } + _ => details.push(format!( + "'{name}' does not take ctx as its only positional parameter" + )), + } + if function.variadic || function.keywords_variadic { + details.push(format!("'{name}' takes *args or **kwargs")); + } + details.extend( + function + .keyword_only + .iter() + .filter(|parameter| parameter.annotation.is_none()) + .map(|parameter| format!("'{name}' does not annotate provider '{}'", parameter.name)), + ); + details +} + +/// Why `function` sets a docstring or lacks description=, if it does. +fn description_detail(function: &UnitFunction, decorator: &UnitDecorator) -> Option { + let name: &str = &function.name; + let described: bool = decorator + .keywords + .iter() + .any(|keyword| keyword == DESCRIPTION_KEYWORD); + if decorator.name == LOADER_DECORATOR { + return (described || function.docstring) + .then(|| format!("loader '{name}' sets a description or docstring")); + } + if function.docstring { + return Some(format!("'{name}' has a docstring")); + } + (!described).then(|| format!("'{name}' does not set description=")) +} + +/// A factory's loops, generated names and generated declarations. +fn factory_faults(file: &UnitFile<'_>, factory: &UnitFunction, names: &UnitNames) -> Vec { + let mut details: Vec = Vec::new(); + if factory.loops.is_empty() { + details.push(format!( + "factory '{}' does not loop over specs", + factory.name + )); + } + if factory.loops.iter().any(|iterable| { + !iterable + .as_ref() + .is_some_and(|name| names.specs.contains(name)) + }) { + details.push(format!( + "factory '{}' loops over something that is not imported from its unit's constants.py or models.py", + factory.name + )); + } + let mut faults: Vec = details + .iter() + .filter_map(|detail| unit_fault(FACTORY_SHAPE_CODE, file.path, factory.position, detail)) + .collect(); + for inner in &factory.inner { + let Some((what, decorator)) = declared(inner) else { + continue; + }; + if !decorator + .keywords + .iter() + .any(|keyword| keyword == NAME_KEYWORD) + { + faults.extend(unit_fault( + FACTORY_SHAPE_CODE, + file.path, + inner.position, + &format!("generated '{}' does not set name=", inner.name), + )); + } + faults.extend(declaration_fault_list( + file, + inner, + (what, decorator), + names, + )); + } + faults +} diff --git a/crates/sqlbuild-rules/src/python_rules/_helpers/units/project.rs b/crates/sqlbuild-rules/src/python_rules/_helpers/units/project.rs new file mode 100644 index 000000000..c967c6df6 --- /dev/null +++ b/crates/sqlbuild-rules/src/python_rules/_helpers/units/project.rs @@ -0,0 +1,222 @@ +//! The project's node units, read from parsed `python/` modules, and the unit rules run over them. + +use std::collections::BTreeSet; + +use sqlbuild_pysource::node_units::main::node_unit_module::node_unit_module; +use sqlbuild_pysource::node_units::models::{ + UnitDecorator, UnitFunction, UnitImport, UnitPosition, UnitStatementKind, +}; + +use crate::models::{Fault, PythonSource}; +use crate::python_rules::_helpers::faults::catalogue_fault; +use crate::python_rules::_helpers::units::imports::import_boundary_faults; +use crate::python_rules::_helpers::units::layout::{ + declared_outside_unit_faults, unit_layout_faults, +}; +use crate::python_rules::_helpers::units::modules::module_shape_faults; +use crate::python_rules::_helpers::units::nodes::declaration_faults; +use crate::python_rules::constants::{ + CHECK_KIND, FACTORY_DECORATORS, INIT_FILE, NODE_KINDS, PACKAGE_ROOT, UNIT_NODE_FILE, + UNIT_RULE_CODES, +}; +use crate::python_rules::models::{UnitFile, UnitProject}; + +/// Faults of the selected unit rules among `codes`. +pub(crate) fn unit_faults( + sources: &[PythonSource], + codes: &BTreeSet<&str>, + python_version: (u8, u8), +) -> Vec { + if !UNIT_RULE_CODES.iter().any(|code| codes.contains(code)) { + return Vec::new(); + } + let project: UnitProject<'_> = unit_project(sources, python_version); + let mut faults: Vec = unit_layout_faults(&project, sources); + faults.extend(declared_outside_unit_faults(&project)); + faults.extend(module_shape_faults(&project)); + faults.extend(declaration_faults(&project)); + faults.extend(import_boundary_faults(&project)); + faults.retain(|fault| codes.contains(fault.code.as_str())); + faults +} + +fn unit_project(sources: &[PythonSource], python_version: (u8, u8)) -> UnitProject<'_> { + let package: Vec<(&PythonSource, Vec<&str>)> = sources + .iter() + .filter_map(|source| { + let (root, rest) = source.relative_path.split_once('/')?; + (root == PACKAGE_ROOT).then(|| (source, rest.split('/').collect())) + }) + .collect(); + let units: BTreeSet = package + .iter() + .filter( + |(_, parts)| matches!(parts.as_slice(), [_, _, .., last] if *last == UNIT_NODE_FILE), + ) + .map(|(_, parts)| parts[..parts.len() - 1].join("/")) + .collect(); + let files: Vec> = package + .iter() + .filter_map(|(source, parts)| { + Some(UnitFile { + path: source.relative_path.as_str(), + module: node_unit_module(&source.text, python_version)?, + unit: nearest_unit(parts, &units), + parts: parts.clone(), + }) + }) + .collect(); + UnitProject { + files, + units, + paths: sources + .iter() + .map(|source| source.relative_path.as_str()) + .collect(), + modules: sources + .iter() + .flat_map(|source| dotted_names(&source.relative_path)) + .collect(), + } +} + +/// The deepest unit folder among the file's folders. +fn nearest_unit(parts: &[&str], units: &BTreeSet) -> Option { + (1..parts.len()) + .rev() + .map(|depth| parts[..depth].join("/")) + .find(|folder| units.contains(folder)) +} + +/// The dotted module of a Python path and of every package above it. +fn dotted_names(path: &str) -> Vec { + let stem: &str = path.strip_suffix(".py").unwrap_or(path); + let parts: Vec<&str> = stem + .split('/') + .filter(|part| *part != INIT_FILE.trim_end_matches(".py")) + .collect(); + (1..=parts.len()) + .map(|depth| parts[..depth].join(".")) + .collect() +} + +/// A fault of rule `code` at `position` in `path`. +pub(crate) fn unit_fault( + code: &str, + path: &str, + position: UnitPosition, + detail: &str, +) -> Option { + catalogue_fault( + code, + path, + (position.line as u64, position.column as u64), + detail, + ) +} + +/// The file's position in its unit: the file name when it sits directly in the unit folder. +pub(crate) fn unit_file_name<'a>(file: &UnitFile<'a>) -> Option<&'a str> { + let unit: &str = file.unit.as_deref()?; + let depth: usize = unit.split('/').count(); + (file.parts.len() == depth + 1).then(|| file.parts[depth]) +} + +/// The file in `project` at `python//`. +pub(crate) fn unit_member<'p, 'a>( + project: &'p UnitProject<'a>, + unit: &str, + name: &str, +) -> Option<&'p UnitFile<'a>> { + let path: String = format!("{PACKAGE_ROOT}/{unit}/{name}"); + project.files.iter().find(|file| file.path == path) +} + +/// What a decorator declares: a node kind, `check`, or a factory. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub(crate) enum Declared { + Node(&'static str, &'static str), + Check, + Factory, +} + +/// What `function`'s decorators declare, if anything. +pub(crate) fn declared(function: &UnitFunction) -> Option<(Declared, &UnitDecorator)> { + function + .decorators + .iter() + .find_map(|decorator| decorator_declares(&decorator.name).map(|what| (what, decorator))) +} + +/// What a decorator named `name` declares, if anything. +fn decorator_declares(name: &str) -> Option { + if let Some((kind, context)) = NODE_KINDS.iter().find(|(kind, _)| *kind == name) { + return Some(Declared::Node(kind, context)); + } + if name == CHECK_KIND.0 { + return Some(Declared::Check); + } + FACTORY_DECORATORS + .contains(&name) + .then_some(Declared::Factory) +} + +/// The module's top-level functions. +pub(crate) fn top_functions<'f>(file: &'f UnitFile<'_>) -> impl Iterator { + file.module + .statements + .iter() + .filter_map(|statement| match &statement.kind { + UnitStatementKind::Function(function) => Some(function), + _ => None, + }) +} + +/// The absolute dotted module `import` reads, resolving relative levels and `from package import module`. +pub(crate) fn imported_module( + import: &UnitImport, + file: &UnitFile<'_>, + project: &UnitProject<'_>, +) -> Option { + let mut base: Vec<&str> = Vec::new(); + if import.level > 0 { + base.push(PACKAGE_ROOT); + base.extend(&file.parts[..file.parts.len() - 1]); + for _ in 1..import.level { + base.pop()?; + } + } + base.extend(import.module.split('.').filter(|part| !part.is_empty())); + let module: String = base.join("."); + let Some(name) = &import.name else { + return Some(module); + }; + let submodule: String = format!("{module}.{name}"); + Some(if project.modules.contains(&submodule) { + submodule + } else { + module + }) +} + +/// The names `file` binds by importing from `python..`, one of `modules`. +pub(crate) fn names_from_unit_modules( + file: &UnitFile<'_>, + project: &UnitProject<'_>, + unit: &str, + modules: &[&str], +) -> BTreeSet { + let targets: Vec = modules + .iter() + .map(|module| format!("{PACKAGE_ROOT}.{}.{module}", unit.replace('/', "."))) + .collect(); + file.module + .imports + .iter() + .filter(|import| import.name.is_some()) + .filter(|import| { + imported_module(import, file, project).is_some_and(|module| targets.contains(&module)) + }) + .map(|import| import.local.clone()) + .collect() +} diff --git a/crates/sqlbuild-rules/src/python_rules/constants.rs b/crates/sqlbuild-rules/src/python_rules/constants.rs index f946cac88..1436b554a 100644 --- a/crates/sqlbuild-rules/src/python_rules/constants.rs +++ b/crates/sqlbuild-rules/src/python_rules/constants.rs @@ -5,7 +5,7 @@ use fensu_native::rules::constants as fensu; use crate::python_rules::models::{PythonFiles, PythonRuleSpec}; /// Every `SQBRPYTHON` rule; numbers are never reused, and new rules append to their band. -pub(crate) const PYTHON_RULES: [PythonRuleSpec; 64] = [ +pub(crate) const PYTHON_RULES: [PythonRuleSpec; 74] = [ PythonRuleSpec { code: "SQBRPYTHON001", fensu_code: None, @@ -16,7 +16,7 @@ pub(crate) const PYTHON_RULES: [PythonRuleSpec; 64] = [ enabled_by_default: true, options: &[], message: "python/ may hold only domain packages, utils/ and __init__.py", - remediation: "Move direct modules into a domain package, and nest node-kind and role folders such as loaders/, tasks/, main/ and _helpers/ inside a domain.", + remediation: "Move direct modules into a domain package. Give each node its own unit folder holding node.py and main.py inside its domain, such as python/orders/partner_export/, and keep role folders such as _helpers/ inside the domain or unit.", }, PythonRuleSpec { code: "SQBRPYTHON002", @@ -30,6 +30,126 @@ pub(crate) const PYTHON_RULES: [PythonRuleSpec; 64] = [ message: "packages under python/ must be named for what they own", remediation: "Rename generic packages such as common/, shared/ or helpers/ after the domain or capability they hold; utils/ is allowed only directly under python/.", }, + PythonRuleSpec { + code: "SQBRPYTHON003", + fensu_code: None, + slug: "node-unit-layout", + family: "units", + owner: "file", + applies: PythonFiles::Package, + enabled_by_default: true, + options: &[], + message: "a unit folder holds node.py, main.py, optional role files and _helpers/ only", + remediation: "Add main.py beside node.py, and move any other module into _helpers/ or a named role file: checks.py, models.py, types.py, constants.py or exceptions.py.", + }, + PythonRuleSpec { + code: "SQBRPYTHON004", + fensu_code: None, + slug: "node-module-shape", + family: "units", + owner: "file", + applies: PythonFiles::Package, + enabled_by_default: true, + options: &[], + message: "node.py holds imports and exactly one node or factory declaration", + remediation: "Keep one @loader, @task, @asset or @factory function in node.py; move helpers to _helpers/, constants to constants.py and checks to checks.py.", + }, + PythonRuleSpec { + code: "SQBRPYTHON005", + fensu_code: None, + slug: "node-delegates-to-main", + family: "units", + owner: "file", + applies: PythonFiles::Package, + enabled_by_default: true, + options: &[], + message: "a node declaration's body is `return main(...)` from its unit's main.py, with every argument passed by keyword", + remediation: "Move the logic into main() in the unit's main.py, import it with `from .main import main` or its full python. path, and pass each argument as name=value, including ctx.", + }, + PythonRuleSpec { + code: "SQBRPYTHON006", + fensu_code: None, + slug: "main-module-shape", + family: "units", + owner: "file", + applies: PythonFiles::Package, + enabled_by_default: true, + options: &[], + message: "main.py holds imports and one public main() whose return annotation fits the node kind", + remediation: "Move other functions and classes into _helpers/ or a role file, and annotate main() with the result it returns: ctx.result(...) or ctx.skip(...) for a task or asset, and rows or a loader result for a loader.", + }, + PythonRuleSpec { + code: "SQBRPYTHON007", + fensu_code: None, + slug: "node-signature", + family: "units", + owner: "file", + applies: PythonFiles::Package, + enabled_by_default: true, + options: &[], + message: "a node takes ctx as its only positional parameter and its providers as annotated keyword-only parameters", + remediation: "Write the signature as (ctx: Context, *, provider: ProviderClass), with no *args or **kwargs.", + }, + PythonRuleSpec { + code: "SQBRPYTHON008", + fensu_code: None, + slug: "node-import-boundaries", + family: "units", + owner: "file", + applies: PythonFiles::Package, + enabled_by_default: true, + options: &[], + message: "a unit imports only its own files, its domain's role files and _helpers/, python/utils/, provider role files, and other units' node.py for depends_on", + remediation: "Import shared code from python/utils// or the domain's role files, and name another unit's node only in depends_on; never import another unit's main.py, role files or _helpers/.", + }, + PythonRuleSpec { + code: "SQBRPYTHON009", + fensu_code: None, + slug: "checks-module-shape", + family: "units", + owner: "file", + applies: PythonFiles::Package, + enabled_by_default: true, + options: &[], + message: "checks.py holds imports and @check functions, or one @factory, each depending on the unit's own node", + remediation: "Name the unit's node in each check's depends_on, return directly from each check, and move other logic to _helpers/.", + }, + PythonRuleSpec { + code: "SQBRPYTHON010", + fensu_code: None, + slug: "factory-shape", + family: "units", + owner: "file", + applies: PythonFiles::Package, + enabled_by_default: true, + options: &[], + message: "a factory iterates over specs from its unit's constants.py or models.py and declares named nodes that return main(...)", + remediation: "Loop over a spec collection imported from the unit's constants.py or models.py, give every generated node name=, and make each body `return main(...)` with keyword arguments.", + }, + PythonRuleSpec { + code: "SQBRPYTHON011", + fensu_code: None, + slug: "node-description-not-docstring", + family: "units", + owner: "file", + applies: PythonFiles::Package, + enabled_by_default: true, + options: &[], + message: "@task, @asset and @check set description= without a docstring, and @loader sets neither", + remediation: "Move the docstring text into description=; for a loader, describe the managed source in its YAML instead.", + }, + PythonRuleSpec { + code: "SQBRPYTHON012", + fensu_code: None, + slug: "node-declared-in-unit", + family: "units", + owner: "file", + applies: PythonFiles::Package, + enabled_by_default: true, + options: &[], + message: "nodes are declared in a unit's node.py, and checks in a unit's checks.py", + remediation: "Move the declaration into its own unit folder under its domain, with the implementation in that folder's main.py.", + }, PythonRuleSpec { code: "SQBRPYTHON101", fensu_code: Some(fensu::PARAMETER_ANNOTATION_CODE), @@ -904,3 +1024,89 @@ pub(crate) const GENERIC_PACKAGE_NAMES: [&str; 9] = [ pub(crate) const SHARED_UTILITIES_PACKAGE: &str = "utils"; pub(crate) const PROJECT_ROOT_ALLOWLIST_CODE: &str = "SQBRPYTHON001"; pub(crate) const RUNTIME_PACKAGE_OWNER_NAMES_CODE: &str = "SQBRPYTHON002"; +pub(crate) const NODE_UNIT_LAYOUT_CODE: &str = "SQBRPYTHON003"; +pub(crate) const NODE_MODULE_SHAPE_CODE: &str = "SQBRPYTHON004"; +pub(crate) const NODE_DELEGATES_TO_MAIN_CODE: &str = "SQBRPYTHON005"; +pub(crate) const MAIN_MODULE_SHAPE_CODE: &str = "SQBRPYTHON006"; +pub(crate) const NODE_SIGNATURE_CODE: &str = "SQBRPYTHON007"; +pub(crate) const NODE_IMPORT_BOUNDARIES_CODE: &str = "SQBRPYTHON008"; +pub(crate) const CHECKS_MODULE_SHAPE_CODE: &str = "SQBRPYTHON009"; +pub(crate) const FACTORY_SHAPE_CODE: &str = "SQBRPYTHON010"; +pub(crate) const NODE_DESCRIPTION_CODE: &str = "SQBRPYTHON011"; +pub(crate) const NODE_DECLARED_IN_UNIT_CODE: &str = "SQBRPYTHON012"; +/// The file that declares a unit's node and makes its folder a unit. +pub(crate) const UNIT_NODE_FILE: &str = "node.py"; +/// The file holding a unit's checks. +pub(crate) const UNIT_CHECKS_FILE: &str = "checks.py"; +/// The files a unit folder may hold beside `node.py` and `main.py`. +pub(crate) const UNIT_ROLE_FILES: [&str; 6] = [ + "checks.py", + "models.py", + "types.py", + "constants.py", + "exceptions.py", + INIT_FILE, +]; +/// The one folder a unit may hold. +pub(crate) const UNIT_HELPERS_FOLDER: &str = "_helpers"; +/// The role modules a unit may import from its domain and from a provider. +pub(crate) const SHARED_ROLE_MODULES: [&str; 4] = ["models", "types", "constants", "exceptions"]; +/// The modules a factory may take its specs from. +pub(crate) const SPEC_MODULES: [&str; 2] = ["constants", "models"]; +/// The project folder holding providers, imported as `providers`. +pub(crate) const PROVIDERS_ROOT: &str = "providers"; +/// The public files of a provider capability a unit may import, as module names. +pub(crate) const PROVIDER_ROLE_MODULES: [&str; 5] = + ["provider", "models", "types", "constants", "exceptions"]; +/// The module a node delegates to, and the function it calls. +pub(crate) const MAIN_NAME: &str = "main"; +/// The module a unit declares its node in. +pub(crate) const NODE_MODULE: &str = "node"; +/// The only positional parameter of a node or check. +pub(crate) const CONTEXT_PARAMETER: &str = "ctx"; +/// The decorator keywords the unit rules read. +pub(crate) const DESCRIPTION_KEYWORD: &str = "description"; +pub(crate) const NAME_KEYWORD: &str = "name"; +/// Node decorators and the context type each node's `ctx` is annotated with. +pub(crate) const NODE_KINDS: [(&str, &str); 3] = [ + ("loader", "LoaderContext"), + ("task", "TaskContext"), + ("asset", "AssetContext"), +]; +/// The check decorator and its context type. +pub(crate) const CHECK_KIND: (&str, &str) = ("check", "CheckContext"); +/// Decorators that generate nodes or checks. +pub(crate) const FACTORY_DECORATORS: [&str; 1] = ["factory"]; +/// The loader decorator, which takes no description because its managed source has one. +pub(crate) const LOADER_DECORATOR: &str = "loader"; +/// Return annotations `main()` may use, by node decorator. +pub(crate) const MAIN_RESULTS: [(&str, &[&str]); 3] = [ + ("task", &["PythonNodeResult", "PythonNodeSkipResult"]), + ("asset", &["PythonNodeResult", "PythonNodeSkipResult"]), + ( + "loader", + &[ + "list", + "Iterator", + "Iterable", + "Generator", + "Sequence", + "LoaderResult", + "LoaderSkipResult", + "None", + ], + ), +]; +/// The unit rules, which read parsed modules under `python/`. +pub(crate) const UNIT_RULE_CODES: [&str; 10] = [ + NODE_UNIT_LAYOUT_CODE, + NODE_MODULE_SHAPE_CODE, + NODE_DELEGATES_TO_MAIN_CODE, + MAIN_MODULE_SHAPE_CODE, + NODE_SIGNATURE_CODE, + NODE_IMPORT_BOUNDARIES_CODE, + CHECKS_MODULE_SHAPE_CODE, + FACTORY_SHAPE_CODE, + NODE_DESCRIPTION_CODE, + NODE_DECLARED_IN_UNIT_CODE, +]; diff --git a/crates/sqlbuild-rules/src/python_rules/main/evaluate_python_rules.rs b/crates/sqlbuild-rules/src/python_rules/main/evaluate_python_rules.rs index 35d02041a..299aa5633 100644 --- a/crates/sqlbuild-rules/src/python_rules/main/evaluate_python_rules.rs +++ b/crates/sqlbuild-rules/src/python_rules/main/evaluate_python_rules.rs @@ -10,6 +10,7 @@ use crate::python_rules::_helpers::layout::layout_faults; use crate::python_rules::_helpers::placement::python_targets; use crate::python_rules::_helpers::programs::Programs; use crate::python_rules::_helpers::project_plane::loaded_project; +use crate::python_rules::_helpers::units::project::unit_faults; use crate::python_rules::models::{ CallInputs, PythonCacheEntry, PythonEvaluation, PythonRuleSpec, PythonTarget, }; @@ -41,6 +42,11 @@ pub(crate) fn evaluate_python_rules( evaluation .faults .extend(layout_faults(&request.python_sources, &codes)); + evaluation.faults.extend(unit_faults( + &request.python_sources, + &codes, + request.python_version, + )); evaluation.faults.sort_by(|left, right| { (&left.path, left.line, left.column, &left.code).cmp(&( &right.path, diff --git a/crates/sqlbuild-rules/src/python_rules/models.rs b/crates/sqlbuild-rules/src/python_rules/models.rs index a0227a87d..eb3925134 100644 --- a/crates/sqlbuild-rules/src/python_rules/models.rs +++ b/crates/sqlbuild-rules/src/python_rules/models.rs @@ -101,3 +101,26 @@ pub(crate) struct CacheSplit<'a> { pub fresh: Vec<(String, PythonCacheEntry)>, pub stale: Vec>, } + +/// One parsed module under `python/` and the unit folder it sits in. +#[derive(Debug)] +pub(crate) struct UnitFile<'a> { + pub path: &'a str, + /// Path parts below `python/`. + pub parts: Vec<&'a str>, + pub module: sqlbuild_pysource::node_units::models::UnitModule, + /// The nearest unit folder below `python/` that contains the file, at any depth. + pub unit: Option, +} + +/// The project's parsed `python/` modules, its unit folders and every module name it holds. +#[derive(Debug)] +pub(crate) struct UnitProject<'a> { + pub files: Vec>, + /// Folders below `python/` that hold a `node.py`, below a domain. + pub units: std::collections::BTreeSet, + /// Every project Python path. + pub paths: std::collections::BTreeSet<&'a str>, + /// Every project module and package as a dotted name, such as `python.orders.constants`. + pub modules: std::collections::BTreeSet, +} diff --git a/crates/sqlbuild-rules/src/python_rules/tests.rs b/crates/sqlbuild-rules/src/python_rules/tests.rs index 33bff6f6d..0506832c2 100644 --- a/crates/sqlbuild-rules/src/python_rules/tests.rs +++ b/crates/sqlbuild-rules/src/python_rules/tests.rs @@ -2,6 +2,8 @@ mod evaluate_python_rules; #[path = "tests/helpers.rs"] mod helpers; +#[path = "tests/test_node_units.rs"] +mod node_units; #[path = "tests/test_python_cache.rs"] mod python_cache; #[path = "tests/test_python_catalogue.rs"] diff --git a/crates/sqlbuild-rules/src/python_rules/tests/test_node_units.rs b/crates/sqlbuild-rules/src/python_rules/tests/test_node_units.rs new file mode 100644 index 000000000..e79a0225d --- /dev/null +++ b/crates/sqlbuild-rules/src/python_rules/tests/test_node_units.rs @@ -0,0 +1,614 @@ +use serde_json::json; + +use crate::python_rules::tests::helpers::{owned_faults, python_rule_faults}; +use crate::python_rules::tests::test_types::PythonRunTestCase; + +const UNIT_RULES: [&str; 10] = [ + "SQBRPYTHON003", + "SQBRPYTHON004", + "SQBRPYTHON005", + "SQBRPYTHON006", + "SQBRPYTHON007", + "SQBRPYTHON008", + "SQBRPYTHON009", + "SQBRPYTHON010", + "SQBRPYTHON011", + "SQBRPYTHON012", +]; + +const DAILY_TASK: &str = r#"from .main import main + + +@task(description="Daily.") +def daily(ctx: TaskContext) -> None: + return main(ctx=ctx) +"#; + +const TASK_MAIN: &str = r#"from sqlbuild import PythonNodeResult + + +def main(ctx: TaskContext) -> PythonNodeResult: + return ctx.result() +"#; + +#[test] +fn given_node_units_when_evaluating_unit_rules_then_reports_each_broken_convention() { + let test_cases = [ + PythonRunTestCase { + description: "a project laid out in units reports nothing", + files: &[ + ("providers/store/provider.py", "class Store:\n pass\n"), + ( + "python/utils/text.py", + "def clean() -> str:\n return ''\n", + ), + ("python/orders/constants.py", "LIMIT: int = 3\n"), + ( + "python/orders/_helpers/dates.py", + "def today() -> str:\n return ''\n", + ), + ( + "python/orders/daily/node.py", + r#"from providers.store.provider import Store + +from .main import main + + +@task(description="Daily.") +def daily(ctx: TaskContext, *, store: Store) -> None: + return main(ctx=ctx, store=store) +"#, + ), + ( + "python/orders/daily/main.py", + r#""""Daily totals.""" +from sqlbuild import PythonNodeResult, PythonNodeSkipResult + +from providers.store.provider import Store +from python.orders._helpers.dates import today +from python.orders.constants import LIMIT +from python.utils.text import clean + +from ._helpers.rows import rows + + +def main(ctx: TaskContext, store: Store) -> PythonNodeResult | PythonNodeSkipResult: + return ctx.result(rows(today(), clean(), LIMIT, store)) +"#, + ), + ( + "python/orders/daily/_helpers/rows.py", + "def rows() -> list:\n return []\n", + ), + ( + "python/orders/daily/checks.py", + r#"from .node import daily + + +@check(depends_on=[daily], description="Totals are positive.") +def totals_positive(ctx: CheckContext) -> bool: + return ctx.ok() +"#, + ), + ( + "python/orders/weekly/node.py", + r#"from python.orders.daily.node import daily +from python.orders.weekly.main import main + + +@asset(depends_on=[daily], description="Weekly.") +def weekly(ctx: AssetContext) -> None: + return main(ctx=ctx) +"#, + ), + ( + "python/orders/weekly/main.py", + "def main(ctx: AssetContext) -> PythonNodeResult:\n return ctx.result()\n", + ), + ( + "python/orders/raw/node.py", + r#"from .main import main + + +@loader +def raw(ctx: LoaderContext) -> list: + return main(ctx=ctx) +"#, + ), + ( + "python/orders/raw/main.py", + "def main(ctx: LoaderContext) -> Optional[Iterator[dict]]:\n return None\n", + ), + ( + "python/orders/regions/node.py", + r#"from .constants import REGIONS +from .main import main + + +@factory +def region_exports(): + for region in REGIONS: + + @task(name=f"export_{region}", description="Export one region.") + def export(ctx: TaskContext) -> None: + return main(ctx=ctx, region=region) + + @check(name=f"export_{region}_ok", depends_on=[export], description="Exported.") + def exported(ctx: CheckContext) -> bool: + return True +"#, + ), + ( + "python/orders/regions/constants.py", + "REGIONS: tuple = ('eu', 'us')\n", + ), + ( + "python/orders/regions/main.py", + "def main(ctx: TaskContext, region: str) -> PythonNodeResult:\n return ctx.result()\n", + ), + ], + config: json!({ "select": UNIT_RULES }), + expected_faults: vec![], + }, + PythonRunTestCase { + description: "unit layout: a missing main.py, a stray module and a folder that is not _helpers/", + files: &[ + ("python/orders/daily/node.py", DAILY_TASK), + ("python/orders/weekly/node.py", ""), + ("python/orders/weekly/main.py", ""), + ("python/orders/weekly/report.py", ""), + ("python/orders/weekly/sql/query.py", ""), + ("python/orders/weekly/_helpers/rows.py", ""), + ], + config: json!({ "select": ["SQBRPYTHON003"] }), + expected_faults: vec![ + ("SQBRPYTHON003", "python/orders/daily/node.py", 1, 1), + ("SQBRPYTHON003", "python/orders/weekly/report.py", 1, 1), + ("SQBRPYTHON003", "python/orders/weekly/sql/query.py", 1, 1), + ], + }, + PythonRunTestCase { + description: "node.py: statements, helpers and a second declaration report; an empty node.py reports at its start", + files: &[ + ( + "python/orders/daily/node.py", + r#""""Daily.""" +from .main import main + +LIMIT = 3 + + +def helper() -> int: + return 1 + + +@task(description="Daily.") +def daily(ctx: TaskContext) -> None: + return main(ctx=ctx) + + +@task(description="Extra.") +def extra(ctx: TaskContext) -> None: + return main(ctx=ctx) +"#, + ), + ("python/orders/weekly/node.py", "from .main import main\n"), + ], + config: json!({ "select": ["SQBRPYTHON004"] }), + expected_faults: vec![ + ("SQBRPYTHON004", "python/orders/daily/node.py", 4, 1), + ("SQBRPYTHON004", "python/orders/daily/node.py", 7, 5), + ("SQBRPYTHON004", "python/orders/daily/node.py", 17, 5), + ("SQBRPYTHON004", "python/orders/weekly/node.py", 1, 1), + ], + }, + PythonRunTestCase { + description: "delegation: positional arguments, extra statements, unpacking and another unit's main report", + files: &[ + ( + "python/orders/daily/node.py", + r#"from python.orders.daily.main import main +from python.orders.weekly.main import main as weekly_main + + +@task(description="A.") +def by_keyword(ctx: TaskContext) -> None: + return main(ctx=ctx) + + +@task(description="B.") +def positional(ctx: TaskContext) -> None: + return main(ctx) + + +@task(description="C.") +def computes(ctx: TaskContext) -> None: + rows = main(ctx=ctx) + return rows + + +@task(description="D.") +def unpacked(ctx: TaskContext) -> None: + return main(**ctx) +"#, + ), + ("python/orders/daily/main.py", TASK_MAIN), + ( + "python/orders/weekly/node.py", + r#"from python.orders.daily.main import main + + +@task(description="W.") +def weekly(ctx: TaskContext) -> None: + return main(ctx=ctx) +"#, + ), + ("python/orders/weekly/main.py", TASK_MAIN), + ], + config: json!({ "select": ["SQBRPYTHON005"] }), + expected_faults: vec![ + ("SQBRPYTHON005", "python/orders/daily/node.py", 11, 5), + ("SQBRPYTHON005", "python/orders/daily/node.py", 16, 5), + ("SQBRPYTHON005", "python/orders/daily/node.py", 22, 5), + ("SQBRPYTHON005", "python/orders/weekly/node.py", 5, 5), + ], + }, + PythonRunTestCase { + description: "main.py: other statements, a result that does not fit the node kind, and no main()", + files: &[ + ("python/orders/daily/node.py", DAILY_TASK), + ( + "python/orders/daily/main.py", + r#"from sqlbuild import PythonNodeResult + +LIMIT = 3 + + +def main(ctx: TaskContext) -> list: + return [] + + +def helper() -> None: + pass +"#, + ), + ("python/orders/weekly/node.py", ""), + ( + "python/orders/weekly/main.py", + "def run() -> None:\n pass\n", + ), + ("python/orders/stock/node.py", ""), + ("python/orders/stock/main.py", "def main(ctx):\n pass\n"), + ( + "python/orders/raw/node.py", + "from .main import main\n\n\n@loader\ndef raw(ctx: LoaderContext) -> list:\n return main(ctx=ctx)\n", + ), + ( + "python/orders/raw/main.py", + "def main(ctx: LoaderContext) -> PythonNodeResult:\n return []\n", + ), + ], + config: json!({ "select": ["SQBRPYTHON006"] }), + expected_faults: vec![ + ("SQBRPYTHON006", "python/orders/daily/main.py", 3, 1), + ("SQBRPYTHON006", "python/orders/daily/main.py", 6, 5), + ("SQBRPYTHON006", "python/orders/daily/main.py", 10, 5), + ("SQBRPYTHON006", "python/orders/raw/main.py", 1, 5), + ("SQBRPYTHON006", "python/orders/stock/main.py", 1, 5), + ("SQBRPYTHON006", "python/orders/weekly/main.py", 1, 1), + ("SQBRPYTHON006", "python/orders/weekly/main.py", 1, 5), + ], + }, + PythonRunTestCase { + description: "signature: ctx missing or mistyped, variadics and unannotated providers report; positional-only ctx does not", + files: &[ + ( + "python/orders/daily/node.py", + r#"from .main import main + + +@task(description="A.") +def no_ctx(context: TaskContext) -> None: + return main(ctx=context) + + +@task(description="B.") +def wrong_type(ctx: AssetContext, *, store: Store) -> None: + return main(ctx=ctx, store=store) + + +@task(description="C.") +def loose(ctx: TaskContext, *args, store, **kwargs) -> None: + return main(ctx=ctx) + + +@task(description="D.") +def positional_only(ctx: TaskContext, /) -> None: + return main(ctx=ctx) +"#, + ), + ( + "python/orders/daily/checks.py", + r#"from .node import no_ctx + + +@check(depends_on=[no_ctx], description="Q.") +def quality(ctx: TaskContext) -> bool: + return True +"#, + ), + ], + config: json!({ "select": ["SQBRPYTHON007"] }), + expected_faults: vec![ + ("SQBRPYTHON007", "python/orders/daily/checks.py", 5, 5), + ("SQBRPYTHON007", "python/orders/daily/node.py", 5, 5), + ("SQBRPYTHON007", "python/orders/daily/node.py", 10, 5), + ("SQBRPYTHON007", "python/orders/daily/node.py", 15, 5), + ("SQBRPYTHON007", "python/orders/daily/node.py", 15, 5), + ], + }, + PythonRunTestCase { + description: "imports: another unit's main, role files and helpers, a domain module, and a node used outside depends_on report", + files: &[ + ("providers/store/provider.py", ""), + ("providers/store/client.py", ""), + ("python/utils/text.py", ""), + ("python/orders/constants.py", ""), + ( + "python/orders/report.py", + "from python.orders.weekly.main import main\n", + ), + ("python/orders/_helpers/dates.py", ""), + ("python/orders/daily/node.py", DAILY_TASK), + ("python/orders/daily/_helpers/rows.py", ""), + ( + "python/orders/daily/main.py", + r#"import os +from sqlbuild import PythonNodeResult +from providers.store.provider import Store +from python.utils.text import clean +from python.orders.constants import LIMIT +from python.orders._helpers.dates import today +from ._helpers.rows import rows +from python.orders.report import summary +from python.orders.weekly.main import main as weekly +from python.orders.weekly.constants import WEEK +from python.orders.weekly._helpers.x import x +from python.billing.invoices.node import invoices +from providers.store.client import Client + + +def main(ctx: TaskContext) -> PythonNodeResult: + return invoices +"#, + ), + ( + "python/orders/weekly/node.py", + r#"from python.orders.daily.node import daily +from .main import main + + +@task(depends_on=[daily], description="W.") +def weekly(ctx: TaskContext) -> None: + return main(ctx=ctx) +"#, + ), + ("python/orders/weekly/main.py", TASK_MAIN), + ("python/orders/weekly/constants.py", ""), + ("python/orders/weekly/_helpers/x.py", ""), + ("python/billing/invoices/node.py", ""), + ("python/billing/invoices/main.py", ""), + ], + config: json!({ "select": ["SQBRPYTHON008"] }), + expected_faults: vec![ + ("SQBRPYTHON008", "python/orders/daily/main.py", 8, 34), + ("SQBRPYTHON008", "python/orders/daily/main.py", 9, 39), + ("SQBRPYTHON008", "python/orders/daily/main.py", 10, 44), + ("SQBRPYTHON008", "python/orders/daily/main.py", 11, 45), + ("SQBRPYTHON008", "python/orders/daily/main.py", 12, 42), + ("SQBRPYTHON008", "python/orders/daily/main.py", 13, 36), + ], + }, + PythonRunTestCase { + description: "checks.py: statements, a check of another unit's node, a check that computes, and a plain function report", + files: &[ + ("python/orders/daily/node.py", DAILY_TASK), + ( + "python/orders/weekly/node.py", + "@task\ndef weekly(ctx):\n pass\n", + ), + ( + "python/orders/daily/checks.py", + r#"from python.orders.weekly.node import weekly + +from .node import daily + +LIMIT = 3 + + +@check(depends_on=[daily], description="A.") +def owns(ctx: CheckContext) -> bool: + return True + + +@check(depends_on=[weekly], description="B.") +def foreign(ctx: CheckContext) -> bool: + return True + + +@check(depends_on=[daily], description="C.") +def computes(ctx: CheckContext) -> bool: + total = 1 + return total > 0 + + +def helper() -> None: + pass +"#, + ), + ], + config: json!({ "select": ["SQBRPYTHON009"] }), + expected_faults: vec![ + ("SQBRPYTHON009", "python/orders/daily/checks.py", 5, 1), + ("SQBRPYTHON009", "python/orders/daily/checks.py", 14, 5), + ("SQBRPYTHON009", "python/orders/daily/checks.py", 19, 5), + ("SQBRPYTHON009", "python/orders/daily/checks.py", 24, 5), + ], + }, + PythonRunTestCase { + description: "factories: specs from outside the unit, an unnamed generated node and no loop report", + files: &[ + ("python/orders/constants.py", "MARKETS: tuple = ()\n"), + ( + "python/orders/markets/node.py", + r#"from python.orders.constants import MARKETS +from .main import main + + +@factory +def market_exports(): + for market in MARKETS: + + @task(description="M.") + def export(ctx: TaskContext) -> None: + return main(ctx=ctx) +"#, + ), + ( + "python/orders/once/node.py", + r#"from .main import main + + +@factory +def once(): + @task(name="once", description="O.") + def run(ctx: TaskContext) -> None: + return main(ctx=ctx) +"#, + ), + ], + config: json!({ "select": ["SQBRPYTHON010"] }), + expected_faults: vec![ + ("SQBRPYTHON010", "python/orders/markets/node.py", 6, 5), + ("SQBRPYTHON010", "python/orders/markets/node.py", 10, 13), + ("SQBRPYTHON010", "python/orders/once/node.py", 5, 5), + ], + }, + PythonRunTestCase { + description: "descriptions: a docstring, a described loader and an undescribed check report", + files: &[ + ( + "python/orders/daily/node.py", + r#"from .main import main + + +@task(description="A.") +def described(ctx: TaskContext) -> None: + return main(ctx=ctx) + + +@asset +def bare(ctx: AssetContext) -> None: + """Docstring.""" + return main(ctx=ctx) +"#, + ), + ( + "python/orders/daily/checks.py", + r#"from .node import described + + +@check(depends_on=[described]) +def quality(ctx: CheckContext) -> bool: + return True +"#, + ), + ( + "python/orders/raw/node.py", + r#"from .main import main + + +@loader(description="Raw.") +def raw(ctx: LoaderContext) -> list: + return main(ctx=ctx) +"#, + ), + ], + config: json!({ "select": ["SQBRPYTHON011"] }), + expected_faults: vec![ + ("SQBRPYTHON011", "python/orders/daily/checks.py", 5, 5), + ("SQBRPYTHON011", "python/orders/daily/node.py", 10, 5), + ("SQBRPYTHON011", "python/orders/raw/node.py", 5, 5), + ], + }, + PythonRunTestCase { + description: "declarations outside a unit's node.py or checks.py report; a factory's checks in node.py do not", + files: &[ + ( + "python/orders/loaders.py", + r#"@loader +def raw_orders(ctx: LoaderContext) -> list: + return [] + + +@check(depends_on=[raw_orders], description="Q.") +def raw_orders_present(ctx: CheckContext) -> bool: + return True +"#, + ), + ( + "python/orders/daily/main.py", + "@task(description=\"X.\")\ndef stray(ctx: TaskContext) -> None:\n return None\n", + ), + ( + "python/orders/daily/node.py", + r#"from .constants import TABLES +from .main import main + + +@factory +def tables(): + for table in TABLES: + + @task(name=table, description="T.") + def load(ctx: TaskContext) -> None: + return main(ctx=ctx) + + @check(name=f"{table}_ok", depends_on=[load], description="C.") + def ok(ctx: CheckContext) -> bool: + return True +"#, + ), + ( + "python/orders/daily/checks.py", + "@factory\ndef audits():\n pass\n", + ), + ( + "python/orders/quality/factories.py", + "@audit_factory\ndef audits():\n pass\n", + ), + ( + "python/utils/flat.py", + "@task\ndef util_task(ctx):\n return 1\n", + ), + ], + config: json!({ "select": ["SQBRPYTHON012"] }), + expected_faults: vec![ + ("SQBRPYTHON012", "python/orders/daily/main.py", 2, 5), + ("SQBRPYTHON012", "python/orders/loaders.py", 2, 5), + ("SQBRPYTHON012", "python/orders/loaders.py", 7, 5), + ("SQBRPYTHON012", "python/utils/flat.py", 2, 5), + ], + }, + ]; + for test_case in test_cases { + let faults = python_rule_faults(test_case.files, test_case.config.clone()); + + assert_eq!( + faults, + Ok(owned_faults(&test_case.expected_faults)), + "{}", + test_case.description + ); + } +} diff --git a/crates/sqlbuild-rules/src/python_rules/tests/test_python_catalogue.rs b/crates/sqlbuild-rules/src/python_rules/tests/test_python_catalogue.rs index 46b636470..f8be29887 100644 --- a/crates/sqlbuild-rules/src/python_rules/tests/test_python_catalogue.rs +++ b/crates/sqlbuild-rules/src/python_rules/tests/test_python_catalogue.rs @@ -14,14 +14,14 @@ use crate::rules::models::BuiltinOptionKind; fn given_the_catalogue_when_listing_python_rules_then_each_has_its_own_project_wide_code() { let test_cases = [ CatalogueTestCase { - description: "two SQLBuild layout rules and 62 Fensu core rules have distinct codes", + description: "12 SQLBuild layout and unit rules and 62 Fensu core rules have distinct codes", count: distinct_python_codes, - expected_count: 64, + expected_count: 74, }, CatalogueTestCase { description: "every Python rule reports against the project in the python family", count: project_wide_python_rules, - expected_count: 64, + expected_count: 74, }, ]; for test_case in test_cases { diff --git a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/python-nodes/overview.md b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/python-nodes/overview.md index 18fd0ee66..148823360 100644 --- a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/python-nodes/overview.md +++ b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/python-nodes/overview.md @@ -57,7 +57,7 @@ python/ factories/regional.py ``` -Grouping by domain (`python/orders/`, `python/customers/`) works just as well, and a single file may mix kinds. A project that selects the [Python Rules](../rules/python-rules.md#laying-out-python) commits to grouping by domain: `SQBRPYTHON001` reports node-kind folders such as `python/loaders/` and modules directly under `python/`. Import other project modules through the `python` package, for example `from python.tasks.orders import export_orders` or `from python._helpers import normalize_status`. +Grouping by domain (`python/orders/`, `python/customers/`) works just as well, and a single file may mix kinds. A project that selects the [Python Rules](../rules/python-rules.md#laying-out-python) commits to grouping by domain, with each node in its own [unit folder](../rules/python-rules.md#writing-nodes-as-units) holding `node.py` and `main.py`: `SQBRPYTHON001` reports node-kind folders such as `python/loaders/` and modules directly under `python/`, and `SQBRPYTHON012` reports nodes declared outside a unit's `node.py`. Import other project modules through the `python` package, for example `from python.tasks.orders import export_orders` or `from python._helpers import normalize_status`. Code that extends SQLBuild itself keeps its own folder, such as `rules/`, `materializations/`, `adapters/`, `providers/`, `sinks/`, `hooks/python/`, `functions/python/`, and `macros/`. Python outside the supported locations fails compilation with `D016`; see [Project Python](../project-configuration.md#project-python) for the full list. diff --git a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/rules/python-rules.md b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/rules/python-rules.md index c3d55f151..60562d761 100644 --- a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/rules/python-rules.md +++ b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/rules/python-rules.md @@ -11,6 +11,7 @@ Online: https://sqlbuild.com/docs/concepts/rules/python-rules/ - Selecting the Python Rules - Which files are checked - Laying out `python/` +- Writing nodes as units - Main modules and entry modules - Unreachable definitions - Rule options @@ -77,7 +78,7 @@ python/ fulfillment/ shipment_export/ node.py # @task shipment_export - main.py # export_shipments, the unit's one entry + main.py # main(), the unit's implementation utils/ currency/ format.py @@ -95,6 +96,69 @@ The generic names are `base`, `common`, `general`, `helpers`, `lib`, `misc`, `sh `utils`. `utils/` is allowed directly under `python/`. A finding about a folder points at its `__init__.py`, or at its first Python file when it has none. +## Writing nodes as units + +A unit is one folder that declares one node, or one factory, and holds its implementation. +`SQBRPYTHON003` to `SQBRPYTHON012` keep each node in its own unit, grouped by domain: + +```text +python/orders/ + constants.py # shared by the orders units + partner_export/ + node.py # the @asset declaration + main.py # main(), the implementation + checks.py # @check declarations for this unit + _helpers/ # private functions for this unit + refund_review/ + node.py # @task + main.py +``` + +A unit folder may also hold `models.py`, `types.py`, `constants.py`, `exceptions.py` and +`__init__.py`. The declaration only passes its arguments to `main()`: + +```python +# python/orders/partner_export/node.py +from sqlbuild.assets import AssetContext, asset +from sqlbuild.executor.python_nodes.models import PythonNodeResult + +from providers.partner_files.provider import PartnerFiles +from python.orders.partner_export.main import main + +@asset(description="Write the daily partner orders file.") +def partner_export(ctx: AssetContext, *, partner_files: PartnerFiles) -> PythonNodeResult: + return main(ctx=ctx, partner_files=partner_files) +``` + +These are reported: + +| Code | Finding | Fix | +|---|---|---| +| `SQBRPYTHON003` | A unit without `main.py`, or another module or folder in it | Add `main.py`; move other code into `_helpers/` or a role file | +| `SQBRPYTHON004` | `node.py` holding a helper, a constant, a class or a second declaration | Keep one `@loader`, `@task`, `@asset` or `@factory` function and its imports | +| `SQBRPYTHON005` | A declaration that does more than `return main(...)`, or passes a positional argument | Move the logic into `main()` and pass every argument as `name=value` | +| `SQBRPYTHON006` | `main.py` holding more than one function, or a `main()` whose return annotation does not fit the node | Move other code into `_helpers/` and annotate `main()` with its result | +| `SQBRPYTHON007` | A signature other than `(ctx: Context, *, provider: ProviderClass)` | Take `ctx` positionally and providers after `*`, each annotated | +| `SQBRPYTHON008` | An import of another unit's `main.py`, role files or `_helpers/`, or another domain's code | Import shared code from `python/utils/` or the domain's role files | +| `SQBRPYTHON009` | `checks.py` holding other code, or a check that does not depend on the unit's node | Name the unit's node in `depends_on` and return directly | +| `SQBRPYTHON010` | A factory that does not loop over the unit's specs, or a generated node without `name=` | Loop over specs from the unit's `constants.py` or `models.py` and name each node | +| `SQBRPYTHON011` | A docstring on a node or check, a task, asset or check without `description=`, or a described loader | Use `description=`; a loader takes its description from its managed source | +| `SQBRPYTHON012` | A node declared outside a unit's `node.py`, or a check outside its `checks.py` | Move it into its own unit folder | + +`main()` returns: + +| Node | Return annotation | +|---|---| +| `@task`, `@asset` | `PythonNodeResult` or `PythonNodeSkipResult`, from `ctx.result(...)` and `ctx.skip(...)` | +| `@loader` | rows such as `list[...]` or `Iterator[...]`, `LoaderResult`, `LoaderSkipResult`, or `None` | + +A unit may import another unit's `node.py` only to name that node in `depends_on`. It may import +`provider.py`, `models.py`, `types.py`, `constants.py` and `exceptions.py` from +`providers//`. + +A factory's `node.py` holds one `@factory` function, and a check generated by a factory may stay in +`node.py`. `@audit_factory` functions are not units and may sit anywhere under `python/`. + ## Main modules and entry modules Some Rules apply only to a domain's main modules: files under a `main/` folder, and a unit's @@ -161,6 +225,21 @@ remediation. | `SQBRPYTHON001` | project-root-allowlist | `python/` | yes | `python/` may hold only domain packages, `utils/` and `__init__.py` | | `SQBRPYTHON002` | runtime-package-owner-names | `python/` | yes | Packages under `python/` must be named for what they own | +### Node units + +| Code | Name | Files | Default | Checks | +|---|---|---|---|---| +| `SQBRPYTHON003` | node-unit-layout | `python/` | yes | A unit folder holds `node.py`, `main.py`, optional role files and `_helpers/` only | +| `SQBRPYTHON004` | node-module-shape | `python/` | yes | `node.py` holds imports and exactly one node or factory declaration | +| `SQBRPYTHON005` | node-delegates-to-main | `python/` | yes | A declaration's body is `return main(...)` from its unit, with keyword arguments | +| `SQBRPYTHON006` | main-module-shape | `python/` | yes | `main.py` holds imports and one `main()` whose return annotation fits the node | +| `SQBRPYTHON007` | node-signature | `python/` | yes | `ctx` is the only positional parameter; providers are annotated and keyword-only | +| `SQBRPYTHON008` | node-import-boundaries | `python/` | yes | A unit imports only what it owns, its domain's role files, `python/utils/`, provider role files and other nodes for `depends_on` | +| `SQBRPYTHON009` | checks-module-shape | `python/` | yes | `checks.py` holds checks of the unit's node that return directly, or one factory | +| `SQBRPYTHON010` | factory-shape | `python/` | yes | A factory loops over its unit's specs and names every node it generates | +| `SQBRPYTHON011` | node-description-not-docstring | `python/` | yes | Tasks, assets and checks set `description=` without a docstring; loaders set neither | +| `SQBRPYTHON012` | node-declared-in-unit | `python/` | yes | Nodes are declared in a unit's `node.py`, and checks in its `checks.py` | + ### Type annotations | Code | Name | Files | Default | Checks | diff --git a/tests/integration/src/sqlbuild/cli/commands/main/test_rules.py b/tests/integration/src/sqlbuild/cli/commands/main/test_rules.py index 3b3cad7f4..8fda0b9c7 100644 --- a/tests/integration/src/sqlbuild/cli/commands/main/test_rules.py +++ b/tests/integration/src/sqlbuild/cli/commands/main/test_rules.py @@ -3512,7 +3512,7 @@ def test_given_project_python_when_running_python_rules_then_findings_match_comp _PYTHON_ANNOTATED_ALERT, ), ), - expected_hits_and_misses=((0, 1), (0, 9), (8, 1), (0, 9), (10, 0)), + expected_hits_and_misses=((0, 1), (0, 12), (11, 1), (0, 12), (13, 0)), expected_python_cache=(False, True, True, True, True), ) ], diff --git a/website/src/content/docs/docs/concepts/python-nodes/overview.mdx b/website/src/content/docs/docs/concepts/python-nodes/overview.mdx index 9bcca3521..25068aab9 100644 --- a/website/src/content/docs/docs/concepts/python-nodes/overview.mdx +++ b/website/src/content/docs/docs/concepts/python-nodes/overview.mdx @@ -41,7 +41,7 @@ python/ factories/regional.py ``` -Grouping by domain (`python/orders/`, `python/customers/`) works just as well, and a single file may mix kinds. A project that selects the [Python Rules](/docs/concepts/rules/python-rules#laying-out-python) commits to grouping by domain: `SQBRPYTHON001` reports node-kind folders such as `python/loaders/` and modules directly under `python/`. Import other project modules through the `python` package, for example `from python.tasks.orders import export_orders` or `from python._helpers import normalize_status`. +Grouping by domain (`python/orders/`, `python/customers/`) works just as well, and a single file may mix kinds. A project that selects the [Python Rules](/docs/concepts/rules/python-rules#laying-out-python) commits to grouping by domain, with each node in its own [unit folder](/docs/concepts/rules/python-rules#writing-nodes-as-units) holding `node.py` and `main.py`: `SQBRPYTHON001` reports node-kind folders such as `python/loaders/` and modules directly under `python/`, and `SQBRPYTHON012` reports nodes declared outside a unit's `node.py`. Import other project modules through the `python` package, for example `from python.tasks.orders import export_orders` or `from python._helpers import normalize_status`. Code that extends SQLBuild itself keeps its own folder, such as `rules/`, `materializations/`, `adapters/`, `providers/`, `sinks/`, `hooks/python/`, `functions/python/`, and `macros/`. Python outside the supported locations fails compilation with `D016`; see [Project Python](/docs/concepts/project-configuration#project-python) for the full list. diff --git a/website/src/content/docs/docs/concepts/rules/python-rules.mdx b/website/src/content/docs/docs/concepts/rules/python-rules.mdx index b2aa1e660..a6d101d2e 100644 --- a/website/src/content/docs/docs/concepts/rules/python-rules.mdx +++ b/website/src/content/docs/docs/concepts/rules/python-rules.mdx @@ -63,7 +63,7 @@ python/ fulfillment/ shipment_export/ node.py # @task shipment_export - main.py # export_shipments, the unit's one entry + main.py # main(), the unit's implementation utils/ currency/ format.py @@ -81,6 +81,70 @@ The generic names are `base`, `common`, `general`, `helpers`, `lib`, `misc`, `sh `utils`. `utils/` is allowed directly under `python/`. A finding about a folder points at its `__init__.py`, or at its first Python file when it has none. +## Writing nodes as units + +A unit is one folder that declares one node, or one factory, and holds its implementation. +`SQBRPYTHON003` to `SQBRPYTHON012` keep each node in its own unit, grouped by domain: + +```text +python/orders/ + constants.py # shared by the orders units + partner_export/ + node.py # the @asset declaration + main.py # main(), the implementation + checks.py # @check declarations for this unit + _helpers/ # private functions for this unit + refund_review/ + node.py # @task + main.py +``` + +A unit folder may also hold `models.py`, `types.py`, `constants.py`, `exceptions.py` and +`__init__.py`. The declaration only passes its arguments to `main()`: + +```python +# python/orders/partner_export/node.py +from sqlbuild.assets import AssetContext, asset +from sqlbuild.executor.python_nodes.models import PythonNodeResult + +from providers.partner_files.provider import PartnerFiles +from python.orders.partner_export.main import main + + +@asset(description="Write the daily partner orders file.") +def partner_export(ctx: AssetContext, *, partner_files: PartnerFiles) -> PythonNodeResult: + return main(ctx=ctx, partner_files=partner_files) +``` + +These are reported: + +| Code | Finding | Fix | +|---|---|---| +| `SQBRPYTHON003` | A unit without `main.py`, or another module or folder in it | Add `main.py`; move other code into `_helpers/` or a role file | +| `SQBRPYTHON004` | `node.py` holding a helper, a constant, a class or a second declaration | Keep one `@loader`, `@task`, `@asset` or `@factory` function and its imports | +| `SQBRPYTHON005` | A declaration that does more than `return main(...)`, or passes a positional argument | Move the logic into `main()` and pass every argument as `name=value` | +| `SQBRPYTHON006` | `main.py` holding more than one function, or a `main()` whose return annotation does not fit the node | Move other code into `_helpers/` and annotate `main()` with its result | +| `SQBRPYTHON007` | A signature other than `(ctx: Context, *, provider: ProviderClass)` | Take `ctx` positionally and providers after `*`, each annotated | +| `SQBRPYTHON008` | An import of another unit's `main.py`, role files or `_helpers/`, or another domain's code | Import shared code from `python/utils/` or the domain's role files | +| `SQBRPYTHON009` | `checks.py` holding other code, or a check that does not depend on the unit's node | Name the unit's node in `depends_on` and return directly | +| `SQBRPYTHON010` | A factory that does not loop over the unit's specs, or a generated node without `name=` | Loop over specs from the unit's `constants.py` or `models.py` and name each node | +| `SQBRPYTHON011` | A docstring on a node or check, a task, asset or check without `description=`, or a described loader | Use `description=`; a loader takes its description from its managed source | +| `SQBRPYTHON012` | A node declared outside a unit's `node.py`, or a check outside its `checks.py` | Move it into its own unit folder | + +`main()` returns: + +| Node | Return annotation | +|---|---| +| `@task`, `@asset` | `PythonNodeResult` or `PythonNodeSkipResult`, from `ctx.result(...)` and `ctx.skip(...)` | +| `@loader` | rows such as `list[...]` or `Iterator[...]`, `LoaderResult`, `LoaderSkipResult`, or `None` | + +A unit may import another unit's `node.py` only to name that node in `depends_on`. It may import +`provider.py`, `models.py`, `types.py`, `constants.py` and `exceptions.py` from +`providers//`. + +A factory's `node.py` holds one `@factory` function, and a check generated by a factory may stay in +`node.py`. `@audit_factory` functions are not units and may sit anywhere under `python/`. + ## Main modules and entry modules Some Rules apply only to a domain's main modules: files under a `main/` folder, and a unit's @@ -147,6 +211,21 @@ remediation. | `SQBRPYTHON001` | project-root-allowlist | `python/` | yes | `python/` may hold only domain packages, `utils/` and `__init__.py` | | `SQBRPYTHON002` | runtime-package-owner-names | `python/` | yes | Packages under `python/` must be named for what they own | +### Node units + +| Code | Name | Files | Default | Checks | +|---|---|---|---|---| +| `SQBRPYTHON003` | node-unit-layout | `python/` | yes | A unit folder holds `node.py`, `main.py`, optional role files and `_helpers/` only | +| `SQBRPYTHON004` | node-module-shape | `python/` | yes | `node.py` holds imports and exactly one node or factory declaration | +| `SQBRPYTHON005` | node-delegates-to-main | `python/` | yes | A declaration's body is `return main(...)` from its unit, with keyword arguments | +| `SQBRPYTHON006` | main-module-shape | `python/` | yes | `main.py` holds imports and one `main()` whose return annotation fits the node | +| `SQBRPYTHON007` | node-signature | `python/` | yes | `ctx` is the only positional parameter; providers are annotated and keyword-only | +| `SQBRPYTHON008` | node-import-boundaries | `python/` | yes | A unit imports only what it owns, its domain's role files, `python/utils/`, provider role files and other nodes for `depends_on` | +| `SQBRPYTHON009` | checks-module-shape | `python/` | yes | `checks.py` holds checks of the unit's node that return directly, or one factory | +| `SQBRPYTHON010` | factory-shape | `python/` | yes | A factory loops over its unit's specs and names every node it generates | +| `SQBRPYTHON011` | node-description-not-docstring | `python/` | yes | Tasks, assets and checks set `description=` without a docstring; loaders set neither | +| `SQBRPYTHON012` | node-declared-in-unit | `python/` | yes | Nodes are declared in a unit's `node.py`, and checks in its `checks.py` | + ### Type annotations | Code | Name | Files | Default | Checks | From f6c6c3fe8320436a6fd9cf58d6053ff621965e9e Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Sun, 15 Nov 2026 02:13:38 +0000 Subject: [PATCH 2/3] fix: tighten SQBRPYTHON unit detection, import boundaries and factory loops Role folders and shallow node.py files no longer make units. Factory loops record the name they read through calls and attributes, import-only TYPE_CHECKING blocks count as imports and are boundary-checked, dotted imports of other units' nodes may be named in depends_on, and an unused import is no longer treated as depends_on-only. main bound to a module no longer counts as delegating. The unit project indexes files by path and parses them in parallel. A scaling guard covers a chain of linked units with dense findings. The Python rule helpers are grouped into engine/ and units/ buckets. --- .../src/node_units/_helpers/functions.rs | 43 +++-- .../src/node_units/_helpers/imports.rs | 34 +++- .../src/node_units/constants.rs | 2 + .../src/node_units/main/node_unit_module.rs | 3 +- .../src/node_units/models.rs | 5 +- .../src/node_units/tests/helpers.rs | 33 ++-- .../node_units/tests/test_node_unit_module.rs | 35 ++-- .../_helpers/{ => engine}/contexts.rs | 0 .../_helpers/{ => engine}/execution.rs | 8 +- .../_helpers/{ => engine}/faults.rs | 2 +- .../_helpers/{ => engine}/file_cache.rs | 0 .../_helpers/{ => engine}/layout.rs | 4 +- .../src/python_rules/_helpers/engine/mod.rs | 9 + .../_helpers/{ => engine}/observations.rs | 4 +- .../_helpers/{ => engine}/placement.rs | 0 .../_helpers/{ => engine}/programs.rs | 0 .../_helpers/{ => engine}/project_plane.rs | 2 +- .../src/python_rules/_helpers/mod.rs | 10 +- .../python_rules/_helpers/units/imports.rs | 8 +- .../src/python_rules/_helpers/units/layout.rs | 19 +-- .../src/python_rules/_helpers/units/nodes.rs | 21 ++- .../python_rules/_helpers/units/project.rs | 91 ++++++++-- .../src/python_rules/constants.rs | 4 + .../main/evaluate_python_rules.rs | 16 +- .../sqlbuild-rules/src/python_rules/models.rs | 8 +- .../src/python_rules/tests/helpers.rs | 54 ++++++ .../src/python_rules/tests/test_node_units.rs | 161 ++++++++++++++++++ .../python_rules/tests/test_python_cache.rs | 2 +- .../python_rules/tests/test_python_scaling.rs | 55 +++++- .../src/python_rules/tests/test_types.rs | 12 ++ 30 files changed, 513 insertions(+), 132 deletions(-) rename crates/sqlbuild-rules/src/python_rules/_helpers/{ => engine}/contexts.rs (100%) rename crates/sqlbuild-rules/src/python_rules/_helpers/{ => engine}/execution.rs (97%) rename crates/sqlbuild-rules/src/python_rules/_helpers/{ => engine}/faults.rs (98%) rename crates/sqlbuild-rules/src/python_rules/_helpers/{ => engine}/file_cache.rs (100%) rename crates/sqlbuild-rules/src/python_rules/_helpers/{ => engine}/layout.rs (96%) create mode 100644 crates/sqlbuild-rules/src/python_rules/_helpers/engine/mod.rs rename crates/sqlbuild-rules/src/python_rules/_helpers/{ => engine}/observations.rs (97%) rename crates/sqlbuild-rules/src/python_rules/_helpers/{ => engine}/placement.rs (100%) rename crates/sqlbuild-rules/src/python_rules/_helpers/{ => engine}/programs.rs (100%) rename crates/sqlbuild-rules/src/python_rules/_helpers/{ => engine}/project_plane.rs (98%) diff --git a/crates/sqlbuild-pysource/src/node_units/_helpers/functions.rs b/crates/sqlbuild-pysource/src/node_units/_helpers/functions.rs index d063339fe..6c0b6ff71 100644 --- a/crates/sqlbuild-pysource/src/node_units/_helpers/functions.rs +++ b/crates/sqlbuild-pysource/src/node_units/_helpers/functions.rs @@ -1,6 +1,7 @@ //! A function's decorators, signature and body shape, and the decorated functions inside it. use ruff_python_ast::{Decorator, Expr, Parameter, Stmt, StmtFunctionDef}; +use ruff_python_parser::{Mode, ParseOptions}; use ruff_text_size::Ranged; use crate::cpython_tree::main::walk_breadth_first::walk_breadth_first; @@ -54,10 +55,7 @@ pub(crate) fn unit_function(function: &StmtFunctionDef, source: &UnitText<'_>) - .iter() .filter_map(|node| match node { CpythonNode::Stmt(Stmt::For(loop_statement)) => { - Some(match loop_statement.iter.as_ref() { - Expr::Name(name) => Some(name.id.to_string()), - _ => None, - }) + Some(root_name(&loop_statement.iter)) } _ => None, }) @@ -154,6 +152,8 @@ fn union_names(annotation: &Expr) -> Vec { names } Expr::NoneLiteral(_) => vec![NONE_NAME.to_owned()], + Expr::StringLiteral(literal) => string_expression(literal.value.to_str()) + .map_or_else(Vec::new, |expression| union_names(&expression)), Expr::Subscript(subscript) if last_name(&subscript.value) .is_some_and(|name| UNION_NAMES.contains(&name.as_str())) => @@ -177,19 +177,28 @@ fn last_name(expression: &Expr) -> Option { Expr::Name(name) => Some(name.id.to_string()), Expr::Attribute(attribute) => Some(attribute.attr.to_string()), Expr::Subscript(subscript) => last_name(&subscript.value), - Expr::StringLiteral(literal) => Some( - literal - .value - .to_str() - .rsplit('.') - .next() - .unwrap_or_default() - .split('[') - .next() - .unwrap_or_default() - .trim() - .to_owned(), - ), + Expr::StringLiteral(literal) => last_name(&string_expression(literal.value.to_str())?), + _ => None, + } +} + +/// The expression a string annotation holds, if it parses. +fn string_expression(text: &str) -> Option { + let parsed = + ruff_python_parser::parse_unchecked(text.trim(), ParseOptions::from(Mode::Expression)); + if !parsed.has_no_syntax_errors() { + return None; + } + Some(*parsed.try_into_expression()?.into_syntax().body) +} + +/// The name an iterable is read from: `SPECS` for `SPECS`, `SPECS.items()`, `SPECS[1:]` or `SPECS.rows`. +fn root_name(expression: &Expr) -> Option { + match expression { + Expr::Name(name) => Some(name.id.to_string()), + Expr::Attribute(attribute) => root_name(&attribute.value), + Expr::Call(call) => root_name(&call.func), + Expr::Subscript(subscript) => root_name(&subscript.value), _ => None, } } diff --git a/crates/sqlbuild-pysource/src/node_units/_helpers/imports.rs b/crates/sqlbuild-pysource/src/node_units/_helpers/imports.rs index 6c7fe2184..3d18e8519 100644 --- a/crates/sqlbuild-pysource/src/node_units/_helpers/imports.rs +++ b/crates/sqlbuild-pysource/src/node_units/_helpers/imports.rs @@ -1,12 +1,12 @@ //! Module-level imports and whether each bound name is used only to name a dependency. -use ruff_python_ast::{Expr, Stmt}; +use ruff_python_ast::{Expr, Stmt, StmtIf}; use ruff_text_size::{Ranged, TextRange, TextSize}; use crate::cpython_tree::main::walk_breadth_first::walk_breadth_first; use crate::cpython_tree::models::CpythonNode; use crate::node_units::_helpers::positions::{UnitText, unit_position}; -use crate::node_units::constants::DEPENDS_ON_KEYWORD; +use crate::node_units::constants::{DEPENDS_ON_KEYWORD, TYPE_CHECKING_NAME}; use crate::node_units::models::UnitImport; /// Every name the module's top-level imports bind, in source order. @@ -15,7 +15,7 @@ pub(crate) fn module_imports(body: &[Stmt], source: &UnitText<'_>) -> Vec = depends_on_ranges(&nodes); let mut imports: Vec = Vec::new(); - for statement in body { + for statement in body.iter().flat_map(import_statements) { match statement { Stmt::Import(import) => imports.extend(import.names.iter().map(|alias| UnitImport { module: alias.name.to_string(), @@ -59,9 +59,33 @@ pub(crate) fn module_imports(body: &[Stmt], source: &UnitText<'_>) -> Vec], local: &str, ranges: &[TextRange]) -> bool { - name_uses(nodes, local).all(|start| within_any(ranges, start)) + let mut uses = name_uses(nodes, local).peekable(); + uses.peek().is_some() && uses.all(|start| within_any(ranges, start)) +} + +/// `statement` itself, or the imports of an import-only `if TYPE_CHECKING:` block. +fn import_statements(statement: &Stmt) -> &[Stmt] { + match statement { + Stmt::If(block) if is_type_checking_imports(block) => &block.body, + other => std::slice::from_ref(other), + } +} + +/// Whether `block` is `if TYPE_CHECKING:` or `if typing.TYPE_CHECKING:`, holding only imports and no other branch. +pub(crate) fn is_type_checking_imports(block: &StmtIf) -> bool { + let guard: bool = match block.test.as_ref() { + Expr::Name(name) => name.id.as_str() == TYPE_CHECKING_NAME, + Expr::Attribute(attribute) => attribute.attr.as_str() == TYPE_CHECKING_NAME, + _ => false, + }; + guard + && block.elif_else_clauses.is_empty() + && block + .body + .iter() + .all(|statement| matches!(statement, Stmt::Import(_) | Stmt::ImportFrom(_))) } fn within_any(ranges: &[TextRange], offset: TextSize) -> bool { diff --git a/crates/sqlbuild-pysource/src/node_units/constants.rs b/crates/sqlbuild-pysource/src/node_units/constants.rs index 98869cd03..850676e31 100644 --- a/crates/sqlbuild-pysource/src/node_units/constants.rs +++ b/crates/sqlbuild-pysource/src/node_units/constants.rs @@ -10,3 +10,5 @@ pub(crate) const NONE_NAME: &str = "None"; pub(crate) const OPTIONAL_NAME: &str = "Optional"; /// The typing forms whose members a return annotation unions. pub(crate) const UNION_NAMES: [&str; 2] = ["Union", OPTIONAL_NAME]; +/// The typing flag that guards imports needed only for annotations. +pub(crate) const TYPE_CHECKING_NAME: &str = "TYPE_CHECKING"; diff --git a/crates/sqlbuild-pysource/src/node_units/main/node_unit_module.rs b/crates/sqlbuild-pysource/src/node_units/main/node_unit_module.rs index 405a78069..1ad582ceb 100644 --- a/crates/sqlbuild-pysource/src/node_units/main/node_unit_module.rs +++ b/crates/sqlbuild-pysource/src/node_units/main/node_unit_module.rs @@ -6,7 +6,7 @@ use ruff_text_size::Ranged; use crate::cpython_tree::main::parse_cpython_module::parse_cpython_module; use crate::cpython_tree::models::ParsedSource; use crate::node_units::_helpers::functions::{leading_docstring, unit_function}; -use crate::node_units::_helpers::imports::module_imports; +use crate::node_units::_helpers::imports::{is_type_checking_imports, module_imports}; use crate::node_units::_helpers::positions::{UnitText, unit_position}; use crate::node_units::models::{UnitModule, UnitStatement, UnitStatementKind}; @@ -28,6 +28,7 @@ pub fn node_unit_module(contents: &str, python_version: (u8, u8)) -> Option UnitStatementKind::Docstring, Stmt::Import(_) | Stmt::ImportFrom(_) => UnitStatementKind::Import, + Stmt::If(block) if is_type_checking_imports(block) => UnitStatementKind::Import, Stmt::FunctionDef(function) => { UnitStatementKind::Function(unit_function(function, &source)) } diff --git a/crates/sqlbuild-pysource/src/node_units/models.rs b/crates/sqlbuild-pysource/src/node_units/models.rs index dcf052101..b746a58b4 100644 --- a/crates/sqlbuild-pysource/src/node_units/models.rs +++ b/crates/sqlbuild-pysource/src/node_units/models.rs @@ -26,6 +26,7 @@ pub struct UnitStatement { pub enum UnitStatementKind { /// The module docstring. Docstring, + /// An import, or an `if TYPE_CHECKING:` block that only imports. Import, Function(UnitFunction), /// Any other statement, such as a class, an assignment or a loop. @@ -43,7 +44,7 @@ pub struct UnitImport { /// The name the import binds in the module. pub local: String, pub position: UnitPosition, - /// Whether every use of the bound name sits inside a decorator's `depends_on` value. + /// Whether the bound name is used, and every use sits inside a decorator's `depends_on` value. pub only_in_depends_on: bool, } @@ -70,7 +71,7 @@ pub struct UnitFunction { pub delegation: Option, /// The decorated functions defined anywhere in the body. pub inner: Vec, - /// The iterable of each `for` loop in the body: its name, or `None` when it is not a plain name. + /// The name each `for` loop in the body reads its iterable from, such as `SPECS` for `SPECS.items()`; `None` when it reads no name. pub loops: Vec>, } diff --git a/crates/sqlbuild-pysource/src/node_units/tests/helpers.rs b/crates/sqlbuild-pysource/src/node_units/tests/helpers.rs index cdd0da3f1..1f767fd75 100644 --- a/crates/sqlbuild-pysource/src/node_units/tests/helpers.rs +++ b/crates/sqlbuild-pysource/src/node_units/tests/helpers.rs @@ -1,41 +1,42 @@ use crate::node_units::main::node_unit_module::node_unit_module; use crate::node_units::models::{ - UnitDecorator, UnitFunction, UnitImport, UnitParameter, UnitPosition, UnitStatementKind, + UnitDecorator, UnitImport, UnitParameter, UnitPosition, UnitStatementKind, }; const PYTHON_VERSION: (u8, u8) = (3, 12); -/// The kind of each top-level statement of `contents`, or `None` when it does not parse. -pub(super) fn statement_kinds(contents: &str) -> Option> { +/// The kind of each top-level statement of `contents` by variant name, or `None` when it does not parse. +pub(super) fn statement_kinds(contents: &str) -> Option> { let module = node_unit_module(contents, PYTHON_VERSION)?; Some( module .statements .iter() - .map(|statement| match statement.kind { - UnitStatementKind::Docstring => "docstring", - UnitStatementKind::Import => "import", - UnitStatementKind::Function(_) => "function", - UnitStatementKind::Other => "other", - }) + .map(|statement| variant_name(&statement.kind)) .collect(), ) } +/// The variant name of `kind`, such as `Function` for a function. +fn variant_name(kind: &UnitStatementKind) -> String { + format!("{kind:?}") + .split('(') + .next() + .unwrap_or_default() + .to_owned() +} + /// The imports `contents` binds at module level. pub(super) fn imports(contents: &str) -> Vec { node_unit_module(contents, PYTHON_VERSION).map_or_else(Vec::new, |module| module.imports) } -/// The first top-level function of `contents`. -pub(super) fn first_function(contents: &str) -> Option { +/// What the last top-level statement of `contents` is. +pub(super) fn last_statement(contents: &str) -> Option { node_unit_module(contents, PYTHON_VERSION)? .statements - .into_iter() - .find_map(|statement| match statement.kind { - UnitStatementKind::Function(function) => Some(function), - _ => None, - }) + .pop() + .map(|statement| statement.kind) } pub(super) fn at(line: usize, column: usize) -> UnitPosition { diff --git a/crates/sqlbuild-pysource/src/node_units/tests/test_node_unit_module.rs b/crates/sqlbuild-pysource/src/node_units/tests/test_node_unit_module.rs index a8f1a8f9b..a561a18f6 100644 --- a/crates/sqlbuild-pysource/src/node_units/tests/test_node_unit_module.rs +++ b/crates/sqlbuild-pysource/src/node_units/tests/test_node_unit_module.rs @@ -1,6 +1,6 @@ -use crate::node_units::models::{UnitFunction, UnitImport}; +use crate::node_units::models::{UnitFunction, UnitImport, UnitStatementKind}; use crate::node_units::tests::helpers::{ - at, decorator, first_function, import, imports, names, parameter, statement_kinds, + at, decorator, import, imports, last_statement, names, parameter, statement_kinds, }; use crate::node_units::tests::test_types::{ FunctionTestCase, ImportsTestCase, StatementKindsTestCase, @@ -17,12 +17,12 @@ fn given_module_when_reading_then_each_top_level_statement_has_its_kind() { description: "a CRLF node module with a docstring, imports, a value and a node", contents: CRLF_NODE_MODULE, expected_kinds: Some(vec![ - "docstring", - "import", - "import", - "import", - "other", - "function", + "Docstring", + "Import", + "Import", + "Import", + "Other", + "Function", ]), }, StatementKindsTestCase { @@ -34,7 +34,9 @@ fn given_module_when_reading_then_each_top_level_statement_has_its_kind() { for test_case in test_cases { assert_eq!( statement_kinds(test_case.contents), - test_case.expected_kinds, + test_case + .expected_kinds + .map(|kinds| kinds.into_iter().map(str::to_owned).collect()), "{}", test_case.description ); @@ -44,7 +46,7 @@ fn given_module_when_reading_then_each_top_level_statement_has_its_kind() { #[test] fn given_imports_when_reading_then_each_bound_name_records_whether_only_depends_on_uses_it() { let test_cases = [ImportsTestCase { - description: "a relative import, an aliased node used in depends_on and a dotted import", + description: "a relative import, an aliased node used in depends_on and an unused dotted import", contents: CRLF_NODE_MODULE, expected_imports: vec![ UnitImport { @@ -60,10 +62,7 @@ fn given_imports_when_reading_then_each_bound_name_records_whether_only_depends_ at(3, 38), ) }, - UnitImport { - only_in_depends_on: true, - ..import("python.utils.text", None, "python", at(4, 8)) - }, + import("python.utils.text", None, "python", at(4, 8)), ], }]; for test_case in test_cases { @@ -103,7 +102,7 @@ fn given_declaration_when_reading_then_signature_body_and_inner_declarations_are }, }, FunctionTestCase { - description: "a factory whose generated node computes before returning", + description: "a factory looping over a name and a call, whose generated node computes before returning", contents: FACTORY_MODULE, expected_function: UnitFunction { name: "tables".to_owned(), @@ -132,7 +131,7 @@ fn given_declaration_when_reading_then_signature_body_and_inner_declarations_are inner: vec![], loops: vec![], }], - loops: vec![Some("TABLES".to_owned()), None], + loops: vec![Some("TABLES".to_owned()), Some("specs".to_owned())], }, }, FunctionTestCase { @@ -157,8 +156,8 @@ fn given_declaration_when_reading_then_signature_body_and_inner_declarations_are ]; for test_case in test_cases { assert_eq!( - first_function(test_case.contents), - Some(test_case.expected_function), + last_statement(test_case.contents), + Some(UnitStatementKind::Function(test_case.expected_function)), "{}", test_case.description ); diff --git a/crates/sqlbuild-rules/src/python_rules/_helpers/contexts.rs b/crates/sqlbuild-rules/src/python_rules/_helpers/engine/contexts.rs similarity index 100% rename from crates/sqlbuild-rules/src/python_rules/_helpers/contexts.rs rename to crates/sqlbuild-rules/src/python_rules/_helpers/engine/contexts.rs diff --git a/crates/sqlbuild-rules/src/python_rules/_helpers/execution.rs b/crates/sqlbuild-rules/src/python_rules/_helpers/engine/execution.rs similarity index 97% rename from crates/sqlbuild-rules/src/python_rules/_helpers/execution.rs rename to crates/sqlbuild-rules/src/python_rules/_helpers/engine/execution.rs index ca6e48483..9ef5c21bd 100644 --- a/crates/sqlbuild-rules/src/python_rules/_helpers/execution.rs +++ b/crates/sqlbuild-rules/src/python_rules/_helpers/engine/execution.rs @@ -13,10 +13,10 @@ use fensu_native::{ use rayon::iter::{IntoParallelIterator, ParallelIterator}; use crate::models::{Fault, RuleMetadata}; -use crate::python_rules::_helpers::contexts::{rule_context, threshold_calls}; -use crate::python_rules::_helpers::faults::python_fault; -use crate::python_rules::_helpers::file_cache::{answers_digest, file_identity}; -use crate::python_rules::_helpers::observations::{ +use crate::python_rules::_helpers::engine::contexts::{rule_context, threshold_calls}; +use crate::python_rules::_helpers::engine::faults::python_fault; +use crate::python_rules::_helpers::engine::file_cache::{answers_digest, file_identity}; +use crate::python_rules::_helpers::engine::observations::{ observations, relative_queries, repository_index, }; use crate::python_rules::constants::{PACKAGE_ROOT, PYTHON_RULES, ROOT_SCOPE}; diff --git a/crates/sqlbuild-rules/src/python_rules/_helpers/faults.rs b/crates/sqlbuild-rules/src/python_rules/_helpers/engine/faults.rs similarity index 98% rename from crates/sqlbuild-rules/src/python_rules/_helpers/faults.rs rename to crates/sqlbuild-rules/src/python_rules/_helpers/engine/faults.rs index 1f03073c1..5b31b6d6c 100644 --- a/crates/sqlbuild-rules/src/python_rules/_helpers/faults.rs +++ b/crates/sqlbuild-rules/src/python_rules/_helpers/engine/faults.rs @@ -3,7 +3,7 @@ use fensu_native::NativeFaultRow; use crate::models::Fault; -use crate::python_rules::_helpers::programs::Programs; +use crate::python_rules::_helpers::engine::programs::Programs; use crate::python_rules::constants::{CATALOGUE_REMEDIATION_CODES, PACKAGE_ROOT, PYTHON_RULES}; use crate::python_rules::models::{PythonFiles, PythonRuleSpec, PythonTarget}; diff --git a/crates/sqlbuild-rules/src/python_rules/_helpers/file_cache.rs b/crates/sqlbuild-rules/src/python_rules/_helpers/engine/file_cache.rs similarity index 100% rename from crates/sqlbuild-rules/src/python_rules/_helpers/file_cache.rs rename to crates/sqlbuild-rules/src/python_rules/_helpers/engine/file_cache.rs diff --git a/crates/sqlbuild-rules/src/python_rules/_helpers/layout.rs b/crates/sqlbuild-rules/src/python_rules/_helpers/engine/layout.rs similarity index 96% rename from crates/sqlbuild-rules/src/python_rules/_helpers/layout.rs rename to crates/sqlbuild-rules/src/python_rules/_helpers/engine/layout.rs index cd0590b83..c0db10bcc 100644 --- a/crates/sqlbuild-rules/src/python_rules/_helpers/layout.rs +++ b/crates/sqlbuild-rules/src/python_rules/_helpers/engine/layout.rs @@ -3,8 +3,8 @@ use std::collections::BTreeSet; use crate::models::{Fault, PythonSource}; -use crate::python_rules::_helpers::faults::catalogue_fault; -use crate::python_rules::_helpers::placement::package_anchor; +use crate::python_rules::_helpers::engine::faults::catalogue_fault; +use crate::python_rules::_helpers::engine::placement::package_anchor; use crate::python_rules::constants::{ GENERIC_PACKAGE_NAMES, INIT_FILE, NODE_KIND_DIRECTORIES, PACKAGE_ROOT, PROJECT_ROOT_ALLOWLIST_CODE, ROLE_DIRECTORIES, RUNTIME_PACKAGE_OWNER_NAMES_CODE, diff --git a/crates/sqlbuild-rules/src/python_rules/_helpers/engine/mod.rs b/crates/sqlbuild-rules/src/python_rules/_helpers/engine/mod.rs new file mode 100644 index 000000000..6685ab9f6 --- /dev/null +++ b/crates/sqlbuild-rules/src/python_rules/_helpers/engine/mod.rs @@ -0,0 +1,9 @@ +pub(crate) mod contexts; +pub(crate) mod execution; +pub(crate) mod faults; +pub(crate) mod file_cache; +pub(crate) mod layout; +pub(crate) mod observations; +pub(crate) mod placement; +pub(crate) mod programs; +pub(crate) mod project_plane; diff --git a/crates/sqlbuild-rules/src/python_rules/_helpers/observations.rs b/crates/sqlbuild-rules/src/python_rules/_helpers/engine/observations.rs similarity index 97% rename from crates/sqlbuild-rules/src/python_rules/_helpers/observations.rs rename to crates/sqlbuild-rules/src/python_rules/_helpers/engine/observations.rs index 89fde7800..d34f67a6c 100644 --- a/crates/sqlbuild-rules/src/python_rules/_helpers/observations.rs +++ b/crates/sqlbuild-rules/src/python_rules/_helpers/engine/observations.rs @@ -8,8 +8,8 @@ use fensu_facts::snapshot::models::{ }; use fensu_native::NativeProjectQuery; -use crate::python_rules::_helpers::placement::package_anchor; -use crate::python_rules::_helpers::programs::Programs; +use crate::python_rules::_helpers::engine::placement::package_anchor; +use crate::python_rules::_helpers::engine::programs::Programs; use crate::python_rules::constants::PROJECT_ROOT_QUERY_PATH; const TRUE_TEXT: &str = "true"; diff --git a/crates/sqlbuild-rules/src/python_rules/_helpers/placement.rs b/crates/sqlbuild-rules/src/python_rules/_helpers/engine/placement.rs similarity index 100% rename from crates/sqlbuild-rules/src/python_rules/_helpers/placement.rs rename to crates/sqlbuild-rules/src/python_rules/_helpers/engine/placement.rs diff --git a/crates/sqlbuild-rules/src/python_rules/_helpers/programs.rs b/crates/sqlbuild-rules/src/python_rules/_helpers/engine/programs.rs similarity index 100% rename from crates/sqlbuild-rules/src/python_rules/_helpers/programs.rs rename to crates/sqlbuild-rules/src/python_rules/_helpers/engine/programs.rs diff --git a/crates/sqlbuild-rules/src/python_rules/_helpers/project_plane.rs b/crates/sqlbuild-rules/src/python_rules/_helpers/engine/project_plane.rs similarity index 98% rename from crates/sqlbuild-rules/src/python_rules/_helpers/project_plane.rs rename to crates/sqlbuild-rules/src/python_rules/_helpers/engine/project_plane.rs index 6c2ca577a..a34758191 100644 --- a/crates/sqlbuild-rules/src/python_rules/_helpers/project_plane.rs +++ b/crates/sqlbuild-rules/src/python_rules/_helpers/engine/project_plane.rs @@ -10,7 +10,7 @@ use fensu_native::{NativeProjectModule, NativeProjectPlane}; use sqlbuild_pysource::definition_locations::main::decorated_functions::decorated_functions; use crate::models::EvaluateRequest; -use crate::python_rules::_helpers::programs::Programs; +use crate::python_rules::_helpers::engine::programs::Programs; use crate::python_rules::constants::{ ENTRYPOINT_KIND, INIT_FILE, LOADED_DECORATORS, PACKAGE_ROOT, ROOT_SCOPE, }; diff --git a/crates/sqlbuild-rules/src/python_rules/_helpers/mod.rs b/crates/sqlbuild-rules/src/python_rules/_helpers/mod.rs index dd40c5313..dd2a058c6 100644 --- a/crates/sqlbuild-rules/src/python_rules/_helpers/mod.rs +++ b/crates/sqlbuild-rules/src/python_rules/_helpers/mod.rs @@ -1,10 +1,2 @@ -pub(crate) mod contexts; -pub(crate) mod execution; -pub(crate) mod faults; -pub(crate) mod file_cache; -pub(crate) mod layout; -pub(crate) mod observations; -pub(crate) mod placement; -pub(crate) mod programs; -pub(crate) mod project_plane; +pub(crate) mod engine; pub(crate) mod units; diff --git a/crates/sqlbuild-rules/src/python_rules/_helpers/units/imports.rs b/crates/sqlbuild-rules/src/python_rules/_helpers/units/imports.rs index 97e800af4..90569546b 100644 --- a/crates/sqlbuild-rules/src/python_rules/_helpers/units/imports.rs +++ b/crates/sqlbuild-rules/src/python_rules/_helpers/units/imports.rs @@ -5,7 +5,7 @@ use sqlbuild_pysource::node_units::models::UnitImport; use crate::models::Fault; use crate::python_rules::_helpers::units::project::{imported_module, unit_fault}; use crate::python_rules::constants::{ - NODE_IMPORT_BOUNDARIES_CODE, NODE_MODULE, PACKAGE_ROOT, PROVIDER_ROLE_MODULES, PROVIDERS_ROOT, + NODE_IMPORT_BOUNDARIES_CODE, PACKAGE_ROOT, PROVIDER_ROLE_MODULES, PROVIDERS_ROOT, SHARED_ROLE_MODULES, SHARED_UTILITIES_PACKAGE, UNIT_HELPERS_FOLDER, }; use crate::python_rules::models::UnitProject; @@ -55,11 +55,7 @@ fn allowed(module: &str, import: &UnitImport, unit: &str, project: &UnitProject< if shared.iter().any(|prefix| within(module, prefix)) || provider_role(module) { return true; } - let other_node: bool = project - .units - .iter() - .any(|other| module == format!("{PACKAGE_ROOT}.{}.{NODE_MODULE}", other.replace('/', "."))); - other_node && import.name.is_some() && import.only_in_depends_on + project.node_modules.contains(module) && import.only_in_depends_on } /// Whether `module` is a public role file of a provider capability, `providers..`. diff --git a/crates/sqlbuild-rules/src/python_rules/_helpers/units/layout.rs b/crates/sqlbuild-rules/src/python_rules/_helpers/units/layout.rs index 887547907..797537a2d 100644 --- a/crates/sqlbuild-rules/src/python_rules/_helpers/units/layout.rs +++ b/crates/sqlbuild-rules/src/python_rules/_helpers/units/layout.rs @@ -5,15 +5,14 @@ use std::collections::BTreeSet; use sqlbuild_pysource::node_units::models::UnitFunction; use crate::models::{Fault, PythonSource}; -use crate::python_rules::_helpers::faults::catalogue_fault; -use crate::python_rules::_helpers::placement::package_anchor; +use crate::python_rules::_helpers::engine::faults::catalogue_fault; +use crate::python_rules::_helpers::engine::placement::package_anchor; use crate::python_rules::_helpers::units::project::{ - Declared, declared, top_functions, unit_fault, unit_file_name, + Declared, declared, is_role_folder, top_functions, unit_fault, unit_file_name, }; use crate::python_rules::constants::{ - NODE_DECLARED_IN_UNIT_CODE, NODE_KIND_DIRECTORIES, NODE_UNIT_LAYOUT_CODE, PACKAGE_ROOT, - ROLE_DIRECTORIES, SHARED_UTILITIES_PACKAGE, UNIT_CHECKS_FILE, UNIT_HELPERS_FOLDER, - UNIT_MAIN_FILE, UNIT_NODE_FILE, UNIT_ROLE_FILES, + NODE_DECLARED_IN_UNIT_CODE, NODE_UNIT_LAYOUT_CODE, PACKAGE_ROOT, UNIT_CHECKS_FILE, + UNIT_HELPERS_FOLDER, UNIT_MAIN_FILE, UNIT_NODE_FILE, UNIT_ROLE_FILES, }; use crate::python_rules::models::{UnitFile, UnitProject}; @@ -145,13 +144,9 @@ fn domain(file: &UnitFile<'_>) -> String { let [first, _, ..] = file.parts.as_slice() else { return "".to_owned(); }; - let first: &str = first; - let not_a_domain: bool = first == SHARED_UTILITIES_PACKAGE - || NODE_KIND_DIRECTORIES.contains(&first) - || ROLE_DIRECTORIES.iter().any(|(name, _)| *name == first); - if not_a_domain { + if is_role_folder(first) { "".to_owned() } else { - first.to_owned() + (*first).to_owned() } } diff --git a/crates/sqlbuild-rules/src/python_rules/_helpers/units/nodes.rs b/crates/sqlbuild-rules/src/python_rules/_helpers/units/nodes.rs index 05ead161e..f125ff824 100644 --- a/crates/sqlbuild-rules/src/python_rules/_helpers/units/nodes.rs +++ b/crates/sqlbuild-rules/src/python_rules/_helpers/units/nodes.rs @@ -6,7 +6,8 @@ use sqlbuild_pysource::node_units::models::{UnitDecorator, UnitFunction}; use crate::models::Fault; use crate::python_rules::_helpers::units::project::{ - Declared, declared, names_from_unit_modules, top_functions, unit_fault, unit_file_name, + Declared, binds_module, declared, names_from_unit_modules, top_functions, unit_fault, + unit_file_name, unit_imports, }; use crate::python_rules::constants::{ CHECK_KIND, CONTEXT_PARAMETER, DESCRIPTION_KEYWORD, FACTORY_SHAPE_CODE, LOADER_DECORATOR, @@ -32,7 +33,11 @@ pub(crate) fn declaration_faults(project: &UnitProject<'_>) -> Vec { continue; }; let names = UnitNames { - main: names_from_unit_modules(file, project, unit, &[MAIN_NAME]), + main: unit_imports(file, project, unit, &[MAIN_NAME]) + .into_iter() + .filter(|import| !binds_module(import, file, project)) + .map(|import| import.local.clone()) + .collect(), specs: names_from_unit_modules(file, project, unit, &SPEC_MODULES), }; for function in top_functions(file) { @@ -148,17 +153,17 @@ fn description_detail(function: &UnitFunction, decorator: &UnitDecorator) -> Opt /// A factory's loops, generated names and generated declarations. fn factory_faults(file: &UnitFile<'_>, factory: &UnitFunction, names: &UnitNames) -> Vec { let mut details: Vec = Vec::new(); + let reads_specs: bool = factory + .loops + .iter() + .flatten() + .any(|name| names.specs.contains(name)); if factory.loops.is_empty() { details.push(format!( "factory '{}' does not loop over specs", factory.name )); - } - if factory.loops.iter().any(|iterable| { - !iterable - .as_ref() - .is_some_and(|name| names.specs.contains(name)) - }) { + } else if !reads_specs { details.push(format!( "factory '{}' loops over something that is not imported from its unit's constants.py or models.py", factory.name diff --git a/crates/sqlbuild-rules/src/python_rules/_helpers/units/project.rs b/crates/sqlbuild-rules/src/python_rules/_helpers/units/project.rs index c967c6df6..ff3c6791b 100644 --- a/crates/sqlbuild-rules/src/python_rules/_helpers/units/project.rs +++ b/crates/sqlbuild-rules/src/python_rules/_helpers/units/project.rs @@ -2,13 +2,15 @@ use std::collections::BTreeSet; +use rayon::iter::{IntoParallelRefIterator, ParallelIterator}; + use sqlbuild_pysource::node_units::main::node_unit_module::node_unit_module; use sqlbuild_pysource::node_units::models::{ UnitDecorator, UnitFunction, UnitImport, UnitPosition, UnitStatementKind, }; use crate::models::{Fault, PythonSource}; -use crate::python_rules::_helpers::faults::catalogue_fault; +use crate::python_rules::_helpers::engine::faults::catalogue_fault; use crate::python_rules::_helpers::units::imports::import_boundary_faults; use crate::python_rules::_helpers::units::layout::{ declared_outside_unit_faults, unit_layout_faults, @@ -16,7 +18,8 @@ use crate::python_rules::_helpers::units::layout::{ use crate::python_rules::_helpers::units::modules::module_shape_faults; use crate::python_rules::_helpers::units::nodes::declaration_faults; use crate::python_rules::constants::{ - CHECK_KIND, FACTORY_DECORATORS, INIT_FILE, NODE_KINDS, PACKAGE_ROOT, UNIT_NODE_FILE, + CHECK_KIND, FACTORY_DECORATORS, INIT_FILE, MIN_UNIT_DEPTH, NODE_KIND_DIRECTORIES, NODE_KINDS, + NODE_MODULE, PACKAGE_ROOT, ROLE_DIRECTORIES, SHARED_UTILITIES_PACKAGE, UNIT_NODE_FILE, UNIT_RULE_CODES, }; use crate::python_rules::models::{UnitFile, UnitProject}; @@ -40,6 +43,15 @@ pub(crate) fn unit_faults( faults } +/// The unit folder `parts` declares, when it is a `node.py` deep enough and outside every role folder. +fn unit_folder(parts: &[&str]) -> Option { + let (last, folders) = parts.split_last()?; + let declares: bool = *last == UNIT_NODE_FILE + && folders.len() >= MIN_UNIT_DEPTH + && !folders.iter().copied().any(is_role_folder); + declares.then(|| folders.join("/")) +} + fn unit_project(sources: &[PythonSource], python_version: (u8, u8)) -> UnitProject<'_> { let package: Vec<(&PythonSource, Vec<&str>)> = sources .iter() @@ -50,13 +62,10 @@ fn unit_project(sources: &[PythonSource], python_version: (u8, u8)) -> UnitProje .collect(); let units: BTreeSet = package .iter() - .filter( - |(_, parts)| matches!(parts.as_slice(), [_, _, .., last] if *last == UNIT_NODE_FILE), - ) - .map(|(_, parts)| parts[..parts.len() - 1].join("/")) + .filter_map(|(_, parts)| unit_folder(parts)) .collect(); let files: Vec> = package - .iter() + .par_iter() .filter_map(|(source, parts)| { Some(UnitFile { path: source.relative_path.as_str(), @@ -67,6 +76,15 @@ fn unit_project(sources: &[PythonSource], python_version: (u8, u8)) -> UnitProje }) .collect(); UnitProject { + index: files + .iter() + .enumerate() + .map(|(position, file)| (file.path, position)) + .collect(), + node_modules: units + .iter() + .map(|unit| format!("{PACKAGE_ROOT}.{}.{NODE_MODULE}", unit.replace('/', "."))) + .collect(), files, units, paths: sources @@ -129,7 +147,17 @@ pub(crate) fn unit_member<'p, 'a>( name: &str, ) -> Option<&'p UnitFile<'a>> { let path: String = format!("{PACKAGE_ROOT}/{unit}/{name}"); - project.files.iter().find(|file| file.path == path) + project + .index + .get(path.as_str()) + .and_then(|position| project.files.get(*position)) +} + +/// Whether a folder under `python/` holds shared or role code rather than a domain or a unit. +pub(crate) fn is_role_folder(folder: &str) -> bool { + folder == SHARED_UTILITIES_PACKAGE + || NODE_KIND_DIRECTORIES.contains(&folder) + || ROLE_DIRECTORIES.iter().any(|(name, _)| *name == folder) } /// What a decorator declares: a node kind, `check`, or a factory. @@ -142,10 +170,12 @@ pub(crate) enum Declared { /// What `function`'s decorators declare, if anything. pub(crate) fn declared(function: &UnitFunction) -> Option<(Declared, &UnitDecorator)> { - function - .decorators - .iter() - .find_map(|decorator| decorator_declares(&decorator.name).map(|what| (what, decorator))) + function.decorators.iter().find_map(declaration) +} + +/// What `decorator` declares, with the decorator, if anything. +fn declaration(decorator: &UnitDecorator) -> Option<(Declared, &UnitDecorator)> { + Some((decorator_declares(&decorator.name)?, decorator)) } /// What a decorator named `name` declares, if anything. @@ -199,13 +229,13 @@ pub(crate) fn imported_module( }) } -/// The names `file` binds by importing from `python..`, one of `modules`. -pub(crate) fn names_from_unit_modules( - file: &UnitFile<'_>, +/// The imports in `file` of `python..`, one of `modules`, or of names from it. +pub(crate) fn unit_imports<'f>( + file: &'f UnitFile<'_>, project: &UnitProject<'_>, unit: &str, modules: &[&str], -) -> BTreeSet { +) -> Vec<&'f UnitImport> { let targets: Vec = modules .iter() .map(|module| format!("{PACKAGE_ROOT}.{}.{module}", unit.replace('/', "."))) @@ -217,6 +247,35 @@ pub(crate) fn names_from_unit_modules( .filter(|import| { imported_module(import, file, project).is_some_and(|module| targets.contains(&module)) }) + .collect() +} + +/// The names `file` binds by importing from `python..`, one of `modules`. +pub(crate) fn names_from_unit_modules( + file: &UnitFile<'_>, + project: &UnitProject<'_>, + unit: &str, + modules: &[&str], +) -> BTreeSet { + unit_imports(file, project, unit, modules) + .into_iter() .map(|import| import.local.clone()) .collect() } + +/// Whether `import` binds a module, as `from python.orders.daily import main` binds `main.py` itself. +pub(crate) fn binds_module( + import: &UnitImport, + file: &UnitFile<'_>, + project: &UnitProject<'_>, +) -> bool { + let Some(name) = &import.name else { + return true; + }; + let package = UnitImport { + name: None, + ..import.clone() + }; + imported_module(import, file, project) + == imported_module(&package, file, project).map(|module| format!("{module}.{name}")) +} diff --git a/crates/sqlbuild-rules/src/python_rules/constants.rs b/crates/sqlbuild-rules/src/python_rules/constants.rs index 1436b554a..114a17029 100644 --- a/crates/sqlbuild-rules/src/python_rules/constants.rs +++ b/crates/sqlbuild-rules/src/python_rules/constants.rs @@ -1034,6 +1034,8 @@ pub(crate) const CHECKS_MODULE_SHAPE_CODE: &str = "SQBRPYTHON009"; pub(crate) const FACTORY_SHAPE_CODE: &str = "SQBRPYTHON010"; pub(crate) const NODE_DESCRIPTION_CODE: &str = "SQBRPYTHON011"; pub(crate) const NODE_DECLARED_IN_UNIT_CODE: &str = "SQBRPYTHON012"; +/// The fewest folders below `python/` a unit sits in: its domain and its own. +pub(crate) const MIN_UNIT_DEPTH: usize = 2; /// The file that declares a unit's node and makes its folder a unit. pub(crate) const UNIT_NODE_FILE: &str = "node.py"; /// The file holding a unit's checks. @@ -1087,6 +1089,8 @@ pub(crate) const MAIN_RESULTS: [(&str, &[&str]); 3] = [ "loader", &[ "list", + "tuple", + "Collection", "Iterator", "Iterable", "Generator", diff --git a/crates/sqlbuild-rules/src/python_rules/main/evaluate_python_rules.rs b/crates/sqlbuild-rules/src/python_rules/main/evaluate_python_rules.rs index 299aa5633..0ebea79d5 100644 --- a/crates/sqlbuild-rules/src/python_rules/main/evaluate_python_rules.rs +++ b/crates/sqlbuild-rules/src/python_rules/main/evaluate_python_rules.rs @@ -3,13 +3,15 @@ use std::collections::{BTreeMap, BTreeSet}; use crate::models::{EvaluateRequest, RuleMetadata}; -use crate::python_rules::_helpers::contexts::context_settings; -use crate::python_rules::_helpers::execution::{evaluated_files, file_plans, selected_rules}; -use crate::python_rules::_helpers::file_cache::project_digest; -use crate::python_rules::_helpers::layout::layout_faults; -use crate::python_rules::_helpers::placement::python_targets; -use crate::python_rules::_helpers::programs::Programs; -use crate::python_rules::_helpers::project_plane::loaded_project; +use crate::python_rules::_helpers::engine::contexts::context_settings; +use crate::python_rules::_helpers::engine::execution::{ + evaluated_files, file_plans, selected_rules, +}; +use crate::python_rules::_helpers::engine::file_cache::project_digest; +use crate::python_rules::_helpers::engine::layout::layout_faults; +use crate::python_rules::_helpers::engine::placement::python_targets; +use crate::python_rules::_helpers::engine::programs::Programs; +use crate::python_rules::_helpers::engine::project_plane::loaded_project; use crate::python_rules::_helpers::units::project::unit_faults; use crate::python_rules::models::{ CallInputs, PythonCacheEntry, PythonEvaluation, PythonRuleSpec, PythonTarget, diff --git a/crates/sqlbuild-rules/src/python_rules/models.rs b/crates/sqlbuild-rules/src/python_rules/models.rs index eb3925134..80628f54f 100644 --- a/crates/sqlbuild-rules/src/python_rules/models.rs +++ b/crates/sqlbuild-rules/src/python_rules/models.rs @@ -56,8 +56,8 @@ pub(crate) struct RuleCall { #[derive(Debug)] pub(crate) struct CallInputs<'a> { pub request: &'a crate::models::EvaluateRequest, - pub settings: crate::python_rules::_helpers::contexts::ContextSettings, - pub programs: &'a crate::python_rules::_helpers::programs::Programs<'a>, + pub settings: crate::python_rules::_helpers::engine::contexts::ContextSettings, + pub programs: &'a crate::python_rules::_helpers::engine::programs::Programs<'a>, /// Cached findings by file from the last run with the same ruleset. pub cached: &'a std::collections::BTreeMap, /// The digest of the ruleset and interpreter version the cached findings must match. @@ -121,6 +121,10 @@ pub(crate) struct UnitProject<'a> { pub units: std::collections::BTreeSet, /// Every project Python path. pub paths: std::collections::BTreeSet<&'a str>, + /// Each file's position in `files`, by path. + pub index: std::collections::BTreeMap<&'a str, usize>, + /// The dotted module of every unit's `node.py`, such as `python.orders.daily.node`. + pub node_modules: std::collections::BTreeSet, /// Every project module and package as a dotted name, such as `python.orders.constants`. pub modules: std::collections::BTreeSet, } diff --git a/crates/sqlbuild-rules/src/python_rules/tests/helpers.rs b/crates/sqlbuild-rules/src/python_rules/tests/helpers.rs index ab947c647..32e134882 100644 --- a/crates/sqlbuild-rules/src/python_rules/tests/helpers.rs +++ b/crates/sqlbuild-rules/src/python_rules/tests/helpers.rs @@ -194,6 +194,60 @@ pub(super) fn scaling_project( .collect() } +/// A project of a chain of `units` node units over 50 domains, each node depending on the one before, plus `undescribed` unlinked units whose nodes have no description. +pub(super) fn unit_scaling_project(units: usize, undescribed: usize) -> Vec<(String, String)> { + let head = unit_files(0, "", "description=\"Unit 0.\""); + let chain = (1..units).flat_map(|unit| { + let previous = unit - 1; + unit_files( + unit, + &format!( + "from {}.node import node_{previous}\n", + unit_module(previous) + ), + &format!("depends_on=[node_{previous}], description=\"Unit {unit}.\""), + ) + }); + let bare = (units..units + undescribed).flat_map(|unit| unit_files(unit, "", "")); + let utils = ( + "python/utils/text/join.py".to_owned(), + "def joined(value: str) -> str:\n return value\n".to_owned(), + ); + head.into_iter() + .chain(chain) + .chain(bare) + .chain(std::iter::once(utils)) + .collect() +} + +fn unit_module(unit: usize) -> String { + format!("python.area{}.unit_{unit}", unit % 50) +} + +/// The node, main and checks files of `unit`, its node importing `upstream` and declared with `arguments`. +fn unit_files(unit: usize, upstream: &str, arguments: &str) -> [(String, String); 3] { + let module = unit_module(unit); + let dir = module.replace('.', "/"); + [ + ( + format!("{dir}/node.py"), + format!( + "{upstream}from {module}.main import main\n\n\n@task({arguments})\ndef node_{unit}(ctx: TaskContext) -> None:\n return main(ctx=ctx)\n" + ), + ), + ( + format!("{dir}/main.py"), + "from sqlbuild import PythonNodeResult\n\nfrom python.utils.text.join import joined\n\n\ndef main(ctx: TaskContext) -> PythonNodeResult:\n return ctx.result(joined(\"\"))\n".to_owned(), + ), + ( + format!("{dir}/checks.py"), + format!( + "from {module}.node import node_{unit}\n\n\n@check(depends_on=[node_{unit}], description=\"Ran.\")\ndef ran_{unit}(ctx: CheckContext) -> bool:\n return ctx.ok()\n" + ), + ), + ] +} + /// The codes of the Python faults `files` produce under `config` with the cache off, and how long evaluation took. pub(super) fn timed_python_codes( files: &[(String, String)], diff --git a/crates/sqlbuild-rules/src/python_rules/tests/test_node_units.rs b/crates/sqlbuild-rules/src/python_rules/tests/test_node_units.rs index e79a0225d..bc2befd42 100644 --- a/crates/sqlbuild-rules/src/python_rules/tests/test_node_units.rs +++ b/crates/sqlbuild-rules/src/python_rules/tests/test_node_units.rs @@ -612,3 +612,164 @@ def tables(): ); } } + +#[test] +fn given_valid_shapes_and_hidden_mistakes_when_evaluating_unit_rules_then_reports_only_the_mistakes() + { + let test_cases = [ + PythonRunTestCase { + description: "a node.py in utils/, a role folder or a unit's _helpers/ is not a unit", + files: &[ + ("python/utils/graph/node.py", "class Node:\n pass\n"), + ("python/orders/models/node.py", "class Node:\n pass\n"), + ("python/orders/daily/node.py", DAILY_TASK), + ("python/orders/daily/main.py", TASK_MAIN), + ( + "python/orders/daily/_helpers/node.py", + "class Node:\n pass\n", + ), + ( + "python/orders/daily/_helpers/rows.py", + "from python.orders.daily.main import main\n", + ), + ], + config: json!({ "select": UNIT_RULES }), + expected_faults: vec![], + }, + PythonRunTestCase { + description: "a factory may read specs through a method or a module attribute, and loop again inside", + files: &[ + ( + "python/orders/regions/constants.py", + "SPECS: dict = {}\nTABLES: tuple = ()\n", + ), + ( + "python/orders/regions/node.py", + r#"from . import constants +from .constants import SPECS +from .main import main + + +@factory +def region_exports(): + for region, spec in SPECS.items(): + for column in spec.columns: + + @task(name=f"export_{region}_{column}", description="E.") + def export(ctx: TaskContext) -> None: + return main(ctx=ctx, region=region) + + for table in constants.TABLES: + + @task(name=table, description="T.") + def load(ctx: TaskContext) -> None: + return main(ctx=ctx) +"#, + ), + ("python/orders/regions/main.py", TASK_MAIN), + ], + config: json!({ "select": ["SQBRPYTHON010"] }), + expected_faults: vec![], + }, + PythonRunTestCase { + description: "imports under if TYPE_CHECKING: are imports, and their boundaries are checked", + files: &[ + ("providers/store/provider.py", ""), + ( + "python/orders/daily/node.py", + r#"from typing import TYPE_CHECKING + +from .main import main + +if TYPE_CHECKING: + from providers.store.provider import Store + from python.orders.weekly.main import main as weekly_main + + +@task(description="Daily.") +def daily(ctx: TaskContext, *, store: "Store") -> None: + return main(ctx=ctx, store=store) +"#, + ), + ("python/orders/daily/main.py", TASK_MAIN), + ("python/orders/weekly/node.py", DAILY_TASK), + ("python/orders/weekly/main.py", TASK_MAIN), + ], + config: json!({ "select": ["SQBRPYTHON004", "SQBRPYTHON007", "SQBRPYTHON008"] }), + expected_faults: vec![("SQBRPYTHON008", "python/orders/daily/node.py", 7, 43)], + }, + PythonRunTestCase { + description: "a node named through import a.b in depends_on passes; an unused import of a node does not", + files: &[ + ("python/orders/daily/node.py", DAILY_TASK), + ("python/orders/daily/main.py", TASK_MAIN), + ( + "python/orders/weekly/node.py", + r#"import python.orders.daily.node + +from .main import main + + +@task(depends_on=[python.orders.daily.node.daily], description="W.") +def weekly(ctx: TaskContext) -> None: + return main(ctx=ctx) +"#, + ), + ("python/orders/weekly/main.py", TASK_MAIN), + ( + "python/orders/weekly/_helpers/rows.py", + "from python.orders.daily.node import daily\n", + ), + ], + config: json!({ "select": ["SQBRPYTHON008"] }), + expected_faults: vec![( + "SQBRPYTHON008", + "python/orders/weekly/_helpers/rows.py", + 1, + 38, + )], + }, + PythonRunTestCase { + description: "a quoted union result and a loader's tuple of rows fit their node", + files: &[ + ("python/orders/daily/node.py", DAILY_TASK), + ( + "python/orders/daily/main.py", + "def main(ctx: TaskContext) -> \"PythonNodeResult | PythonNodeSkipResult\":\n return ctx.result()\n", + ), + ( + "python/orders/raw/node.py", + "from .main import main\n\n\n@loader\ndef raw(ctx: LoaderContext) -> tuple:\n return main(ctx=ctx)\n", + ), + ( + "python/orders/raw/main.py", + "def main(ctx: LoaderContext) -> \"tuple[python.orders.raw.types.Row, ...]\":\n return ()\n", + ), + ], + config: json!({ "select": ["SQBRPYTHON006"] }), + expected_faults: vec![], + }, + PythonRunTestCase { + description: "a node that calls the main.py module instead of main() reports", + files: &[ + ( + "python/orders/daily/node.py", + "from . import main\n\n\n@task(description=\"D.\")\ndef daily(ctx: TaskContext) -> None:\n return main(ctx=ctx)\n", + ), + ("python/orders/daily/main.py", TASK_MAIN), + ], + config: json!({ "select": ["SQBRPYTHON005"] }), + expected_faults: vec![("SQBRPYTHON005", "python/orders/daily/node.py", 5, 5)], + }, + ]; + for test_case in test_cases { + let faults = python_rule_faults(test_case.files, test_case.config.clone()); + + assert_eq!( + faults, + Ok(owned_faults(&test_case.expected_faults)), + "{}", + test_case.description + ); + } +} diff --git a/crates/sqlbuild-rules/src/python_rules/tests/test_python_cache.rs b/crates/sqlbuild-rules/src/python_rules/tests/test_python_cache.rs index 6f4cfdfda..33e247394 100644 --- a/crates/sqlbuild-rules/src/python_rules/tests/test_python_cache.rs +++ b/crates/sqlbuild-rules/src/python_rules/tests/test_python_cache.rs @@ -2,7 +2,7 @@ use serde_json::json; use fensu_native::NativeProjectQuery; -use crate::python_rules::_helpers::observations::relative_query; +use crate::python_rules::_helpers::engine::observations::relative_query; use crate::python_rules::tests::helpers::{cached_runs, named_faults}; use crate::python_rules::tests::test_types::{ CachedRuns, PythonCacheTestCase, RelativeQueryTestCase, diff --git a/crates/sqlbuild-rules/src/python_rules/tests/test_python_scaling.rs b/crates/sqlbuild-rules/src/python_rules/tests/test_python_scaling.rs index d959136f5..2c17e9de5 100644 --- a/crates/sqlbuild-rules/src/python_rules/tests/test_python_scaling.rs +++ b/crates/sqlbuild-rules/src/python_rules/tests/test_python_scaling.rs @@ -1,11 +1,14 @@ +use std::collections::BTreeMap; use std::time::Duration; use serde_json::json; use crate::python_rules::tests::helpers::{ - median_growth, scaling_project, serial_timing, timed_python_codes, + median_growth, scaling_project, serial_timing, timed_python_codes, unit_scaling_project, +}; +use crate::python_rules::tests::test_types::{ + PythonGrowthTestCase, PythonScalingTestCase, UnitScalingTestCase, }; -use crate::python_rules::tests::test_types::{PythonGrowthTestCase, PythonScalingTestCase}; #[test] fn given_many_modules_and_dense_findings_when_evaluating_python_rules_then_finishes_within_budget() @@ -75,3 +78,51 @@ fn given_four_times_the_modules_when_finding_unreachable_definitions_then_time_g ); } } + +#[test] +fn given_many_linked_node_units_when_evaluating_unit_rules_then_finishes_within_budget_and_grows_near_linearly() + { + let _serial = serial_timing(); + let test_cases = [UnitScalingTestCase { + description: "default rules over a chain of 2000 units of node, main and checks, and 200 undescribed units", + units: 2000, + undescribed: 200, + config: json!({ "select": ["SQBRPYTHON"] }), + expected_codes: vec![("SQBRPYTHON011", 200)], + expected_max_duration: Duration::from_secs(5), + growth: 4, + pairs: 5, + expected_max_ratio: 6.5, + }]; + for test_case in test_cases { + let large = unit_scaling_project(test_case.units, test_case.undescribed); + let (codes, elapsed) = + timed_python_codes(&large, test_case.config.clone()).expect("rules run"); + let mut counts: BTreeMap<&str, usize> = BTreeMap::new(); + for code in &codes { + *counts.entry(code.as_str()).or_default() += 1; + } + assert_eq!( + counts.into_iter().collect::>(), + test_case.expected_codes, + "{}", + test_case.description + ); + assert!( + elapsed < test_case.expected_max_duration, + "{}: took {elapsed:?}", + test_case.description + ); + let small = unit_scaling_project( + test_case.units / test_case.growth, + test_case.undescribed / test_case.growth, + ); + let ratio: f64 = median_growth(&small, &large, &test_case.config, test_case.pairs); + assert!( + ratio < test_case.expected_max_ratio, + "{}: growing the project {}x multiplied the time by {ratio:.2}", + test_case.description, + test_case.growth + ); + } +} diff --git a/crates/sqlbuild-rules/src/python_rules/tests/test_types.rs b/crates/sqlbuild-rules/src/python_rules/tests/test_types.rs index 865513b6d..41b7f36ce 100644 --- a/crates/sqlbuild-rules/src/python_rules/tests/test_types.rs +++ b/crates/sqlbuild-rules/src/python_rules/tests/test_types.rs @@ -74,6 +74,18 @@ pub(super) struct PythonScalingTestCase { pub(super) expected_max_duration: std::time::Duration, } +pub(super) struct UnitScalingTestCase { + pub(super) description: &'static str, + pub(super) units: usize, + pub(super) undescribed: usize, + pub(super) config: serde_json::Value, + pub(super) expected_codes: Vec<(&'static str, usize)>, + pub(super) expected_max_duration: std::time::Duration, + pub(super) growth: usize, + pub(super) pairs: usize, + pub(super) expected_max_ratio: f64, +} + pub(super) struct PythonGrowthTestCase { pub(super) description: &'static str, pub(super) units: usize, From 8d0c5dc266ecf092c7f6cefc81420c4fe67f2e03 Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Sun, 15 Nov 2026 02:24:17 +0000 Subject: [PATCH 3/3] fix: accept SQBRPYTHON factory loops that wrap specs in a call A factory loop now records every name its iterable loads, so loops over enumerate(SPECS) or sorted(SPECS) read the spec instead of the builtin. --- .../src/node_units/_helpers/functions.rs | 29 +++++++++++++------ .../src/node_units/models.rs | 4 +-- .../node_units/tests/test_node_unit_module.rs | 2 +- .../src/python_rules/tests/test_node_units.rs | 19 +++++++++++- 4 files changed, 41 insertions(+), 13 deletions(-) diff --git a/crates/sqlbuild-pysource/src/node_units/_helpers/functions.rs b/crates/sqlbuild-pysource/src/node_units/_helpers/functions.rs index 6c0b6ff71..934d7ac58 100644 --- a/crates/sqlbuild-pysource/src/node_units/_helpers/functions.rs +++ b/crates/sqlbuild-pysource/src/node_units/_helpers/functions.rs @@ -1,5 +1,6 @@ //! A function's decorators, signature and body shape, and the decorated functions inside it. +use ruff_python_ast::visitor::{Visitor, walk_expr}; use ruff_python_ast::{Decorator, Expr, Parameter, Stmt, StmtFunctionDef}; use ruff_python_parser::{Mode, ParseOptions}; use ruff_text_size::Ranged; @@ -55,7 +56,7 @@ pub(crate) fn unit_function(function: &StmtFunctionDef, source: &UnitText<'_>) - .iter() .filter_map(|node| match node { CpythonNode::Stmt(Stmt::For(loop_statement)) => { - Some(root_name(&loop_statement.iter)) + Some(loaded_names(&loop_statement.iter)) } _ => None, }) @@ -192,13 +193,23 @@ fn string_expression(text: &str) -> Option { Some(*parsed.try_into_expression()?.into_syntax().body) } -/// The name an iterable is read from: `SPECS` for `SPECS`, `SPECS.items()`, `SPECS[1:]` or `SPECS.rows`. -fn root_name(expression: &Expr) -> Option { - match expression { - Expr::Name(name) => Some(name.id.to_string()), - Expr::Attribute(attribute) => root_name(&attribute.value), - Expr::Call(call) => root_name(&call.func), - Expr::Subscript(subscript) => root_name(&subscript.value), - _ => None, +/// Every name `expression` loads, in source order: `enumerate` and `SPECS` for `enumerate(SPECS.items())`. +fn loaded_names(expression: &Expr) -> Vec { + let mut names = LoadedNames::default(); + names.visit_expr(expression); + names.names +} + +#[derive(Default)] +struct LoadedNames { + names: Vec, +} + +impl<'a> Visitor<'a> for LoadedNames { + fn visit_expr(&mut self, expression: &'a Expr) { + if let Expr::Name(name) = expression { + self.names.push(name.id.to_string()); + } + walk_expr(self, expression); } } diff --git a/crates/sqlbuild-pysource/src/node_units/models.rs b/crates/sqlbuild-pysource/src/node_units/models.rs index b746a58b4..fb2ddaa36 100644 --- a/crates/sqlbuild-pysource/src/node_units/models.rs +++ b/crates/sqlbuild-pysource/src/node_units/models.rs @@ -71,8 +71,8 @@ pub struct UnitFunction { pub delegation: Option, /// The decorated functions defined anywhere in the body. pub inner: Vec, - /// The name each `for` loop in the body reads its iterable from, such as `SPECS` for `SPECS.items()`; `None` when it reads no name. - pub loops: Vec>, + /// The names each `for` loop's iterable in the body loads, such as `enumerate` and `SPECS` for `enumerate(SPECS.items())`. + pub loops: Vec>, } /// A decorator by its last name, the keywords it is called with and the names its `depends_on` reads. diff --git a/crates/sqlbuild-pysource/src/node_units/tests/test_node_unit_module.rs b/crates/sqlbuild-pysource/src/node_units/tests/test_node_unit_module.rs index a561a18f6..18c4892f5 100644 --- a/crates/sqlbuild-pysource/src/node_units/tests/test_node_unit_module.rs +++ b/crates/sqlbuild-pysource/src/node_units/tests/test_node_unit_module.rs @@ -131,7 +131,7 @@ fn given_declaration_when_reading_then_signature_body_and_inner_declarations_are inner: vec![], loops: vec![], }], - loops: vec![Some("TABLES".to_owned()), Some("specs".to_owned())], + loops: vec![names(&["TABLES"]), names(&["specs"])], }, }, FunctionTestCase { diff --git a/crates/sqlbuild-rules/src/python_rules/tests/test_node_units.rs b/crates/sqlbuild-rules/src/python_rules/tests/test_node_units.rs index bc2befd42..2a8ce141c 100644 --- a/crates/sqlbuild-rules/src/python_rules/tests/test_node_units.rs +++ b/crates/sqlbuild-rules/src/python_rules/tests/test_node_units.rs @@ -637,7 +637,7 @@ fn given_valid_shapes_and_hidden_mistakes_when_evaluating_unit_rules_then_report expected_faults: vec![], }, PythonRunTestCase { - description: "a factory may read specs through a method or a module attribute, and loop again inside", + description: "a factory may read specs through a method, a module attribute or a builtin call, and loop again inside", files: &[ ( "python/orders/regions/constants.py", @@ -667,6 +667,23 @@ def region_exports(): "#, ), ("python/orders/regions/main.py", TASK_MAIN), + ("python/orders/ranked/constants.py", "SPECS: dict = {}\n"), + ( + "python/orders/ranked/node.py", + r#"from .constants import SPECS +from .main import main + + +@factory +def ranked_exports(): + for rank, region in enumerate(sorted(SPECS)): + + @task(name=f"export_{rank}_{region}", description="E.") + def export(ctx: TaskContext) -> None: + return main(ctx=ctx, region=region) +"#, + ), + ("python/orders/ranked/main.py", TASK_MAIN), ], config: json!({ "select": ["SQBRPYTHON010"] }), expected_faults: vec![],