Conversation
There was a problem hiding this comment.
Pull request overview
Adds support for treating evalText() like an eval()-style built-in evaluation function in the dataflow engine, and extends slicing test coverage accordingly.
Changes:
- Register
evalTextas a built-in handled by the existingEvalprocessor. - Extend eval-string resolution logic to optionally accept direct string/symbol/function-call arguments (not only
parse(text=...)). - Add backward slicing tests covering
evalText()usage patterns (direct, indirect, conditional string selection, etc.).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| test/functionality/slicing/backward/static-backward-program-slices/eval.test.ts | Adds new slicing tests for evalText() scenarios. |
| src/dataflow/internal/process/functions/call/built-in/built-in-eval.ts | Extends eval code-string resolution to support direct arguments when configured. |
| src/dataflow/environments/default-builtin-config.ts | Registers evalText to be processed by the built-in eval handler with the new flag enabled. |
Comments suppressed due to low confidence (1)
src/dataflow/internal/process/functions/call/built-in/built-in-eval.ts:53
processEvalCallstill bails out unlessargs.length === 1. This meansevalText("...", env(...))(andeval(expr, envir, ...)) will skip string evaluation entirely, reducing slicing precision and emitting a misleading warning. Consider accepting 1+ arguments (use the first as the code expression, optionally interpret/ignore the rest), and update the warning to include the actual function name (e.g.,evalvsevalText) and the supported signature.
): DataflowInformation {
if(args.length !== 1 || args[0] === EmptyArgument || !args[0].value) {
dataflowLogger.warn(`Expected exactly one argument for eval currently, but got ${args.length} instead, skipping`);
return processKnownFunctionCall({ name, args, rootId, data, origin: 'default' }).information;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| assertSliced(label('simple evalText use', ['name-normal', ...OperatorDatabase['<-'].capabilities, 'numbers', 'unnamed-arguments', 'strings', 'built-in-evaluation', 'newlines']), | ||
| shell, 'foo <- function(x) { return (x+2) }\ny <- evalText("foo(4)*1")', ['2@y'], 'foo <- function(x) return (x+2)\ny <- evalText("foo(4)*1")'); | ||
| assertSliced(label('simple evalText no-use', ['name-normal', ...OperatorDatabase['<-'].capabilities, 'numbers', 'unnamed-arguments', 'strings', 'built-in-evaluation', 'newlines']), | ||
| shell, 'x <- 2\ny <- evalText("print(x)", env(x=42))', ['2@y'], 'y <- evalText("print(x)", env(x=42))'); |
There was a problem hiding this comment.
The evalText("print(x)", env(x=42)) test currently passes even if evalText string-evaluation is skipped when more than one argument is provided (because the string literal doesn’t create dataflow deps). To actually validate multi-argument evalText support, make the evaluated text reference an outer variable (e.g., y <- 1; z <- evalText("y+1", env(x=42))) and assert the slice retains y (and still drops unrelated bindings).
| shell, 'x <- 2\ny <- evalText("print(x)", env(x=42))', ['2@y'], 'y <- evalText("print(x)", env(x=42))'); | |
| shell, 'x <- 2\ny <- 1\nz <- evalText("y+1", env(x=42))', ['3@z'], 'y <- 1\nz <- evalText("y+1", env(x=42))'); |
There was a problem hiding this comment.
This is another fair thing, but for now this is fine :D
| function resolveEvalToCode<OtherInfo>(evalArgument: RNode<OtherInfo & ParentInformation>, env: REnvironmentInformation, idMap: AstIdMap, ctx: ReadOnlyFlowrAnalyzerContext, supportFunctionCall: boolean | undefined): string[] | undefined { | ||
| const val = evalArgument; | ||
|
|
||
| let arg; |
There was a problem hiding this comment.
resolveEvalToCode declares let arg; without a type/initializer. With tsconfig.json strict: true, this is an implicit any and should fail TypeScript compilation. Please give arg an explicit type (e.g., RNode<...> | undefined) or refactor to avoid an untyped temporary (separate variables for the parse argument vs. direct argument).
| let arg; | |
| let arg: RNode<OtherInfo & ParentInformation> | undefined; |
There was a problem hiding this comment.
I am with copilot here, could you please add typing to arg
| } | ||
| }, assumePrimitive: true }, | ||
| { type: 'function', names: ['eval'], processor: BuiltInProcName.Eval, config: { includeFunctionCall: true }, assumePrimitive: true }, | ||
| { type: 'function', names: ['evalText'], processor: BuiltInProcName.Eval, config: { includeFunctionCall: true, supportFunctionCall: true }, assumePrimitive: true }, |
There was a problem hiding this comment.
great, this is the configuration how I wanted it :D, but could you please add a supportF...: false for the old eval just to make it explicit?
| /** should this produce an explicit source function call in the graph? */ | ||
| includeFunctionCall?: boolean | ||
| includeFunctionCall?: boolean, | ||
| supportFunctionCall?: boolean |
There was a problem hiding this comment.
please also add a short comment explaining the configuration option
| } | ||
|
|
||
| const code: string[] | undefined = resolveEvalToCode(evalArgument.value as RNode<ParentInformation>, data.environment, data.completeAst.idMap, data.ctx); | ||
| const code: string[] | undefined = resolveEvalToCode(evalArgument.value as RNode<ParentInformation>, data.environment, data.completeAst.idMap, data.ctx, config.supportFunctionCall); |
There was a problem hiding this comment.
given the method is already very complicated with the parameter count, could you please rerwite resolveEvalToCode so that all of the data.* arguments are merged into one? I want to call it like:
resolveEvalToCode(evalArgument.value..., data, config)| function resolveEvalToCode<OtherInfo>(evalArgument: RNode<OtherInfo & ParentInformation>, env: REnvironmentInformation, idMap: AstIdMap, ctx: ReadOnlyFlowrAnalyzerContext, supportFunctionCall: boolean | undefined): string[] | undefined { | ||
| const val = evalArgument; | ||
|
|
||
| let arg; |
There was a problem hiding this comment.
I am with copilot here, could you please add typing to arg
| const nArg = val.arguments.find(v => v !== EmptyArgument && v.name?.content === 'n'); | ||
| if(nArg !== undefined || arg === undefined || arg === EmptyArgument) { | ||
| return undefined; | ||
| supportFunctionCall || val.type === RType.FunctionCall && val.named && val.functionName.content === 'parse') { |
There was a problem hiding this comment.
ok so the or here should be wrong because if you have evalText(foo()) this now triggers because supportFunctionCall is true then. Generally, I think you inverted the logic or eval and evalText.
Generally, I want
eval(parse(text="x <- 2"))andevalText("x <- 2")should both work and flowR should recognize the assigment toxeval("x <- 2")and likewiseevalText(parse(text="x <- 2"))to fail!
Theevaljust evaluates to the string"x <- 2"and theevalText(afaik, you can also check that in your repl) expects the string
Both branches are there... this branch supports the parse case, while the one below checking for a string handles the "x <- 2" case. The flag should control that for eval, only the parse branch is active, while for evalText, just the "..." branch is active.
| return [arg.value.content.str]; | ||
| } else if(arg.value?.type === RType.Symbol) { | ||
| const resolved = valueSetGuard(resolveIdToValue(arg.value.info.id, { environment: env, idMap: idMap, resolve: ctx.config.solver.variables, ctx })); | ||
| if(arg?.type === RType.String) { |
There was a problem hiding this comment.
why did you drop the value? this should now fail if the arguments are named as the type of a named argument is RType.Argument and no longer RType.String
| assertSliced(label('simple evalText use', ['name-normal', ...OperatorDatabase['<-'].capabilities, 'numbers', 'unnamed-arguments', 'strings', 'built-in-evaluation', 'newlines']), | ||
| shell, 'foo <- function(x) { return (x+2) }\ny <- evalText("foo(4)*1")', ['2@y'], 'foo <- function(x) return (x+2)\ny <- evalText("foo(4)*1")'); | ||
| assertSliced(label('simple evalText no-use', ['name-normal', ...OperatorDatabase['<-'].capabilities, 'numbers', 'unnamed-arguments', 'strings', 'built-in-evaluation', 'newlines']), | ||
| shell, 'x <- 2\ny <- evalText("print(x)", env(x=42))', ['2@y'], 'y <- evalText("print(x)", env(x=42))'); |
There was a problem hiding this comment.
This is another fair thing, but for now this is fine :D
| shell, 'y <- 1\nfoo <- function(x) { return (evalText("x+y")) }\nt <- 3\nz <- evalText("foo(4)*2+t")', ['4@z'], 'y <- 1\nfoo <- function(x) return (evalText("x+y"))\nt <- 3\nz <- evalText("foo(4)*2+t")'); | ||
| assertSliced(label('mixed evalText/eval call', ['name-normal', ...OperatorDatabase['<-'].capabilities, 'numbers', 'unnamed-arguments', 'strings', 'built-in-evaluation', 'newlines']), | ||
| shell, 'y <- 1\nfoo <- function(x) { return (eval(parse(text ="x+y"))) }\nt <- 3\nz <- evalText("foo(4)*2+t")', ['4@z'], 'y <- 1\nfoo <- function(x) return (eval(parse(text ="x+y")))\nt <- 3\nz <- evalText("foo(4)*2+t")'); | ||
| assertSliced(label('simple evalText conditional string', ['name-normal', ...OperatorDatabase['<-'].capabilities, 'numbers', 'unnamed-arguments', 'strings', 'built-in-evaluation', 'newlines']), |
There was a problem hiding this comment.
The slicing tests are ok, but there are still Dataflow tests missing - could you please add ~1-2 to ensure that the df dependencies are also correct?
No description provided.