Skip to content

Fix typed trigger parameter bindings #36

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
yenda wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix typed trigger parameter bindings #36

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 56 additions & 2 deletions crates/allium-parser/src/analysis.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4485,8 +4485,14 @@ fn collect_bound_names<'a>(expr: &'a Expr, out: &mut HashSet<&'a str>) {
}
Expr::Call { args, .. } => {
for arg in args {
if let CallArg::Positional(Expr::Ident(id)) = arg {
out.insert(&id.name);
match arg {
CallArg::Positional(Expr::Ident(id)) => {
out.insert(&id.name);
}
CallArg::Named(named) if is_type_annotation_expr(&named.value) => {
out.insert(&named.name.name);
}
_ => {}
}
}
}
Expand All @@ -4498,6 +4504,21 @@ fn collect_bound_names<'a>(expr: &'a Expr, out: &mut HashSet<&'a str>) {
}
}

fn is_type_annotation_expr(expr: &Expr) -> bool {
match expr {
Expr::Ident(id) => starts_uppercase(&id.name),
Expr::QualifiedName(q) => starts_uppercase(&q.name),
Expr::GenericType { name, args, .. } => {
is_type_annotation_expr(name) && args.iter().all(is_type_annotation_expr)
}
Expr::Pipe { left, right, .. } => {
is_type_annotation_expr(left) && is_type_annotation_expr(right)
}
Expr::TypeOptional { inner, .. } => is_type_annotation_expr(inner),
_ => false,
}
}

fn check_unbound_roots(
expr: &Expr,
bound: &HashSet<&str>,
Expand Down Expand Up @@ -5092,6 +5113,39 @@ mod tests {
assert!(!has_code(&ds, "allium.surface.unusedBinding"));
}

// -- Rule bindings --

#[test]
fn typed_call_trigger_param_is_bound() {
let ds = analyze_src(
"entity Account {\n name: String\n}\n\n\
entity Greeting {\n label: String\n}\n\n\
rule TypedParam {\n when: AccountSeen(account: Account)\n \
ensures: Greeting.created(label: account.name)\n}\n",
);
assert!(!has_code(&ds, "allium.rule.undefinedBinding"));
}

#[test]
fn generic_typed_call_trigger_param_is_bound() {
let ds = analyze_src(
"entity Account {\n name: String\n}\n\n\
entity Greeting {\n label: String\n}\n\n\
rule GenericTypedParam {\n when: AccountsSeen(accounts: List<Account>)\n \
ensures: Greeting.created(label: accounts.count)\n}\n",
);
assert!(!has_code(&ds, "allium.rule.undefinedBinding"));
}

#[test]
fn named_literal_trigger_arg_is_not_binding() {
let ds = analyze_src(
"rule LiteralFilter {\n when: CommandInvoked(name: \"npm run test\")\n \
requires: name.status = active\n ensures: Done()\n}\n",
);
assert!(has_code(&ds, "allium.rule.undefinedBinding"));
}

// -- Status state machine --

#[test]
Expand Down
129 changes: 128 additions & 1 deletion extensions/allium/src/language-tools/analyzer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2437,11 +2437,16 @@ function collectRuleBoundNames(
/^\s*when\s*:\s*[A-Za-z_][A-Za-z0-9_]*(?:\/[A-Za-z_][A-Za-z0-9_]*)?\s*\(([^)]*)\)/m;
const whenCallMatch = ruleBody.match(whenCallPattern);
if (whenCallMatch) {
for (const raw of whenCallMatch[1].split(",")) {
for (const raw of splitTopLevelArgs(whenCallMatch[1])) {
const name = raw.trim();
if (name.length === 0 || name === "_") {
continue;
}
const typed = name.match(/^([A-Za-z_][A-Za-z0-9_]*)\s*:\s*(.+)$/);
if (typed && isTypeAnnotationText(typed[2])) {
bound.add(typed[1]);
continue;
}
if (/^[A-Za-z_][A-Za-z0-9_]*\??$/.test(name)) {
bound.add(name.replace(/\?$/, ""));
}
Expand Down Expand Up @@ -2487,6 +2492,128 @@ function collectRuleBoundNames(
return bound;
}

function splitTopLevelArgs(argsText: string): string[] {
const parts: string[] = [];
let start = 0;
let angleDepth = 0;
let parenDepth = 0;
let bracketDepth = 0;
let braceDepth = 0;
let stringQuote: string | undefined;
let escaped = false;

for (let i = 0; i < argsText.length; i += 1) {
const ch = argsText[i];

if (stringQuote) {
if (escaped) {
escaped = false;
} else if (ch === "\\") {
escaped = true;
} else if (ch === stringQuote) {
stringQuote = undefined;
}
continue;
}

if (ch === '"' || ch === "'") {
stringQuote = ch;
continue;
}

if (ch === "<") {
angleDepth += 1;
} else if (ch === ">" && angleDepth > 0) {
angleDepth -= 1;
} else if (ch === "(") {
parenDepth += 1;
} else if (ch === ")" && parenDepth > 0) {
parenDepth -= 1;
} else if (ch === "[") {
bracketDepth += 1;
} else if (ch === "]" && bracketDepth > 0) {
bracketDepth -= 1;
} else if (ch === "{") {
braceDepth += 1;
} else if (ch === "}" && braceDepth > 0) {
braceDepth -= 1;
} else if (
ch === "," &&
angleDepth === 0 &&
parenDepth === 0 &&
bracketDepth === 0 &&
braceDepth === 0
) {
parts.push(argsText.slice(start, i));
start = i + 1;
}
}

parts.push(argsText.slice(start));
return parts;
}

function isTypeAnnotationText(text: string): boolean {
const trimmed = text.trim();
if (trimmed.length === 0) {
return false;
}
if (trimmed.endsWith("?")) {
return isTypeAnnotationText(trimmed.slice(0, -1));
}

const generic = parseGenericTypeText(trimmed);
if (generic) {
const args = splitTopLevelArgs(generic.args).map((arg) => arg.trim());
return (
isTypeAnnotationText(generic.name) &&
args.length > 0 &&
args.every((arg) => arg.length > 0 && isTypeAnnotationText(arg))
);
}

return /^(?:[A-Za-z_][A-Za-z0-9_]*\/)?[A-Z][A-Za-z0-9_]*$/.test(trimmed);
}

function parseGenericTypeText(
text: string,
): { name: string; args: string } | undefined {
let depth = 0;
let genericStart = -1;
let genericEnd = -1;

for (let i = 0; i < text.length; i += 1) {
const ch = text[i];
if (ch === "<") {
if (depth === 0 && genericStart === -1) {
genericStart = i;
}
depth += 1;
} else if (ch === ">") {
depth -= 1;
if (depth < 0) {
return undefined;
}
if (depth === 0) {
genericEnd = i;
}
}
}

if (
depth !== 0 ||
genericStart === -1 ||
genericEnd !== text.length - 1
) {
return undefined;
}

return {
name: text.slice(0, genericStart).trim(),
args: text.slice(genericStart + 1, genericEnd).trim(),
};
}

function isInsideDoubleQuotedStringAtIndex(
text: string,
index: number,
Expand Down
34 changes: 34 additions & 0 deletions extensions/allium/test/analyzer.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -779,6 +779,40 @@ test("does not report binding defined by trigger parameter", () => {
);
});

test("does not report binding defined by typed trigger parameter", () => {
const findings = analyzeAllium(
`entity User {\n status: String\n}\n\nrule Notify {\n when: UserUpdated(user: User)\n requires: user.status = active\n ensures: Done()\n}\n`,
);
assert.equal(
findings.some((f) => f.code === "allium.rule.undefinedBinding"),
false,
);
});

test("does not report binding defined by generic typed trigger parameter", () => {
const findings = analyzeAllium(
`entity User {\n status: String\n}\n\nrule Notify {\n when: UsersUpdated(users: List<User?>, metadata: Map<String, String>)\n requires: users.count > 0\n requires: metadata.count > 0\n ensures: Done()\n}\n`,
);
assert.equal(
findings.some((f) => f.code === "allium.rule.undefinedBinding"),
false,
);
});

test("does not treat named literal trigger argument as binding", () => {
const findings = analyzeAllium(
`rule Notify {\n when: CommandInvoked(name: "npm run test")\n requires: name.status = active\n ensures: Done()\n}\n`,
);
assert.ok(findings.some((f) => f.code === "allium.rule.undefinedBinding"));
});

test("does not treat lowercase named trigger argument as typed binding", () => {
const findings = analyzeAllium(
`rule Notify {\n when: FieldSelected(field: course_name)\n requires: field.status = active\n ensures: Done()\n}\n`,
);
assert.ok(findings.some((f) => f.code === "allium.rule.undefinedBinding"));
});

test("does not report binding defined in context block", () => {
const findings = analyzeAllium(
`entity User {\n status: String\n}\n\ngiven {\n user: User\n}\n\nrule Notify {\n when: Ping()\n requires: user.status = active\n ensures: Done()\n}\n`,
Expand Down
Loading