From 97af467dcd108d90fa60fcc905d8f6bd709c6da1 Mon Sep 17 00:00:00 2001 From: Srihari S Date: Tue, 7 Jun 2022 22:37:34 +0530 Subject: [PATCH 1/2] Fixed bug - passing parameter values --- javascript/src/refactor.js | 126 ++++++++++++++++++++++++++++++------- 1 file changed, 105 insertions(+), 21 deletions(-) diff --git a/javascript/src/refactor.js b/javascript/src/refactor.js index 7e0060a69..520d7e5d6 100644 --- a/javascript/src/refactor.js +++ b/javascript/src/refactor.js @@ -136,7 +136,6 @@ class RefactorEngine { if (func.body === null || func.body.body == null) { return false; } - const returnIndex = func.body.body.length - 1; if (returnIndex < 0) @@ -144,7 +143,6 @@ class RefactorEngine { return false; else { var returnNode = func.body.body[returnIndex]; - if ( returnNode.type === 'ReturnStatement' && returnNode.argument !== null && @@ -322,24 +320,23 @@ class RefactorEngine { enter: function (node) { if (node.type === 'CallExpression') { if (methodHashMap.has(node.callee.name)) { - const argumentIndex = methodHashMap.get(node.callee.name).argumentIndex; - const nodeArgument = node.arguments[argumentIndex]; - + const argumentIndex = methodHashMap.get(node.callee.name).argumentIndex; // 0 + const nodeArgument = node.arguments[argumentIndex]; // node.arguments[0] = 'featureFlag' || featureFlag let nodeArgumentIsFlag = false; switch (nodeArgument.type) { - case 'Identifier': - nodeArgumentIsFlag = nodeArgument.name === engine.flagname; + case 'Identifier': // node.arguments[0] = featureFlag + nodeArgumentIsFlag = nodeArgument.name === engine.flagname; // checks if the flag under consideration is the required flag break; - case 'Literal': + case 'Literal': // node.arguments[0] = 'featureFlag' nodeArgumentIsFlag = nodeArgument.value === engine.flagname; break; } if (nodeArgumentIsFlag) { - const flagType = methodHashMap.get(node.callee.name).flagType; + const flagType = methodHashMap.get(node.callee.name).flagType; // control || treated engine.changed = true; if ( - (flagType === 'treated' && engine.behaviour) || + (flagType === 'treated' && engine.behaviour) || // engine.behaviour = true => treated (flagType === 'control' && !engine.behaviour) ) { return engine.trueLiteral(); @@ -352,6 +349,8 @@ class RefactorEngine { }, fallback: 'iteration', // Ignore nodes in the AST that estraverse does not recognize + // By passing visitor.fallback option, we can control the behavior when encountering unknown nodes. + // Iterating the child **nodes** of unknown nodes. }); } @@ -361,7 +360,6 @@ class RefactorEngine { // NOT true -> false, NOT false -> true evalBoolExpressions() { var engine = this; - estraverse.replace(this.ast, { leave: function (node) { if (node.type === 'LogicalExpression') { @@ -409,10 +407,10 @@ class RefactorEngine { engine.isPiranhaLiteral(node.test) ) { if (node.test.value === true) { + // If statement is true // node.consequent is always non-null so no check required engine.changed = true; engine.moveCommentsToConsequent(node); - return node.consequent; } else if (node.test.value === false) { if (node.alternate == null) { @@ -448,9 +446,9 @@ class RefactorEngine { // var foo = cond -> _ // where cond evaluates to a Boolean literal in a previous step getRedundantVarnames() { + // Makes the assignment between literal and value var assignments = {}; var engine = this; - // Get a list of variable names which are assigned to a boolean literal estraverse.traverse(this.ast, { enter: function (node) { @@ -462,7 +460,7 @@ class RefactorEngine { engine.isPiranhaLiteral(declaration.init) && typeof declaration.init.value === 'boolean' ) { - assignments[declaration.id.name] = declaration.init.value; + assignments[declaration.id.name] = declaration.init.value; // Assigning a -> true } } } else if (node.type === 'AssignmentExpression') { @@ -478,11 +476,39 @@ class RefactorEngine { return assignments; } + adjustFunctionParams(parameterList) { + var engine = this; + var assignments = {}; + estraverse.replace(this.ast, { + enter: function (node, parent) { + if (node.type === 'Identifier' && parent.type === 'FunctionDeclaration') { + for (let i = 0; i < parent.params.length; i++) { + if (node.name === parent.params[i].name) { + for (let j = 0; j < parameterList.length; j++) { + if ( + parameterList[j].functionName === parent.id.name && + parameterList[j].parameterIdx === i + ) { + assignments[parent.params[i].name] = parameterList[j].value; + engine.pruneVarReferencesinFunc(parent, assignments); + engine.changed = true; + if (parameterList[j].value === true) return engine.trueLiteral(); + return engine.falseLiteral(); + } + } + } + } + } + }, + + fallback: 'iteration', + }); + } + // Remove all variable declarations corresponding variables in `assignments` // Replace all references of variables in `assignments` with the corresponding literal - pruneVarReferences(assignments) { + pruneVarReferences(assignments, parameterList) { var engine = this; - // Remove redundant variables by deleting declarations and replacing variable references estraverse.replace(this.ast, { enter: function (node, parent) { @@ -498,6 +524,64 @@ class RefactorEngine { return this.remove(); } } else if (node.type === 'Identifier') { + // this is the part where the function arguments are substituted values if they are a part of the assignments + if (node.name in assignments) { + if (assignments[node.name] === true) { + engine.changed = true; + return engine.trueLiteral(); + } else if (assignments[node.name] === false) { + engine.changed = true; + return engine.falseLiteral(); + } + } + } else if (node.type === 'CallExpression') { + for (let i = 0; i < node.arguments.length; i++) { + let argValue; + if (node.arguments[i].createdByPiranha) { + argValue = node.arguments[i].value; + parameterList.push({ + functionName: node.callee.name, + parameterIdx: i, + value: argValue, + }); + } + } + } + }, + + // After previous step, some declaration may have no declarators, delete them. + leave: function (node, parent) { + if (node.type === 'VariableDeclaration') { + if (node.declarations.length === 0) { + engine.preserveCommentsBasedOnOption(node, parent, engine.keep_comments); + engine.changed = true; + return this.remove(); + } + } + }, + + fallback: 'iteration', + }); + return parameterList; + } + + pruneVarReferencesinFunc(nn, assignments) { + var engine = this; + estraverse.replace(nn, { + enter: function (node, parent) { + if (node.type === 'VariableDeclarator') { + if (node.id.name in assignments) { + engine.changed = true; + return this.remove(); + } + } else if (node.type === 'ExpressionStatement' && node.expression.type === 'AssignmentExpression') { + if (node.expression.left.name in assignments) { + engine.preserveCommentsBasedOnOption(node, parent, engine.keep_comments); + engine.changed = true; + return this.remove(); + } + } else if (node.type === 'Identifier') { + // this is the part where the function arguments are substituted values if they are a part of the assignments if (node.name in assignments) { if (assignments[node.name] === true) { engine.changed = true; @@ -536,7 +620,7 @@ class RefactorEngine { estraverse.traverse(this.ast, { enter: function (node, parent) { if (node.type === 'FunctionDeclaration') { - current = node.id.name; + current = node.id.name; // name of the function numReturns[current] = 0; } else if (node.type === 'FunctionExpression') { if (parent.type === 'VariableDeclarator') { @@ -567,7 +651,6 @@ class RefactorEngine { singleReturn[fun] = 1; } } - return singleReturn; } @@ -679,15 +762,16 @@ class RefactorEngine { this.reduceIfStatements(); var redundantVarnames = this.getRedundantVarnames(); - this.pruneVarReferences(redundantVarnames); - + let parameterList = []; + this.pruneVarReferences(redundantVarnames, parameterList); + this.adjustFunctionParams(parameterList); var functionsWithSingleReturn = this.getFunctionsWithSingleReturn(); var redundantFunctions = this.getRedundantFunctions(functionsWithSingleReturn); this.pruneFuncReferences(redundantFunctions); iterations++; } - + console.log(this.changed); this.finalizeLiterals(); if (this.print_to_console) { From 3157e4916a08422b2868815c97f46534d4509e57 Mon Sep 17 00:00:00 2001 From: Srihari S Date: Wed, 15 Jun 2022 10:53:56 +0530 Subject: [PATCH 2/2] Addressed Comments in PR --- javascript/src/refactor.js | 51 ++++++++++++++++++++++++-------------- 1 file changed, 33 insertions(+), 18 deletions(-) diff --git a/javascript/src/refactor.js b/javascript/src/refactor.js index 520d7e5d6..25c7a6b31 100644 --- a/javascript/src/refactor.js +++ b/javascript/src/refactor.js @@ -107,7 +107,7 @@ class RefactorEngine { // Verify this is a literal introduced by Piranha. // We only refactor code associated with literals having this property and leave original code untouched isPiranhaLiteral(node) { - return node.type === 'Literal' && node.createdByPiranha !== undefined; + return node != null && node.type === 'Literal' && node.createdByPiranha !== undefined; } reduceLogicalExpression(literal, expression, operator) { @@ -320,23 +320,23 @@ class RefactorEngine { enter: function (node) { if (node.type === 'CallExpression') { if (methodHashMap.has(node.callee.name)) { - const argumentIndex = methodHashMap.get(node.callee.name).argumentIndex; // 0 - const nodeArgument = node.arguments[argumentIndex]; // node.arguments[0] = 'featureFlag' || featureFlag + const argumentIndex = methodHashMap.get(node.callee.name).argumentIndex; + const nodeArgument = node.arguments[argumentIndex]; let nodeArgumentIsFlag = false; switch (nodeArgument.type) { - case 'Identifier': // node.arguments[0] = featureFlag - nodeArgumentIsFlag = nodeArgument.name === engine.flagname; // checks if the flag under consideration is the required flag + case 'Identifier': + nodeArgumentIsFlag = nodeArgument.name === engine.flagname; break; - case 'Literal': // node.arguments[0] = 'featureFlag' + case 'Literal': nodeArgumentIsFlag = nodeArgument.value === engine.flagname; break; } if (nodeArgumentIsFlag) { - const flagType = methodHashMap.get(node.callee.name).flagType; // control || treated + const flagType = methodHashMap.get(node.callee.name).flagType; engine.changed = true; if ( - (flagType === 'treated' && engine.behaviour) || // engine.behaviour = true => treated + (flagType === 'treated' && engine.behaviour) || (flagType === 'control' && !engine.behaviour) ) { return engine.trueLiteral(); @@ -460,7 +460,7 @@ class RefactorEngine { engine.isPiranhaLiteral(declaration.init) && typeof declaration.init.value === 'boolean' ) { - assignments[declaration.id.name] = declaration.init.value; // Assigning a -> true + assignments[declaration.id.name] = declaration.init.value; } } } else if (node.type === 'AssignmentExpression') { @@ -476,22 +476,24 @@ class RefactorEngine { return assignments; } + // This function helps in passing the refactored value of a parameter in the function call to the corresponding argument in the function declaration adjustFunctionParams(parameterList) { var engine = this; var assignments = {}; estraverse.replace(this.ast, { enter: function (node, parent) { if (node.type === 'Identifier' && parent.type === 'FunctionDeclaration') { + // this is the part where the function arguments are substituted values if they are a part of the assignments for (let i = 0; i < parent.params.length; i++) { if (node.name === parent.params[i].name) { for (let j = 0; j < parameterList.length; j++) { if ( parameterList[j].functionName === parent.id.name && + parent.params.length === parameterList[j].totalParameters && parameterList[j].parameterIdx === i ) { assignments[parent.params[i].name] = parameterList[j].value; engine.pruneVarReferencesinFunc(parent, assignments); - engine.changed = true; if (parameterList[j].value === true) return engine.trueLiteral(); return engine.falseLiteral(); } @@ -505,6 +507,21 @@ class RefactorEngine { }); } + // this function removes the literals after the refactoring is terminated. + // f1(true) -> a function call becomes f1() + removeLiteralParameters() { + // var engine = this; + estraverse.replace(this.ast, { + enter: function (node, parent) { + if (node.type === 'Literal' && node.createdByPiranha === true) { + return this.remove(); + } + }, + + fallback: 'iteration', + }); + } + // Remove all variable declarations corresponding variables in `assignments` // Replace all references of variables in `assignments` with the corresponding literal pruneVarReferences(assignments, parameterList) { @@ -526,12 +543,9 @@ class RefactorEngine { } else if (node.type === 'Identifier') { // this is the part where the function arguments are substituted values if they are a part of the assignments if (node.name in assignments) { - if (assignments[node.name] === true) { - engine.changed = true; - return engine.trueLiteral(); - } else if (assignments[node.name] === false) { + if (assignments[node.name] === true || assignments[node.name] === false) { engine.changed = true; - return engine.falseLiteral(); + return assignments[node.name] === true ? engine.trueLiteral() : engine.falseLiteral(); } } } else if (node.type === 'CallExpression') { @@ -543,6 +557,7 @@ class RefactorEngine { functionName: node.callee.name, parameterIdx: i, value: argValue, + totalParameters: node.arguments.length, }); } } @@ -565,6 +580,7 @@ class RefactorEngine { return parameterList; } + // The references of the refactored function parameters, within the function are pruned with their new refactored value in this function pruneVarReferencesinFunc(nn, assignments) { var engine = this; estraverse.replace(nn, { @@ -620,7 +636,7 @@ class RefactorEngine { estraverse.traverse(this.ast, { enter: function (node, parent) { if (node.type === 'FunctionDeclaration') { - current = node.id.name; // name of the function + current = node.id.name; numReturns[current] = 0; } else if (node.type === 'FunctionExpression') { if (parent.type === 'VariableDeclarator') { @@ -768,10 +784,9 @@ class RefactorEngine { var functionsWithSingleReturn = this.getFunctionsWithSingleReturn(); var redundantFunctions = this.getRedundantFunctions(functionsWithSingleReturn); this.pruneFuncReferences(redundantFunctions); - iterations++; } - console.log(this.changed); + this.removeLiteralParameters(); this.finalizeLiterals(); if (this.print_to_console) {