From ffdd70e768a4b0a34ef7ac2d4d9c27e9f3763af5 Mon Sep 17 00:00:00 2001 From: Gama11 Date: Tue, 1 Mar 2016 16:26:59 +0100 Subject: [PATCH 1/2] Add a basic NullParameter check to detect redundancy --- checkstyle/Main.hx | 3 +- checkstyle/checks/NullParameterCheck.hx | 27 ++++++++++++++++++ resources/sample-config.json | 6 ++++ resources/static-analysis.xml | 2 ++ test/AccessOrderCheckTest.hx | 2 +- test/DynamicCheckTest.hx | 2 +- test/NullParameterCheckTest.hx | 37 +++++++++++++++++++++++++ test/TestMain.hx | 3 +- 8 files changed, 77 insertions(+), 5 deletions(-) create mode 100644 checkstyle/checks/NullParameterCheck.hx create mode 100644 test/NullParameterCheckTest.hx diff --git a/checkstyle/Main.hx b/checkstyle/Main.hx index 281d473b..3d1109c5 100644 --- a/checkstyle/Main.hx +++ b/checkstyle/Main.hx @@ -116,9 +116,8 @@ class Main { var check:Check = cast info.build(checkConf.type); if (check == null) return; verifyAllowedFields(checkConf, ["type", "props"], check.getModuleName()); - if (checkConf.props == null) checkConf.props = []; - var props = Reflect.fields(checkConf.props); + var props = (checkConf.props == null) ? [] : Reflect.fields(checkConf.props); for (prop in props) { var val = Reflect.field(checkConf.props, prop); if (!Reflect.hasField(check, prop)) { diff --git a/checkstyle/checks/NullParameterCheck.hx b/checkstyle/checks/NullParameterCheck.hx new file mode 100644 index 00000000..c8e765bd --- /dev/null +++ b/checkstyle/checks/NullParameterCheck.hx @@ -0,0 +1,27 @@ +package checkstyle.checks; + +import checkstyle.LintMessage.SeverityLevel; +import haxe.macro.Expr.FunctionArg; +import haxe.macro.Expr.Position; +using haxe.macro.ExprTools; + +@name("NullParameter") +@desc("Checks for parameters with a null default value") +class NullParameterCheck extends Check { + + override function actualRun() { + forEachField(function(field, _) { + switch (field.kind) { + case FFun(f): + for (arg in f.args) checkArgument(arg); + case _: + } + }); + } + + function checkArgument(arg:FunctionArg) { + if (arg.opt && arg.value.toString() == "null") { + logPos('Parameter ${arg.name} has a \'?\' and a default value of \'null\', which is redundant', arg.value.pos, Reflect.field(SeverityLevel, severity)); + } + } +} \ No newline at end of file diff --git a/resources/sample-config.json b/resources/sample-config.json index c1da409d..f388cae5 100644 --- a/resources/sample-config.json +++ b/resources/sample-config.json @@ -371,6 +371,12 @@ "props": { "severity": "ERROR" } + }, + { + "type": "NullParameter", + "props": { + "severity": "ERROR" + } } ] } \ No newline at end of file diff --git a/resources/static-analysis.xml b/resources/static-analysis.xml index a99ffe55..cc20b4f2 100644 --- a/resources/static-analysis.xml +++ b/resources/static-analysis.xml @@ -62,6 +62,8 @@ + + diff --git a/test/AccessOrderCheckTest.hx b/test/AccessOrderCheckTest.hx index 991f0836..d993db56 100644 --- a/test/AccessOrderCheckTest.hx +++ b/test/AccessOrderCheckTest.hx @@ -1,4 +1,4 @@ -package ; +package; import checkstyle.checks.AccessOrderCheck; diff --git a/test/DynamicCheckTest.hx b/test/DynamicCheckTest.hx index 8ba5c5f7..8514909c 100644 --- a/test/DynamicCheckTest.hx +++ b/test/DynamicCheckTest.hx @@ -1,4 +1,4 @@ -package ; +package; import checkstyle.checks.DynamicCheck; diff --git a/test/NullParameterCheckTest.hx b/test/NullParameterCheckTest.hx new file mode 100644 index 00000000..f425e003 --- /dev/null +++ b/test/NullParameterCheckTest.hx @@ -0,0 +1,37 @@ +package; + +import checkstyle.checks.NullParameterCheck; + +class NullParameterCheckTest extends CheckTestCase { + + public function testDetectRedundancy() { + assertMsg(new NullParameterCheck(), NullParameterCheckTests.TEST1, + "Parameter a has a '?' and a default value of 'null', which is redundant"); + } + + public function testSuppressRedundancy() { + assertMsg(new NullParameterCheck(), NullParameterCheckTests.TEST2, ''); + } + + public function testNoRedundancy() { + assertMsg(new NullParameterCheck(), NullParameterCheckTests.TEST3, ''); + } +} + +class NullParameterCheckTests { + public static inline var TEST1:String = " + abstractAndClass Test { + function test(?a:Int = null) {} + }"; + + public static inline var TEST2:String = " + abstractAndClass Test { + @SuppressWarnings('checkstyle:NullParameter') + function test(?a:Int = null) {} + }"; + + public static inline var TEST3:String = " + abstractAndClass Test { + function test(?a:Int, b:Null = null) {} + }"; +} \ No newline at end of file diff --git a/test/TestMain.hx b/test/TestMain.hx index 6f1f828f..6a7a39f1 100644 --- a/test/TestMain.hx +++ b/test/TestMain.hx @@ -1,4 +1,4 @@ -package ; +package; class TestMain { @@ -33,6 +33,7 @@ class TestMain { runner.add(new NestedForDepthCheckTest()); runner.add(new NestedIfDepthCheckTest()); runner.add(new NestedTryDepthCheckTest()); + runner.add(new NullParameterCheckTest()); runner.add(new ParameterNameCheckTest()); runner.add(new ParameterNumberCheckTest()); runner.add(new PublicPrivateCheckTest()); From d9569dd8ec70b98cfad2bf6b4ba52346dc5e9e00 Mon Sep 17 00:00:00 2001 From: Gama11 Date: Tue, 1 Mar 2016 16:55:38 +0100 Subject: [PATCH 2/2] Add a nullDefaultValueStyle option to NullParameterCheck --- checkstyle/checks/NullParameterCheck.hx | 33 ++++++++++++++++++++++--- resources/sample-config.json | 3 ++- test/NullParameterCheckTest.hx | 26 +++++++++++++++---- 3 files changed, 52 insertions(+), 10 deletions(-) diff --git a/checkstyle/checks/NullParameterCheck.hx b/checkstyle/checks/NullParameterCheck.hx index c8e765bd..82238cb9 100644 --- a/checkstyle/checks/NullParameterCheck.hx +++ b/checkstyle/checks/NullParameterCheck.hx @@ -9,19 +9,44 @@ using haxe.macro.ExprTools; @desc("Checks for parameters with a null default value") class NullParameterCheck extends Check { + public static inline var EITHER:String = "either"; + public static inline var QUESTION_MARK:String = "questionmark"; + public static inline var NULL:String = "null"; + + public var nullDefaultValueStyle:String; + + public function new() { + super(); + nullDefaultValueStyle = QUESTION_MARK; + } + override function actualRun() { forEachField(function(field, _) { switch (field.kind) { case FFun(f): - for (arg in f.args) checkArgument(arg); + for (arg in f.args) checkArgument(arg, field.pos); case _: } }); } - function checkArgument(arg:FunctionArg) { - if (arg.opt && arg.value.toString() == "null") { - logPos('Parameter ${arg.name} has a \'?\' and a default value of \'null\', which is redundant', arg.value.pos, Reflect.field(SeverityLevel, severity)); + function checkArgument(arg:FunctionArg, pos:Position) { + var hasNullDefault = arg.value.toString() == "null"; + var prefix = 'Parameter \'${arg.name}\' '; + if (arg.opt && hasNullDefault) { + logPos(prefix + "is marked as optional with '?' and has a default value of 'null', which is redundant", + arg.value.pos, Reflect.field(SeverityLevel, severity)); + } + else { + var line = checker.getLinePos(pos.min).line + 1; + if (hasNullDefault && nullDefaultValueStyle == QUESTION_MARK) { + log(prefix + "should be marked as optional with '?' instead of using a null default value", + line, 0, null, Reflect.field(SeverityLevel, severity)); + } + else if (arg.opt && nullDefaultValueStyle == NULL) { + log(prefix + "should have a null default value instead of being marked as optional with '?'", + line, 0, null, Reflect.field(SeverityLevel, severity)); + } } } } \ No newline at end of file diff --git a/resources/sample-config.json b/resources/sample-config.json index f388cae5..69b0eb5f 100644 --- a/resources/sample-config.json +++ b/resources/sample-config.json @@ -375,7 +375,8 @@ { "type": "NullParameter", "props": { - "severity": "ERROR" + "severity": "ERROR", + "nullDefaultValueStyle": "questionmark" } } ] diff --git a/test/NullParameterCheckTest.hx b/test/NullParameterCheckTest.hx index f425e003..b48cc6d7 100644 --- a/test/NullParameterCheckTest.hx +++ b/test/NullParameterCheckTest.hx @@ -6,15 +6,31 @@ class NullParameterCheckTest extends CheckTestCase { public function testDetectRedundancy() { assertMsg(new NullParameterCheck(), NullParameterCheckTests.TEST1, - "Parameter a has a '?' and a default value of 'null', which is redundant"); + "Parameter 'a' is marked as optional with '?' and has a default value of 'null', which is redundant"); } - public function testSuppressRedundancy() { + public function testSuppress() { assertMsg(new NullParameterCheck(), NullParameterCheckTests.TEST2, ''); } - public function testNoRedundancy() { - assertMsg(new NullParameterCheck(), NullParameterCheckTests.TEST3, ''); + public function testAllowEitherStyle() { + var check = new NullParameterCheck(); + check.nullDefaultValueStyle = NullParameterCheck.EITHER; + assertMsg(check, NullParameterCheckTests.TEST3, ''); + } + + public function testPreferQuestionMark() { + var check = new NullParameterCheck(); + check.nullDefaultValueStyle = NullParameterCheck.QUESTION_MARK; + assertMsg(check, NullParameterCheckTests.TEST3, + "Parameter 'b' should be marked as optional with '?' instead of using a null default value"); + } + + public function testPreferNull() { + var check = new NullParameterCheck(); + check.nullDefaultValueStyle = NullParameterCheck.NULL; + assertMsg(check, NullParameterCheckTests.TEST3, + "Parameter 'a' should have a null default value instead of being marked as optional with '?'"); } } @@ -27,7 +43,7 @@ class NullParameterCheckTests { public static inline var TEST2:String = " abstractAndClass Test { @SuppressWarnings('checkstyle:NullParameter') - function test(?a:Int = null) {} + function test(?a:Int = null, b:Null = null, ?c:Int) {} }"; public static inline var TEST3:String = "