aglinxinyuan commented on code in PR #8640: URL: https://github.com/apache/texera/pull/8640#discussion_r4213850597
########## frontend/src/app/workspace/util/loop-variable.util.ts: ########## @@ -0,0 +1,343 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { WorkflowGraphReadonly } from "../service/workflow-graph/model/workflow-graph"; + +/** + * A plain assignment target: an optional star, an identifier and an optional annotation (`x: int`). + * Identifiers are ASCII because a `$name` reference can only spell ASCII names. + */ +const ASSIGNMENT_TARGET = /^\*?\s*([A-Za-z_][A-Za-z0-9_]*)\s*(?::[^=]*)?$/; + +/** The characters that, right before `=`, make it part of another operator (`==`, `+=`, `<=`, `:=`, ...). */ +const NOT_AN_ASSIGNMENT_BEFORE = "=!<>+-*/%&|^:@"; + +/** Python's keywords, which are never a name (`else: y = 1` must not yield `else`). */ +const PYTHON_KEYWORDS: ReadonlySet<string> = new Set([ + "False", + "None", + "True", + "and", + "as", + "assert", + "async", + "await", + "break", + "class", + "continue", + "def", + "del", + "elif", + "else", + "except", + "finally", + "for", + "from", + "global", + "if", + "import", + "in", + "is", + "lambda", + "nonlocal", + "not", + "or", + "pass", + "raise", + "return", + "try", + "while", + "with", + "yield", +]); + +/** The head of a statement that opens a def or class body, whose names are local to it. */ +const LOCAL_SCOPE_HEADER = /^(?:async\s+)?(?:def|class)\b/; + +/** The head of a compound statement whose body runs at module level; group 1 is the keyword. */ +const COMPOUND_HEADER = /^(?:async\s+)?(if|elif|else|while|for|with|try|except|finally)\b/; + +const OPENING = "([{"; +const CLOSING = ")]}"; + +interface Statement { + text: string; + /** Whether the statement begins a line (as opposed to following a `;`), so that indentation is telling. */ + startsLine: boolean; +} + +/** + * The names a Loop Start's "initialization" Python code binds at module level, in first-binding order and + * without duplicates: the loop's variables, which the operators of its block may refer to as `$name`. + * The backend runs the code with exec and keeps the resulting globals as the loop state, and Python has + * no block scope, so these are the assignments at any depth outside a def or class body, whether at the + * top level or under an if/elif/else, try/except/finally, while, for or with, plus the targets of a for + * loop. + * + * Assignments: `i = 0`, `K = 2; prev = float('-inf')`, tuple targets (`a, b = 1, 2`), chained + * (`a = b = 0`), annotated (`x: int = 0`) and starred (`a, *rest = xs`) ones. Augmented assignments, + * comparisons, keyword arguments, attribute or subscript targets and comments are not assignments of a + * loop variable and are ignored, as are the names an import, a def, a class, `with ... as` or Review Comment: Fixed in 1f076c83ee — names bound by `from ... import` (aliases, relative modules, parenthesized lists) and by `import ... as` now count, so `$pi` works. ########## frontend/src/app/workspace/service/validation/validation-workflow.service.ts: ########## @@ -280,15 +319,39 @@ export class ValidationWorkflowService { throw new Error("operatorSchema doesn't exist"); } - const isValid = this.ajv.validate(operatorSchema.jsonSchema, operator.operatorProperties); - if (isValid) { - return { isValid: true }; + const properties = operator.operatorProperties; + const isValid = this.ajv.validate(operatorSchema.jsonSchema, properties); + let errors = isValid ? [] : this.ajv.errors ?? []; + const validationError: Record<string, string> = {}; + + // Inside a control block a primitive property may hold a loop-variable reference ("$K") that the + // backend binds at run time (issue #8635), so the schema type check is waived exactly where a + // reference sits in a property whose schema type is a primitive (the fields the property panel lets + // take one), and every reference must name a variable an enclosing Loop Start declares. Outside every + // block nothing is waived: a "$K" in an integer property stays a type error, and so does one in an + // array or object property anywhere. + const namesInScope = loopVariablesInScope(this.workflowActionService.getTexeraGraph(), operatorID); + if (namesInScope !== undefined) { + errors = errors.filter( + error => + !( + error.keyword === "type" && + primitiveSchemaType(error.params["type"]) !== undefined && + isReference(valueAtPointer(properties, error.instancePath)) + ) + ); + const validateReference = referenceValidator(namesInScope); + const undeclared = collectReferences(properties) + .map(reference => validateReference(`$${reference.name}`)) + .find(message => message !== undefined); + if (undeclared !== undefined) { + validationError["loopVariable"] = undeclared; Review Comment: Done in a21f5aec4a — an unknown name is now a warning under the setting, and Run stays enabled. ########## frontend/src/app/workspace/component/property-editor/operator-property-edit-frame/operator-property-edit-frame.component.ts: ########## @@ -960,6 +1017,25 @@ export class OperatorPropertyEditFrameComponent implements OnInit, OnChanges, On mappedField.type = customType; } + // Inside a control block a primitive property may hold a loop-variable reference ("$K"), which the + // backend binds at run time (issue #8635): render it as a text input that takes "$K" next to plain + // values, offer the variables the enclosing Loop Starts declare, and flag a name none declares. + // Enums and custom widgets keep their controls; `mappedField.type` still equal to the schema type + // is what tells a plain primitive from a field formly or a widget already resolved otherwise. A + // field with value rules is a primitive too, on the rules' control (setValueRules above): the text + // input replaces that control, keeps its rules for every value but a reference, and offers the + // values they accept next to the variables. + const loopSchemaType = primitiveSchemaType(mapSource.type); + if ( + loopVariableNames !== undefined && + customType === undefined && + loopSchemaType !== undefined && + (mappedField.type === loopSchemaType || isDefined(mapSource.valueRules)) && + !isDefined(mapSource.enum) + ) { + applyLoopVariableField(mappedField, loopSchemaType, loopVariableNames); Review Comment: Done in d6659b1253 — the panel doesn't offer `$i` in a setting that #8750 marks `"noLoopVariable": true` in the schema, or in anything nested under it. ########## frontend/src/app/workspace/service/validation/validation-workflow.service.ts: ########## @@ -227,6 +251,21 @@ export class ValidationWorkflowService { this.updateValidationState(value.operator.operatorID, this.validateOperator(value.operator.operatorID)) ); + // An operator holding a loop-variable reference is judged against the block around it and the + // variables its Loop Starts declare, which other operators' links and properties decide: a link two + // hops away can close or open its block, and renaming a Loop Start's variable can orphan "$K". So + // after every link added or removed, and every property change of a Loop Start, re-validate each Review Comment: Gone in aa22008edb — both now re-check on each compile result, so neither keeps a list of edits. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
