Skip to content
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

Fixed bug - passing parameter values #189

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+113 −14
Open

Fixed bug - passing parameter values #189

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
97af467
Fixed bug - passing parameter values
Srihari123456 Jun 7, 2022
3157e49
Addressed Comments in PR
Srihari123456 Jun 15, 2022
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
126 changes: 105 additions & 21 deletions javascript/src/refactor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,15 +136,13 @@ class RefactorEngine {
if (func.body === null || func.body.body == null) {
return false;
}

const returnIndex = func.body.body.length - 1;

if (returnIndex < 0)
// If returnIndex == -1 when function body is empty
return false;
else {
var returnNode = func.body.body[returnIndex];

if (
returnNode.type === 'ReturnStatement' &&
returnNode.argument !== null &&
Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dangling comments?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll remove this in the updated commit

engine.changed = true;

if (
(flagType === 'treated' && engine.behaviour) ||
(flagType === 'treated' && engine.behaviour) || // engine.behaviour = true => treated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dangling comments?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll remove this in the updated commit

(flagType === 'control' && !engine.behaviour)
) {
return engine.trueLiteral();
Expand All @@ -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.
});
}

Expand All @@ -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') {
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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) {
Expand All @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dangling comments?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll remove this in the updated commit

}
}
} else if (node.type === 'AssignmentExpression') {
Expand All @@ -478,11 +476,39 @@ class RefactorEngine {
return assignments;
}

adjustFunctionParams(parameterList) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we have some comments here - what is adjustFunctionParams doing?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes added necessary comments

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 &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move this check outside of this for loop ? From what i understand, you are checking if the parent function declaration name is same as the function name of parameter_list right? Maybe this could be like the first line inside enter ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also do we also need to check that parameterList.length == parent.params.length ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ketkarameya parameterList contains the details of the parameters to be removed across all the functions in the file. On the other hand, parent.params denotes only the arguments of a specific function declaration. Hence parameterList.length need not be same as parent.params.length

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then how do we know that parent is the correct method declaration? Will this handle overriden methods?

Copy link
Author

@Srihari123456 Srihari123456 Jun 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move this check outside of this for loop ? From what i understand, you are checking if the parent function declaration name is same as the function name of parameter_list right? Maybe this could be like the first line inside enter ?

parameterList is a list of objects, each of which has functionName, parameterIndex, value. So even if its brought under enter, we need to iterate over the list of objects available in parameterList

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got it.

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) {
Expand All @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simplify if-else-if to if-else or ternary.
return assignments[node.name] === true ? engine.trueLiteral() : engine.falseLiteral() ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ketkarameya the value of assignments[node.name] can be true or false or undefined. Thats why I didnt code it in if-else. Anyways I'll incorporate this suggestion and refine the code

engine.changed = true;
return engine.trueLiteral();
} else if (assignments[node.name] === false) {
engine.changed = true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should engine.changed be false here? if not, can we pull out the two assignments outside the if statement

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

engine.changed must be true here. I'll refine the code such that it isnt redundant

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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we have some comments on what this function is doing?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added necessary comments

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;
Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dangling comment?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll remove this in the updated commit

numReturns[current] = 0;
} else if (node.type === 'FunctionExpression') {
if (parent.type === 'VariableDeclarator') {
Expand Down Expand Up @@ -567,7 +651,6 @@ class RefactorEngine {
singleReturn[fun] = 1;
}
}

return singleReturn;
}

Expand Down Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stale code? could be removed!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll remove this in the updated commit

this.finalizeLiterals();

if (this.print_to_console) {
Expand Down