From 6e0820bf5557ffcfc7b2a5c7e34961b8351f3dbb Mon Sep 17 00:00:00 2001 From: Justin Mclean Date: Sun, 1 Nov 2015 19:18:32 +1100 Subject: [PATCH 1/5] Add tests for rule to check for unused function arguments --- .../com/cflint/TestUnusedArgumentChecker.java | 230 ++++++++++++++++++ 1 file changed, 230 insertions(+) create mode 100644 src/test/java/com/cflint/TestUnusedArgumentChecker.java diff --git a/src/test/java/com/cflint/TestUnusedArgumentChecker.java b/src/test/java/com/cflint/TestUnusedArgumentChecker.java new file mode 100644 index 000000000..7b4c1a7a1 --- /dev/null +++ b/src/test/java/com/cflint/TestUnusedArgumentChecker.java @@ -0,0 +1,230 @@ +package com.cflint; + +import static org.junit.Assert.assertEquals; + +import java.io.IOException; +import java.util.List; +import java.util.Map; + +import org.junit.Before; +import org.junit.Test; + +import cfml.parsing.reporting.ParseException; + +import com.cflint.config.CFLintPluginInfo.PluginInfoRule; +import com.cflint.config.CFLintPluginInfo.PluginInfoRule.PluginMessage; +import com.cflint.config.ConfigRuntime; +import com.cflint.plugins.core.UnusedArgumentChecker; + +public class TestUnusedArgumentChecker { + + private CFLint cfBugs; + + @Before + public void setUp() { + final ConfigRuntime conf = new ConfigRuntime(); + final PluginInfoRule pluginRule = new PluginInfoRule(); + pluginRule.setName("UnusedArgumentChecker"); + conf.getRules().add(pluginRule); + final PluginMessage pluginMessage = new PluginMessage("UNUSED_METHOD_ARGUMENT"); + pluginMessage.setSeverity("INFO"); + cfBugs = new CFLint(conf, new UnusedArgumentChecker()); + } + + @Test + public void testNoArgumentsInScript() throws ParseException, IOException { + final String scriptSrc = "\r\n" + + "component {\r\n" + + "function dummyFunction() {\r\n" + + "}\r\n" + + "}\r\n" + + ""; + + cfBugs.process(scriptSrc, "test"); + final Map> result = cfBugs.getBugs().getBugList(); + assertEquals(0, result.size()); + } + + @Test + public void testArgumentsUsedInScript() throws ParseException, IOException { + final String scriptSrc = "\r\n" + + "component {\r\n" + + "function sum(a,b) {\r\n" + + "return a + b;\r\n" + + "}\r\n" + + "}\r\n" + + ""; + + cfBugs.process(scriptSrc, "test"); + final Map> result = cfBugs.getBugs().getBugList(); + assertEquals(0, result.size()); + } + + @Test + public void testArgumentsNotUsedInScript() throws ParseException, IOException { + final String scriptSrc = "\r\n" + + "component {\r\n" + + "function sum(a,b) {\r\n" + + "c = a + b;" + + "return c;\r\n" + + "}\r\n" + + "}\r\n" + + ""; + + cfBugs.process(scriptSrc, "test"); + final Map> result = cfBugs.getBugs().getBugList(); + assertEquals(0, result.size()); + } + + @Test + public void testArgumentsNotUsedInMethodScript() throws ParseException, IOException { + final String scriptSrc = "\r\n" + + "component {\r\n" + + "function sum(a,b) {\r\n" + + "return other(a,b);\r\n" + + "}\r\n" + + "}\r\n" + + ""; + + cfBugs.process(scriptSrc, "test"); + final Map> result = cfBugs.getBugs().getBugList(); + assertEquals(0, result.size()); + } + + @Test + public void testArgumentsNotUsedInReturnScript() throws ParseException, IOException { + final String scriptSrc = "\r\n" + + "component {\r\n" + + "function sum(a,b) {\r\n" + + "return a;\r\n" + + "}\r\n" + + "}\r\n" + + ""; + + cfBugs.process(scriptSrc, "test"); + final List result = cfBugs.getBugs().getBugList().values().iterator().next(); + assertEquals(1, result.size()); + assertEquals("UNUSED_METHOD_ARGUMENT", result.get(0).getMessageCode()); + assertEquals(3, result.get(0).getLine()); + } + + @Test + public void testMultipleArgumentsNotUsedScript() throws ParseException, IOException { + final String scriptSrc = "\r\n" + + "component {\r\n" + + "function sum(a,b,c,d) {\r\n" + + "return a;\r\n" + + "}\r\n" + + "}\r\n" + + ""; + + cfBugs.process(scriptSrc, "test"); + final List result = cfBugs.getBugs().getBugList().values().iterator().next(); + assertEquals(3, result.size()); + assertEquals("UNUSED_METHOD_ARGUMENT", result.get(0).getMessageCode()); + assertEquals(3, result.get(0).getLine()); + assertEquals("UNUSED_METHOD_ARGUMENT", result.get(1).getMessageCode()); + assertEquals(3, result.get(1).getLine()); + assertEquals("UNUSED_METHOD_ARGUMENT", result.get(2).getMessageCode()); + assertEquals(3, result.get(2).getLine()); + } + + @Test + public void testNoArgumentsInTag() throws ParseException, IOException { + final String tagSrc = "\r\n" + + "\r\n" + + "\r\n" + + "\r\n"; + + cfBugs.process(tagSrc, "test"); + final Map> result = cfBugs.getBugs().getBugList(); + assertEquals(0, result.size()); + } + + @Test + public void testArgumentsUsedInTag() throws ParseException, IOException { + final String tagSrc = "\r\n" + + "\r\n" + + "\r\n" + + "\r\n" + + "\r\n" + + "\r\n" + + "\r\n"; + + cfBugs.process(tagSrc, "test"); + final Map> result = cfBugs.getBugs().getBugList(); + assertEquals(0, result.size()); + } + + @Test + public void testArgumentsNotUsedInTag() throws ParseException, IOException { + final String tagSrc = "\r\n" + + "\r\n" + + "\r\n" + + "\r\n" + + "" + + "\r\n" + + "\r\n" + + "\r\n"; + + cfBugs.process(tagSrc, "test"); + final Map> result = cfBugs.getBugs().getBugList(); + assertEquals(0, result.size()); + } + + @Test + public void testArgumentsNotUsedInMethodTag() throws ParseException, IOException { + final String tagSrc = "\r\n" + + "\r\n" + + "\r\n" + + "\r\n" + + "\r\n" + + "\r\n" + + "\r\n"; + + cfBugs.process(tagSrc, "test"); + final Map> result = cfBugs.getBugs().getBugList(); + assertEquals(0, result.size()); + } + + @Test + public void testArgumentsNotUsedInReturnTag() throws ParseException, IOException { + final String tagSrc = "\r\n" + + "\r\n" + + "\r\n" + + "\r\n" + + "\r\n" + + "\r\n" + + "\r\n"; + + cfBugs.process(tagSrc, "test"); + final List result = cfBugs.getBugs().getBugList().values().iterator().next(); + assertEquals(1, result.size()); + assertEquals("UNUSED_METHOD_ARGUMENT", result.get(0).getMessageCode()); + assertEquals(4, result.get(0).getLine()); + } + + @Test + public void testMultipleArgumentsNotUsedTag() throws ParseException, IOException { + final String tagSrc = "\r\n" + + "\r\n" + + "\r\n" + + "\r\n" + + "\r\n" + + "\r\n" + + "\r\n" + + "\r\n" + + "\r\n"; + + cfBugs.process(tagSrc, "test"); + final List result = cfBugs.getBugs().getBugList().values().iterator().next(); + assertEquals(3, result.size()); + assertEquals("UNUSED_METHOD_ARGUMENT", result.get(0).getMessageCode()); + assertEquals(6, result.get(0).getLine()); + assertEquals("UNUSED_METHOD_ARGUMENT", result.get(1).getMessageCode()); + assertEquals(4, result.get(1).getLine()); + assertEquals("UNUSED_METHOD_ARGUMENT", result.get(2).getMessageCode()); + assertEquals(5, result.get(2).getLine()); + } + +} From 377a7cec65ffa38566075b15998b2b136098a293 Mon Sep 17 00:00:00 2001 From: Justin Mclean Date: Sun, 1 Nov 2015 19:19:00 +1100 Subject: [PATCH 2/5] Add rule to check for unused function / method arguments --- .../plugins/core/UnusedArgumentChecker.java | 95 +++++++++++++++++++ 1 file changed, 95 insertions(+) create mode 100644 src/main/java/com/cflint/plugins/core/UnusedArgumentChecker.java diff --git a/src/main/java/com/cflint/plugins/core/UnusedArgumentChecker.java b/src/main/java/com/cflint/plugins/core/UnusedArgumentChecker.java new file mode 100644 index 000000000..95ddff2a3 --- /dev/null +++ b/src/main/java/com/cflint/plugins/core/UnusedArgumentChecker.java @@ -0,0 +1,95 @@ +package com.cflint.plugins.core; + +import java.util.Map; +import java.util.HashMap; + +import net.htmlparser.jericho.Element; + +import cfml.parsing.cfscript.CFAssignmentExpression; +import cfml.parsing.cfscript.CFExpression; +import cfml.parsing.cfscript.CFIdentifier; +import cfml.parsing.cfscript.CFFullVarExpression; +import cfml.parsing.cfscript.CFVarDeclExpression; +import cfml.parsing.cfscript.script.CFFuncDeclStatement; +import cfml.parsing.cfscript.script.CFFunctionParameter; +import cfml.parsing.cfscript.script.CFScriptStatement; + +import com.cflint.BugInfo; +import com.cflint.BugList; +import com.cflint.plugins.CFLintScannerAdapter; +import com.cflint.plugins.Context; + +public class UnusedArgumentChecker extends CFLintScannerAdapter { + final String severity = "INFO"; + + protected Map methodArguments = new HashMap(); + protected Map argumentLineNo = new HashMap(); + + @Override + public void element(final Element element, final Context context, final BugList bugs) { + if (element.getName().equals("cfargument")) { + final String name = element.getAttributeValue("name"); + methodArguments.put(name, false); + setArgumentLineNo(name, context.startLine()); + } + } + + @Override + public void expression(final CFScriptStatement expression, final Context context, final BugList bugs) { + if (expression instanceof CFFuncDeclStatement) { + final CFFuncDeclStatement function = (CFFuncDeclStatement) expression; + for (final CFFunctionParameter argument : function.getFormals()) { + final String name = argument.getName(); + methodArguments.put(name, false); + setArgumentLineNo(name, function.getLine()); // close enough? + } + } + } + + protected void setArgumentLineNo(final String argument, final Integer lineNo) { + if (argumentLineNo.get(argument) == null) { + argumentLineNo.put(argument, lineNo); + } + } + + @Override + public void expression(final CFExpression expression, final Context context, final BugList bugs) { + if (expression instanceof CFFullVarExpression) { + CFExpression variable = ((CFFullVarExpression) expression).getExpressions().get(0); + if (variable instanceof CFIdentifier) { + String name = ((CFIdentifier) variable).getName(); + if (methodArguments.get(name) != null) { + methodArguments.put(name, true); + } + } + } + else if (expression instanceof CFIdentifier) { + String name = ((CFIdentifier) expression).getName(); + if (methodArguments.get(name) != null) { + methodArguments.put(name, true); + } + } + } + + @Override + public void startFunction(Context context, BugList bugs) { + methodArguments.clear(); + } + + @Override + public void endFunction(Context context, BugList bugs) { + // TODO perhaps sort by line number? + for (Map.Entry method : methodArguments.entrySet()) { + Boolean used = method.getValue(); + if (!used) { + final String name = method.getKey(); + final Integer lineNo = argumentLineNo.get(name); + bugs.add(new BugInfo.BugInfoBuilder().setLine(lineNo).setMessageCode("UNUSED_METHOD_ARGUMENT") + .setSeverity(severity).setFilename(context.getFilename()) + .setMessage("Argument " + name + " is not used in function " + context.getFunctionName() + ", consider removing it.") + .build()); + } + } + } + +} From 30059b6f76abd455f043a3550abcf9719c388af2 Mon Sep 17 00:00:00 2001 From: Justin Mclean Date: Sun, 1 Nov 2015 19:20:13 +1100 Subject: [PATCH 3/5] Move startFunction and startComponent to before the component or function tag / script equiv. is processed --- src/main/java/com/cflint/CFLint.java | 102 +++++++++++++-------------- 1 file changed, 50 insertions(+), 52 deletions(-) diff --git a/src/main/java/com/cflint/CFLint.java b/src/main/java/com/cflint/CFLint.java index 4014ccddc..24d6ca5d3 100644 --- a/src/main/java/com/cflint/CFLint.java +++ b/src/main/java/com/cflint/CFLint.java @@ -291,10 +291,34 @@ private void process(final Element elem, final String space, Context context) currentElement.push(elem); if (elem.getName().equalsIgnoreCase("cfcomponent")) { + inComponent = true; + handler.push("component"); + + for (final CFLintStructureListener structurePlugin : getStructureListeners(extensions)) { + try{ + structurePlugin.startComponent(context, bugs); + }catch(Exception e){ + e.printStackTrace(); + bugs = new BugList(null); + } + } + + context.setInComponent(true); context.setComponentName(elem.getAttributeValue("displayname")); } else if (elem.getName().equalsIgnoreCase("cffunction")) { context.setFunctionName(elem.getAttributeValue("name")); + inFunction = true; + handler.push("function"); + + for (final CFLintStructureListener structurePlugin : getStructureListeners(extensions)) { + try{ + structurePlugin.startFunction(context, bugs); + }catch(Exception e){ + e.printStackTrace(); + bugs = new BugList(null); + } + } } try{ @@ -378,17 +402,6 @@ else if (elem.getName().equalsIgnoreCase("cffunction")) { } if (elem.getName().equalsIgnoreCase("cffunction")) { - inFunction = true; - handler.push("function"); - - for (final CFLintStructureListener structurePlugin : getStructureListeners(extensions)) { - try{ - structurePlugin.startFunction(context, bugs); - }catch(Exception e){ - e.printStackTrace(); - bugs = new BugList(null); - } - } processStack(elem.getChildElements(), space + " ", context); for (final CFLintStructureListener structurePlugin : getStructureListeners(extensions)) { try{ @@ -401,17 +414,6 @@ else if (elem.getName().equalsIgnoreCase("cffunction")) { inFunction = false; handler.pop(); } else if (elem.getName().equalsIgnoreCase("cfcomponent")) { - inComponent = true; - handler.push("component"); - for (final CFLintStructureListener structurePlugin : getStructureListeners(extensions)) { - try{ - structurePlugin.startComponent(context, bugs); - }catch(Exception e){ - e.printStackTrace(); - bugs = new BugList(null); - } - } - context.setInComponent(true); processStack(elem.getChildElements(), space + " ", context); for (final CFLintStructureListener structurePlugin : getStructureListeners(extensions)) { try{ @@ -478,11 +480,35 @@ private void process(final CFScriptStatement expression, final String filename, final Context context = new Context(filename, elem, functionName, inAssignment, handler); context.setInComponent(inComponent); - - if (expression instanceof CFFuncDeclStatement) { + if (expression instanceof CFCompDeclStatement) { + inComponent = true; + //do startComponent notifications + for (final CFLintStructureListener structurePlugin : getStructureListeners(extensions)) { + try{ + structurePlugin.startComponent(context, bugs); + }catch(Exception e){ + e.printStackTrace(); + bugs = new BugList(null); + } + } + } + else if (expression instanceof CFFuncDeclStatement) { final CFFuncDeclStatement function = (CFFuncDeclStatement) expression; functionName = function.getName().getName(); context.setFunctionName(functionName); + inFunction = true; + handler.push("function"); + for (final CFFunctionParameter param : function.getFormals()) { + handler.addArgument(param.getName()); + } + for (final CFLintStructureListener structurePlugin : getStructureListeners(extensions)) { + try{ + structurePlugin.startFunction(context, bugs); + }catch(Exception e){ + e.printStackTrace(); + bugs = new BugList(null); + } + } } for (final CFLintScanner plugin : extensions) { @@ -507,16 +533,6 @@ private void process(final CFScriptStatement expression, final String filename, } else if (expression instanceof CFExpressionStatement) { process(((CFExpressionStatement) expression).getExpression(), filename, elem, functionName); } else if (expression instanceof CFCompDeclStatement) { - inComponent = true; - //do startComponent notifications - for (final CFLintStructureListener structurePlugin : getStructureListeners(extensions)) { - try{ - structurePlugin.startComponent(context, bugs); - }catch(Exception e){ - e.printStackTrace(); - bugs = new BugList(null); - } - } //process the component declaration process(((CFCompDeclStatement) expression).getBody(), filename, elem, functionName); //do endComponent notifications @@ -563,24 +579,6 @@ private void process(final CFScriptStatement expression, final String filename, } else if (expression instanceof CFFuncDeclStatement) { final CFFuncDeclStatement function = (CFFuncDeclStatement) expression; - inFunction = true; - handler.push("function"); -// if ("init".equalsIgnoreCase(function.getName())) { -// inFunction = false; -// } - - for (final CFFunctionParameter param : function.getFormals()) { - handler.addArgument(param.getName()); - } - for (final CFLintStructureListener structurePlugin : getStructureListeners(extensions)) { - try{ - structurePlugin.startFunction(context, bugs); - }catch(Exception e){ - e.printStackTrace(); - bugs = new BugList(null); - } - } - process(function.getBody(), filename, elem, function.getName()); for (final CFLintStructureListener structurePlugin : getStructureListeners(extensions)) { try{ From a93179ea5ddfe53c90073a8749fe65e382388cef Mon Sep 17 00:00:00 2001 From: Justin Mclean Date: Fri, 6 Nov 2015 08:08:13 +1100 Subject: [PATCH 4/5] addd unused arguments to config files --- src/main/resources/cflint.definition.json | 11 +++++++++++ src/main/resources/cflint.definition.xml | 5 +++++ 2 files changed, 16 insertions(+) diff --git a/src/main/resources/cflint.definition.json b/src/main/resources/cflint.definition.json index 823ce4984..299e0072d 100644 --- a/src/main/resources/cflint.definition.json +++ b/src/main/resources/cflint.definition.json @@ -727,6 +727,17 @@ } ], "parameter": [] + }, + { + "name": "UnusedArgumentChecker", + "className": "UnusedArgumentChecker", + "message": [ + { + "code": "UNUSED_METHOD_ARGUMENT", + "severity": "INFO" + } + ], + "parameter": [] } ] } \ No newline at end of file diff --git a/src/main/resources/cflint.definition.xml b/src/main/resources/cflint.definition.xml index 9e088fb0b..c69d7ea46 100644 --- a/src/main/resources/cflint.definition.xml +++ b/src/main/resources/cflint.definition.xml @@ -334,4 +334,9 @@ Avoid leaving debug attribute on tags. + + + INFO + + From 9512cba6cac2f36a9701c9d8e03eef26677b0b26 Mon Sep 17 00:00:00 2001 From: Justin Mclean Date: Mon, 9 Nov 2015 08:03:37 +1100 Subject: [PATCH 5/5] Don't remove all bugs when an exception occurs. --- src/main/java/com/cflint/CFLint.java | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/src/main/java/com/cflint/CFLint.java b/src/main/java/com/cflint/CFLint.java index 24d6ca5d3..764a43c94 100644 --- a/src/main/java/com/cflint/CFLint.java +++ b/src/main/java/com/cflint/CFLint.java @@ -153,7 +153,6 @@ public CFLint(ConfigRuntime configuration,final CFLintScanner... bugsScanners) { bugs = new BugList(filter); } catch (IOException e1) { e1.printStackTrace(); - bugs = new BugList(null); } if(exceptionListeners.size() == 0){ addExceptionListener(new DefaultCFLintExceptionListener(bugs)); @@ -299,7 +298,6 @@ private void process(final Element elem, final String space, Context context) structurePlugin.startComponent(context, bugs); }catch(Exception e){ e.printStackTrace(); - bugs = new BugList(null); } } @@ -316,7 +314,6 @@ else if (elem.getName().equalsIgnoreCase("cffunction")) { structurePlugin.startFunction(context, bugs); }catch(Exception e){ e.printStackTrace(); - bugs = new BugList(null); } } } @@ -408,7 +405,6 @@ else if (elem.getName().equalsIgnoreCase("cffunction")) { structurePlugin.endFunction(context, bugs); }catch(Exception e){ e.printStackTrace(); - bugs = new BugList(null); } } inFunction = false; @@ -420,7 +416,6 @@ else if (elem.getName().equalsIgnoreCase("cffunction")) { structurePlugin.endComponent(context, bugs); }catch(Exception e){ e.printStackTrace(); - bugs = new BugList(null); } } @@ -488,7 +483,6 @@ private void process(final CFScriptStatement expression, final String filename, structurePlugin.startComponent(context, bugs); }catch(Exception e){ e.printStackTrace(); - bugs = new BugList(null); } } } @@ -506,7 +500,6 @@ else if (expression instanceof CFFuncDeclStatement) { structurePlugin.startFunction(context, bugs); }catch(Exception e){ e.printStackTrace(); - bugs = new BugList(null); } } } @@ -541,7 +534,6 @@ else if (expression instanceof CFFuncDeclStatement) { structurePlugin.endComponent(context, bugs); }catch(Exception e){ e.printStackTrace(); - bugs = new BugList(null); } } @@ -585,7 +577,6 @@ else if (expression instanceof CFFuncDeclStatement) { structurePlugin.endFunction(context, bugs); }catch(Exception e){ e.printStackTrace(); - bugs = new BugList(null); } } inFunction = false; @@ -880,7 +871,6 @@ protected void fireStartedProcessing(final String srcidentifier) { structurePlugin.startFile(srcidentifier, bugs); }catch(Exception e){ e.printStackTrace(); - bugs = new BugList(null); } } for (final ScanProgressListener p : scanProgressListeners) { @@ -894,7 +884,6 @@ protected void fireFinishedProcessing(final String srcidentifier) { structurePlugin.endFile(srcidentifier, bugs); }catch(Exception e){ e.printStackTrace(); - bugs = new BugList(null); } } for (final ScanProgressListener p : scanProgressListeners) {