From 5c2035ff764d1985207a8cabec4ea948b75a8287 Mon Sep 17 00:00:00 2001 From: WebFreak001 Date: Thu, 29 Jun 2023 12:52:01 +0200 Subject: [PATCH] add end line/column to warnings --- dub.json | 2 +- dub.selections.json | 2 +- libdparse | 2 +- src/dscanner/analysis/alias_syntax_check.d | 6 +- src/dscanner/analysis/allman.d | 7 +- src/dscanner/analysis/asm_style.d | 5 +- src/dscanner/analysis/assert_without_msg.d | 13 +- src/dscanner/analysis/auto_function.d | 72 ++++++-- src/dscanner/analysis/auto_ref_assignment.d | 22 +-- src/dscanner/analysis/base.d | 139 +++++++++++++- .../analysis/body_on_disabled_funcs.d | 45 +++-- .../analysis/builtin_property_names.d | 15 +- src/dscanner/analysis/comma_expression.d | 2 +- src/dscanner/analysis/constructors.d | 42 +++-- src/dscanner/analysis/cyclomatic_complexity.d | 32 ++-- src/dscanner/analysis/del.d | 8 +- src/dscanner/analysis/duplicate_attribute.d | 81 +++++---- src/dscanner/analysis/enumarrayliteral.d | 2 +- .../analysis/explicitly_annotated_unittests.d | 14 +- src/dscanner/analysis/final_attribute.d | 72 +++++--- src/dscanner/analysis/fish.d | 26 ++- src/dscanner/analysis/function_attributes.d | 28 +-- src/dscanner/analysis/has_public_example.d | 36 ++-- src/dscanner/analysis/helpers.d | 92 ++++++++-- src/dscanner/analysis/if_constraints_indent.d | 37 ++-- src/dscanner/analysis/if_statements.d | 6 +- src/dscanner/analysis/ifelsesame.d | 22 ++- src/dscanner/analysis/imports_sortedness.d | 140 ++++++++++---- .../analysis/incorrect_infinite_range.d | 26 ++- .../analysis/label_var_same_name_check.d | 17 +- src/dscanner/analysis/lambda_return_check.d | 15 +- src/dscanner/analysis/length_subtraction.d | 14 +- src/dscanner/analysis/line_length.d | 4 +- src/dscanner/analysis/local_imports.d | 6 +- src/dscanner/analysis/logic_precedence.d | 8 +- src/dscanner/analysis/mismatched_args.d | 32 ++-- src/dscanner/analysis/numbers.d | 13 +- src/dscanner/analysis/objectconst.d | 15 +- .../analysis/opequals_without_tohash.d | 44 +++-- src/dscanner/analysis/pokemon.d | 18 +- .../properly_documented_public_functions.d | 171 +++++++++++------- src/dscanner/analysis/range.d | 19 +- src/dscanner/analysis/redundant_attributes.d | 36 ++-- src/dscanner/analysis/redundant_parens.d | 6 +- .../analysis/redundant_storage_class.d | 2 +- src/dscanner/analysis/run.d | 52 +++++- src/dscanner/analysis/static_if_else.d | 8 +- src/dscanner/analysis/style.d | 36 ++-- src/dscanner/analysis/trust_too_much.d | 26 +-- src/dscanner/analysis/undocumented.d | 45 +++-- src/dscanner/analysis/unmodified.d | 14 +- src/dscanner/analysis/unused.d | 10 +- src/dscanner/analysis/unused_label.d | 37 ++-- src/dscanner/analysis/unused_parameter.d | 12 +- src/dscanner/analysis/unused_result.d | 26 ++- src/dscanner/analysis/unused_variable.d | 10 +- src/dscanner/analysis/useless_assert.d | 11 +- src/dscanner/analysis/useless_initializer.d | 85 +++++---- src/dscanner/analysis/vcall_in_ctor.d | 14 +- src/dscanner/main.d | 2 +- src/dscanner/reports.d | 63 +++++-- 61 files changed, 1238 insertions(+), 629 deletions(-) diff --git a/dub.json b/dub.json index 8f02074..e681e86 100644 --- a/dub.json +++ b/dub.json @@ -11,7 +11,7 @@ "built_with_dub" ], "dependencies": { - "libdparse": ">=0.23.0 <0.24.0", + "libdparse": ">=0.23.1 <0.24.0", "dcd:dsymbol": ">=0.16.0-beta.2 <0.17.0", "inifiled": "~>1.3.1", "emsi_containers": "~>0.9.0", diff --git a/dub.selections.json b/dub.selections.json index 2c085ca..5bafd64 100644 --- a/dub.selections.json +++ b/dub.selections.json @@ -6,7 +6,7 @@ "emsi_containers": "0.9.0", "inifiled": "1.3.3", "libddoc": "0.8.0", - "libdparse": "0.23.0", + "libdparse": "0.23.1", "stdx-allocator": "2.77.5" } } diff --git a/libdparse b/libdparse index 86c9bf4..e354f91 160000 --- a/libdparse +++ b/libdparse @@ -1 +1 @@ -Subproject commit 86c9bf44c96e1666eb175c749cc26f62c2008979 +Subproject commit e354f917f20c4a1fae04d1680205486c2a2a8317 diff --git a/src/dscanner/analysis/alias_syntax_check.d b/src/dscanner/analysis/alias_syntax_check.d index e8c1ab2..bfe744f 100644 --- a/src/dscanner/analysis/alias_syntax_check.d +++ b/src/dscanner/analysis/alias_syntax_check.d @@ -29,8 +29,7 @@ final class AliasSyntaxCheck : BaseAnalyzer return; assert(ad.declaratorIdentifierList.identifiers.length > 0, "Identifier list length is zero, libdparse has a bug"); - addErrorMessage(ad.declaratorIdentifierList.identifiers[0].line, - ad.declaratorIdentifierList.identifiers[0].column, KEY, + addErrorMessage(ad, KEY, "Prefer the new \"'alias' identifier '=' type ';'\" syntax" ~ " to the old \"'alias' type identifier ';'\" syntax."); } @@ -48,7 +47,8 @@ unittest StaticAnalysisConfig sac = disabledConfig(); sac.alias_syntax_check = Check.enabled; assertAnalyzerWarnings(q{ - alias int abcde; // [warn]: Prefer the new "'alias' identifier '=' type ';'" syntax to the old "'alias' type identifier ';'" syntax. + alias int abcde; /+ + ^^^^^^^^^^^^^^^^ [warn]: Prefer the new "'alias' identifier '=' type ';'" syntax to the old "'alias' type identifier ';'" syntax.+/ alias abcde = int; }c, sac); diff --git a/src/dscanner/analysis/allman.d b/src/dscanner/analysis/allman.d index 2cbd9cc..10cf50c 100644 --- a/src/dscanner/analysis/allman.d +++ b/src/dscanner/analysis/allman.d @@ -47,7 +47,7 @@ final class AllManCheck : BaseAnalyzer continue; // ignore inline { } braces if (curLine != tokens[i + 1].line) - addErrorMessage(tokens[i].line, tokens[i].column, KEY, MESSAGE); + addErrorMessage(tokens[i], KEY, MESSAGE); } if (tokens[i].type == tok!"}" && curLine == prevTokenLine) { @@ -56,7 +56,7 @@ final class AllManCheck : BaseAnalyzer continue; // ignore inline { } braces if (!tokens[0 .. i].retro.until!(t => t.line != curLine).canFind!(t => t.type == tok!"{")) - addErrorMessage(tokens[i].line, tokens[i].column, KEY, MESSAGE); + addErrorMessage(tokens[i], KEY, MESSAGE); } } } @@ -79,7 +79,8 @@ unittest assertAnalyzerWarnings(q{ void testAllman() { - while (true) { // [warn]: %s + while (true) { /+ + ^ [warn]: %s +/ auto f = 1; } diff --git a/src/dscanner/analysis/asm_style.d b/src/dscanner/analysis/asm_style.d index de54c77..a10d091 100644 --- a/src/dscanner/analysis/asm_style.d +++ b/src/dscanner/analysis/asm_style.d @@ -32,7 +32,7 @@ final class AsmStyleCheck : BaseAnalyzer if (brExp.asmBrExp !is null && brExp.asmBrExp.asmUnaExp !is null && brExp.asmBrExp.asmUnaExp.asmPrimaryExp !is null) { - addErrorMessage(brExp.line, brExp.column, "dscanner.confusing.brexp", + addErrorMessage(brExp, "dscanner.confusing.brexp", "This is confusing because it looks like an array index. Rewrite a[1] as [a + 1] to clarify."); } brExp.accept(this); @@ -50,7 +50,8 @@ unittest { asm { - mov a, someArray[1]; // [warn]: This is confusing because it looks like an array index. Rewrite a[1] as [a + 1] to clarify. + mov a, someArray[1]; /+ + ^^^^^^^^^^^^ [warn]: This is confusing because it looks like an array index. Rewrite a[1] as [a + 1] to clarify. +/ add near ptr [EAX], 3; } } diff --git a/src/dscanner/analysis/assert_without_msg.d b/src/dscanner/analysis/assert_without_msg.d index a6aa4d7..8fad6c9 100644 --- a/src/dscanner/analysis/assert_without_msg.d +++ b/src/dscanner/analysis/assert_without_msg.d @@ -41,7 +41,7 @@ final class AssertWithoutMessageCheck : BaseAnalyzer && expr.assertArguments.message !is null; if (!hasMessage) - addErrorMessage(expr.line, expr.column, KEY, MESSAGE); + addErrorMessage(expr, KEY, MESSAGE); } override void visit(const FunctionCallExpression expr) @@ -55,7 +55,7 @@ final class AssertWithoutMessageCheck : BaseAnalyzer auto ident = iot.identifier; if (ident.text == "enforce" && expr.arguments !is null && expr.arguments.namedArgumentList !is null && expr.arguments.namedArgumentList.items.length < 2) - addErrorMessage(ident.line, ident.column, KEY, MESSAGE); + addErrorMessage(expr, KEY, MESSAGE); } } @@ -112,7 +112,8 @@ unittest assertAnalyzerWarnings(q{ unittest { assert(0, "foo bar"); - assert(0); // [warn]: %s + assert(0); /+ + ^^^^^^^^^ [warn]: %s +/ } }c.format( AssertWithoutMessageCheck.MESSAGE, @@ -121,7 +122,8 @@ unittest assertAnalyzerWarnings(q{ unittest { static assert(0, "foo bar"); - static assert(0); // [warn]: %s + static assert(0); /+ + ^^^^^^^^^ [warn]: %s +/ } }c.format( AssertWithoutMessageCheck.MESSAGE, @@ -133,7 +135,8 @@ unittest enforce(0); // std.exception not imported yet - could be a user-defined symbol import std.exception; enforce(0, "foo bar"); - enforce(0); // [warn]: %s + enforce(0); /+ + ^^^^^^^^^^ [warn]: %s +/ } }c.format( AssertWithoutMessageCheck.MESSAGE, diff --git a/src/dscanner/analysis/auto_function.d b/src/dscanner/analysis/auto_function.d index 659dbff..a969039 100644 --- a/src/dscanner/analysis/auto_function.d +++ b/src/dscanner/analysis/auto_function.d @@ -11,7 +11,7 @@ import dparse.ast; import dparse.lexer; import std.stdio; -import std.algorithm.searching : any; +import std.algorithm : map, filter; /** * Checks for auto functions without return statement. @@ -26,7 +26,8 @@ final class AutoFunctionChecker : BaseAnalyzer private: enum string KEY = "dscanner.suspicious.missing_return"; - enum string MESSAGE = "Auto function without return statement, prefer an explicit void"; + enum string MESSAGE = "Auto function without return statement, prefer replacing auto with void"; + enum string MESSAGE_INSERT = "Auto function without return statement, prefer inserting void to be explicit"; bool[] _returns; size_t _mixinDepth; @@ -44,19 +45,41 @@ public: super(fileName, null, skipTests); } + package static const(Token)[] findAutoReturnType(const(FunctionDeclaration) decl) + { + auto autoFunTokens = decl.storageClasses + .map!(a => a.token.type == tok!"auto" + ? [a.token] + : a.atAttribute + ? a.atAttribute.tokens + : null) + .filter!(a => a.length > 0); + return autoFunTokens.empty ? null : autoFunTokens.front; + } + override void visit(const(FunctionDeclaration) decl) { _returns.length += 1; scope(exit) _returns.length -= 1; _returns[$-1] = false; - const bool autoFun = decl.storageClasses - .any!(a => a.token.type == tok!"auto" || a.atAttribute !is null); + auto autoTokens = findAutoReturnType(decl); + bool isAtAttribute = autoTokens.length > 1; decl.accept(this); - if (decl.functionBody.specifiedFunctionBody && autoFun && !_returns[$-1]) - addErrorMessage(decl.name.line, decl.name.column, KEY, MESSAGE); + if (decl.functionBody.specifiedFunctionBody && autoTokens.length && !_returns[$-1]) + { + if (isAtAttribute) + { + // highlight on the whitespace between attribute and function name + auto tok = autoTokens[$ - 1]; + auto whitespace = tok.column + (tok.text.length ? tok.text.length : str(tok.type).length); + addErrorMessage(tok.line, whitespace, whitespace + 1, KEY, MESSAGE_INSERT); + } + else + addErrorMessage(autoTokens, KEY, MESSAGE); + } } override void visit(const(ReturnStatement) rst) @@ -165,9 +188,12 @@ unittest StaticAnalysisConfig sac = disabledConfig(); sac.auto_function_check = Check.enabled; assertAnalyzerWarnings(q{ - auto ref doStuff(){} // [warn]: %s - auto doStuff(){} // [warn]: %s - int doStuff(){auto doStuff(){}} // [warn]: %s + auto ref doStuff(){} /+ + ^^^^ [warn]: %s +/ + auto doStuff(){} /+ + ^^^^ [warn]: %s +/ + int doStuff(){auto doStuff(){}} /+ + ^^^^ [warn]: %s +/ auto doStuff(){return 0;} int doStuff(){/*error but not the aim*/} }c.format( @@ -177,55 +203,63 @@ unittest ), sac); assertAnalyzerWarnings(q{ - auto doStuff(){assert(true);} // [warn]: %s + auto doStuff(){assert(true);} /+ + ^^^^ [warn]: %s +/ auto doStuff(){assert(false);} }c.format( AutoFunctionChecker.MESSAGE, ), sac); assertAnalyzerWarnings(q{ - auto doStuff(){assert(1);} // [warn]: %s + auto doStuff(){assert(1);} /+ + ^^^^ [warn]: %s +/ auto doStuff(){assert(0);} }c.format( AutoFunctionChecker.MESSAGE, ), sac); assertAnalyzerWarnings(q{ - auto doStuff(){mixin("0+0");} // [warn]: %s + auto doStuff(){mixin("0+0");} /+ + ^^^^ [warn]: %s +/ auto doStuff(){mixin("return 0;");} }c.format( AutoFunctionChecker.MESSAGE, ), sac); assertAnalyzerWarnings(q{ - auto doStuff(){mixin("0+0");} // [warn]: %s + auto doStuff(){mixin("0+0");} /+ + ^^^^ [warn]: %s +/ auto doStuff(){mixin("static if (true)" ~ " return " ~ 0.stringof ~ ";");} }c.format( AutoFunctionChecker.MESSAGE, ), sac); assertAnalyzerWarnings(q{ - auto doStuff(){} // [warn]: %s + auto doStuff(){} /+ + ^^^^ [warn]: %s +/ extern(C) auto doStuff(); }c.format( AutoFunctionChecker.MESSAGE, ), sac); assertAnalyzerWarnings(q{ - auto doStuff(){} // [warn]: %s + auto doStuff(){} /+ + ^^^^ [warn]: %s +/ @disable auto doStuff(); }c.format( AutoFunctionChecker.MESSAGE, ), sac); assertAnalyzerWarnings(q{ - @property doStuff(){} // [warn]: %s - @safe doStuff(){} // [warn]: %s + @property doStuff(){} /+ + ^ [warn]: %s +/ + @safe doStuff(){} /+ + ^ [warn]: %s +/ @disable doStuff(); @safe void doStuff(); }c.format( - AutoFunctionChecker.MESSAGE, - AutoFunctionChecker.MESSAGE, + AutoFunctionChecker.MESSAGE_INSERT, + AutoFunctionChecker.MESSAGE_INSERT, ), sac); assertAnalyzerWarnings(q{ diff --git a/src/dscanner/analysis/auto_ref_assignment.d b/src/dscanner/analysis/auto_ref_assignment.d index dbd5760..47c1485 100644 --- a/src/dscanner/analysis/auto_ref_assignment.d +++ b/src/dscanner/analysis/auto_ref_assignment.d @@ -54,29 +54,29 @@ final class AutoRefAssignmentCheck : BaseAnalyzer { if (assign.operator == tok!"" || scopes.length == 0) return; - interest++; + interest ~= assign; assign.ternaryExpression.accept(this); - interest--; + interest.length--; } override void visit(const IdentifierOrTemplateInstance ioti) { import std.algorithm.searching : canFind; - if (ioti.identifier == tok!"" || interest <= 0) + if (ioti.identifier == tok!"" || !interest.length) return; if (scopes[$ - 1].canFind(ioti.identifier.text)) - addErrorMessage(ioti.identifier.line, ioti.identifier.column, KEY, MESSAGE); + addErrorMessage(interest[$ - 1], KEY, MESSAGE); } override void visit(const IdentifierChain ic) { import std.algorithm.searching : canFind; - if (ic.identifiers.length == 0 || interest <= 0) + if (ic.identifiers.length == 0 || !interest.length) return; if (scopes[$ - 1].canFind(ic.identifiers[0].text)) - addErrorMessage(ic.identifiers[0].line, ic.identifiers[0].column, KEY, MESSAGE); + addErrorMessage(interest[$ - 1], KEY, MESSAGE); } alias visit = BaseAnalyzer.visit; @@ -86,12 +86,7 @@ private: enum string MESSAGE = "Assignment to auto-ref function parameter."; enum string KEY = "dscanner.suspicious.auto_ref_assignment"; - invariant - { - assert(interest >= 0); - } - - int interest; + const(AssignExpression)[] interest; void addSymbol(string symbolName) { @@ -123,7 +118,8 @@ unittest assertAnalyzerWarnings(q{ int doStuff(T)(auto ref int a) { - a = 10; // [warn]: %s + a = 10; /+ + ^^^^^^ [warn]: %s +/ } int doStuff(T)(ref int a) diff --git a/src/dscanner/analysis/base.d b/src/dscanner/analysis/base.d index 0b92a5c..91cd896 100644 --- a/src/dscanner/analysis/base.d +++ b/src/dscanner/analysis/base.d @@ -5,24 +5,94 @@ import std.string; import dparse.ast; import std.array; import dsymbol.scope_ : Scope; +import dparse.lexer : Token, str, IdType; struct Message { - /// Name of the file where the warning was triggered - string fileName; - /// Line number where the warning was triggered - size_t line; - /// Column number where the warning was triggered (in bytes) - size_t column; + struct Diagnostic + { + /// Name of the file where the warning was triggered. + string fileName; + /// Line number where the warning was triggered, 1-based. + size_t startLine, endLine; + /// Column number where the warning was triggered. (in bytes) + size_t startColumn, endColumn; + /// Warning message, may be null for supplemental diagnostics. + string message; + + // TODO: add auto-fix suggestion API here + + deprecated("Use startLine instead") alias line = startLine; + deprecated("Use startColumn instead") alias column = startColumn; + + static Diagnostic from(string fileName, const BaseNode node, string message) + { + return from(fileName, node !is null ? node.tokens : [], message); + } + + static Diagnostic from(string fileName, const Token token, string message) + { + auto text = token.text.length ? token.text : str(token.type); + return from(fileName, token.line, token.column, token.column + text.length, message); + } + + static Diagnostic from(string fileName, const Token[] tokens, string message) + { + auto start = tokens.length ? tokens[0] : Token.init; + auto end = tokens.length ? tokens[$ - 1] : Token.init; + auto endText = end.text.length ? end.text : str(end.type); + return from(fileName, start.line, end.line, start.column, end.column + endText.length, message); + } + + static Diagnostic from(string fileName, size_t line, size_t startColumn, size_t endColumn, string message) + { + return Message.Diagnostic(fileName, line, line, startColumn, endColumn, message); + } + + static Diagnostic from(string fileName, size_t startLine, size_t endLine, size_t startColumn, size_t endColumn, string message) + { + return Message.Diagnostic(fileName, startLine, endLine, startColumn, endColumn, message); + } + } + + /// Primary warning + Diagnostic diagnostic; + /// List of supplemental warnings / hint locations + Diagnostic[] supplemental; /// Name of the warning string key; - /// Warning message - string message; /// Check name string checkName; + + deprecated this(string fileName, size_t line, size_t column, string key = null, string message = null, string checkName = null) + { + diagnostic.fileName = fileName; + diagnostic.startLine = diagnostic.endLine = line; + diagnostic.startColumn = diagnostic.endColumn = column; + diagnostic.message = message; + this.key = key; + this.checkName = checkName; + } + + this(Diagnostic diagnostic, string key = null, string checkName = null) + { + this.diagnostic = diagnostic; + this.key = key; + this.checkName = checkName; + } + + this(Diagnostic diagnostic, Diagnostic[] supplemental, string key = null, string checkName = null) + { + this.diagnostic = diagnostic; + this.supplemental = supplemental; + this.key = key; + this.checkName = checkName; + } + + alias diagnostic this; } -enum comparitor = q{ a.line < b.line || (a.line == b.line && a.column < b.column) }; +enum comparitor = q{ a.startLine < b.startLine || (a.startLine == b.startLine && a.startColumn < b.startColumn) }; alias MessageSet = RedBlackTree!(Message, comparitor, true); @@ -86,11 +156,48 @@ protected: } } + deprecated("Use the overload taking start and end locations or a Node instead") void addErrorMessage(size_t line, size_t column, string key, string message) { _messages.insert(Message(fileName, line, column, key, message, getName())); } + void addErrorMessage(const BaseNode node, string key, string message) + { + addErrorMessage(Message.Diagnostic.from(fileName, node, message), key); + } + + void addErrorMessage(const Token token, string key, string message) + { + addErrorMessage(Message.Diagnostic.from(fileName, token, message), key); + } + + void addErrorMessage(const Token[] tokens, string key, string message) + { + addErrorMessage(Message.Diagnostic.from(fileName, tokens, message), key); + } + + void addErrorMessage(size_t line, size_t startColumn, size_t endColumn, string key, string message) + { + addErrorMessage(line, line, startColumn, endColumn, key, message); + } + + void addErrorMessage(size_t startLine, size_t endLine, size_t startColumn, size_t endColumn, string key, string message) + { + auto d = Message.Diagnostic(fileName, startLine, endLine, startColumn, endColumn, message); + _messages.insert(Message(d, key, getName())); + } + + void addErrorMessage(Message.Diagnostic diagnostic, string key) + { + _messages.insert(Message(diagnostic, key, getName())); + } + + void addErrorMessage(Message.Diagnostic diagnostic, Message.Diagnostic[] supplemental, string key) + { + _messages.insert(Message(diagnostic, supplemental, key, getName())); + } + /** * The file name */ @@ -100,3 +207,17 @@ protected: MessageSet _messages; } + +/// Find the token with the given type, otherwise returns the whole range or a user-specified fallback, if set. +const(Token)[] findTokenForDisplay(const BaseNode node, IdType type, const(Token)[] fallback = null) +{ + return node.tokens.findTokenForDisplay(type, fallback); +} +/// ditto +const(Token)[] findTokenForDisplay(const Token[] tokens, IdType type, const(Token)[] fallback = null) +{ + foreach (i, token; tokens) + if (token.type == type) + return tokens[i .. i + 1]; + return fallback is null ? tokens : fallback; +} diff --git a/src/dscanner/analysis/body_on_disabled_funcs.d b/src/dscanner/analysis/body_on_disabled_funcs.d index 6d4248c..9772efd 100644 --- a/src/dscanner/analysis/body_on_disabled_funcs.d +++ b/src/dscanner/analysis/body_on_disabled_funcs.d @@ -83,7 +83,7 @@ private: && dec.functionDeclaration.functionBody !is null && dec.functionDeclaration.functionBody.missingFunctionBody is null) { - addErrorMessage(dec.functionDeclaration.name.line, dec.functionDeclaration.name.column, + addErrorMessage(dec.functionDeclaration.functionBody, KEY, "Function marked with '@disabled' should not have a body"); } else if (dec.constructor !is null @@ -91,7 +91,7 @@ private: && dec.constructor.functionBody !is null && dec.constructor.functionBody.missingFunctionBody is null) { - addErrorMessage(dec.constructor.line, dec.constructor.column, + addErrorMessage(dec.constructor.functionBody, KEY, "Constructor marked with '@disabled' should not have a body"); } else if (dec.destructor !is null @@ -99,7 +99,7 @@ private: && dec.destructor.functionBody !is null && dec.destructor.functionBody.missingFunctionBody is null) { - addErrorMessage(dec.destructor.line, dec.destructor.column, + addErrorMessage(dec.destructor.functionBody, KEY, "Destructor marked with '@disabled' should not have a body"); } } @@ -159,9 +159,12 @@ unittest } } - this() {} // [warn]: Constructor marked with '@disabled' should not have a body - void doThing() {} // [warn]: Function marked with '@disabled' should not have a body - ~this() {} // [warn]: Destructor marked with '@disabled' should not have a body + this() {} /+ + ^^ [warn]: Constructor marked with '@disabled' should not have a body +/ + void doThing() {} /+ + ^^ [warn]: Function marked with '@disabled' should not have a body +/ + ~this() {} /+ + ^^ [warn]: Destructor marked with '@disabled' should not have a body +/ this(); void doThing(); @@ -170,18 +173,28 @@ unittest class C2 { - @disable this() {} // [warn]: Constructor marked with '@disabled' should not have a body - @disable { this() {} } // [warn]: Constructor marked with '@disabled' should not have a body - this() @disable {} // [warn]: Constructor marked with '@disabled' should not have a body + @disable this() {} /+ + ^^ [warn]: Constructor marked with '@disabled' should not have a body +/ + @disable { this() {} } /+ + ^^ [warn]: Constructor marked with '@disabled' should not have a body +/ + this() @disable {} /+ + ^^ [warn]: Constructor marked with '@disabled' should not have a body +/ - @disable void doThing() {} // [warn]: Function marked with '@disabled' should not have a body - @disable doThing() {} // [warn]: Function marked with '@disabled' should not have a body - @disable { void doThing() {} } // [warn]: Function marked with '@disabled' should not have a body - void doThing() @disable {} // [warn]: Function marked with '@disabled' should not have a body + @disable void doThing() {} /+ + ^^ [warn]: Function marked with '@disabled' should not have a body +/ + @disable doThing() {} /+ + ^^ [warn]: Function marked with '@disabled' should not have a body +/ + @disable { void doThing() {} } /+ + ^^ [warn]: Function marked with '@disabled' should not have a body +/ + void doThing() @disable {} /+ + ^^ [warn]: Function marked with '@disabled' should not have a body +/ - @disable ~this() {} // [warn]: Destructor marked with '@disabled' should not have a body - @disable { ~this() {} } // [warn]: Destructor marked with '@disabled' should not have a body - ~this() @disable {} // [warn]: Destructor marked with '@disabled' should not have a body + @disable ~this() {} /+ + ^^ [warn]: Destructor marked with '@disabled' should not have a body +/ + @disable { ~this() {} } /+ + ^^ [warn]: Destructor marked with '@disabled' should not have a body +/ + ~this() @disable {} /+ + ^^ [warn]: Destructor marked with '@disabled' should not have a body +/ @disable this(); @disable { this(); } diff --git a/src/dscanner/analysis/builtin_property_names.d b/src/dscanner/analysis/builtin_property_names.d index cadc220..7d66b38 100644 --- a/src/dscanner/analysis/builtin_property_names.d +++ b/src/dscanner/analysis/builtin_property_names.d @@ -42,7 +42,7 @@ final class BuiltinPropertyNameCheck : BaseAnalyzer { if (depth > 0 && isBuiltinProperty(fd.name.text)) { - addErrorMessage(fd.name.line, fd.name.column, KEY, generateErrorMessage(fd.name.text)); + addErrorMessage(fd.name, KEY, generateErrorMessage(fd.name.text)); } fd.accept(this); } @@ -62,14 +62,14 @@ final class BuiltinPropertyNameCheck : BaseAnalyzer foreach (i; ad.parts.map!(a => a.identifier)) { if (isBuiltinProperty(i.text)) - addErrorMessage(i.line, i.column, KEY, generateErrorMessage(i.text)); + addErrorMessage(i, KEY, generateErrorMessage(i.text)); } } override void visit(const Declarator d) { if (depth > 0 && isBuiltinProperty(d.name.text)) - addErrorMessage(d.name.line, d.name.column, KEY, generateErrorMessage(d.name.text)); + addErrorMessage(d.name, KEY, generateErrorMessage(d.name.text)); } override void visit(const StructBody sb) @@ -111,9 +111,12 @@ unittest assertAnalyzerWarnings(q{ class SomeClass { - void init(); // [warn]: Avoid naming members 'init'. This can confuse code that depends on the '.init' property of a type. - int init; // [warn]: Avoid naming members 'init'. This can confuse code that depends on the '.init' property of a type. - auto init = 10; // [warn]: Avoid naming members 'init'. This can confuse code that depends on the '.init' property of a type. + void init(); /+ + ^^^^ [warn]: Avoid naming members 'init'. This can confuse code that depends on the '.init' property of a type. +/ + int init; /+ + ^^^^ [warn]: Avoid naming members 'init'. This can confuse code that depends on the '.init' property of a type. +/ + auto init = 10; /+ + ^^^^ [warn]: Avoid naming members 'init'. This can confuse code that depends on the '.init' property of a type. +/ } }c, sac); diff --git a/src/dscanner/analysis/comma_expression.d b/src/dscanner/analysis/comma_expression.d index 45c7d68..aa53803 100644 --- a/src/dscanner/analysis/comma_expression.d +++ b/src/dscanner/analysis/comma_expression.d @@ -28,7 +28,7 @@ final class CommaExpressionCheck : BaseAnalyzer { if (ex.items.length > 1 && interest > 0) { - addErrorMessage(ex.line, ex.column, KEY, "Avoid using the comma expression."); + addErrorMessage(ex, KEY, "Avoid using the comma expression."); } ex.accept(this); } diff --git a/src/dscanner/analysis/constructors.d b/src/dscanner/analysis/constructors.d index 2a65e06..d163e09 100644 --- a/src/dscanner/analysis/constructors.d +++ b/src/dscanner/analysis/constructors.d @@ -3,6 +3,7 @@ module dscanner.analysis.constructors; import dparse.ast; import dparse.lexer; import std.stdio; +import std.typecons : Rebindable; import dscanner.analysis.base; import dscanner.analysis.helpers; import dsymbol.scope_ : Scope; @@ -20,19 +21,25 @@ final class ConstructorCheck : BaseAnalyzer override void visit(const ClassDeclaration classDeclaration) { - immutable bool oldHasDefault = hasDefaultArgConstructor; - immutable bool oldHasNoArg = hasNoArgConstructor; - hasNoArgConstructor = false; - hasDefaultArgConstructor = false; + const oldHasDefault = hasDefaultArgConstructor; + const oldHasNoArg = hasNoArgConstructor; + hasNoArgConstructor = null; + hasDefaultArgConstructor = null; immutable State prev = state; state = State.inClass; classDeclaration.accept(this); if (hasNoArgConstructor && hasDefaultArgConstructor) { - addErrorMessage(classDeclaration.name.line, - classDeclaration.name.column, "dscanner.confusing.constructor_args", + addErrorMessage( + Message.Diagnostic.from(fileName, classDeclaration.name, "This class has a zero-argument constructor as well as a" - ~ " constructor with one default argument. This can be confusing."); + ~ " constructor with one default argument. This can be confusing."), + [ + Message.Diagnostic.from(fileName, hasNoArgConstructor, "zero-argument constructor defined here"), + Message.Diagnostic.from(fileName, hasDefaultArgConstructor, "default argument constructor defined here") + ], + "dscanner.confusing.constructor_args" + ); } hasDefaultArgConstructor = oldHasDefault; hasNoArgConstructor = oldHasNoArg; @@ -55,7 +62,11 @@ final class ConstructorCheck : BaseAnalyzer if (constructor.parameters.parameters.length == 1 && constructor.parameters.parameters[0].default_ !is null) { - addErrorMessage(constructor.line, constructor.column, + const(Token)[] tokens = constructor.parameters.parameters[0].default_.tokens; + assert(tokens.length); + // we extend the token range to the `=` sign, since it's continuous + tokens = (tokens.ptr - 1)[0 .. tokens.length + 1]; + addErrorMessage(tokens, "dscanner.confusing.struct_constructor_default_args", "This struct constructor can never be called with its " ~ "default argument."); @@ -65,10 +76,10 @@ final class ConstructorCheck : BaseAnalyzer if (constructor.parameters.parameters.length == 1 && constructor.parameters.parameters[0].default_ !is null) { - hasDefaultArgConstructor = true; + hasDefaultArgConstructor = constructor; } else if (constructor.parameters.parameters.length == 0) - hasNoArgConstructor = true; + hasNoArgConstructor = constructor; break; case State.ignoring: break; @@ -86,8 +97,8 @@ private: State state; - bool hasNoArgConstructor; - bool hasDefaultArgConstructor; + Rebindable!(const Constructor) hasNoArgConstructor; + Rebindable!(const Constructor) hasDefaultArgConstructor; } unittest @@ -96,8 +107,10 @@ unittest StaticAnalysisConfig sac = disabledConfig(); sac.constructor_check = Check.enabled; + // TODO: test supplemental diagnostics assertAnalyzerWarnings(q{ - class Cat // [warn]: This class has a zero-argument constructor as well as a constructor with one default argument. This can be confusing. + class Cat /+ + ^^^ [warn]: This class has a zero-argument constructor as well as a constructor with one default argument. This can be confusing. +/ { this() {} this(string name = "kittie") {} @@ -106,7 +119,8 @@ unittest struct Dog { this() {} - this(string name = "doggie") {} // [warn]: This struct constructor can never be called with its default argument. + this(string name = "doggie") {} /+ + ^^^^^^^^^^ [warn]: This struct constructor can never be called with its default argument. +/ } }c, sac); diff --git a/src/dscanner/analysis/cyclomatic_complexity.d b/src/dscanner/analysis/cyclomatic_complexity.d index 56b2035..31d60db 100644 --- a/src/dscanner/analysis/cyclomatic_complexity.d +++ b/src/dscanner/analysis/cyclomatic_complexity.d @@ -105,7 +105,7 @@ final class CyclomaticComplexityCheck : BaseAnalyzer inLoop.length--; } fun.functionBody.accept(this); - testComplexity(fun.name.line, fun.name.column); + testComplexity(fun.name); } override void visit(const Unittest unittest_) @@ -120,7 +120,7 @@ final class CyclomaticComplexityCheck : BaseAnalyzer inLoop.length--; } unittest_.accept(this); - testComplexity(unittest_.line, unittest_.column); + testComplexity(unittest_.findTokenForDisplay(tok!"unittest")); } } @@ -129,12 +129,12 @@ private: int[] complexityStack = [0]; bool[] inLoop = [false]; - void testComplexity(size_t line, size_t column) + void testComplexity(T)(T annotatable) { auto complexity = complexityStack[$ - 1]; if (complexity > maxCyclomaticComplexity) { - addErrorMessage(line, column, KEY, format!MESSAGE(complexity)); + addErrorMessage(annotatable, KEY, format!MESSAGE(complexity)); } } @@ -169,24 +169,28 @@ unittest sac.cyclomatic_complexity = Check.enabled; sac.max_cyclomatic_complexity = 0; assertAnalyzerWarnings(q{ -unittest // [warn]: Cyclomatic complexity of this function is 1. +unittest /+ +^^^^^^^^ [warn]: Cyclomatic complexity of this function is 1. +/ { } -unittest // [warn]: Cyclomatic complexity of this function is 1. +unittest /+ +^^^^^^^^ [warn]: Cyclomatic complexity of this function is 1. +/ { writeln("hello"); writeln("world"); } -void main(string[] args) // [warn]: Cyclomatic complexity of this function is 3. +void main(string[] args) /+ + ^^^^ [warn]: Cyclomatic complexity of this function is 3. +/ { if (!args.length) return; writeln("hello ", args); } -unittest // [warn]: Cyclomatic complexity of this function is 1. +unittest /+ +^^^^^^^^ [warn]: Cyclomatic complexity of this function is 1. +/ { // static if / static foreach does not increase cyclomatic complexity static if (stuff) @@ -194,7 +198,8 @@ unittest // [warn]: Cyclomatic complexity of this function is 1. int a; } -unittest // [warn]: Cyclomatic complexity of this function is 2. +unittest /+ +^^^^^^^^ [warn]: Cyclomatic complexity of this function is 2. +/ { foreach (i; 0 .. 2) { @@ -202,7 +207,8 @@ unittest // [warn]: Cyclomatic complexity of this function is 2. int a; } -unittest // [warn]: Cyclomatic complexity of this function is 3. +unittest /+ +^^^^^^^^ [warn]: Cyclomatic complexity of this function is 3. +/ { foreach (i; 0 .. 2) { @@ -211,7 +217,8 @@ unittest // [warn]: Cyclomatic complexity of this function is 3. int a; } -unittest // [warn]: Cyclomatic complexity of this function is 2. +unittest /+ +^^^^^^^^ [warn]: Cyclomatic complexity of this function is 2. +/ { switch (x) { @@ -223,7 +230,8 @@ unittest // [warn]: Cyclomatic complexity of this function is 2. int a; } -bool shouldRun(check : BaseAnalyzer)( // [warn]: Cyclomatic complexity of this function is 20. +bool shouldRun(check : BaseAnalyzer)( /+ + ^^^^^^^^^ [warn]: Cyclomatic complexity of this function is 20. +/ string moduleName, const ref StaticAnalysisConfig config) { enum string a = check.name; diff --git a/src/dscanner/analysis/del.d b/src/dscanner/analysis/del.d index 99ca640..e32aba3 100644 --- a/src/dscanner/analysis/del.d +++ b/src/dscanner/analysis/del.d @@ -27,7 +27,7 @@ final class DeleteCheck : BaseAnalyzer override void visit(const DeleteExpression d) { - addErrorMessage(d.line, d.column, "dscanner.deprecated.delete_keyword", + addErrorMessage(d.tokens[0], "dscanner.deprecated.delete_keyword", "Avoid using the 'delete' keyword."); d.accept(this); } @@ -44,10 +44,12 @@ unittest void testDelete() { int[int] data = [1 : 2]; - delete data[1]; // [warn]: Avoid using the 'delete' keyword. + delete data[1]; /+ + ^^^^^^ [warn]: Avoid using the 'delete' keyword. +/ auto a = new Class(); - delete a; // [warn]: Avoid using the 'delete' keyword. + delete a; /+ + ^^^^^^ [warn]: Avoid using the 'delete' keyword. +/ } }c, sac); diff --git a/src/dscanner/analysis/duplicate_attribute.d b/src/dscanner/analysis/duplicate_attribute.d index 539da98..59b5afe 100644 --- a/src/dscanner/analysis/duplicate_attribute.d +++ b/src/dscanner/analysis/duplicate_attribute.d @@ -46,18 +46,18 @@ final class DuplicateAttributeCheck : BaseAnalyzer // Check the attributes foreach (attribute; node.attributes) { - size_t line, column; - string attributeName = getAttributeName(attribute, line, column); - if (!attributeName || line == 0 || column == 0) + const(Token)[] tokens; + string attributeName = getAttributeName(attribute, tokens); + if (!attributeName || !tokens.length) return; // Check for the attributes - checkDuplicateAttribute(attributeName, "property", line, column, hasProperty); - checkDuplicateAttribute(attributeName, "safe", line, column, hasSafe); - checkDuplicateAttribute(attributeName, "trusted", line, column, hasTrusted); - checkDuplicateAttribute(attributeName, "system", line, column, hasSystem); - checkDuplicateAttribute(attributeName, "pure", line, column, hasPure); - checkDuplicateAttribute(attributeName, "nothrow", line, column, hasNoThrow); + checkDuplicateAttribute(attributeName, "property", tokens, hasProperty); + checkDuplicateAttribute(attributeName, "safe", tokens, hasSafe); + checkDuplicateAttribute(attributeName, "trusted", tokens, hasTrusted); + checkDuplicateAttribute(attributeName, "system", tokens, hasSystem); + checkDuplicateAttribute(attributeName, "pure", tokens, hasPure); + checkDuplicateAttribute(attributeName, "nothrow", tokens, hasNoThrow); } // Just return if missing function nodes @@ -67,23 +67,23 @@ final class DuplicateAttributeCheck : BaseAnalyzer // Check the functions foreach (memberFunctionAttribute; node.functionDeclaration.memberFunctionAttributes) { - size_t line, column; - string attributeName = getAttributeName(memberFunctionAttribute, line, column); - if (!attributeName || line == 0 || column == 0) + const(Token)[] tokens; + string attributeName = getAttributeName(memberFunctionAttribute, tokens); + if (!attributeName || !tokens.length) return; // Check for the attributes - checkDuplicateAttribute(attributeName, "property", line, column, hasProperty); - checkDuplicateAttribute(attributeName, "safe", line, column, hasSafe); - checkDuplicateAttribute(attributeName, "trusted", line, column, hasTrusted); - checkDuplicateAttribute(attributeName, "system", line, column, hasSystem); - checkDuplicateAttribute(attributeName, "pure", line, column, hasPure); - checkDuplicateAttribute(attributeName, "nothrow", line, column, hasNoThrow); + checkDuplicateAttribute(attributeName, "property", tokens, hasProperty); + checkDuplicateAttribute(attributeName, "safe", tokens, hasSafe); + checkDuplicateAttribute(attributeName, "trusted", tokens, hasTrusted); + checkDuplicateAttribute(attributeName, "system", tokens, hasSystem); + checkDuplicateAttribute(attributeName, "pure", tokens, hasPure); + checkDuplicateAttribute(attributeName, "nothrow", tokens, hasNoThrow); } } void checkDuplicateAttribute(const string attributeName, - const string attributeDesired, size_t line, size_t column, ref bool hasAttribute) + const string attributeDesired, const(Token)[] tokens, ref bool hasAttribute) { // Just return if not an attribute if (attributeName != attributeDesired) @@ -93,14 +93,14 @@ final class DuplicateAttributeCheck : BaseAnalyzer if (hasAttribute) { string message = "Attribute '%s' is duplicated.".format(attributeName); - addErrorMessage(line, column, "dscanner.unnecessary.duplicate_attribute", message); + addErrorMessage(tokens, "dscanner.unnecessary.duplicate_attribute", message); } // Mark it as having that attribute hasAttribute = true; } - string getAttributeName(const Attribute attribute, ref size_t line, ref size_t column) + string getAttributeName(const Attribute attribute, ref const(Token)[] outTokens) { // Get the name from the attribute identifier if (attribute && attribute.atAttribute && attribute.atAttribute.identifier !is Token.init @@ -108,16 +108,14 @@ final class DuplicateAttributeCheck : BaseAnalyzer && attribute.atAttribute.identifier.text.length) { auto token = attribute.atAttribute.identifier; - line = token.line; - column = token.column; + outTokens = attribute.atAttribute.tokens; return token.text; } // Get the attribute from the storage class token if (attribute && attribute.attribute.type != tok!"") { - line = attribute.attribute.line; - column = attribute.attribute.column; + outTokens = attribute.tokens; return attribute.attribute.type.str; } @@ -125,14 +123,14 @@ final class DuplicateAttributeCheck : BaseAnalyzer } string getAttributeName(const MemberFunctionAttribute memberFunctionAttribute, - ref size_t line, ref size_t column) + ref const(Token)[] outTokens) { // Get the name from the tokenType if (memberFunctionAttribute && memberFunctionAttribute.tokenType !is IdType.init && memberFunctionAttribute.tokenType.str && memberFunctionAttribute.tokenType.str.length) { - // FIXME: How do we get the line/column number? + outTokens = memberFunctionAttribute.tokens; return memberFunctionAttribute.tokenType.str; } @@ -144,8 +142,7 @@ final class DuplicateAttributeCheck : BaseAnalyzer && memberFunctionAttribute.atAttribute.identifier.text.length) { auto iden = memberFunctionAttribute.atAttribute.identifier; - line = iden.line; - column = iden.column; + outTokens = memberFunctionAttribute.atAttribute.tokens; return iden.text; } @@ -168,25 +165,29 @@ unittest } // Duplicate before - @property @property bool aaa() // [warn]: Attribute 'property' is duplicated. + @property @property bool aaa() /+ + ^^^^^^^^^ [warn]: Attribute 'property' is duplicated. +/ { return false; } // Duplicate after - bool bbb() @safe @safe // [warn]: Attribute 'safe' is duplicated. + bool bbb() @safe @safe /+ + ^^^^^ [warn]: Attribute 'safe' is duplicated. +/ { return false; } // Duplicate before and after - @system bool ccc() @system // [warn]: Attribute 'system' is duplicated. + @system bool ccc() @system /+ + ^^^^^^^ [warn]: Attribute 'system' is duplicated. +/ { return false; } // Duplicate before and after - @trusted bool ddd() @trusted // [warn]: Attribute 'trusted' is duplicated. + @trusted bool ddd() @trusted /+ + ^^^^^^^^ [warn]: Attribute 'trusted' is duplicated. +/ { return false; } @@ -199,24 +200,26 @@ unittest return false; } - pure pure bool bbb() // [warn]: Attribute 'pure' is duplicated. + pure pure bool bbb() /+ + ^^^^ [warn]: Attribute 'pure' is duplicated. +/ { return false; } - // FIXME: There is no way to get the line/column number of the attribute like this - bool ccc() pure pure // FIXME: [warn]: Attribute 'pure' is duplicated. + bool ccc() pure pure /+ + ^^^^ [warn]: Attribute 'pure' is duplicated. +/ { return false; } - nothrow nothrow bool ddd() // [warn]: Attribute 'nothrow' is duplicated. + nothrow nothrow bool ddd() /+ + ^^^^^^^ [warn]: Attribute 'nothrow' is duplicated. +/ { return false; } - // FIXME: There is no way to get the line/column number of the attribute like this - bool eee() nothrow nothrow // FIXME: [warn]: Attribute 'nothrow' is duplicated. + bool eee() nothrow nothrow /+ + ^^^^^^^ [warn]: Attribute 'nothrow' is duplicated. +/ { return false; } diff --git a/src/dscanner/analysis/enumarrayliteral.d b/src/dscanner/analysis/enumarrayliteral.d index c52c726..68f973f 100644 --- a/src/dscanner/analysis/enumarrayliteral.d +++ b/src/dscanner/analysis/enumarrayliteral.d @@ -45,7 +45,7 @@ final class EnumArrayLiteralCheck : BaseAnalyzer continue; if (part.initializer.nonVoidInitializer.arrayInitializer is null) continue; - addErrorMessage(part.identifier.line, part.identifier.column, + addErrorMessage(part.initializer.nonVoidInitializer, "dscanner.performance.enum_array_literal", "This enum may lead to unnecessary allocation at run-time." ~ " Use 'static immutable " diff --git a/src/dscanner/analysis/explicitly_annotated_unittests.d b/src/dscanner/analysis/explicitly_annotated_unittests.d index c5afc01..680c959 100644 --- a/src/dscanner/analysis/explicitly_annotated_unittests.d +++ b/src/dscanner/analysis/explicitly_annotated_unittests.d @@ -44,7 +44,7 @@ final class ExplicitlyAnnotatedUnittestCheck : BaseAnalyzer } } if (!isSafeOrSystem) - addErrorMessage(decl.unittest_.line, decl.unittest_.column, KEY, MESSAGE); + addErrorMessage(decl.unittest_.findTokenForDisplay(tok!"unittest"), KEY, MESSAGE); } decl.accept(this); } @@ -68,8 +68,10 @@ unittest @system unittest {} pure nothrow @system @nogc unittest {} - unittest {} // [warn]: %s - pure nothrow @nogc unittest {} // [warn]: %s + unittest {} /+ + ^^^^^^^^ [warn]: %s +/ + pure nothrow @nogc unittest {} /+ + ^^^^^^^^ [warn]: %s +/ }c.format( ExplicitlyAnnotatedUnittestCheck.MESSAGE, ExplicitlyAnnotatedUnittestCheck.MESSAGE, @@ -82,8 +84,10 @@ unittest @safe unittest {} @system unittest {} - unittest {} // [warn]: %s - pure nothrow @nogc unittest {} // [warn]: %s + unittest {} /+ + ^^^^^^^^ [warn]: %s +/ + pure nothrow @nogc unittest {} /+ + ^^^^^^^^ [warn]: %s +/ } }c.format( ExplicitlyAnnotatedUnittestCheck.MESSAGE, diff --git a/src/dscanner/analysis/final_attribute.d b/src/dscanner/analysis/final_attribute.d index 8df2adb..66cc284 100644 --- a/src/dscanner/analysis/final_attribute.d +++ b/src/dscanner/analysis/final_attribute.d @@ -54,12 +54,10 @@ private: bool _blockStatic; Parent _parent = Parent.module_; - void addError(T)(T t, string msg) + void addError(T)(const Token finalToken, T t, string msg) { import std.format : format; - const size_t lne = t.name.line; - const size_t col = t.name.column; - addErrorMessage(lne, col, KEY, MSGB.format(msg)); + addErrorMessage(finalToken.type ? finalToken : t.name, KEY, MSGB.format(msg)); } public: @@ -178,6 +176,11 @@ public: const bool isFinal = d.attributes .canFind!(a => a.attribute.type == tok!"final"); + const Token finalToken = isFinal + ? d.attributes + .filter!(a => a.attribute.type == tok!"final") + .front.attribute + : Token.init; const bool isStaticOnce = d.attributes .canFind!(a => a.attribute.type == tok!"static"); @@ -203,9 +206,9 @@ public: if (_finalAggregate && savedParent == Parent.module_) { if (d.structDeclaration) - addError(d.structDeclaration, MESSAGE.struct_i); + addError(finalToken, d.structDeclaration, MESSAGE.struct_i); else if (d.unionDeclaration) - addError(d.unionDeclaration, MESSAGE.union_i); + addError(finalToken, d.unionDeclaration, MESSAGE.union_i); } } @@ -220,29 +223,29 @@ public: { case Parent.class_: if (fd.templateParameters) - addError(fd, MESSAGE.class_t); + addError(finalToken, fd, MESSAGE.class_t); if (isPrivate) - addError(fd, MESSAGE.class_p); + addError(finalToken, fd, MESSAGE.class_p); else if (isStaticOnce || _alwaysStatic || _blockStatic) - addError(fd, MESSAGE.class_s); + addError(finalToken, fd, MESSAGE.class_s); else if (_finalAggregate) - addError(fd, MESSAGE.class_f); + addError(finalToken, fd, MESSAGE.class_f); break; case Parent.interface_: if (fd.templateParameters) - addError(fd, MESSAGE.interface_t); + addError(finalToken, fd, MESSAGE.interface_t); break; case Parent.struct_: - addError(fd, MESSAGE.struct_f); + addError(finalToken, fd, MESSAGE.struct_f); break; case Parent.union_: - addError(fd, MESSAGE.union_f); + addError(finalToken, fd, MESSAGE.union_f); break; case Parent.function_: - addError(fd, MESSAGE.func_n); + addError(finalToken, fd, MESSAGE.func_n); break; case Parent.module_: - addError(fd, MESSAGE.func_g); + addError(finalToken, fd, MESSAGE.func_g); break; } } @@ -317,13 +320,15 @@ public: // fail assertAnalyzerWarnings(q{ - final void foo(){} // [warn]: %s + final void foo(){} /+ + ^^^^^ [warn]: %s +/ }c.format( FinalAttributeChecker.MSGB.format(FinalAttributeChecker.MESSAGE.func_g) ), sac); assertAnalyzerWarnings(q{ - void foo(){final void foo(){}} // [warn]: %s + void foo(){final void foo(){}} /+ + ^^^^^ [warn]: %s +/ }c.format( FinalAttributeChecker.MSGB.format(FinalAttributeChecker.MESSAGE.func_n) ), sac); @@ -332,56 +337,65 @@ public: void foo() { static if (true) - final class A{ private: final protected void foo(){}} // [warn]: %s + final class A{ private: final protected void foo(){}} /+ + ^^^^^ [warn]: %s +/ } }c.format( FinalAttributeChecker.MSGB.format(FinalAttributeChecker.MESSAGE.class_f) ), sac); assertAnalyzerWarnings(q{ - final struct Foo{} // [warn]: %s + final struct Foo{} /+ + ^^^^^ [warn]: %s +/ }c.format( FinalAttributeChecker.MSGB.format(FinalAttributeChecker.MESSAGE.struct_i) ), sac); assertAnalyzerWarnings(q{ - final union Foo{} // [warn]: %s + final union Foo{} /+ + ^^^^^ [warn]: %s +/ }c.format( FinalAttributeChecker.MSGB.format(FinalAttributeChecker.MESSAGE.union_i) ), sac); assertAnalyzerWarnings(q{ - class Foo{private final void foo(){}} // [warn]: %s + class Foo{private final void foo(){}} /+ + ^^^^^ [warn]: %s +/ }c.format( FinalAttributeChecker.MSGB.format(FinalAttributeChecker.MESSAGE.class_p) ), sac); assertAnalyzerWarnings(q{ - class Foo{private: final void foo(){}} // [warn]: %s + class Foo{private: final void foo(){}} /+ + ^^^^^ [warn]: %s +/ }c.format( FinalAttributeChecker.MSGB.format(FinalAttributeChecker.MESSAGE.class_p) ), sac); assertAnalyzerWarnings(q{ - interface Foo{final void foo(T)(){}} // [warn]: %s + interface Foo{final void foo(T)(){}} /+ + ^^^^^ [warn]: %s +/ }c.format( FinalAttributeChecker.MSGB.format(FinalAttributeChecker.MESSAGE.interface_t) ), sac); assertAnalyzerWarnings(q{ - final class Foo{final void foo(){}} // [warn]: %s + final class Foo{final void foo(){}} /+ + ^^^^^ [warn]: %s +/ }c.format( FinalAttributeChecker.MSGB.format(FinalAttributeChecker.MESSAGE.class_f) ), sac); assertAnalyzerWarnings(q{ - private: final class Foo {public: private final void foo(){}} // [warn]: %s + private: final class Foo {public: private final void foo(){}} /+ + ^^^^^ [warn]: %s +/ }c.format( FinalAttributeChecker.MSGB.format(FinalAttributeChecker.MESSAGE.class_p) ), sac); assertAnalyzerWarnings(q{ - class Foo {final static void foo(){}} // [warn]: %s + class Foo {final static void foo(){}} /+ + ^^^^^ [warn]: %s +/ }c.format( FinalAttributeChecker.MSGB.format(FinalAttributeChecker.MESSAGE.class_s) ), sac); @@ -390,7 +404,8 @@ public: class Foo { void foo(){} - static: final void foo(){} // [warn]: %s + static: final void foo(){} /+ + ^^^^^ [warn]: %s +/ } }c.format( FinalAttributeChecker.MSGB.format(FinalAttributeChecker.MESSAGE.class_s) @@ -400,7 +415,8 @@ public: class Foo { void foo(){} - static{ final void foo(){}} // [warn]: %s + static{ final void foo(){}} /+ + ^^^^^ [warn]: %s +/ void foo(){} } }c.format( diff --git a/src/dscanner/analysis/fish.d b/src/dscanner/analysis/fish.d index 0417e48..d31dcb3 100644 --- a/src/dscanner/analysis/fish.d +++ b/src/dscanner/analysis/fish.d @@ -34,7 +34,7 @@ final class FloatOperatorCheck : BaseAnalyzer || r.operator == tok!"!<" || r.operator == tok!"!<>=" || r.operator == tok!"!>=" || r.operator == tok!"!<=") { - addErrorMessage(r.line, r.column, KEY, + addErrorMessage(r, KEY, "Avoid using the deprecated floating-point operators."); } r.accept(this); @@ -52,14 +52,22 @@ unittest { float z = 1.5f; bool a; - a = z !<>= z; // [warn]: Avoid using the deprecated floating-point operators. - a = z !<> z; // [warn]: Avoid using the deprecated floating-point operators. - a = z <> z; // [warn]: Avoid using the deprecated floating-point operators. - a = z <>= z; // [warn]: Avoid using the deprecated floating-point operators. - a = z !> z; // [warn]: Avoid using the deprecated floating-point operators. - a = z !>= z; // [warn]: Avoid using the deprecated floating-point operators. - a = z !< z; // [warn]: Avoid using the deprecated floating-point operators. - a = z !<= z; // [warn]: Avoid using the deprecated floating-point operators. + a = z !<>= z; /+ + ^^^^^^^^ [warn]: Avoid using the deprecated floating-point operators. +/ + a = z !<> z; /+ + ^^^^^^^ [warn]: Avoid using the deprecated floating-point operators. +/ + a = z <> z; /+ + ^^^^^^ [warn]: Avoid using the deprecated floating-point operators. +/ + a = z <>= z; /+ + ^^^^^^^ [warn]: Avoid using the deprecated floating-point operators. +/ + a = z !> z; /+ + ^^^^^^ [warn]: Avoid using the deprecated floating-point operators. +/ + a = z !>= z; /+ + ^^^^^^^ [warn]: Avoid using the deprecated floating-point operators. +/ + a = z !< z; /+ + ^^^^^^ [warn]: Avoid using the deprecated floating-point operators. +/ + a = z !<= z; /+ + ^^^^^^^ [warn]: Avoid using the deprecated floating-point operators. +/ } }c, sac); diff --git a/src/dscanner/analysis/function_attributes.d b/src/dscanner/analysis/function_attributes.d index fe6b151..1560850 100644 --- a/src/dscanner/analysis/function_attributes.d +++ b/src/dscanner/analysis/function_attributes.d @@ -81,8 +81,7 @@ final class FunctionAttributeCheck : BaseAnalyzer { if (inInterface && dec.attribute.attribute == tok!"abstract") { - addErrorMessage(dec.attribute.attribute.line, - dec.attribute.attribute.column, KEY, ABSTRACT_MESSAGE); + addErrorMessage(dec.attribute, KEY, ABSTRACT_MESSAGE); } } @@ -105,7 +104,7 @@ final class FunctionAttributeCheck : BaseAnalyzer } if (foundProperty && !foundConst) { - addErrorMessage(dec.name.line, dec.name.column, KEY, + addErrorMessage(dec.name, KEY, "Zero-parameter '@property' function should be" ~ " marked 'const', 'inout', or 'immutable'."); } @@ -124,7 +123,7 @@ final class FunctionAttributeCheck : BaseAnalyzer continue; if (attr.attribute == tok!"abstract" && inInterface) { - addErrorMessage(attr.attribute.line, attr.attribute.column, KEY, ABSTRACT_MESSAGE); + addErrorMessage(attr.attribute, KEY, ABSTRACT_MESSAGE); continue; } if (attr.attribute == tok!"static") @@ -137,8 +136,7 @@ final class FunctionAttributeCheck : BaseAnalyzer import std.string : format; immutable string attrString = str(attr.attribute.type); - addErrorMessage(dec.functionDeclaration.name.line, - dec.functionDeclaration.name.column, KEY, format( + addErrorMessage(attr.attribute, KEY, format( "'%s' is not an attribute of the return type." ~ " Place it after the parameter list to clarify.", attrString)); } @@ -172,31 +170,37 @@ unittest int foo() @property { return 0; } class ClassName { - const int confusingConst() { return 0; } // [warn]: 'const' is not an attribute of the return type. Place it after the parameter list to clarify. + const int confusingConst() { return 0; } /+ + ^^^^^ [warn]: 'const' is not an attribute of the return type. Place it after the parameter list to clarify. +/ - int bar() @property { return 0; } // [warn]: Zero-parameter '@property' function should be marked 'const', 'inout', or 'immutable'. + int bar() @property { return 0; } /+ + ^^^ [warn]: Zero-parameter '@property' function should be marked 'const', 'inout', or 'immutable'. +/ static int barStatic() @property { return 0; } int barConst() const @property { return 0; } } struct StructName { - int bar() @property { return 0; } // [warn]: Zero-parameter '@property' function should be marked 'const', 'inout', or 'immutable'. + int bar() @property { return 0; } /+ + ^^^ [warn]: Zero-parameter '@property' function should be marked 'const', 'inout', or 'immutable'. +/ static int barStatic() @property { return 0; } int barConst() const @property { return 0; } } union UnionName { - int bar() @property { return 0; } // [warn]: Zero-parameter '@property' function should be marked 'const', 'inout', or 'immutable'. + int bar() @property { return 0; } /+ + ^^^ [warn]: Zero-parameter '@property' function should be marked 'const', 'inout', or 'immutable'. +/ static int barStatic() @property { return 0; } int barConst() const @property { return 0; } } interface InterfaceName { - int bar() @property; // [warn]: Zero-parameter '@property' function should be marked 'const', 'inout', or 'immutable'. + int bar() @property; /+ + ^^^ [warn]: Zero-parameter '@property' function should be marked 'const', 'inout', or 'immutable'. +/ static int barStatic() @property { return 0; } int barConst() const @property; - abstract int method(); // [warn]: 'abstract' attribute is redundant in interface declarations + abstract int method(); /+ + ^^^^^^^^ [warn]: 'abstract' attribute is redundant in interface declarations +/ } }c, sac); diff --git a/src/dscanner/analysis/has_public_example.d b/src/dscanner/analysis/has_public_example.d index 0d82977..9c7747d 100644 --- a/src/dscanner/analysis/has_public_example.d +++ b/src/dscanner/analysis/has_public_example.d @@ -157,14 +157,14 @@ private: { foreach (property; possibleDeclarations) if (auto fn = mixin("decl." ~ property)) - addMessage(fn.name.line, fn.name.column, fn.name.text); + addMessage(fn.name.type ? [fn.name] : fn.tokens, fn.name.text); } - void addMessage(size_t line, size_t column, string name) + void addMessage(const Token[] tokens, string name) { import std.string : format; - addErrorMessage(line, column, "dscanner.style.has_public_example", name is null + addErrorMessage(tokens, "dscanner.style.has_public_example", name is null ? "Public declaration has no documented example." : format("Public declaration '%s' has no documented example.", name)); } @@ -220,27 +220,33 @@ unittest // enums or variables don't need to have public unittest assertAnalyzerWarnings(q{ /// C - class C{} // [warn]: Public declaration 'C' has no documented example. + class C{} /+ + ^ [warn]: Public declaration 'C' has no documented example. +/ unittest {} /// I - interface I{} // [warn]: Public declaration 'I' has no documented example. + interface I{} /+ + ^ [warn]: Public declaration 'I' has no documented example. +/ unittest {} /// f - void f(){} // [warn]: Public declaration 'f' has no documented example. + void f(){} /+ + ^ [warn]: Public declaration 'f' has no documented example. +/ unittest {} /// S - struct S{} // [warn]: Public declaration 'S' has no documented example. + struct S{} /+ + ^ [warn]: Public declaration 'S' has no documented example. +/ unittest {} /// T - template T(){} // [warn]: Public declaration 'T' has no documented example. + template T(){} /+ + ^ [warn]: Public declaration 'T' has no documented example. +/ unittest {} /// U - union U{} // [warn]: Public declaration 'U' has no documented example. + union U{} /+ + ^ [warn]: Public declaration 'U' has no documented example. +/ unittest {} }, sac); @@ -248,7 +254,8 @@ unittest assertAnalyzerWarnings(q{ unittest {} /// C - class C{} // [warn]: Public declaration 'C' has no documented example. + class C{} /+ + ^ [warn]: Public declaration 'C' has no documented example. +/ }, sac); // test documented module header unittest @@ -256,13 +263,15 @@ unittest /// unittest {} /// C - class C{} // [warn]: Public declaration 'C' has no documented example. + class C{} /+ + ^ [warn]: Public declaration 'C' has no documented example. +/ }, sac); // test multiple unittest blocks assertAnalyzerWarnings(q{ /// C - class C{} // [warn]: Public declaration 'C' has no documented example. + class C{} /+ + ^ [warn]: Public declaration 'C' has no documented example. +/ unittest {} unittest {} unittest {} @@ -318,7 +327,8 @@ unittest // test reset on private symbols (#500) assertAnalyzerWarnings(q{ /// - void dirName(C)(C[] path) {} // [warn]: Public declaration 'dirName' has no documented example. + void dirName(C)(C[] path) {} /+ + ^^^^^^^ [warn]: Public declaration 'dirName' has no documented example. +/ private void _dirName(R)(R path) {} /// unittest {} diff --git a/src/dscanner/analysis/helpers.d b/src/dscanner/analysis/helpers.d index 6886844..00e0ba2 100644 --- a/src/dscanner/analysis/helpers.d +++ b/src/dscanner/analysis/helpers.d @@ -43,7 +43,11 @@ S after(S)(S value, S separator) if (isSomeString!S) /** * This assert function will analyze the passed in code, get the warnings, * and make sure they match the warnings in the comments. Warnings are - * marked like so: // [warn]: Failed to do somethings. + * marked like so if range doesn't matter: // [warn]: Failed to do somethings. + * + * To test for start and end column, mark warnings as multi-line comments like + * this: /+ + * ^^^^^ [warn]: Failed to do somethings. +/ */ void assertAnalyzerWarnings(string code, const StaticAnalysisConfig config, string file = __FILE__, size_t line = __LINE__) @@ -62,12 +66,18 @@ void assertAnalyzerWarnings(string code, const StaticAnalysisConfig config, MessageSet rawWarnings = analyze("test", m, config, moduleCache, tokens); string[] codeLines = code.splitLines(); + struct FoundWarning + { + string msg; + size_t startColumn, endColumn; + } + // Get the warnings ordered by line - string[size_t] warnings; + FoundWarning[size_t] warnings; foreach (rawWarning; rawWarnings[]) { // Skip the warning if it is on line zero - immutable size_t rawLine = rawWarning.line; + immutable size_t rawLine = rawWarning.endLine; if (rawLine == 0) { stderr.writefln("!!! Skipping warning because it is on line zero:\n%s", @@ -76,28 +86,49 @@ void assertAnalyzerWarnings(string code, const StaticAnalysisConfig config, } size_t warnLine = line - 1 + rawLine; - warnings[warnLine] = format("[warn]: %s", rawWarning.message); + warnings[warnLine] = FoundWarning( + format("[warn]: %s", rawWarning.message), + rawWarning.startLine != rawWarning.endLine ? 1 : rawWarning.startColumn, + rawWarning.endColumn, + ); } // Get all the messages from the comments in the code - string[size_t] messages; + FoundWarning[size_t] messages; + bool lastLineStartedComment = false; foreach (i, codeLine; codeLines) { - // Skip if no [warn] comment - if (codeLine.indexOf("// [warn]:") == -1) - continue; - - // Skip if there is no comment or code - immutable string codePart = codeLine.before("// "); - immutable string commentPart = codeLine.after("// "); - if (!codePart.length || !commentPart.length) - continue; + scope (exit) + lastLineStartedComment = codeLine.stripRight.endsWith("/+", "/*") > 0; // Get the line of this code line size_t lineNo = i + line; - // Get the message - messages[lineNo] = commentPart; + if (codeLine.stripLeft.startsWith("^") && lastLineStartedComment) + { + auto start = codeLine.indexOf("^") + 1; + assert(start != 0); + auto end = codeLine.indexOfNeither("^", start) + 1; + assert(end != 0); + auto warn = codeLine.indexOf("[warn]:"); + assert(warn != -1, "malformed line, expected `[warn]: text` after `^^^^^` part"); + auto message = codeLine[warn .. $].stripRight; + if (message.endsWith("+/", "*/")) + message = message[0 .. $ - 2].stripRight; + messages[lineNo - 1] = FoundWarning(message, start, end); + } + // Skip if no [warn] comment + else if (codeLine.indexOf("// [warn]:") != -1) + { + // Skip if there is no comment or code + immutable string codePart = codeLine.before("// "); + immutable string commentPart = codeLine.after("// "); + if (!codePart.length || !commentPart.length) + continue; + + // Get the message + messages[lineNo] = FoundWarning(commentPart); + } } // Throw an assert error if any messages are not listed in the warnings @@ -111,12 +142,39 @@ void assertAnalyzerWarnings(string code, const StaticAnalysisConfig config, throw new AssertError(errors, file, lineNo); } // Different warning - else if (warnings[lineNo] != messages[lineNo]) + else if (warnings[lineNo].msg != messages[lineNo].msg) { immutable string errors = "Expected warning:\n%s\nBut was:\n%s\nFrom source code at (%s:?):\n%s".format( messages[lineNo], warnings[lineNo], lineNo, codeLines[lineNo - line]); throw new AssertError(errors, file, lineNo); } + + // specified column range + if ((message.startColumn || message.endColumn) + && warnings[lineNo] != message) + { + import std.algorithm : max; + import std.array : array; + import std.range : repeat; + import std.string : replace; + + const(char)[] expectedRange = ' '.repeat(max(0, cast(int)message.startColumn - 1)).array + ~ '^'.repeat(max(0, cast(int)(message.endColumn - message.startColumn))).array; + const(char)[] actualRange; + if (!warnings[lineNo].startColumn || warnings[lineNo].startColumn == warnings[lineNo].endColumn) + actualRange = "no column range defined!"; + else + actualRange = ' '.repeat(max(0, cast(int)warnings[lineNo].startColumn - 1)).array + ~ '^'.repeat(max(0, cast(int)(warnings[lineNo].endColumn - warnings[lineNo].startColumn))).array; + size_t paddingWidth = max(expectedRange.length, actualRange.length); + immutable string errors = "Wrong warning range: expected %s, but was %s\nFrom source code at (%s:?):\n%s\n%-*s <-- expected\n%-*s <-- actual".format( + [message.startColumn, message.endColumn], + [warnings[lineNo].startColumn, warnings[lineNo].endColumn], + lineNo, codeLines[lineNo - line].replace("\t", " "), + paddingWidth, expectedRange, + paddingWidth, actualRange); + throw new AssertError(errors, file, lineNo); + } } // Throw an assert error if there were any warnings that were not expected diff --git a/src/dscanner/analysis/if_constraints_indent.d b/src/dscanner/analysis/if_constraints_indent.d index 9757c07..25e8be1 100644 --- a/src/dscanner/analysis/if_constraints_indent.d +++ b/src/dscanner/analysis/if_constraints_indent.d @@ -109,17 +109,21 @@ private: void checkConstraintSpace(const Constraint constraint, size_t line) { + import std.algorithm : min; + // dscanner lines start at 1 auto pDecl = firstSymbolAtLine[line - 1]; // search for constraint if (might not be on the same line as the expression) auto r = firstSymbolAtLine[line .. constraint.expression.line].retro.filter!(s => s.isIf); + auto if_ = constraint.tokens.findTokenForDisplay(tok!"if")[0]; + // no hit = constraint is on the same line if (r.empty) - addErrorMessage(constraint.expression.line, constraint.expression.column, KEY, MESSAGE); + addErrorMessage(if_, KEY, MESSAGE); else if (pDecl.column != r.front.column) - addErrorMessage(constraint.expression.line, constraint.expression.column, KEY, MESSAGE); + addErrorMessage(if_.line, min(if_.column, pDecl.column), if_.column + 2, KEY, MESSAGE); } } @@ -138,8 +142,10 @@ void foo(R)(R r) if (R == null) {} +// note: since we are using tabs, the ^ look a bit off here. Use 1-wide tab stops to view tests. void foo(R)(R r) - if (R == null) // [warn]: %s + if (R == null) /+ +^^^ [warn]: %s +/ {} }c.format( IfConstraintsIndentCheck.MESSAGE, @@ -151,11 +157,13 @@ void foo(R)(R r) {} void foo(R)(R r) -if (R == null) // [warn]: %s +if (R == null) /+ +^^ [warn]: %s +/ {} void foo(R)(R r) - if (R == null) // [warn]: %s + if (R == null) /+ + ^^^ [warn]: %s +/ {} }c.format( IfConstraintsIndentCheck.MESSAGE, @@ -168,11 +176,13 @@ if (R == null) // [warn]: %s {} struct Foo(R) -if (R == null) // [warn]: %s +if (R == null) /+ +^^ [warn]: %s +/ {} struct Foo(R) - if (R == null) // [warn]: %s + if (R == null) /+ + ^^^ [warn]: %s +/ {} }c.format( IfConstraintsIndentCheck.MESSAGE, @@ -206,8 +216,9 @@ if (is(typeof(Num.init >= 0)) && is(typeof(-Num.init)) && {} struct Foo(R) -if - (R == null) // [warn]: %s +if /+ +^^ [warn]: %s +/ + (R == null) {} struct Foo(R) @@ -222,8 +233,9 @@ if {} struct Foo(R) - if ( - R == null // [warn]: %s + if ( /+ + ^^^ [warn]: %s +/ + R == null ) {} }c.format( IfConstraintsIndentCheck.MESSAGE, @@ -232,7 +244,8 @@ if // constraint on the same line assertAnalyzerWarnings(q{ - struct CRC(uint N, ulong P) if (N == 32 || N == 64) // [warn]: %s + struct CRC(uint N, ulong P) if (N == 32 || N == 64) /+ + ^^ [warn]: %s +/ {} }c.format( IfConstraintsIndentCheck.MESSAGE, diff --git a/src/dscanner/analysis/if_statements.d b/src/dscanner/analysis/if_statements.d index d469d6c..6687330 100644 --- a/src/dscanner/analysis/if_statements.d +++ b/src/dscanner/analysis/if_statements.d @@ -9,6 +9,7 @@ import dparse.lexer; import dparse.formatter; import dscanner.analysis.base; import dsymbol.scope_ : Scope; +import std.typecons : Rebindable, rebindable; final class IfStatementCheck : BaseAnalyzer { @@ -81,12 +82,12 @@ private: immutable size_t prevLocation = alreadyChecked(app.data, line, column); if (prevLocation != size_t.max) { - addErrorMessage(line, column, KEY, "Expression %s is true: already checked on line %d.".format( + addErrorMessage(expressions[prevLocation].astNode, KEY, "Expression %s is true: already checked on line %d.".format( expressions[prevLocation].formatted, expressions[prevLocation].line)); } else { - expressions ~= ExpressionInfo(app.data, line, column, depth); + expressions ~= ExpressionInfo(app.data, line, column, depth, (cast(const BaseNode) expression).rebindable); sort(expressions); } } @@ -124,4 +125,5 @@ private struct ExpressionInfo size_t line; size_t column; int depth; + Rebindable!(const BaseNode) astNode; } diff --git a/src/dscanner/analysis/ifelsesame.d b/src/dscanner/analysis/ifelsesame.d index e757b91..0c0ef47 100644 --- a/src/dscanner/analysis/ifelsesame.d +++ b/src/dscanner/analysis/ifelsesame.d @@ -34,8 +34,13 @@ final class IfElseSameCheck : BaseAnalyzer override void visit(const IfStatement ifStatement) { if (ifStatement.thenStatement && (ifStatement.thenStatement == ifStatement.elseStatement)) - addErrorMessage(ifStatement.line, ifStatement.column, + { + const(Token)[] tokens = ifStatement.elseStatement.tokens; + // extend 1 past, so we include the `else` token + tokens = (tokens.ptr - 1)[0 .. tokens.length + 1]; + addErrorMessage(tokens, "dscanner.bugs.if_else_same", "'Else' branch is identical to 'Then' branch."); + } ifStatement.accept(this); } @@ -45,7 +50,7 @@ final class IfElseSameCheck : BaseAnalyzer if (e !is null && assignExpression.operator == tok!"=" && e.ternaryExpression == assignExpression.ternaryExpression) { - addErrorMessage(assignExpression.line, assignExpression.column, "dscanner.bugs.self_assignment", + addErrorMessage(assignExpression, "dscanner.bugs.self_assignment", "Left side of assignment operatior is identical to the right side."); } assignExpression.accept(this); @@ -56,7 +61,7 @@ final class IfElseSameCheck : BaseAnalyzer if (andAndExpression.left !is null && andAndExpression.right !is null && andAndExpression.left == andAndExpression.right) { - addErrorMessage(andAndExpression.line, andAndExpression.column, + addErrorMessage(andAndExpression.right, "dscanner.bugs.logic_operator_operands", "Left side of logical and is identical to right side."); } @@ -68,7 +73,7 @@ final class IfElseSameCheck : BaseAnalyzer if (orOrExpression.left !is null && orOrExpression.right !is null && orOrExpression.left == orOrExpression.right) { - addErrorMessage(orOrExpression.line, orOrExpression.column, + addErrorMessage(orOrExpression.right, "dscanner.bugs.logic_operator_operands", "Left side of logical or is identical to right side."); } @@ -86,10 +91,13 @@ unittest void testSizeT() { string person = "unknown"; - if (person == "unknown") // [warn]: 'Else' branch is identical to 'Then' branch. - person = "bobrick"; // same + if (person == "unknown") + person = "bobrick"; /* same */ else - person = "bobrick"; // same + person = "bobrick"; /* same */ /+ +^^^^^^^^^^^^^^^^^^^^^^^ [warn]: 'Else' branch is identical to 'Then' branch. +/ + // note: above ^^^ line spans over multiple lines, so it starts at start of line, since we don't have any way to test this here + // look at the tests using 1-wide tab width for accurate visuals. if (person == "unknown") // ok person = "ricky"; // not same diff --git a/src/dscanner/analysis/imports_sortedness.d b/src/dscanner/analysis/imports_sortedness.d index 4f33fd4..2463814 100644 --- a/src/dscanner/analysis/imports_sortedness.d +++ b/src/dscanner/analysis/imports_sortedness.d @@ -46,19 +46,21 @@ final class ImportSortednessCheck : BaseAnalyzer if (id.importBindings is null || id.importBindings.importBinds.length == 0) { + bool suppress; foreach (singleImport; id.singleImports) { string importModuleName = singleImport.identifierChain.identifiers.map!`a.text`.join("."); - addImport(importModuleName, singleImport); + addImport(importModuleName, singleImport, null, suppress); } } else { string importModuleName = id.importBindings.singleImport.identifierChain.identifiers.map!`a.text`.join("."); + bool suppress; foreach (importBind; id.importBindings.importBinds) { - addImport(importModuleName ~ "-" ~ importBind.left.text, id.importBindings.singleImport); + addImport(importModuleName ~ "-" ~ importBind.left.text, importBind, id.importBindings.singleImport, suppress); } } } @@ -80,14 +82,29 @@ private: } } - void addImport(string importModuleName, const SingleImport singleImport) + void addImport(string importModuleName, const BaseNode range, const BaseNode parent, ref bool suppress) { + import std.algorithm : findSplit; + import std.string : indexOf; import std.uni : sicmp; if (imports[level].length > 0 && imports[level][$ -1].sicmp(importModuleName) > 0) { - addErrorMessage(singleImport.identifierChain.identifiers[0].line, - singleImport.identifierChain.identifiers[0].column, KEY, MESSAGE); + if (parent !is null) + { + auto parentEnd = importModuleName.indexOf("-"); + if (parentEnd != -1 && imports[level][$ -1].findSplit("-")[0].sicmp(importModuleName) > 0) + { + // mark module name as broken, not selected symbols, since it's the module name is not belonging here + if (!suppress) + addErrorMessage(parent, KEY, MESSAGE); + suppress = true; + return; + } + } + if (!suppress) + addErrorMessage(range, KEY, MESSAGE); + suppress = true; } else { @@ -113,7 +130,8 @@ unittest assertAnalyzerWarnings(q{ import foo.bar; - import bar.foo; // [warn]: %s + import bar.foo; /+ + ^^^^^^^ [warn]: %s +/ }c.format( ImportSortednessCheck.MESSAGE, ), sac); @@ -121,19 +139,36 @@ unittest assertAnalyzerWarnings(q{ import c; import c.b; - import c.a; // [warn]: %s + import c.a; /+ + ^^^ [warn]: %s +/ import d.a; - import d; // [warn]: %s + import d; /+ + ^ [warn]: %s +/ }c.format( ImportSortednessCheck.MESSAGE, ImportSortednessCheck.MESSAGE, ), sac); assertAnalyzerWarnings(q{ - import a.b, a.c, a.d; - import a.b, a.d, a.c; // [warn]: %s - import a.c, a.b, a.c; // [warn]: %s - import foo.bar, bar.foo; // [warn]: %s + unittest + { + import a.b, a.c, a.d; + } + unittest + { + import a.b, a.d, a.c; /+ + ^^^ [warn]: %s +/ + } + unittest + { + import a.c, a.b, a.c; /+ + ^^^ [warn]: %s +/ + } + unittest + { + import foo.bar, bar.foo; /+ + ^^^^^^^ [warn]: %s +/ + } }c.format( ImportSortednessCheck.MESSAGE, ImportSortednessCheck.MESSAGE, @@ -143,8 +178,10 @@ unittest // multiple items out of order assertAnalyzerWarnings(q{ import foo.bar; - import bar.foo; // [warn]: %s - import bar.bar.foo; // [warn]: %s + import bar.foo; /+ + ^^^^^^^ [warn]: %s +/ + import bar.bar.foo; /+ + ^^^^^^^^^^^ [warn]: %s +/ }c.format( ImportSortednessCheck.MESSAGE, ImportSortednessCheck.MESSAGE, @@ -158,14 +195,19 @@ unittest // selective imports assertAnalyzerWarnings(q{ import test : foo; - import test : bar; // [warn]: %s + import test : bar; /+ + ^^^ [warn]: %s +/ + import before : zzz; /+ + ^^^^^^ [warn]: %s +/ }c.format( ImportSortednessCheck.MESSAGE, + ImportSortednessCheck.MESSAGE, ), sac); // selective imports assertAnalyzerWarnings(q{ - import test : foo, bar; // [warn]: %s + import test : foo, bar; /+ + ^^^ [warn]: %s +/ }c.format( ImportSortednessCheck.MESSAGE, ), sac); @@ -173,8 +215,10 @@ unittest assertAnalyzerWarnings(q{ import b; import c : foo; - import c : bar; // [warn]: %s - import a; // [warn]: %s + import c : bar; /+ + ^^^ [warn]: %s +/ + import a; /+ + ^ [warn]: %s +/ }c.format( ImportSortednessCheck.MESSAGE, ImportSortednessCheck.MESSAGE, @@ -184,8 +228,10 @@ unittest import c; import c : bar; import d : bar; - import d; // [warn]: %s - import a : bar; // [warn]: %s + import d; /+ + ^ [warn]: %s +/ + import a : bar; /+ + ^ [warn]: %s +/ }c.format( ImportSortednessCheck.MESSAGE, ImportSortednessCheck.MESSAGE, @@ -199,8 +245,10 @@ unittest assertAnalyzerWarnings(q{ import t1 : a, b = foo; - import t1 : b, a = foo; // [warn]: %s - import t0 : a, b = foo; // [warn]: %s + import t1 : b, a = foo; /+ + ^^^^^^^ [warn]: %s +/ + import t0 : a, b = foo; /+ + ^^ [warn]: %s +/ }c.format( ImportSortednessCheck.MESSAGE, ImportSortednessCheck.MESSAGE, @@ -209,11 +257,13 @@ unittest // local imports in functions assertAnalyzerWarnings(q{ import t2; - import t1; // [warn]: %s + import t1; /+ + ^^ [warn]: %s +/ void foo() { import f2; - import f1; // [warn]: %s + import f1; /+ + ^^ [warn]: %s +/ import f3; } void bar() @@ -229,15 +279,18 @@ unittest // local imports in scopes assertAnalyzerWarnings(q{ import t2; - import t1; // [warn]: %s + import t1; /+ + ^^ [warn]: %s +/ void foo() { import f2; - import f1; // [warn]: %s + import f1; /+ + ^^ [warn]: %s +/ import f3; { import f2; - import f1; // [warn]: %s + import f1; /+ + ^^ [warn]: %s +/ import f3; } { @@ -255,15 +308,18 @@ unittest // local imports in functions assertAnalyzerWarnings(q{ import t2; - import t1; // [warn]: %s + import t1; /+ + ^^ [warn]: %s +/ void foo() { import f2; - import f1; // [warn]: %s + import f1; /+ + ^^ [warn]: %s +/ import f3; while (true) { import f2; - import f1; // [warn]: %s + import f1; /+ + ^^ [warn]: %s +/ import f3; } for (;;) { @@ -273,7 +329,8 @@ unittest } foreach (el; arr) { import f2; - import f1; // [warn]: %s + import f1; /+ + ^^ [warn]: %s +/ import f3; } } @@ -287,23 +344,28 @@ unittest // nested scopes assertAnalyzerWarnings(q{ import t2; - import t1; // [warn]: %s + import t1; /+ + ^^ [warn]: %s +/ void foo() { import f2; - import f1; // [warn]: %s + import f1; /+ + ^^ [warn]: %s +/ import f3; { import f2; - import f1; // [warn]: %s + import f1; /+ + ^^ [warn]: %s +/ import f3; { import f2; - import f1; // [warn]: %s + import f1; /+ + ^^ [warn]: %s +/ import f3; { import f2; - import f1; // [warn]: %s + import f1; /+ + ^^ [warn]: %s +/ import f3; } } @@ -320,11 +382,13 @@ unittest // local imports in functions assertAnalyzerWarnings(q{ import t2; - import t1; // [warn]: %s + import t1; /+ + ^^ [warn]: %s +/ struct foo() { import f2; - import f1; // [warn]: %s + import f1; /+ + ^^ [warn]: %s +/ import f3; } class bar() diff --git a/src/dscanner/analysis/incorrect_infinite_range.d b/src/dscanner/analysis/incorrect_infinite_range.d index 055f05b..7d7b0fd 100644 --- a/src/dscanner/analysis/incorrect_infinite_range.d +++ b/src/dscanner/analysis/incorrect_infinite_range.d @@ -10,6 +10,8 @@ import dscanner.analysis.helpers; import dparse.ast; import dparse.lexer; +import std.typecons : Rebindable; + /** * Checks for incorrect infinite range definitions */ @@ -36,9 +38,10 @@ final class IncorrectInfiniteRangeCheck : BaseAnalyzer { if (inStruct > 0 && fd.name.text == "empty") { - line = fd.name.line; - column = fd.name.column; + auto old = parentFunc; + parentFunc = fd; fd.accept(this); + parentFunc = old; } } @@ -63,7 +66,7 @@ final class IncorrectInfiniteRangeCheck : BaseAnalyzer override void visit(const ReturnStatement rs) { - if (inStruct == 0 || line == size_t.max) // not within a struct yet + if (inStruct == 0 || parentFunc == null) // not within a struct yet return; visitReturnExpression(rs.expression); } @@ -79,7 +82,7 @@ final class IncorrectInfiniteRangeCheck : BaseAnalyzer return; if (unary.primaryExpression.primary != tok!"false") return; - addErrorMessage(line, column, KEY, MESSAGE); + addErrorMessage(parentFunc.get, KEY, MESSAGE); } override void visit(const Unittest u) @@ -90,8 +93,7 @@ private: uint inStruct; enum string KEY = "dscanner.suspicious.incorrect_infinite_range"; enum string MESSAGE = "Use `enum bool empty = false;` to define an infinite range."; - size_t line = size_t.max; - size_t column; + Rebindable!(const FunctionDeclaration) parentFunc; } unittest @@ -104,10 +106,12 @@ unittest sac.incorrect_infinite_range_check = Check.enabled; assertAnalyzerWarnings(q{struct InfiniteRange { - bool empty() // [warn]: %1$s + bool empty() { return false; - } + } /+ +^^ [warn]: %1$s+/ + // TODO: test for multiline issues like this bool stuff() { @@ -128,7 +132,8 @@ unittest struct InfiniteRange { - bool empty() => false; // [warn]: %1$s + bool empty() => false; /+ + ^^^^^^^^^^^^^^^^^^^^^^ [warn]: %1$s +/ bool stuff() => false; unittest { @@ -143,7 +148,8 @@ struct InfiniteRange } bool empty() { return false; } -class C { bool empty() { return false; } } // [warn]: %1$s +class C { bool empty() { return false; } } /+ + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [warn]: %1$s +/ }c .format(IncorrectInfiniteRangeCheck.MESSAGE), sac); diff --git a/src/dscanner/analysis/label_var_same_name_check.d b/src/dscanner/analysis/label_var_same_name_check.d index dc85bdb..4402f32 100644 --- a/src/dscanner/analysis/label_var_same_name_check.d +++ b/src/dscanner/analysis/label_var_same_name_check.d @@ -106,7 +106,7 @@ private: { immutable thisKind = fromLabel ? "Label" : "Variable"; immutable otherKind = thing.isVar ? "variable" : "label"; - addErrorMessage(name.line, name.column, "dscanner.suspicious.label_var_same_name", + addErrorMessage(name, "dscanner.suspicious.label_var_same_name", thisKind ~ " \"" ~ fqn ~ "\" has the same name as a " ~ otherKind ~ " defined on line " ~ to!string(thing.line) ~ "."); } @@ -178,14 +178,16 @@ unittest unittest { blah: - int blah; // [warn]: Variable "blah" has the same name as a label defined on line 4. + int blah; /+ + ^^^^ [warn]: Variable "blah" has the same name as a label defined on line 4. +/ } int blah; unittest { static if (stuff) int a; - int a; // [warn]: Variable "a" has the same name as a variable defined on line 11. + int a; /+ + ^ [warn]: Variable "a" has the same name as a variable defined on line 12. +/ } unittest @@ -202,7 +204,8 @@ unittest int a = 10; else int a = 20; - int a; // [warn]: Variable "a" has the same name as a variable defined on line 28. + int a; /+ + ^ [warn]: Variable "a" has the same name as a variable defined on line 30. +/ } template T(stuff) { @@ -225,7 +228,8 @@ unittest int c = 10; else int c = 20; - int c; // [warn]: Variable "c" has the same name as a variable defined on line 51. + int c; /+ + ^ [warn]: Variable "c" has the same name as a variable defined on line 54. +/ } unittest @@ -263,7 +267,8 @@ unittest interface A { int a; - int a; // [warn]: Variable "A.a" has the same name as a variable defined on line 89. + int a; /+ + ^ [warn]: Variable "A.a" has the same name as a variable defined on line 93. +/ } } diff --git a/src/dscanner/analysis/lambda_return_check.d b/src/dscanner/analysis/lambda_return_check.d index e0b0b47..876653f 100644 --- a/src/dscanner/analysis/lambda_return_check.d +++ b/src/dscanner/analysis/lambda_return_check.d @@ -31,7 +31,11 @@ final class LambdaReturnCheck : BaseAnalyzer { return; } - addErrorMessage(fLit.line, fLit.column, KEY, "This lambda returns a lambda. Add parenthesis to clarify."); + auto start = &fLit.tokens[0]; + auto endIncl = &fe.specifiedFunctionBody.tokens[0]; + assert(endIncl >= start); + auto tokens = start[0 .. endIncl - start + 1]; + addErrorMessage(tokens, KEY, "This lambda returns a lambda. Add parenthesis to clarify."); } private: @@ -52,9 +56,12 @@ unittest void main() { int[] b; - auto a = b.map!(a => { return a * a + 2; }).array(); // [warn]: This lambda returns a lambda. Add parenthesis to clarify. - pragma(msg, typeof(a => { return a; })); // [warn]: This lambda returns a lambda. Add parenthesis to clarify. - pragma(msg, typeof((a) => { return a; })); // [warn]: This lambda returns a lambda. Add parenthesis to clarify. + auto a = b.map!(a => { return a * a + 2; }).array(); /+ + ^^^^^^ [warn]: This lambda returns a lambda. Add parenthesis to clarify. +/ + pragma(msg, typeof(a => { return a; })); /+ + ^^^^^^ [warn]: This lambda returns a lambda. Add parenthesis to clarify. +/ + pragma(msg, typeof((a) => { return a; })); /+ + ^^^^^^^^ [warn]: This lambda returns a lambda. Add parenthesis to clarify. +/ pragma(msg, typeof({ return a; })); pragma(msg, typeof(a => () { return a; })); }`c; diff --git a/src/dscanner/analysis/length_subtraction.d b/src/dscanner/analysis/length_subtraction.d index a788ec7..f883e99 100644 --- a/src/dscanner/analysis/length_subtraction.d +++ b/src/dscanner/analysis/length_subtraction.d @@ -34,23 +34,14 @@ final class LengthSubtractionCheck : BaseAnalyzer const UnaryExpression l = cast(const UnaryExpression) addExpression.left; const UnaryExpression r = cast(const UnaryExpression) addExpression.right; if (l is null || r is null) - { - // stderr.writeln(__FILE__, " ", __LINE__); goto end; - } if (r.primaryExpression is null || r.primaryExpression.primary.type != tok!"intLiteral") - { - // stderr.writeln(__FILE__, " ", __LINE__); goto end; - } if (l.identifierOrTemplateInstance is null || l.identifierOrTemplateInstance.identifier.text != "length") - { - // stderr.writeln(__FILE__, " ", __LINE__); goto end; - } const(Token) token = l.identifierOrTemplateInstance.identifier; - addErrorMessage(token.line, token.column, "dscanner.suspicious.length_subtraction", + addErrorMessage(addExpression, "dscanner.suspicious.length_subtraction", "Avoid subtracting from '.length' as it may be unsigned."); } end: @@ -67,7 +58,8 @@ unittest assertAnalyzerWarnings(q{ void testSizeT() { - if (i < a.length - 1) // [warn]: Avoid subtracting from '.length' as it may be unsigned. + if (i < a.length - 1) /+ + ^^^^^^^^^^^^ [warn]: Avoid subtracting from '.length' as it may be unsigned. +/ writeln("something"); } }c, sac); diff --git a/src/dscanner/analysis/line_length.d b/src/dscanner/analysis/line_length.d index 0c28631..2f55aaf 100644 --- a/src/dscanner/analysis/line_length.d +++ b/src/dscanner/analysis/line_length.d @@ -58,9 +58,11 @@ private: void triggerError(ref const Token tok) { + import std.algorithm : max; + if (tok.line != lastErrorLine) { - addErrorMessage(tok.line, tok.column, KEY, message); + addErrorMessage(tok.line, maxLineLength, max(maxLineLength + 1, tok.column + 1), KEY, message); lastErrorLine = tok.line; } } diff --git a/src/dscanner/analysis/local_imports.d b/src/dscanner/analysis/local_imports.d index 4489ce7..6db4d85 100644 --- a/src/dscanner/analysis/local_imports.d +++ b/src/dscanner/analysis/local_imports.d @@ -58,8 +58,7 @@ final class LocalImportCheck : BaseAnalyzer { if (singleImport.rename.text.length == 0) { - addErrorMessage(singleImport.identifierChain.identifiers[0].line, - singleImport.identifierChain.identifiers[0].column, + addErrorMessage(singleImport, "dscanner.suspicious.local_imports", "Local imports should specify" ~ " the symbols being imported to avoid hiding local symbols."); } @@ -93,7 +92,8 @@ unittest assertAnalyzerWarnings(q{ void testLocalImport() { - import std.stdio; // [warn]: Local imports should specify the symbols being imported to avoid hiding local symbols. + import std.stdio; /+ + ^^^^^^^^^ [warn]: Local imports should specify the symbols being imported to avoid hiding local symbols. +/ import std.fish : scales, head; import DAGRON = std.experimental.dragon; } diff --git a/src/dscanner/analysis/logic_precedence.d b/src/dscanner/analysis/logic_precedence.d index 35d3633..fdce466 100644 --- a/src/dscanner/analysis/logic_precedence.d +++ b/src/dscanner/analysis/logic_precedence.d @@ -41,7 +41,7 @@ final class LogicPrecedenceCheck : BaseAnalyzer return; if ((left !is null && left.right is null) && (right !is null && right.right is null)) return; - addErrorMessage(orOr.line, orOr.column, KEY, + addErrorMessage(orOr, KEY, "Use parenthesis to clarify this expression."); orOr.accept(this); } @@ -56,9 +56,11 @@ unittest assertAnalyzerWarnings(q{ void testFish() { - if (a && b || c) {} // [warn]: Use parenthesis to clarify this expression. + if (a && b || c) {} /+ + ^^^^^^^^^^^ [warn]: Use parenthesis to clarify this expression. +/ if ((a && b) || c) {} // Good - if (b || c && d) {} // [warn]: Use parenthesis to clarify this expression. + if (b || c && d) {} /+ + ^^^^^^^^^^^ [warn]: Use parenthesis to clarify this expression. +/ if (b || (c && d)) {} // Good } }c, sac); diff --git a/src/dscanner/analysis/mismatched_args.d b/src/dscanner/analysis/mismatched_args.d index c323bfc..031c93f 100644 --- a/src/dscanner/analysis/mismatched_args.d +++ b/src/dscanner/analysis/mismatched_args.d @@ -5,7 +5,7 @@ import dscanner.utils : safeAccess; import dsymbol.scope_; import dsymbol.symbol; import dparse.ast; -import dparse.lexer : tok; +import dparse.lexer : tok, Token; import dsymbol.builtin.names; /// Checks for mismatched argument and parameter names @@ -42,8 +42,7 @@ final class MismatchedArgumentCheck : BaseAnalyzer static struct ErrorMessage { - size_t line; - size_t column; + const(Token)[] range; string message; } @@ -60,17 +59,16 @@ final class MismatchedArgumentCheck : BaseAnalyzer matched = true; else { - foreach (size_t i, ref const mm; mismatches) + foreach (ref const mm; mismatches) { - messages ~= ErrorMessage(argVisitor.lines[i], - argVisitor.columns[i], createWarningFromMismatch(mm)); + messages ~= ErrorMessage(argVisitor.tokens[mm.argIndex], createWarningFromMismatch(mm)); } } } if (!matched) foreach (m; messages) - addErrorMessage(m.line, m.column, KEY, m.message); + addErrorMessage(m.range, KEY, m.message); } alias visit = ASTVisitor.visit; @@ -123,8 +121,7 @@ final class ArgVisitor : ASTVisitor // if we didn't get an identifier in the unary expression, // assume it's a good argument args ~= istring.init; - lines ~= a.tokens.length ? a.tokens[0].line : size_t.max; - columns ~= a.tokens.length ? a.tokens[0].column : size_t.max; + tokens ~= a.tokens; } } } @@ -138,16 +135,14 @@ final class ArgVisitor : ASTVisitor if (iot.identifier == tok!"") return; immutable t = iot.identifier; - lines ~= t.line; - columns ~= t.column; + tokens ~= [t]; args ~= internString(t.text); } } alias visit = ASTVisitor.visit; - size_t[] lines; - size_t[] columns; + const(Token[])[] tokens; istring[] args; } @@ -271,14 +266,19 @@ unittest { int x = 1; int y = 2; - foo(y, x); // [warn]: Argument 2 is named 'x', but this is the name of parameter 1 - foo(y + 1, x); // [warn]: Argument 2 is named 'x', but this is the name of parameter 1 + foo(y, x); /+ + ^ [warn]: Argument 2 is named 'x', but this is the name of parameter 1 +/ + foo(y + 1, x); /+ + ^ [warn]: Argument 2 is named 'x', but this is the name of parameter 1 +/ foo(y + 1, f(x)); foo(x: y, y: x); + foo(y, 0); /+ + ^ [warn]: Argument 1 is named 'y', but this is the name of parameter 2 +/ // foo(y: y, x); // TODO: this shouldn't error foo(x, y: x); // TODO: this should error - foo(y, y: x); // [warn]: Argument 1 is named 'y', but this is the name of parameter 2 + foo(y, y: x); /+ + ^ [warn]: Argument 1 is named 'y', but this is the name of parameter 2 +/ } }c, sac); stderr.writeln("Unittest for MismatchedArgumentCheck passed."); diff --git a/src/dscanner/analysis/numbers.d b/src/dscanner/analysis/numbers.d index 8e8d55d..d388df8 100644 --- a/src/dscanner/analysis/numbers.d +++ b/src/dscanner/analysis/numbers.d @@ -39,7 +39,7 @@ public: && ((t.text.startsWith("0b") && !t.text.matchFirst(badBinaryRegex) .empty) || !t.text.matchFirst(badDecimalRegex).empty)) { - addErrorMessage(t.line, t.column, "dscanner.style.number_literals", + addErrorMessage(t, "dscanner.style.number_literals", "Use underscores to improve number constant readability."); } } @@ -62,10 +62,13 @@ unittest a = 1; // ok a = 10; // ok a = 100; // ok - a = 1000; // FIXME: boom - a = 10000; // [warn]: Use underscores to improve number constant readability. - a = 100000; // [warn]: Use underscores to improve number constant readability. - a = 1000000; // [warn]: Use underscores to improve number constant readability. + a = 1000; // ok + a = 10000; /+ + ^^^^^ [warn]: Use underscores to improve number constant readability. +/ + a = 100000; /+ + ^^^^^^ [warn]: Use underscores to improve number constant readability. +/ + a = 1000000; /+ + ^^^^^^^ [warn]: Use underscores to improve number constant readability. +/ } }c, sac); diff --git a/src/dscanner/analysis/objectconst.d b/src/dscanner/analysis/objectconst.d index 7de6182..275a291 100644 --- a/src/dscanner/analysis/objectconst.d +++ b/src/dscanner/analysis/objectconst.d @@ -68,8 +68,7 @@ final class ObjectConstCheck : BaseAnalyzer if (inAggregate && !constColon && !constBlock && !isDeclationDisabled && isInteresting(fd.name.text) && !hasConst(fd.memberFunctionAttributes)) { - addErrorMessage(d.functionDeclaration.name.line, - d.functionDeclaration.name.column, "dscanner.suspicious.object_const", + addErrorMessage(d.functionDeclaration.name, "dscanner.suspicious.object_const", "Methods 'opCmp', 'toHash', 'opEquals', 'opCast', and/or 'toString' are non-const."); } } @@ -157,22 +156,26 @@ unittest // Will warn, because none are const class Dog { - bool opEquals(Object a, Object b) // [warn]: Methods 'opCmp', 'toHash', 'opEquals', 'opCast', and/or 'toString' are non-const. + bool opEquals(Object a, Object b) /+ + ^^^^^^^^ [warn]: Methods 'opCmp', 'toHash', 'opEquals', 'opCast', and/or 'toString' are non-const. +/ { return true; } - int opCmp(Object o) // [warn]: Methods 'opCmp', 'toHash', 'opEquals', 'opCast', and/or 'toString' are non-const. + int opCmp(Object o) /+ + ^^^^^ [warn]: Methods 'opCmp', 'toHash', 'opEquals', 'opCast', and/or 'toString' are non-const. +/ { return 1; } - hash_t toHash() // [warn]: Methods 'opCmp', 'toHash', 'opEquals', 'opCast', and/or 'toString' are non-const. + hash_t toHash() /+ + ^^^^^^ [warn]: Methods 'opCmp', 'toHash', 'opEquals', 'opCast', and/or 'toString' are non-const. +/ { return 0; } - string toString() // [warn]: Methods 'opCmp', 'toHash', 'opEquals', 'opCast', and/or 'toString' are non-const. + string toString() /+ + ^^^^^^^^ [warn]: Methods 'opCmp', 'toHash', 'opEquals', 'opCast', and/or 'toString' are non-const. +/ { return "Dog"; } diff --git a/src/dscanner/analysis/opequals_without_tohash.d b/src/dscanner/analysis/opequals_without_tohash.d index f3b854d..e8db7ee 100644 --- a/src/dscanner/analysis/opequals_without_tohash.d +++ b/src/dscanner/analysis/opequals_without_tohash.d @@ -5,12 +5,13 @@ module dscanner.analysis.opequals_without_tohash; -import std.stdio; import dparse.ast; import dparse.lexer; import dscanner.analysis.base; import dscanner.analysis.helpers; import dsymbol.scope_ : Scope; +import std.stdio; +import std.typecons : Rebindable; /** * Checks for when a class/struct has the method opEquals without toHash, or @@ -41,9 +42,9 @@ final class OpEqualsWithoutToHashCheck : BaseAnalyzer private void actualCheck(const Token name, const StructBody structBody) { - bool hasOpEquals; - bool hasToHash; - bool hasOpCmp; + Rebindable!(const Declaration) hasOpEquals; + Rebindable!(const Declaration) hasToHash; + Rebindable!(const Declaration) hasOpCmp; // Just return if missing children if (!structBody || !structBody.declarations || name is Token.init) @@ -72,24 +73,36 @@ final class OpEqualsWithoutToHashCheck : BaseAnalyzer // Check if opEquals or toHash immutable string methodName = declaration.functionDeclaration.name.text; if (methodName == "opEquals") - hasOpEquals = true; + hasOpEquals = declaration; else if (methodName == "toHash") - hasToHash = true; + hasToHash = declaration; else if (methodName == "opCmp") - hasOpCmp = true; + hasOpCmp = declaration; } // Warn if has opEquals, but not toHash if (hasOpEquals && !hasToHash) { string message = "'" ~ name.text ~ "' has method 'opEquals', but not 'toHash'."; - addErrorMessage(name.line, name.column, KEY, message); + addErrorMessage( + Message.Diagnostic.from(fileName, name, message), + [ + Message.Diagnostic.from(fileName, hasOpEquals.get, "'opEquals' defined here") + ], + KEY + ); } // Warn if has toHash, but not opEquals else if (!hasOpEquals && hasToHash) { string message = "'" ~ name.text ~ "' has method 'toHash', but not 'opEquals'."; - addErrorMessage(name.line, name.column, KEY, message); + addErrorMessage( + Message.Diagnostic.from(fileName, name, message), + [ + Message.Diagnostic.from(fileName, hasToHash.get, "'toHash' defined here") + ], + KEY + ); } } @@ -102,6 +115,7 @@ unittest StaticAnalysisConfig sac = disabledConfig(); sac.opequals_tohash_check = Check.enabled; + // TODO: test supplemental diagnostics assertAnalyzerWarnings(q{ // Success because it has opEquals and toHash class Chimp @@ -127,7 +141,8 @@ unittest } // Fail on class opEquals - class Rabbit // [warn]: 'Rabbit' has method 'opEquals', but not 'toHash'. + class Rabbit /+ + ^^^^^^ [warn]: 'Rabbit' has method 'opEquals', but not 'toHash'. +/ { const bool opEquals(Object a, Object b) { @@ -136,7 +151,8 @@ unittest } // Fail on class toHash - class Kangaroo // [warn]: 'Kangaroo' has method 'toHash', but not 'opEquals'. + class Kangaroo /+ + ^^^^^^^^ [warn]: 'Kangaroo' has method 'toHash', but not 'opEquals'. +/ { override const hash_t toHash() { @@ -145,7 +161,8 @@ unittest } // Fail on struct opEquals - struct Tarantula // [warn]: 'Tarantula' has method 'opEquals', but not 'toHash'. + struct Tarantula /+ + ^^^^^^^^^ [warn]: 'Tarantula' has method 'opEquals', but not 'toHash'. +/ { const bool opEquals(Object a, Object b) { @@ -154,7 +171,8 @@ unittest } // Fail on struct toHash - struct Puma // [warn]: 'Puma' has method 'toHash', but not 'opEquals'. + struct Puma /+ + ^^^^ [warn]: 'Puma' has method 'toHash', but not 'opEquals'. +/ { const nothrow @safe hash_t toHash() { diff --git a/src/dscanner/analysis/pokemon.d b/src/dscanner/analysis/pokemon.d index c3dbb11..35fa0bc 100644 --- a/src/dscanner/analysis/pokemon.d +++ b/src/dscanner/analysis/pokemon.d @@ -38,7 +38,7 @@ final class PokemonExceptionCheck : BaseAnalyzer override void visit(const LastCatch lc) { - addErrorMessage(lc.line, lc.column, KEY, MESSAGE); + addErrorMessage(lc.tokens[0], KEY, MESSAGE); lc.accept(this); } @@ -76,9 +76,7 @@ final class PokemonExceptionCheck : BaseAnalyzer if (identOrTemplate.identifier.text == "Throwable" || identOrTemplate.identifier.text == "Error") { - immutable column = identOrTemplate.identifier.column; - immutable line = identOrTemplate.identifier.line; - addErrorMessage(line, column, KEY, MESSAGE); + addErrorMessage(identOrTemplate, KEY, MESSAGE); } } } @@ -108,19 +106,23 @@ unittest { } - catch (Error err) // [warn]: Catching Error or Throwable is almost always a bad idea. + catch (Error err) /+ + ^^^^^ [warn]: Catching Error or Throwable is almost always a bad idea. +/ { } - catch (Throwable err) // [warn]: Catching Error or Throwable is almost always a bad idea. + catch (Throwable err) /+ + ^^^^^^^^^ [warn]: Catching Error or Throwable is almost always a bad idea. +/ { } - catch (shared(Error) err) // [warn]: Catching Error or Throwable is almost always a bad idea. + catch (shared(Error) err) /+ + ^^^^^ [warn]: Catching Error or Throwable is almost always a bad idea. +/ { } - catch // [warn]: Catching Error or Throwable is almost always a bad idea. + catch /+ + ^^^^^ [warn]: Catching Error or Throwable is almost always a bad idea. +/ { } diff --git a/src/dscanner/analysis/properly_documented_public_functions.d b/src/dscanner/analysis/properly_documented_public_functions.d index 6847f76..5d41419 100644 --- a/src/dscanner/analysis/properly_documented_public_functions.d +++ b/src/dscanner/analysis/properly_documented_public_functions.d @@ -157,8 +157,8 @@ final class ProperlyDocumentedPublicFunctions : BaseAnalyzer override void visit(const TemplateDeclaration decl) { - setLastDdocParams(decl.name.line, decl.name.column, decl.comment); - checkDdocParams(decl.name.line, decl.name.column, decl.templateParameters); + setLastDdocParams(decl.name, decl.comment); + checkDdocParams(decl.templateParameters); withinTemplate = true; scope(exit) withinTemplate = false; @@ -172,15 +172,15 @@ final class ProperlyDocumentedPublicFunctions : BaseAnalyzer override void visit(const StructDeclaration decl) { - setLastDdocParams(decl.name.line, decl.name.column, decl.comment); - checkDdocParams(decl.name.line, decl.name.column, decl.templateParameters); + setLastDdocParams(decl.name, decl.comment); + checkDdocParams(decl.templateParameters); decl.accept(this); } override void visit(const ClassDeclaration decl) { - setLastDdocParams(decl.name.line, decl.name.column, decl.comment); - checkDdocParams(decl.name.line, decl.name.column, decl.templateParameters); + setLastDdocParams(decl.name, decl.comment); + checkDdocParams(decl.templateParameters); decl.accept(this); } @@ -205,20 +205,31 @@ final class ProperlyDocumentedPublicFunctions : BaseAnalyzer { Appender!(char[]) app; astFmt(&app, t); - addErrorMessage(decl.name.line, decl.name.column, MISSING_THROW_KEY, + addErrorMessage(decl.name, MISSING_THROW_KEY, MISSING_THROW_MESSAGE.format(app.data)); } } if (nestedFuncs == 1) { - auto comment = setLastDdocParams(decl.name.line, decl.name.column, decl.comment); - checkDdocParams(decl.name.line, decl.name.column, decl.parameters, decl.templateParameters); + auto comment = setLastDdocParams(decl.name, decl.comment); + checkDdocParams(decl.parameters, decl.templateParameters); enum voidType = tok!"void"; if (decl.returnType is null || decl.returnType.type2.builtinType != voidType) if (!(comment.isDitto || withinTemplate || comment.sections.any!(s => s.name == "Returns"))) - addErrorMessage(decl.name.line, decl.name.column, - MISSING_RETURNS_KEY, MISSING_RETURNS_MESSAGE); + { + import dscanner.analysis.auto_function : AutoFunctionChecker; + + const(Token)[] typeRange; + if (decl.returnType !is null) + typeRange = decl.returnType.tokens; + else + typeRange = AutoFunctionChecker.findAutoReturnType(decl); + + if (!typeRange.length) + typeRange = [decl.name]; + addErrorMessage(typeRange, MISSING_RETURNS_KEY, MISSING_RETURNS_MESSAGE); + } } } @@ -257,7 +268,7 @@ private: static struct Function { bool active; - size_t line, column; + Token name; const(string)[] ddocParams; bool[string] params; } @@ -274,7 +285,7 @@ private: if (lastSeenFun.active) foreach (p; lastSeenFun.ddocParams) if (p !in lastSeenFun.params) - addErrorMessage(lastSeenFun.line, lastSeenFun.column, NON_EXISTENT_PARAMS_KEY, + addErrorMessage(lastSeenFun.name, NON_EXISTENT_PARAMS_KEY, NON_EXISTENT_PARAMS_MESSAGE.format(p)); lastSeenFun.active = false; @@ -289,7 +300,7 @@ private: return comment.isDitto || comment.sections.canFind!(s => s.name == "Throws"); } - auto setLastDdocParams(size_t line, size_t column, string commentText) + auto setLastDdocParams(Token name, string commentText) { import ddoc.comments : parseComment; import std.algorithm.searching : find; @@ -312,19 +323,19 @@ private: const paramSection = comment.sections.find!(s => s.name == "Params"); if (paramSection.empty) { - lastSeenFun = Function(true, line, column, null); + lastSeenFun = Function(true, name, null); } else { auto ddocParams = paramSection[0].mapping.map!(a => a[0]).array; - lastSeenFun = Function(true, line, column, ddocParams); + lastSeenFun = Function(true, name, ddocParams); } } return comment; } - void checkDdocParams(size_t line, size_t column, const Parameters params, + void checkDdocParams(const Parameters params, const TemplateParameters templateParameters = null) { import std.array : array; @@ -338,7 +349,7 @@ private: if (const tp = templateParameters) if (const tpl = tp.templateParameterList) templateList = tpl.items; - string[] tlList = templateList.map!(a => templateParamName(a)).array; + string[] tlList = templateList.map!(a => templateParamName(a).text).array; // make a copy of all parameters and remove the seen ones later during the loop size_t[] unseenTemplates = templateList.length.iota.array; @@ -369,7 +380,7 @@ private: } if (!lastSeenFun.ddocParams.canFind(p.name.text)) - addErrorMessage(line, column, MISSING_PARAMS_KEY, + addErrorMessage(p.name, MISSING_PARAMS_KEY, format(MISSING_PARAMS_MESSAGE, p.name.text)); else lastSeenFun.params[p.name.text] = true; @@ -377,45 +388,45 @@ private: // now check the remaining, not used template parameters auto unseenTemplatesArr = templateList.indexed(unseenTemplates).array; - checkDdocParams(line, column, unseenTemplatesArr); + checkDdocParams(unseenTemplatesArr); } - void checkDdocParams(size_t line, size_t column, const TemplateParameters templateParams) + void checkDdocParams(const TemplateParameters templateParams) { if (lastSeenFun.active && templateParams !is null && templateParams.templateParameterList !is null) - checkDdocParams(line, column, templateParams.templateParameterList.items); + checkDdocParams(templateParams.templateParameterList.items); } - void checkDdocParams(size_t line, size_t column, const TemplateParameter[] templateParams) + void checkDdocParams(const TemplateParameter[] templateParams) { import std.algorithm.searching : canFind; foreach (p; templateParams) { const name = templateParamName(p); - assert(name, "Invalid template parameter name."); // this shouldn't happen - if (!lastSeenFun.ddocParams.canFind(name)) - addErrorMessage(line, column, MISSING_PARAMS_KEY, - format(MISSING_TEMPLATE_PARAMS_MESSAGE, name)); + assert(name !is Token.init, "Invalid template parameter name."); // this shouldn't happen + if (!lastSeenFun.ddocParams.canFind(name.text)) + addErrorMessage(name, MISSING_PARAMS_KEY, + format(MISSING_TEMPLATE_PARAMS_MESSAGE, name.text)); else - lastSeenFun.params[name] = true; + lastSeenFun.params[name.text] = true; } } - static string templateParamName(const TemplateParameter p) + static Token templateParamName(const TemplateParameter p) { if (p.templateTypeParameter) - return p.templateTypeParameter.identifier.text; + return p.templateTypeParameter.identifier; if (p.templateValueParameter) - return p.templateValueParameter.identifier.text; + return p.templateValueParameter.identifier; if (p.templateAliasParameter) - return p.templateAliasParameter.identifier.text; + return p.templateAliasParameter.identifier; if (p.templateTupleParameter) - return p.templateTupleParameter.identifier.text; + return p.templateTupleParameter.identifier; if (p.templateThisParameter) - return p.templateThisParameter.templateTypeParameter.identifier.text; + return p.templateThisParameter.templateTypeParameter.identifier; - return null; + return Token.init; } bool hasDeclaration(const Declaration decl) @@ -483,7 +494,8 @@ unittest /** Some text */ - void foo(int k){} // [warn]: %s + void foo(int k){} /+ + ^ [warn]: %s +/ }c.format( ProperlyDocumentedPublicFunctions.MISSING_PARAMS_MESSAGE.format("k") ), sac); @@ -492,7 +504,8 @@ unittest /** Some text */ - void foo(int K)(){} // [warn]: %s + void foo(int K)(){} /+ + ^ [warn]: %s +/ }c.format( ProperlyDocumentedPublicFunctions.MISSING_TEMPLATE_PARAMS_MESSAGE.format("K") ), sac); @@ -501,7 +514,8 @@ unittest /** Some text */ - struct Foo(Bar){} // [warn]: %s + struct Foo(Bar){} /+ + ^^^ [warn]: %s +/ }c.format( ProperlyDocumentedPublicFunctions.MISSING_TEMPLATE_PARAMS_MESSAGE.format("Bar") ), sac); @@ -510,7 +524,8 @@ unittest /** Some text */ - class Foo(Bar){} // [warn]: %s + class Foo(Bar){} /+ + ^^^ [warn]: %s +/ }c.format( ProperlyDocumentedPublicFunctions.MISSING_TEMPLATE_PARAMS_MESSAGE.format("Bar") ), sac); @@ -519,7 +534,8 @@ unittest /** Some text */ - template Foo(Bar){} // [warn]: %s + template Foo(Bar){} /+ + ^^^ [warn]: %s +/ }c.format( ProperlyDocumentedPublicFunctions.MISSING_TEMPLATE_PARAMS_MESSAGE.format("Bar") ), sac); @@ -558,7 +574,8 @@ unittest /** Some text */ - int foo(){} // [warn]: %s + int foo(){} /+ + ^^^ [warn]: %s +/ }c.format( ProperlyDocumentedPublicFunctions.MISSING_RETURNS_MESSAGE, ), sac); @@ -567,7 +584,8 @@ unittest /** Some text */ - auto foo(){} // [warn]: %s + auto foo(){} /+ + ^^^^ [warn]: %s +/ }c.format( ProperlyDocumentedPublicFunctions.MISSING_RETURNS_MESSAGE, ), sac); @@ -594,10 +612,12 @@ unittest */ private void foo(int k){} /// - public int bar(){} // [warn]: %s + public int bar(){} /+ + ^^^ [warn]: %s +/ public: /// - int foobar(){} // [warn]: %s + int foobar(){} /+ + ^^^ [warn]: %s +/ }c.format( ProperlyDocumentedPublicFunctions.MISSING_RETURNS_MESSAGE, ProperlyDocumentedPublicFunctions.MISSING_RETURNS_MESSAGE, @@ -611,10 +631,12 @@ unittest */ private template foo(int k){} /// - public template bar(T){} // [warn]: %s + public template bar(T){} /+ + ^ [warn]: %s +/ public: /// - template foobar(T){} // [warn]: %s + template foobar(T){} /+ + ^ [warn]: %s +/ }c.format( ProperlyDocumentedPublicFunctions.MISSING_TEMPLATE_PARAMS_MESSAGE.format("T"), ProperlyDocumentedPublicFunctions.MISSING_TEMPLATE_PARAMS_MESSAGE.format("T"), @@ -628,10 +650,12 @@ unittest */ private struct foo(int k){} /// - public struct bar(T){} // [warn]: %s + public struct bar(T){} /+ + ^ [warn]: %s +/ public: /// - struct foobar(T){} // [warn]: %s + struct foobar(T){} /+ + ^ [warn]: %s +/ }c.format( ProperlyDocumentedPublicFunctions.MISSING_TEMPLATE_PARAMS_MESSAGE.format("T"), ProperlyDocumentedPublicFunctions.MISSING_TEMPLATE_PARAMS_MESSAGE.format("T"), @@ -653,7 +677,8 @@ unittest * Returns: * A long description. */ -int foo(int k){} // [warn]: %s +int foo(int k){} /+ + ^ [warn]: %s +/ }c.format( ProperlyDocumentedPublicFunctions.MISSING_PARAMS_MESSAGE.format("k") ), sac); @@ -667,7 +692,8 @@ int foo(int k){} // [warn]: %s * Returns: * A long description. */ -int foo(int k) => k; // [warn]: %s +int foo(int k) => k; /+ + ^ [warn]: %s +/ }c.format( ProperlyDocumentedPublicFunctions.MISSING_PARAMS_MESSAGE.format("k") ), sac); @@ -683,7 +709,8 @@ k = A stupid parameter Returns: A long description. */ -int foo(int k){} // [warn]: %s +int foo(int k){} /+ + ^^^ [warn]: %s +/ }c.format( ProperlyDocumentedPublicFunctions.NON_EXISTENT_PARAMS_MESSAGE.format("val") ), sac); @@ -697,7 +724,8 @@ Params: Returns: A long description. */ -int foo(int k){} // [warn]: %s +int foo(int k){} /+ + ^ [warn]: %s +/ }c.format( ProperlyDocumentedPublicFunctions.MISSING_PARAMS_MESSAGE.format("k") ), sac); @@ -714,7 +742,8 @@ foobar = A stupid parameter Returns: A long description. */ -int foo(int foo, int foobar){} // [warn]: %s +int foo(int foo, int foobar){} /+ + ^^^ [warn]: %s +/ }c.format( ProperlyDocumentedPublicFunctions.NON_EXISTENT_PARAMS_MESSAGE.format("bad") ), sac); @@ -731,7 +760,8 @@ foobar = A stupid parameter Returns: A long description. */ -struct foo(int foo, int foobar){} // [warn]: %s +struct foo(int foo, int foobar){} /+ + ^^^ [warn]: %s +/ }c.format( ProperlyDocumentedPublicFunctions.NON_EXISTENT_PARAMS_MESSAGE.format("bad") ), sac); @@ -835,7 +865,8 @@ int bar(int f){} int foo(int k){} /// ditto -int bar(int bar){} // [warn]: %s +int bar(int bar){} /+ + ^^^ [warn]: %s +/ }c.format( ProperlyDocumentedPublicFunctions.MISSING_PARAMS_MESSAGE.format("bar") ), sac); @@ -854,7 +885,8 @@ int bar(int bar){} // [warn]: %s * See_Also: * $(REF takeExactly, std,range) */ -int foo(int k){} // [warn]: %s +int foo(int k){} /+ + ^^^ [warn]: %s +/ /// ditto int bar(int bar){} @@ -956,7 +988,8 @@ Params: Returns: Awesome values. +/ -string bar(P, R)(R r){}// [warn]: %s +string bar(P, R)(R r){}/+ + ^ [warn]: %s +/ }c.format( ProperlyDocumentedPublicFunctions.MISSING_TEMPLATE_PARAMS_MESSAGE.format("P") ), sac); @@ -1020,7 +1053,8 @@ unittest /++ Simple case +/ -void bar(){throw new Exception("bla");} // [warn]: %s +void bar(){throw new Exception("bla");} /+ + ^^^ [warn]: %s +/ }c.format( ProperlyDocumentedPublicFunctions.MISSING_THROW_MESSAGE.format("Exception") ), sac); @@ -1051,7 +1085,8 @@ unittest /++ rethrow +/ -void bar(){try throw new Exception("bla"); catch(Exception) throw new Error();} // [warn]: %s +void bar(){try throw new Exception("bla"); catch(Exception) throw new Error();} /+ + ^^^ [warn]: %s +/ }c.format( ProperlyDocumentedPublicFunctions.MISSING_THROW_MESSAGE.format("Error") ), sac); @@ -1079,7 +1114,8 @@ unittest /++ case of throw in nested func +/ -void bar() // [warn]: %s +void bar() /+ + ^^^ [warn]: %s +/ { void foo(){throw new AssertError("bla");} foo(); @@ -1116,7 +1152,8 @@ unittest /++ case of double throw in nested func but only 1 caught +/ -void bar() // [warn]: %s +void bar() /+ + ^^^ [warn]: %s +/ { void foo(){throw new AssertError("bla");throw new Error("bla");} try foo(); @@ -1136,7 +1173,8 @@ unittest /++ enforce +/ -void bar() // [warn]: %s +void bar() /+ + ^^^ [warn]: %s +/ { enforce(condition); } @@ -1154,7 +1192,8 @@ unittest /++ enforce +/ -void bar() // [warn]: %s +void bar() /+ + ^^^ [warn]: %s +/ { enforce!AssertError(condition); } @@ -1172,7 +1211,8 @@ unittest /++ enforce +/ -void bar() // [warn]: %s +void bar() /+ + ^^^ [warn]: %s +/ { enforce!(AssertError)(condition); } @@ -1190,7 +1230,8 @@ unittest /++ enforce +/ -void foo() // [warn]: %s +void foo() /+ + ^^^ [warn]: %s +/ { void bar() { diff --git a/src/dscanner/analysis/range.d b/src/dscanner/analysis/range.d index 1b78840..5c00a9f 100644 --- a/src/dscanner/analysis/range.d +++ b/src/dscanner/analysis/range.d @@ -50,7 +50,11 @@ final class BackwardsRangeCheck : BaseAnalyzer string message = format( "%d is larger than %d. Did you mean to use 'foreach_reverse( ... ; %d .. %d)'?", left, right, right, left); - addErrorMessage(line, this.column, KEY, message); + auto start = &foreachStatement.low.tokens[0]; + auto endIncl = &foreachStatement.high.tokens[$ - 1]; + assert(endIncl >= start); + auto tokens = start[0 .. endIncl - start + 1]; + addErrorMessage(tokens, KEY, message); } hasLeft = false; hasRight = false; @@ -82,9 +86,6 @@ final class BackwardsRangeCheck : BaseAnalyzer return; if (state == State.left) { - line = primary.primary.line; - this.column = primary.primary.column; - try left = parseNumber(primary.primary.text); catch (ConvException e) @@ -116,7 +117,7 @@ final class BackwardsRangeCheck : BaseAnalyzer string message = format("%d is larger than %d. This slice is likely incorrect.", left, right); - addErrorMessage(line, this.column, KEY, message); + addErrorMessage(index, KEY, message); } hasLeft = false; hasRight = false; @@ -129,8 +130,6 @@ private: bool hasRight; long left; long right; - size_t column; - size_t line; enum State { ignore, @@ -173,10 +172,12 @@ unittest int[] data = [1, 2, 3, 4, 5]; data = data[1 .. 3]; // ok - data = data[3 .. 1]; // [warn]: 3 is larger than 1. This slice is likely incorrect. + data = data[3 .. 1]; /+ + ^^^^^^ [warn]: 3 is larger than 1. This slice is likely incorrect. +/ foreach (n; 1 .. 3) { } // ok - foreach (n; 3 .. 1) { } // [warn]: 3 is larger than 1. Did you mean to use 'foreach_reverse( ... ; 1 .. 3)'? + foreach (n; 3 .. 1) { } /+ + ^^^^^^ [warn]: 3 is larger than 1. Did you mean to use 'foreach_reverse( ... ; 1 .. 3)'? +/ } }c, sac); diff --git a/src/dscanner/analysis/redundant_attributes.d b/src/dscanner/analysis/redundant_attributes.d index 0b3a467..9929690 100644 --- a/src/dscanner/analysis/redundant_attributes.d +++ b/src/dscanner/analysis/redundant_attributes.d @@ -103,8 +103,7 @@ private: auto match = currentAttributes.find!(a => a.attribute.type == attr.attribute.type); if (!match.empty) { - auto token = attr.attribute; - addErrorMessage(token.line, token.column, KEY, + addErrorMessage(attr, KEY, text("same visibility attribute used as defined on line ", match.front.attribute.line.to!string, ".")); return false; @@ -194,12 +193,15 @@ unittest unittest { private: - private int blah; // [warn]: same visibility attribute used as defined on line 4. + private int blah; /+ + ^^^^^^^ [warn]: same visibility attribute used as defined on line 4. +/ protected { - protected int blah; // [warn]: same visibility attribute used as defined on line 6. + protected int blah; /+ + ^^^^^^^^^ [warn]: same visibility attribute used as defined on line 7. +/ } - private int blah; // [warn]: same visibility attribute used as defined on line 4. + private int blah; /+ + ^^^^^^^ [warn]: same visibility attribute used as defined on line 4. +/ }}c, sac); // test labels vs. block attributes @@ -207,11 +209,14 @@ protected unittest { private: - private: // [warn]: same visibility attribute used as defined on line 4. + private: /+ + ^^^^^^^ [warn]: same visibility attribute used as defined on line 4. +/ public: private int a; - public int b; // [warn]: same visibility attribute used as defined on line 6. - public // [warn]: same visibility attribute used as defined on line 6. + public int b; /+ + ^^^^^^ [warn]: same visibility attribute used as defined on line 7. +/ + public /+ + ^^^^^^ [warn]: same visibility attribute used as defined on line 7. +/ { int c; } @@ -222,8 +227,10 @@ unittest unittest { private: - private int foo2; // [warn]: same visibility attribute used as defined on line 4. - private void foo() // [warn]: same visibility attribute used as defined on line 4. + private int foo2; /+ + ^^^^^^^ [warn]: same visibility attribute used as defined on line 4. +/ + private void foo() /+ + ^^^^^^^ [warn]: same visibility attribute used as defined on line 4. +/ { private int blah; } @@ -235,7 +242,8 @@ unittest { private: public int a; -private: // [warn]: same visibility attribute used as defined on line 4. +private: /+ +^^^^^^^ [warn]: same visibility attribute used as defined on line 4. +/ }}c, sac); // test conditional compilation @@ -245,7 +253,8 @@ unittest version(unittest) { private: - private int foo; // [warn]: same visibility attribute used as defined on line 6. + private int foo; /+ + ^^^^^^^ [warn]: same visibility attribute used as defined on line 6. +/ } private int foo2; }}c, sac); @@ -263,7 +272,8 @@ public: { public int b; } - public int b; // [warn]: same visibility attribute used as defined on line 4. + public int b; /+ + ^^^^^^ [warn]: same visibility attribute used as defined on line 4. +/ }}c, sac); } diff --git a/src/dscanner/analysis/redundant_parens.d b/src/dscanner/analysis/redundant_parens.d index b804e4f..fa20f8f 100644 --- a/src/dscanner/analysis/redundant_parens.d +++ b/src/dscanner/analysis/redundant_parens.d @@ -37,8 +37,7 @@ final class RedundantParenCheck : BaseAnalyzer goto end; if (unary.primaryExpression.expression is null) goto end; - addErrorMessage(unary.primaryExpression.expression.line, - unary.primaryExpression.expression.column, KEY, "Redundant parenthesis."); + addErrorMessage(unary.primaryExpression, KEY, "Redundant parenthesis."); end: statement.accept(this); } @@ -55,8 +54,7 @@ final class RedundantParenCheck : BaseAnalyzer goto end; if (unary.primaryExpression.expression is null) goto end; - addErrorMessage(primaryExpression.expression.line, - primaryExpression.expression.column, KEY, "Redundant parenthesis."); + addErrorMessage(primaryExpression, KEY, "Redundant parenthesis."); end: primaryExpression.accept(this); } diff --git a/src/dscanner/analysis/redundant_storage_class.d b/src/dscanner/analysis/redundant_storage_class.d index b03a876..782c10e 100644 --- a/src/dscanner/analysis/redundant_storage_class.d +++ b/src/dscanner/analysis/redundant_storage_class.d @@ -59,7 +59,7 @@ final class RedundantStorageClassCheck : BaseAnalyzer return; auto t = vd.declarators[0].name; string message = REDUNDANT_VARIABLE_ATTRIBUTES.format(t.text, globalAttributes); - addErrorMessage(t.line, t.column, "dscanner.unnecessary.duplicate_attribute", message); + addErrorMessage(t, "dscanner.unnecessary.duplicate_attribute", message); } } } diff --git a/src/dscanner/analysis/run.d b/src/dscanner/analysis/run.d index 6d15f13..fb636f7 100644 --- a/src/dscanner/analysis/run.d +++ b/src/dscanner/analysis/run.d @@ -106,8 +106,10 @@ void messageFunctionFormat(string format, Message message, bool isError) auto s = format; s = s.replace("{filepath}", message.fileName); - s = s.replace("{line}", to!string(message.line)); - s = s.replace("{column}", to!string(message.column)); + s = s.replace("{line}", to!string(message.startLine)); + s = s.replace("{column}", to!string(message.startColumn)); + s = s.replace("{endLine}", to!string(message.endLine)); + s = s.replace("{endColumn}", to!string(message.endColumn)); s = s.replace("{type}", isError ? "error" : "warn"); s = s.replace("{message}", message.message); s = s.replace("{name}", message.checkName); @@ -122,7 +124,7 @@ void messageFunction(Message message, bool isError) void messageFunctionJSON(string fileName, size_t line, size_t column, string message, bool) { - writeJSON(Message(fileName, line, column, "dscanner.syntax", message)); + writeJSON(Message(Message.Diagnostic.from(fileName, line, column, column, message), "dscanner.syntax")); } void writeJSON(Message message) @@ -138,9 +140,35 @@ void writeJSON(Message message) writeln(` "name": "`, message.checkName, `",`); } writeln(` "fileName": "`, message.fileName.replace("\\", "\\\\").replace(`"`, `\"`), `",`); - writeln(` "line": `, message.line, `,`); - writeln(` "column": `, message.column, `,`); - writeln(` "message": "`, message.message.replace("\\", "\\\\").replace(`"`, `\"`), `"`); + writeln(` "line": `, message.startLine, `,`); + writeln(` "column": `, message.startColumn, `,`); + writeln(` "endLine": `, message.endLine, `,`); + writeln(` "endColumn": `, message.endColumn, `,`); + writeln(` "message": "`, message.message.replace("\\", "\\\\").replace(`"`, `\"`), `",`); + if (message.supplemental.length) + { + writeln(` "supplemental": [`); + foreach (i, suppl; message.supplemental) + { + if (i != 0) + writeln(","); + writeln(` {`); + if (message.fileName != suppl.fileName) + writeln(` "fileName": `, suppl.fileName, `,`); + if (suppl.message.length) + writeln(` "message": `, suppl.message, `,`); + writeln(` "line": `, suppl.startLine, `,`); + writeln(` "column": `, suppl.startColumn, `,`); + writeln(` "endLine": `, suppl.endLine, `,`); + writeln(` "endColumn": `, suppl.endColumn); + write(` }`); + } + if (message.supplemental.length) + writeln(); + writeln(` ]`); + } + else + writeln(` "supplemental": []`); write(" }"); } @@ -156,7 +184,9 @@ void generateReport(string[] fileNames, const StaticAnalysisConfig config, auto reporter = new DScannerJsonReporter(); auto writeMessages = delegate void(string fileName, size_t line, size_t column, string message, bool isError){ - reporter.addMessage(Message(fileName, line, column, "dscanner.syntax", message), isError); + reporter.addMessage( + Message(Message.Diagnostic.from(fileName, line, column, column, message), "dscanner.syntax"), + isError); }; first = true; @@ -194,7 +224,9 @@ void generateSonarQubeGenericIssueDataReport(string[] fileNames, const StaticAna auto reporter = new SonarQubeGenericIssueDataReporter(); auto writeMessages = delegate void(string fileName, size_t line, size_t column, string message, bool isError){ - reporter.addMessage(Message(fileName, line, column, "dscanner.syntax", message), isError); + reporter.addMessage( + Message(Message.Diagnostic.from(fileName, line, column, column, message), "dscanner.syntax"), + isError); }; foreach (fileName; fileNames) @@ -280,7 +312,9 @@ const(Module) parseModule(string fileName, ubyte[] code, RollbackAllocator* p, ulong* linesOfCode = null, uint* errorCount = null, uint* warningCount = null) { auto writeMessages = delegate(string fileName, size_t line, size_t column, string message, bool isError){ - return messageFunctionFormat(errorFormat, Message(fileName, line, column, "dscanner.syntax", message), isError); + return messageFunctionFormat(errorFormat, + Message(Message.Diagnostic.from(fileName, line, column, column, message), "dscanner.syntax"), + isError); }; return parseModule(fileName, code, p, cache, tokens, diff --git a/src/dscanner/analysis/static_if_else.d b/src/dscanner/analysis/static_if_else.d index 17966da..e9c2d87 100644 --- a/src/dscanner/analysis/static_if_else.d +++ b/src/dscanner/analysis/static_if_else.d @@ -45,7 +45,10 @@ final class StaticIfElse : BaseAnalyzer { return; } - addErrorMessage(ifStmt.line, ifStmt.column, KEY, "Mismatched static if. Use 'else static if' here."); + auto tokens = ifStmt.tokens[0 .. 1]; + // extend one token to include `else` before this if + tokens = (tokens.ptr - 1)[0 .. 2]; + addErrorMessage(tokens, KEY, "Mismatched static if. Use 'else static if' here."); } const(IfStatement) getIfStatement(const ConditionalStatement cc) @@ -68,7 +71,8 @@ unittest void foo() { static if (false) auto a = 0; - else if (true) // [warn]: Mismatched static if. Use 'else static if' here. + else if (true) /+ + ^^^^^^^ [warn]: Mismatched static if. Use 'else static if' here. +/ auto b = 1; } }c, sac); diff --git a/src/dscanner/analysis/style.d b/src/dscanner/analysis/style.d index 96de04b..2ffbbcb 100644 --- a/src/dscanner/analysis/style.d +++ b/src/dscanner/analysis/style.d @@ -36,7 +36,7 @@ final class StyleChecker : BaseAnalyzer foreach (part; dec.moduleName.identifiers) { if (part.text.matchFirst(moduleNameRegex).length == 0) - addErrorMessage(part.line, part.column, KEY, + addErrorMessage(part, KEY, "Module/package name '" ~ part.text ~ "' does not match style guidelines."); } } @@ -102,7 +102,7 @@ final class StyleChecker : BaseAnalyzer void checkLowercaseName(string type, ref const Token name) { if (name.text.length > 0 && name.text.matchFirst(varFunNameRegex).length == 0) - addErrorMessage(name.line, name.column, KEY, + addErrorMessage(name, KEY, type ~ " name '" ~ name.text ~ "' does not match style guidelines."); } @@ -135,7 +135,7 @@ final class StyleChecker : BaseAnalyzer void checkAggregateName(string aggregateType, ref const Token name) { if (name.text.length > 0 && name.text.matchFirst(aggregateNameRegex).length == 0) - addErrorMessage(name.line, name.column, KEY, + addErrorMessage(name, KEY, aggregateType ~ " name '" ~ name.text ~ "' does not match style guidelines."); } @@ -166,17 +166,22 @@ unittest sac.style_check = Check.enabled; assertAnalyzerWarnings(q{ - module AMODULE; // [warn]: Module/package name 'AMODULE' does not match style guidelines. + module AMODULE; /+ + ^^^^^^^ [warn]: Module/package name 'AMODULE' does not match style guidelines. +/ bool A_VARIABLE; // FIXME: bool a_variable; // ok bool aVariable; // ok void A_FUNCTION() {} // FIXME: - class cat {} // [warn]: Class name 'cat' does not match style guidelines. - interface puma {} // [warn]: Interface name 'puma' does not match style guidelines. - struct dog {} // [warn]: Struct name 'dog' does not match style guidelines. - enum racoon { a } // [warn]: Enum name 'racoon' does not match style guidelines. + class cat {} /+ + ^^^ [warn]: Class name 'cat' does not match style guidelines. +/ + interface puma {} /+ + ^^^^ [warn]: Interface name 'puma' does not match style guidelines. +/ + struct dog {} /+ + ^^^ [warn]: Struct name 'dog' does not match style guidelines. +/ + enum racoon { a } /+ + ^^^^^^ [warn]: Enum name 'racoon' does not match style guidelines. +/ enum bool something = false; enum bool someThing = false; enum Cat { fritz, } @@ -194,7 +199,8 @@ unittest assertAnalyzerWarnings(q{ extern(Windows) { - extern(D) bool Fun2(); // [warn]: Function name 'Fun2' does not match style guidelines. + extern(D) bool Fun2(); /+ + ^^^^ [warn]: Function name 'Fun2' does not match style guidelines. +/ bool Fun3(); } }c, sac); @@ -203,8 +209,10 @@ unittest extern(Windows) { extern(C): - extern(D) bool Fun4(); // [warn]: Function name 'Fun4' does not match style guidelines. - bool Fun5(); // [warn]: Function name 'Fun5' does not match style guidelines. + extern(D) bool Fun4(); /+ + ^^^^ [warn]: Function name 'Fun4' does not match style guidelines. +/ + bool Fun5(); /+ + ^^^^ [warn]: Function name 'Fun5' does not match style guidelines. +/ } }c, sac); @@ -214,12 +222,14 @@ unittest bool Fun7(); extern(D): void okOkay(); - void NotReallyOkay(); // [warn]: Function name 'NotReallyOkay' does not match style guidelines. + void NotReallyOkay(); /+ + ^^^^^^^^^^^^^ [warn]: Function name 'NotReallyOkay' does not match style guidelines. +/ }c, sac); assertAnalyzerWarnings(q{ extern(Windows): - bool WinButWithBody(){} // [warn]: Function name 'WinButWithBody' does not match style guidelines. + bool WinButWithBody(){} /+ + ^^^^^^^^^^^^^^ [warn]: Function name 'WinButWithBody' does not match style guidelines. +/ }c, sac); stderr.writeln("Unittest for StyleChecker passed."); diff --git a/src/dscanner/analysis/trust_too_much.d b/src/dscanner/analysis/trust_too_much.d index f9eddf4..cd98460 100644 --- a/src/dscanner/analysis/trust_too_much.d +++ b/src/dscanner/analysis/trust_too_much.d @@ -39,10 +39,7 @@ public: override void visit(const AtAttribute d) { if (checkAtAttribute && d.identifier.text == "trusted") - { - const Token t = d.identifier; - addErrorMessage(t.line, t.column, KEY, MESSAGE); - } + addErrorMessage(d, KEY, MESSAGE); d.accept(this); } @@ -91,17 +88,20 @@ unittest //--- fail cases ---// assertAnalyzerWarnings(q{ - @trusted: // [warn]: %s + @trusted: /+ + ^^^^^^^^ [warn]: %s +/ void test(); }c.format(msg), sac); assertAnalyzerWarnings(q{ - @trusted @nogc: // [warn]: %s + @trusted @nogc: /+ + ^^^^^^^^ [warn]: %s +/ void test(); }c.format(msg), sac); assertAnalyzerWarnings(q{ - @trusted { // [warn]: %s + @trusted { /+ + ^^^^^^^^ [warn]: %s +/ void test(); void test(); } @@ -109,27 +109,31 @@ unittest assertAnalyzerWarnings(q{ @safe { - @trusted @nogc { // [warn]: %s + @trusted @nogc { /+ + ^^^^^^^^ [warn]: %s +/ void test(); void test(); }} }c.format(msg), sac); assertAnalyzerWarnings(q{ - @nogc @trusted { // [warn]: %s + @nogc @trusted { /+ + ^^^^^^^^ [warn]: %s +/ void test(); void test(); } }c.format(msg), sac); assertAnalyzerWarnings(q{ - @trusted template foo(){ // [warn]: %s + @trusted template foo(){ /+ + ^^^^^^^^ [warn]: %s +/ } }c.format(msg), sac); assertAnalyzerWarnings(q{ struct foo{ - @trusted: // [warn]: %s + @trusted: /+ + ^^^^^^^^ [warn]: %s +/ } }c.format(msg), sac); //--- pass cases ---// diff --git a/src/dscanner/analysis/undocumented.d b/src/dscanner/analysis/undocumented.d index f1d8157..f0ec55b 100644 --- a/src/dscanner/analysis/undocumented.d +++ b/src/dscanner/analysis/undocumented.d @@ -100,15 +100,14 @@ final class UndocumentedDeclarationCheck : BaseAnalyzer return; if (variable.autoDeclaration !is null) { - addMessage(variable.autoDeclaration.parts[0].identifier.line, - variable.autoDeclaration.parts[0].identifier.column, + addMessage(variable.autoDeclaration.parts[0].identifier, variable.autoDeclaration.parts[0].identifier.text); return; } foreach (dec; variable.declarators) { if (dec.comment.ptr is null) - addMessage(dec.name.line, dec.name.column, dec.name.text); + addMessage(dec.name, dec.name.text); return; } } @@ -174,20 +173,25 @@ private: || isGetterOrSetter(declaration.name.text) || isProperty(declaration))) { - addMessage(declaration.name.line, - declaration.name.column, declaration.name.text); + addMessage(declaration.name, declaration.name.text); } } else { if (declaration.name.type != tok!"") - addMessage(declaration.name.line, - declaration.name.column, declaration.name.text); + addMessage(declaration.name, declaration.name.text); } } else { - addMessage(declaration.line, declaration.column, null); + import std.algorithm : countUntil; + + // like constructors + auto tokens = declaration.tokens.findTokenForDisplay(tok!"this"); + auto earlyEnd = tokens.countUntil!(a => a == tok!"{" || a == tok!"(" || a == tok!";"); + if (earlyEnd != -1) + tokens = tokens[0 .. earlyEnd]; + addMessage(tokens, null); } } static if (!(is(T == TemplateDeclaration) || is(T == FunctionDeclaration))) @@ -215,11 +219,11 @@ private: return false; } - void addMessage(size_t line, size_t column, string name) + void addMessage(T)(T range, string name) { import std.string : format; - addErrorMessage(line, column, "dscanner.style.undocumented_declaration", name is null + addErrorMessage(range, "dscanner.style.undocumented_declaration", name is null ? "Public declaration is undocumented." : format("Public declaration '%s' is undocumented.", name)); } @@ -315,13 +319,20 @@ unittest sac.undocumented_declaration_check = Check.enabled; assertAnalyzerWarnings(q{ - class C{} // [warn]: Public declaration 'C' is undocumented. - interface I{} // [warn]: Public declaration 'I' is undocumented. - enum e = 0; // [warn]: Public declaration 'e' is undocumented. - void f(){} // [warn]: Public declaration 'f' is undocumented. - struct S{} // [warn]: Public declaration 'S' is undocumented. - template T(){} // [warn]: Public declaration 'T' is undocumented. - union U{} // [warn]: Public declaration 'U' is undocumented. + class C{} /+ + ^ [warn]: Public declaration 'C' is undocumented. +/ + interface I{} /+ + ^ [warn]: Public declaration 'I' is undocumented. +/ + enum e = 0; /+ + ^ [warn]: Public declaration 'e' is undocumented. +/ + void f(){} /+ + ^ [warn]: Public declaration 'f' is undocumented. +/ + struct S{} /+ + ^ [warn]: Public declaration 'S' is undocumented. +/ + template T(){} /+ + ^ [warn]: Public declaration 'T' is undocumented. +/ + union U{} /+ + ^ [warn]: Public declaration 'U' is undocumented. +/ }, sac); assertAnalyzerWarnings(q{ diff --git a/src/dscanner/analysis/unmodified.d b/src/dscanner/analysis/unmodified.d index 7bd9ef3..779d6ff 100644 --- a/src/dscanner/analysis/unmodified.d +++ b/src/dscanner/analysis/unmodified.d @@ -63,8 +63,7 @@ final class UnmodifiedFinder : BaseAnalyzer continue; if (initializedFromNew(d.initializer)) continue; - tree[$ - 1].insert(new VariableInfo(d.name.text, d.name.line, - d.name.column, isValueTypeSimple(dec.type))); + tree[$ - 1].insert(new VariableInfo(d.name.text, d.name, isValueTypeSimple(dec.type))); } } dec.accept(this); @@ -84,8 +83,7 @@ final class UnmodifiedFinder : BaseAnalyzer continue; if (initializedFromNew(part.initializer)) continue; - tree[$ - 1].insert(new VariableInfo(part.identifier.text, - part.identifier.line, part.identifier.column)); + tree[$ - 1].insert(new VariableInfo(part.identifier.text, part.identifier)); } } autoDeclaration.accept(this); @@ -292,8 +290,7 @@ private: static struct VariableInfo { string name; - size_t line; - size_t column; + Token token; bool isValueType; } @@ -303,7 +300,7 @@ private: { immutable string errorMessage = "Variable " ~ vi.name ~ " is never modified and could have been declared const or immutable."; - addErrorMessage(vi.line, vi.column, "dscanner.suspicious.unmodified", errorMessage); + addErrorMessage(vi.token, "dscanner.suspicious.unmodified", errorMessage); } tree = tree[0 .. $ - 1]; } @@ -346,7 +343,8 @@ bool isValueTypeSimple(const Type type) pure nothrow @nogc // fails assertAnalyzerWarnings(q{ - void foo(){int i = 1;} // [warn]: Variable i is never modified and could have been declared const or immutable. + void foo(){int i = 1;} /+ + ^ [warn]: Variable i is never modified and could have been declared const or immutable. +/ }, sac); // pass diff --git a/src/dscanner/analysis/unused.d b/src/dscanner/analysis/unused.d index 0aba757..5740ac3 100644 --- a/src/dscanner/analysis/unused.d +++ b/src/dscanner/analysis/unused.d @@ -338,11 +338,11 @@ abstract class UnusedIdentifierCheck : BaseAnalyzer protected Tree[] tree; - protected void variableDeclared(string name, size_t line, size_t column, bool isRef) + protected void variableDeclared(string name, Token token, bool isRef) { if (inAggregateScope || name.all!(a => a == '_')) return; - tree[$ - 1].insert(new UnUsed(name, line, column, isRef)); + tree[$ - 1].insert(new UnUsed(name, token, isRef)); } protected void pushScope() @@ -355,8 +355,7 @@ private: struct UnUsed { string name; - size_t line; - size_t column; + Token token; bool isRef; bool uncertain; } @@ -450,8 +449,7 @@ abstract class UnusedStorageCheck : UnusedIdentifierCheck if (uu.uncertain) continue; immutable string errorMessage = publicType ~ ' ' ~ uu.name ~ " is never used."; - addErrorMessage(uu.line, uu.column, - "dscanner.suspicious." ~ reportType, errorMessage); + addErrorMessage(uu.token, "dscanner.suspicious." ~ reportType, errorMessage); } } } diff --git a/src/dscanner/analysis/unused_label.d b/src/dscanner/analysis/unused_label.d index b70d398..ae79171 100644 --- a/src/dscanner/analysis/unused_label.d +++ b/src/dscanner/analysis/unused_label.d @@ -57,16 +57,15 @@ final class UnusedLabelCheck : BaseAnalyzer override void visit(const LabeledStatement labeledStatement) { - auto token = &labeledStatement.identifier; + auto token = labeledStatement.identifier; Label* label = token.text in current; if (label is null) { - current[token.text] = Label(token.text, token.line, token.column, false); + current[token.text] = Label(token.text, token, false); } else { - label.line = token.line; - label.column = token.column; + label.token = token; } if (labeledStatement.declarationOrStatement !is null) labeledStatement.declarationOrStatement.accept(this); @@ -119,8 +118,7 @@ private: static struct Label { string name; - size_t line; - size_t column; + Token token; bool used; } @@ -140,13 +138,13 @@ private: { foreach (label; current.byValue()) { - if (label.line == size_t.max || label.column == size_t.max) + if (label.token is Token.init) { // TODO: handle unknown labels } else if (!label.used) { - addErrorMessage(label.line, label.column, "dscanner.suspicious.unused_label", + addErrorMessage(label.token, "dscanner.suspicious.unused_label", "Label \"" ~ label.name ~ "\" is not used."); } } @@ -157,7 +155,7 @@ private: { Label* entry = name in current; if (entry is null) - current[name] = Label(name, size_t.max, size_t.max, true); + current[name] = Label(name, Token.init, true); else entry.used = true; } @@ -174,14 +172,16 @@ unittest int testUnusedLabel() { int x = 0; - A: // [warn]: Label "A" is not used. + A: /+ + ^ [warn]: Label "A" is not used. +/ if (x) goto B; x++; B: goto C; void foo() { - C: // [warn]: Label "C" is not used. + C: /+ + ^ [warn]: Label "C" is not used. +/ return; } C: @@ -191,10 +191,12 @@ unittest D: return; } - D: // [warn]: Label "D" is not used. + D: /+ + ^ [warn]: Label "D" is not used. +/ goto E; () { - E: // [warn]: Label "E" is not used. + E: /+ + ^ [warn]: Label "E" is not used. +/ return; }(); E: @@ -203,9 +205,11 @@ unittest F: return; }(); - F: // [warn]: Label "F" is not used. + F: /+ + ^ [warn]: Label "F" is not used. +/ return x; - G: // [warn]: Label "G" is not used. + G: /+ + ^ [warn]: Label "G" is not used. +/ } }c, sac); @@ -221,7 +225,8 @@ unittest void testAsm() { asm { mov RAX,1;} - lbl: // [warn]: Label "lbl" is not used. + lbl: /+ + ^^^ [warn]: Label "lbl" is not used. +/ } }c, sac); diff --git a/src/dscanner/analysis/unused_parameter.d b/src/dscanner/analysis/unused_parameter.d index 6680c0c..934320c 100644 --- a/src/dscanner/analysis/unused_parameter.d +++ b/src/dscanner/analysis/unused_parameter.d @@ -41,8 +41,7 @@ final class UnusedParameterCheck : UnusedStorageCheck immutable bool isPtr = parameter.type && !parameter.type .typeSuffixes.filter!(a => a.star != tok!"").empty; - variableDeclared(parameter.name.text, parameter.name.line, - parameter.name.column, isRef | isPtr); + variableDeclared(parameter.name.text, parameter.name, isRef | isPtr); if (parameter.default_ !is null) { @@ -72,9 +71,11 @@ final class UnusedParameterCheck : UnusedStorageCheck is(StringTypeOf!R)); } - void inPSC(in int a){} // [warn]: Parameter a is never used. + void inPSC(in int a){} /+ + ^ [warn]: Parameter a is never used. +/ - void doStuff(int a, int b) // [warn]: Parameter b is never used. + void doStuff(int a, int b) /+ + ^ [warn]: Parameter b is never used. +/ { return a; } @@ -95,7 +96,8 @@ final class UnusedParameterCheck : UnusedStorageCheck { auto cb1 = delegate(size_t _) {}; cb1(3); - auto cb2 = delegate(size_t a) {}; // [warn]: Parameter a is never used. + auto cb2 = delegate(size_t a) {}; /+ + ^ [warn]: Parameter a is never used. +/ cb2(3); } diff --git a/src/dscanner/analysis/unused_result.d b/src/dscanner/analysis/unused_result.d index 3977e15..f7f4aa3 100644 --- a/src/dscanner/analysis/unused_result.d +++ b/src/dscanner/analysis/unused_result.d @@ -4,14 +4,15 @@ // http://www.boost.org/LICENSE_1_0.txt) module dscanner.analysis.unused_result; +import dparse.ast; +import dparse.lexer; import dscanner.analysis.base; -import dscanner.analysis.mismatched_args : resolveSymbol, IdentVisitor; +import dscanner.analysis.mismatched_args : IdentVisitor, resolveSymbol; import dscanner.utils; import dsymbol.scope_; import dsymbol.symbol; -import dparse.ast, dparse.lexer; -import std.algorithm.searching : canFind; -import std.range: retro; +import std.algorithm : canFind, countUntil; +import std.range : retro; /** * Checks for function call statements which call non-void functions. @@ -93,7 +94,13 @@ public: return; } - addErrorMessage(decl.expression.line, decl.expression.column, KEY, MSG); + const(Token)[] tokens = fce.unaryExpression + ? fce.unaryExpression.tokens + : fce.type + ? fce.type.tokens + : fce.tokens; + + addErrorMessage(tokens, KEY, MSG); } } @@ -128,7 +135,8 @@ unittest int fun() { return 1; } void main() { - fun(); // [warn]: %s + fun(); /+ + ^^^ [warn]: %s +/ } }c.format(UnusedResultChecker.MSG), sac); @@ -143,7 +151,8 @@ unittest alias Bar = Foo; void main() { - Bar.get(); // [warn]: %s + Bar.get(); /+ + ^^^^^^^ [warn]: %s +/ } }c.format(UnusedResultChecker.MSG), sac); @@ -160,7 +169,8 @@ unittest void main() { int fun() { return 1; } - fun(); // [warn]: %s + fun(); /+ + ^^^ [warn]: %s +/ } }c.format(UnusedResultChecker.MSG), sac); diff --git a/src/dscanner/analysis/unused_variable.d b/src/dscanner/analysis/unused_variable.d index c3b3689..176477c 100644 --- a/src/dscanner/analysis/unused_variable.d +++ b/src/dscanner/analysis/unused_variable.d @@ -31,14 +31,14 @@ final class UnusedVariableCheck : UnusedStorageCheck override void visit(const VariableDeclaration variableDeclaration) { foreach (d; variableDeclaration.declarators) - this.variableDeclared(d.name.text, d.name.line, d.name.column, false); + this.variableDeclared(d.name.text, d.name, false); variableDeclaration.accept(this); } override void visit(const AutoDeclaration autoDeclaration) { foreach (t; autoDeclaration.parts.map!(a => a.identifier)) - this.variableDeclared(t.text, t.line, t.column, false); + this.variableDeclared(t.text, t, false); autoDeclaration.accept(this); } } @@ -62,7 +62,8 @@ final class UnusedVariableCheck : UnusedStorageCheck unittest { - int a; // [warn]: Variable a is never used. + int a; /+ + ^ [warn]: Variable a is never used. +/ } // Issue 380 @@ -75,7 +76,8 @@ final class UnusedVariableCheck : UnusedStorageCheck // Issue 380 int otherTemplatedEnum() { - auto a(T) = T.init; // [warn]: Variable a is never used. + auto a(T) = T.init; /+ + ^ [warn]: Variable a is never used. +/ return 0; } diff --git a/src/dscanner/analysis/useless_assert.d b/src/dscanner/analysis/useless_assert.d index 0b75ff2..4477947 100644 --- a/src/dscanner/analysis/useless_assert.d +++ b/src/dscanner/analysis/useless_assert.d @@ -94,7 +94,7 @@ final class UselessAssertCheck : BaseAnalyzer default: return; } - addErrorMessage(ae.line, ae.column, KEY, MESSAGE); + addErrorMessage(unary, KEY, MESSAGE); } private: @@ -113,9 +113,12 @@ unittest assertAnalyzerWarnings(q{ unittest { - assert(true); // [warn]: %1$s - assert(1); // [warn]: %1$s - assert([10]); // [warn]: %1$s + assert(true); /+ + ^^^^ [warn]: %1$s +/ + assert(1); /+ + ^ [warn]: %1$s +/ + assert([10]); /+ + ^^^^ [warn]: %1$s +/ assert(false); assert(0); assert(0.0L); diff --git a/src/dscanner/analysis/useless_initializer.d b/src/dscanner/analysis/useless_initializer.d index 2dc5394..c482db2 100644 --- a/src/dscanner/analysis/useless_initializer.d +++ b/src/dscanner/analysis/useless_initializer.d @@ -155,14 +155,18 @@ public: version(unittest) { - enum warn = q{addErrorMessage(declarator.name.line, - declarator.name.column, key, msg);}; + void warn(const BaseNode range) + { + addErrorMessage(range, key, msg); + } } else { import std.format : format; - enum warn = q{addErrorMessage(declarator.name.line, - declarator.name.column, key, msg.format(declarator.name.text));}; + void warn(const BaseNode range) + { + addErrorMessage(range, key, msg.format(declarator.name.text)); + } } // --- Info about the declaration type --- // @@ -203,32 +207,32 @@ public: case tok!"cent", tok!"ucent": case tok!"bool": if (intDefs.canFind(value.text) || value == tok!"false") - mixin(warn); + warn(nvi); goto default; default: // check for BasicType.init if (ue.primaryExpression.basicType.type == decl.type.type2.builtinType && ue.primaryExpression.primary.text == "init" && !ue.primaryExpression.expression) - mixin(warn); + warn(nvi); } } else if (isSzInt) { if (intDefs.canFind(value.text)) - mixin(warn); + warn(nvi); } else if (isPtr || isStr) { if (str(value.type) == "null") - mixin(warn); + warn(nvi); } else if (isArr) { if (str(value.type) == "null") - mixin(warn); + warn(nvi); else if (nvi.arrayInitializer && nvi.arrayInitializer.arrayMemberInitializations.length == 0) - mixin(warn); + warn(nvi); } } @@ -242,7 +246,7 @@ public: if (customType.text in _structCanBeInit) { if (!_structCanBeInit[customType.text]) - mixin(warn); + warn(nvi); } } } @@ -251,7 +255,7 @@ public: else if (nvi.arrayInitializer && (isArr || isStr)) { if (nvi.arrayInitializer.arrayMemberInitializations.length == 0) - mixin(warn); + warn(nvi); } } @@ -271,25 +275,44 @@ public: // fails assertAnalyzerWarnings(q{ struct S {} - ubyte a = 0x0; // [warn]: X - int a = 0; // [warn]: X - ulong a = 0; // [warn]: X - int* a = null; // [warn]: X - Foo* a = null; // [warn]: X - int[] a = null; // [warn]: X - int[] a = []; // [warn]: X - string a = null; // [warn]: X - string a = null; // [warn]: X - wstring a = null; // [warn]: X - dstring a = null; // [warn]: X - size_t a = 0; // [warn]: X - ptrdiff_t a = 0; // [warn]: X - string a = []; // [warn]: X - char[] a = null; // [warn]: X - int a = int.init; // [warn]: X - char a = char.init; // [warn]: X - S s = S.init; // [warn]: X - bool a = false; // [warn]: X + ubyte a = 0x0; /+ + ^^^ [warn]: X +/ + int a = 0; /+ + ^ [warn]: X +/ + ulong a = 0; /+ + ^ [warn]: X +/ + int* a = null; /+ + ^^^^ [warn]: X +/ + Foo* a = null; /+ + ^^^^ [warn]: X +/ + int[] a = null; /+ + ^^^^ [warn]: X +/ + int[] a = []; /+ + ^^ [warn]: X +/ + string a = null; /+ + ^^^^ [warn]: X +/ + string a = null; /+ + ^^^^ [warn]: X +/ + wstring a = null; /+ + ^^^^ [warn]: X +/ + dstring a = null; /+ + ^^^^ [warn]: X +/ + size_t a = 0; /+ + ^ [warn]: X +/ + ptrdiff_t a = 0; /+ + ^ [warn]: X +/ + string a = []; /+ + ^^ [warn]: X +/ + char[] a = null; /+ + ^^^^ [warn]: X +/ + int a = int.init; /+ + ^^^^^^^^ [warn]: X +/ + char a = char.init; /+ + ^^^^^^^^^ [warn]: X +/ + S s = S.init; /+ + ^^^^^^ [warn]: X +/ + bool a = false; /+ + ^^^^^ [warn]: X +/ }, sac); // passes diff --git a/src/dscanner/analysis/vcall_in_ctor.d b/src/dscanner/analysis/vcall_in_ctor.d index e2a0f6d..9877b9f 100644 --- a/src/dscanner/analysis/vcall_in_ctor.d +++ b/src/dscanner/analysis/vcall_in_ctor.d @@ -136,7 +136,7 @@ private: { if (call == vm) { - addErrorMessage(call.line, call.column, KEY, MSG); + addErrorMessage(call, KEY, MSG); break; } } @@ -306,7 +306,8 @@ unittest assertAnalyzerWarnings(q{ class Bar { - this(){foo();} // [warn]: %s + this(){foo();} /+ + ^^^ [warn]: %s +/ private: public void foo(){} @@ -319,8 +320,10 @@ unittest { this() { - foo(); // [warn]: %s - foo(); // [warn]: %s + foo(); /+ + ^^^ [warn]: %s +/ + foo(); /+ + ^^^ [warn]: %s +/ bar(); } private: void bar(); @@ -334,7 +337,8 @@ unittest this() { foo(); - bar(); // [warn]: %s + bar(); /+ + ^^^ [warn]: %s +/ } private: public void bar(); public private {void foo(){}} diff --git a/src/dscanner/main.d b/src/dscanner/main.d index d8c98a1..7dd0607 100644 --- a/src/dscanner/main.d +++ b/src/dscanner/main.d @@ -406,7 +406,7 @@ Options: Format errors produced by the style/syntax checkers. The default value for the pattern is: "%2$s". Supported placeholders are: {filepath}, {line}, {column}, {type}, - {message}, and {name}. + {endLine}, {endColumn}, {message}, and {name}. --ctags ..., -c ... Generates ctags information from the given source code file. Note that diff --git a/src/dscanner/reports.d b/src/dscanner/reports.d index 3e0c9b7..a2a21b9 100644 --- a/src/dscanner/reports.d +++ b/src/dscanner/reports.d @@ -59,10 +59,24 @@ class DScannerJsonReporter JSONValue js = JSONValue([ "key": JSONValue(issue.message.key), "fileName": JSONValue(issue.message.fileName), - "line": JSONValue(issue.message.line), - "column": JSONValue(issue.message.column), + "line": JSONValue(issue.message.startLine), + "column": JSONValue(issue.message.startColumn), + "endLine": JSONValue(issue.message.endLine), + "endColumn": JSONValue(issue.message.endColumn), "message": JSONValue(issue.message.message), - "type": JSONValue(issue.type) + "type": JSONValue(issue.type), + "supplemental": JSONValue( + issue.message.supplemental.map!(a => + JSONValue([ + "fileName": JSONValue(a.fileName), + "line": JSONValue(a.startLine), + "column": JSONValue(a.startColumn), + "endLine": JSONValue(a.endLine), + "endColumn": JSONValue(a.endColumn), + "message": JSONValue(a.message), + ]) + ).array + ) ]); // dfmt on @@ -129,10 +143,10 @@ class SonarQubeGenericIssueDataReporter struct TextRange { - // SonarQube Generic Issue only allows specifying start line only - // or the complete range, for which start and end has to differ - // D-Scanner does not provide the complete range info long startLine; + long endLine; + long startColumn; + long endColumn; } private Appender!(Issue[]) _issues; @@ -160,6 +174,20 @@ class SonarQubeGenericIssueDataReporter return result.toPrettyString(); } + private static JSONValue toJson(Location location) + { + return JSONValue([ + "message": JSONValue(location.message), + "filePath": JSONValue(location.filePath), + "textRange": JSONValue([ + "startLine": JSONValue(location.textRange.startLine), + "endLine": JSONValue(location.textRange.endLine), + "startColumn": JSONValue(location.textRange.startColumn), + "endColumn": JSONValue(location.textRange.endColumn) + ]), + ]); + } + private static JSONValue toJson(Issue issue) { // dfmt off @@ -168,13 +196,8 @@ class SonarQubeGenericIssueDataReporter "ruleId": JSONValue(issue.ruleId), "severity": JSONValue(issue.severity), "type": JSONValue(issue.type), - "primaryLocation": JSONValue([ - "message": JSONValue(issue.primaryLocation.message), - "filePath": JSONValue(issue.primaryLocation.filePath), - "textRange": JSONValue([ - "startLine": JSONValue(issue.primaryLocation.textRange.startLine) - ]), - ]), + "primaryLocation": toJson(issue.primaryLocation), + "secondaryLocations": JSONValue(issue.secondaryLocations.map!toJson.array), ]); // dfmt on } @@ -189,18 +212,20 @@ class SonarQubeGenericIssueDataReporter // dfmt off Issue issue = { engineId: "dscanner", - ruleId : message.key, - severity : (isError) ? Severity.blocker : getSeverity(message.key), - type : getType(message.key), - primaryLocation : getPrimaryLocation(message) + ruleId: message.key, + severity: (isError) ? Severity.blocker : getSeverity(message.key), + type: getType(message.key), + primaryLocation: getLocation(message.diagnostic), + secondaryLocations: message.supplemental.map!getLocation.array }; // dfmt on return issue; } - private static Location getPrimaryLocation(Message message) + private static Location getLocation(Message.Diagnostic diag) { - return Location(message.message, message.fileName, TextRange(message.line)); + return Location(diag.message, diag.fileName, + TextRange(diag.startLine, diag.endLine, diag.startColumn, diag.endColumn)); } private static string getSeverity(string key)