From f326cad7865b62fe5192c3fa209a2ba9a10e0a48 Mon Sep 17 00:00:00 2001 From: Sebastian Wilzbach Date: Wed, 28 Mar 2018 06:36:20 +0200 Subject: [PATCH 01/10] Improve the binary release building (Linux, OSX) --- .travis.yml | 41 ++++++++++++++++++++++++++++------------- makefile | 24 ++++++++++++++++++++++++ 2 files changed, 52 insertions(+), 13 deletions(-) diff --git a/.travis.yml b/.travis.yml index 3e646df..2a33498 100644 --- a/.travis.yml +++ b/.travis.yml @@ -15,16 +15,31 @@ env: script: "./.travis.sh" jobs: include: - - stage: GitHub Release - d: ldc - os: linux - script: echo "Deploying to GitHub releases ..." && make ldcbuild - deploy: - provider: releases - api_key: - secure: pbrrm6E0SPfVwt9g+e/ZFQfrmRuGBNA6KwMMLUhI+2+kbRzNquxvrYAUC7YcRX7xiRL/gugKHmOXEi1Dv9IEdSQ732M06H7ikZT9T9oQWYbsZzmVICBWgIovyM8XIPpVAwP8D7jq0JgMiBicqfEZfoz2SIJjo6aYbyQbCASCu8U= - file: bin/dscanner - skip_cleanup: true - on: - repo: dlang-community/D-Scanner - tags: true + - stage: GitHub Release + if: tag IS present + d: ldc + os: linux + script: echo "Deploying to GitHub releases ..." && make release + deploy: + provider: releases + api_key: $GH_REPO_TOKEN + file_glob: true + file: bin/dscanner-*.tar.gz + skip_cleanup: true + on: + repo: dlang-community/D-Scanner + tags: true + - stage: GitHub Release + if: tag IS present + d: ldc + os: osx + script: echo "Deploying to GitHub releases ..." && make release + deploy: + provider: releases + api_key: $GH_REPO_TOKEN + file_glob: true + file: bin/dscanner-*.tar.gz + skip_cleanup: true + on: + repo: dlang-community/D-Scanner + tags: true diff --git a/makefile b/makefile index 8fe0445..814e892 100644 --- a/makefile +++ b/makefile @@ -72,3 +72,27 @@ clean: report: all dscanner --report src > src/dscanner-report.json sonar-runner + +.ONESHELL: +release: + @set -eux -o pipefail + VERSION=$$(git describe --abbrev=0 --tags) + ARCH="$${ARCH:-64}" + unameOut="$$(uname -s)" + case "$$unameOut" in + Linux*) OS=linux; ;; + Darwin*) OS=osx; ;; + *) echo "Unknown OS: $$unameOut"; exit 1 + esac + + case "$$ARCH" in + 64) ARCH_SUFFIX="x86_64";; + 32) ARCH_SUFFIX="x86";; + *) echo "Unknown ARCH: $$ARCH"; exit 1 + esac + + archiveName="dscanner-$$VERSION-$$OS-$$ARCH_SUFFIX.tar.gz" + + echo "Building $$archiveName" + ${MAKE} ldcbuild + tar cvfz "bin/$$archiveName" -C bin dscanner From 67e0f8a9e4c2dbd4ab630b593c2cdb3bb8d1dc9d Mon Sep 17 00:00:00 2001 From: Basile Burg Date: Fri, 30 Mar 2018 23:59:36 +0200 Subject: [PATCH 02/10] Use new config name override instead of breaking existing ini --- .dscanner.ini | 2 +- dub.json | 2 +- inifiled | 2 +- src/dscanner/analysis/config.d | 2 +- src/dscanner/main.d | 46 +--------------------------------- 5 files changed, 5 insertions(+), 49 deletions(-) diff --git a/.dscanner.ini b/.dscanner.ini index d0ff8d7..5f3682c 100644 --- a/.dscanner.ini +++ b/.dscanner.ini @@ -1,5 +1,5 @@ ; Configure which static analysis checks are enabled -[dscanner.analysis.config.StaticAnalysisConfig] +[analysis.config.StaticAnalysisConfig] ; Check variable, class, struct, interface, union, and function names against t ; he Phobos style guide style_check="disabled" diff --git a/dub.json b/dub.json index 021543e..295b7e2 100644 --- a/dub.json +++ b/dub.json @@ -14,7 +14,7 @@ "dependencies" : { "libdparse": "~>0.8.0-alpha.5", "dsymbol" : "~>0.3.0-alpha.3", - "inifiled" : "~>1.1.0", + "inifiled" : "~>1.2.0", "emsi_containers" : "~>0.6.0", "libddoc" : "~>0.3.0-beta.1", "stdx-allocator" : "~>2.77.0" diff --git a/inifiled b/inifiled index e15038a..971c535 160000 --- a/inifiled +++ b/inifiled @@ -1 +1 @@ -Subproject commit e15038a5c265a9fdaea354476e7759d04e8d0bf9 +Subproject commit 971c5356388a73ebbf69e32f7f5e97cfc06cdcff diff --git a/src/dscanner/analysis/config.d b/src/dscanner/analysis/config.d index 79e5c99..78f592d 100644 --- a/src/dscanner/analysis/config.d +++ b/src/dscanner/analysis/config.d @@ -54,7 +54,7 @@ StaticAnalysisConfig disabledConfig() return config; } -@INI("Configure which static analysis checks are enabled") +@INI("Configure which static analysis checks are enabled", "analysis.config.StaticAnalysisConfig") struct StaticAnalysisConfig { @INI("Check variable, class, struct, interface, union, and function names against the Phobos style guide") diff --git a/src/dscanner/main.d b/src/dscanner/main.d index 8875797..064ec41 100644 --- a/src/dscanner/main.d +++ b/src/dscanner/main.d @@ -68,7 +68,6 @@ else bool printVersion; bool explore; string errorFormat; - bool patchConfig; try { @@ -97,8 +96,7 @@ else "muffinButton", &muffin, "explore", &explore, "skipTests", &skipTests, - "errorFormat|f", &errorFormat, - "patchConfig", &patchConfig); + "errorFormat|f", &errorFormat); //dfmt on } catch (ConvException e) @@ -235,11 +233,7 @@ else StaticAnalysisConfig config = defaultStaticAnalysisConfig(); string s = configLocation is null ? getConfigurationLocation() : configLocation; if (s.exists()) - { - if (hasWrongIniFileSection(s, patchConfig)) - return 0; readINIFile(config, s); - } if (skipTests) config.enabled2SkipTests; if (report) @@ -472,41 +466,3 @@ string getConfigurationLocation() return getDefaultConfigurationLocation(); } - - -/// Patch the INI file to v0.5.0 format. -//TODO: remove this from v0.6.0 -bool hasWrongIniFileSection(string configFilename, bool patch) -{ - import std.string : indexOf; - import std.array : replace; - - bool result; - - static immutable v1 = "analysis.config.StaticAnalysisConfig"; - static immutable v2 = "dscanner.analysis.config.StaticAnalysisConfig"; - - char[] c = cast(char[]) readFile(configFilename); - try if (c.indexOf(v2) < 0) - { - if (!patch) - { - writeln("warning, the configuration file `", configFilename, "` contains an outdated property"); - writeln("change manually [", v1, "] to [", v2, "]" ); - writeln("or restart D-Scanner with the `--patchConfig` option"); - result = true; - } - else - { - c = replace(c, v1, v2); - std.file.write(configFilename, c); - writeln("the configuration file `", configFilename, "` has been updated correctly"); - } - } - catch(Exception e) - { - stderr.writeln("error encountered when trying to verify the INI file compatibility"); - throw e; - } - return result; -} From bed35ad9b8967439b5282f03592ccda9bddb6a5f Mon Sep 17 00:00:00 2001 From: Basile Burg Date: Sat, 31 Mar 2018 00:49:08 +0200 Subject: [PATCH 03/10] remove help for non existing option --- src/dscanner/main.d | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/dscanner/main.d b/src/dscanner/main.d index 064ec41..d635176 100644 --- a/src/dscanner/main.d +++ b/src/dscanner/main.d @@ -387,10 +387,7 @@ Options: Generates a default configuration file for the static analysis checks, --skipTests - Does not analyze in the unittests. Only works if --styleCheck., - - --patchConfig - Patches the configuration file passed as parameter for v0.5.0.`, + Does not analyze in the unittests. Only works if --styleCheck.`, programName); } From 4b92b87c8cb02bc5ceb01bf0ebed129090ec3091 Mon Sep 17 00:00:00 2001 From: Sebastian Wilzbach Date: Sat, 31 Mar 2018 01:39:19 +0200 Subject: [PATCH 04/10] Fix Make's shell to Bash --- makefile | 1 + 1 file changed, 1 insertion(+) diff --git a/makefile b/makefile index 814e892..0b5a302 100644 --- a/makefile +++ b/makefile @@ -27,6 +27,7 @@ VERSIONS = DEBUG_VERSIONS = -version=dparse_verbose DMD_FLAGS = -w -inline -release -O -J. -od${OBJ_DIR} -version=StdLoggerDisableWarning DMD_TEST_FLAGS = -w -g -J. -version=StdLoggerDisableWarning +SHELL:=/bin/bash all: dmdbuild ldc: ldcbuild From 0e35538bbd8de2a83b9c2a017244c78f41e05543 Mon Sep 17 00:00:00 2001 From: BBasile Date: Mon, 2 Apr 2018 07:48:20 +0200 Subject: [PATCH 05/10] Adds a check for too much trusted scope, close #545 (#581) Adds a check for too much trusted scope, close #545 merged-on-behalf-of: BBasile --- README.md | 1 + src/dscanner/analysis/config.d | 64 ++++++------ src/dscanner/analysis/run.d | 5 + src/dscanner/analysis/trust_too_much.d | 136 +++++++++++++++++++++++++ 4 files changed, 175 insertions(+), 31 deletions(-) create mode 100644 src/dscanner/analysis/trust_too_much.d diff --git a/README.md b/README.md index 54dca6b..ac4ca07 100644 --- a/README.md +++ b/README.md @@ -141,6 +141,7 @@ Note that the "--skipTests" option is the equivalent of changing each * Public declarations without a documented unittest. By default disabled. * Asserts without an explanatory message. By default disabled. * Indentation of if constraints +* Check that `@trusted` is not applied to a whole scope. Trusting a whole scope can be a problem when new declarations are added and if they are not verified manually to be trustable. #### Wishlist diff --git a/src/dscanner/analysis/config.d b/src/dscanner/analysis/config.d index 78f592d..c750f68 100644 --- a/src/dscanner/analysis/config.d +++ b/src/dscanner/analysis/config.d @@ -17,41 +17,40 @@ StaticAnalysisConfig defaultStaticAnalysisConfig() /// Describes how a check is operated. enum Check: string { - /// Check is disabled. - disabled = "disabled", - /// Check is enabled. - enabled = "enabled", - /// Check is enabled but not operated in the unittests. - skipTests = "skip-unittest" + /// Check is disabled. + disabled = "disabled", + /// Check is enabled. + enabled = "enabled", + /// Check is enabled but not operated in the unittests. + skipTests = "skip-unittest" } /// Applies the --skipTests switch, allowing to call Dscanner without config /// and less noise related to the unittests. void enabled2SkipTests(ref StaticAnalysisConfig config) { - foreach (mem; __traits(allMembers, StaticAnalysisConfig)) - { - static if (is(typeof(__traits(getMember, StaticAnalysisConfig, mem)))) - static if (is(typeof(__traits(getMember, config, mem)) == string)) - { - if (__traits(getMember, config, mem) == Check.enabled) - __traits(getMember, config, mem) = Check.skipTests; - - } - } + foreach (mem; __traits(allMembers, StaticAnalysisConfig)) + { + static if (is(typeof(__traits(getMember, StaticAnalysisConfig, mem)))) + static if (is(typeof(__traits(getMember, config, mem)) == string)) + { + if (__traits(getMember, config, mem) == Check.enabled) + __traits(getMember, config, mem) = Check.skipTests; + } + } } /// Returns a config with all the checks disabled. StaticAnalysisConfig disabledConfig() { - StaticAnalysisConfig config; - foreach (mem; __traits(allMembers, StaticAnalysisConfig)) - { - static if (is(typeof(__traits(getMember, StaticAnalysisConfig, mem)))) - static if (is(typeof(__traits(getMember, config, mem)) == string)) - __traits(getMember, config, mem) = Check.disabled; - } - return config; + StaticAnalysisConfig config; + foreach (mem; __traits(allMembers, StaticAnalysisConfig)) + { + static if (is(typeof(__traits(getMember, StaticAnalysisConfig, mem)))) + static if (is(typeof(__traits(getMember, config, mem)) == string)) + __traits(getMember, config, mem) = Check.disabled; + } + return config; } @INI("Configure which static analysis checks are enabled", "analysis.config.StaticAnalysisConfig") @@ -171,14 +170,14 @@ struct StaticAnalysisConfig @INI("Check for properly documented public functions (Returns, Params)") string properly_documented_public_functions = Check.disabled; - @INI("Check for useless usage of the final attribute") - string final_attribute_check = Check.enabled; + @INI("Check for useless usage of the final attribute") + string final_attribute_check = Check.enabled; - @INI("Check for virtual calls in the class constructors") - string vcall_in_ctor = Check.enabled; + @INI("Check for virtual calls in the class constructors") + string vcall_in_ctor = Check.enabled; - @INI("Check for useless user defined initializers") - string useless_initializer = Check.enabled; + @INI("Check for useless user defined initializers") + string useless_initializer = Check.enabled; @INI("Check allman brace style") string allman_braces_check = Check.disabled; @@ -195,6 +194,9 @@ struct StaticAnalysisConfig @INI("Check indent of if constraints") string if_constraints_indent = Check.disabled; + @INI("Check for @trusted applied to a bigger scope than a single function") + string trust_too_much = Check.enabled; + @INI("Module-specific filters") ModuleFilters filters; } @@ -207,7 +209,7 @@ private template ModuleFiltersMixin(A) static if (is(typeof(__traits(getMember, StaticAnalysisConfig, mem)) == string)) s ~= `@INI("Exclude/Import modules") string[] ` ~ mem ~ ";\n"; - return s; + return s; }(); } diff --git a/src/dscanner/analysis/run.d b/src/dscanner/analysis/run.d index 005b288..0442820 100644 --- a/src/dscanner/analysis/run.d +++ b/src/dscanner/analysis/run.d @@ -72,6 +72,7 @@ import dscanner.analysis.redundant_attributes; import dscanner.analysis.has_public_example; import dscanner.analysis.assert_without_msg; import dscanner.analysis.if_constraints_indent; +import dscanner.analysis.trust_too_much; import dsymbol.string_interning : internString; import dsymbol.scope_; @@ -511,6 +512,10 @@ MessageSet analyze(string fileName, const Module m, const StaticAnalysisConfig a checks ~= new IfConstraintsIndentCheck(fileName, tokens, analysisConfig.if_constraints_indent == Check.skipTests && !ut); + if (moduleName.shouldRun!"trust_too_much"(analysisConfig)) + checks ~= new TrustTooMuchCheck(fileName, + analysisConfig.trust_too_much == Check.skipTests && !ut); + version (none) if (moduleName.shouldRun!"redundant_if_check"(analysisConfig)) checks ~= new IfStatementCheck(fileName, moduleScope, diff --git a/src/dscanner/analysis/trust_too_much.d b/src/dscanner/analysis/trust_too_much.d new file mode 100644 index 0000000..84a13e1 --- /dev/null +++ b/src/dscanner/analysis/trust_too_much.d @@ -0,0 +1,136 @@ +// Copyright The dlang community - 2018 +// Distributed under the Boost Software License, Version 1.0. +// (See accompanying file LICENSE_1_0.txt or copy at +// http://www.boost.org/LICENSE_1_0.txt) + +module dscanner.analysis.trust_too_much; + +import std.stdio; +import dparse.ast; +import dparse.lexer; +import dscanner.analysis.base; +import dsymbol.scope_; + +/** + * Checks that `@trusted` is only applied to a a single function + */ +class TrustTooMuchCheck : BaseAnalyzer +{ +private: + + static immutable MESSAGE = "Trusting a whole scope is a bad idea, " ~ + "`@trusted` should only be attached to a single function"; + static immutable string KEY = "dscanner.trust_too_much"; + + bool checkAtAttribute = true; + +public: + + alias visit = BaseAnalyzer.visit; + + /// + this(string fileName, bool skipTests = false) + { + super(fileName, sc, skipTests); + } + + override void visit(const AtAttribute d) + { + if (checkAtAttribute && d.identifier.text == "trusted") + { + const Token t = d.identifier; + addErrorMessage(t.line, t.column, KEY, MESSAGE); + } + d.accept(this); + } + + // always applied to function body, so OK + override void visit(const MemberFunctionAttribute d) + { + const oldCheckAtAttribute = checkAtAttribute; + checkAtAttribute = false; + d.accept(this); + checkAtAttribute = oldCheckAtAttribute; + } + + // handles `@trusted{}` and old style, leading, atAttribute for single funcs + override void visit(const Declaration d) + { + const oldCheckAtAttribute = checkAtAttribute; + checkAtAttribute = d.functionDeclaration is null; + d.accept(this); + checkAtAttribute = oldCheckAtAttribute; + } +} + +unittest +{ + import dscanner.analysis.config : StaticAnalysisConfig, Check, disabledConfig; + import dscanner.analysis.helpers : assertAnalyzerWarnings; + import std.format : format; + + StaticAnalysisConfig sac = disabledConfig(); + sac.trust_too_much = Check.enabled; + const msg = TrustTooMuchCheck.MESSAGE; + + //--- fail cases ---// + + assertAnalyzerWarnings(q{ + @trusted: // [warn]: %s + void test(); + }c.format(msg), sac); + + assertAnalyzerWarnings(q{ + @trusted @nogc: // [warn]: %s + void test(); + }c.format(msg), sac); + + assertAnalyzerWarnings(q{ + @trusted { // [warn]: %s + void test(); + void test(); + } + }c.format(msg), sac); + + assertAnalyzerWarnings(q{ + @safe { + @trusted @nogc { // [warn]: %s + void test(); + void test(); + }} + }c.format(msg), sac); + + assertAnalyzerWarnings(q{ + @nogc @trusted { // [warn]: %s + void test(); + void test(); + } + }c.format(msg), sac); + + assertAnalyzerWarnings(q{ + @trusted template foo(){ // [warn]: %s + } + }c.format(msg), sac); + + assertAnalyzerWarnings(q{ + struct foo{ + @trusted: // [warn]: %s + } + }c.format(msg), sac); + //--- pass cases ---// + + assertAnalyzerWarnings(q{ + void test() @trusted {} + }c, sac); + + assertAnalyzerWarnings(q{ + @trusted void test(); + }c, sac); + + assertAnalyzerWarnings(q{ + @nogc template foo(){ + } + }c , sac); + + stderr.writeln("Unittest for TrustTooMuchCheck passed."); +} From cb31d2501e613e0212d8bc412908c7bb93f50363 Mon Sep 17 00:00:00 2001 From: BBasile Date: Mon, 2 Apr 2018 17:29:36 +0200 Subject: [PATCH 06/10] add IZ safeAccess util and refactor several && chains with it (#577) * add IZ safeAccess util and refactor several && chains with it * show how to make inference working --- src/dscanner/analysis/assert_without_msg.d | 8 +- src/dscanner/analysis/mismatched_args.d | 20 +- src/dscanner/analysis/run.d | 2 +- src/dscanner/analysis/static_if_else.d | 13 +- src/dscanner/analysis/unmodified.d | 8 +- src/dscanner/analysis/useless_initializer.d | 36 ++-- src/dscanner/analysis/vcall_in_ctor.d | 13 +- src/dscanner/imports.d | 2 +- src/dscanner/main.d | 2 +- src/dscanner/readers.d | 71 ------- src/dscanner/utils.d | 194 ++++++++++++++++++++ 11 files changed, 240 insertions(+), 129 deletions(-) delete mode 100644 src/dscanner/readers.d create mode 100644 src/dscanner/utils.d diff --git a/src/dscanner/analysis/assert_without_msg.d b/src/dscanner/analysis/assert_without_msg.d index 40c4863..476746d 100644 --- a/src/dscanner/analysis/assert_without_msg.d +++ b/src/dscanner/analysis/assert_without_msg.d @@ -5,6 +5,7 @@ module dscanner.analysis.assert_without_msg; import dscanner.analysis.base : BaseAnalyzer; +import dscanner.utils : safeAccess; import dsymbol.scope_ : Scope; import dparse.lexer; import dparse.ast; @@ -37,11 +38,10 @@ class AssertWithoutMessageCheck : BaseAnalyzer if (!isStdExceptionImported) return; - if (expr.unaryExpression !is null && - expr.unaryExpression.primaryExpression !is null && - expr.unaryExpression.primaryExpression.identifierOrTemplateInstance !is null) + if (const IdentifierOrTemplateInstance iot = safeAccess(expr) + .unaryExpression.primaryExpression.identifierOrTemplateInstance) { - auto ident = expr.unaryExpression.primaryExpression.identifierOrTemplateInstance.identifier; + auto ident = iot.identifier; if (ident.text == "enforce" && expr.arguments !is null && expr.arguments.argumentList !is null && expr.arguments.argumentList.items.length < 2) addErrorMessage(ident.line, ident.column, KEY, MESSAGE); diff --git a/src/dscanner/analysis/mismatched_args.d b/src/dscanner/analysis/mismatched_args.d index e3e1b37..718b34d 100644 --- a/src/dscanner/analysis/mismatched_args.d +++ b/src/dscanner/analysis/mismatched_args.d @@ -1,6 +1,7 @@ module dscanner.analysis.mismatched_args; import dscanner.analysis.base : BaseAnalyzer; +import dscanner.utils : safeAccess; import dsymbol.scope_; import dsymbol.symbol; import dparse.ast; @@ -126,16 +127,15 @@ final class ArgVisitor : ASTVisitor { import dsymbol.string_interning : internString; - if (unary.primaryExpression is null) - return; - if (unary.primaryExpression.identifierOrTemplateInstance is null) - return; - if (unary.primaryExpression.identifierOrTemplateInstance.identifier == tok!"") - return; - immutable t = unary.primaryExpression.identifierOrTemplateInstance.identifier; - lines ~= t.line; - columns ~= t.column; - args ~= internString(t.text); + if (auto iot = unary.safeAccess.primaryExpression.identifierOrTemplateInstance.unwrap) + { + if (iot.identifier == tok!"") + return; + immutable t = iot.identifier; + lines ~= t.line; + columns ~= t.column; + args ~= internString(t.text); + } } alias visit = ASTVisitor.visit; diff --git a/src/dscanner/analysis/run.d b/src/dscanner/analysis/run.d index 0442820..653d775 100644 --- a/src/dscanner/analysis/run.d +++ b/src/dscanner/analysis/run.d @@ -82,7 +82,7 @@ import dsymbol.conversion.first; import dsymbol.conversion.second; import dsymbol.modulecache : ModuleCache; -import dscanner.readers; +import dscanner.utils; bool first = true; diff --git a/src/dscanner/analysis/static_if_else.d b/src/dscanner/analysis/static_if_else.d index 2b29b49..f23199d 100644 --- a/src/dscanner/analysis/static_if_else.d +++ b/src/dscanner/analysis/static_if_else.d @@ -8,6 +8,7 @@ module dscanner.analysis.static_if_else; import dparse.ast; import dparse.lexer; import dscanner.analysis.base; +import dscanner.utils : safeAccess; /** * Checks for potentially mistaken static if / else if. @@ -47,17 +48,7 @@ class StaticIfElse : BaseAnalyzer const(IfStatement) getIfStatement(const ConditionalStatement cc) { - if (cc.falseStatement.statement) - { - if (cc.falseStatement.statement.statementNoCaseNoDefault) - { - if (cc.falseStatement.statement.statementNoCaseNoDefault.ifStatement) - { - return cc.falseStatement.statement.statementNoCaseNoDefault.ifStatement; - } - } - } - return null; + return safeAccess(cc).falseStatement.statement.statementNoCaseNoDefault.ifStatement; } enum KEY = "dscanner.suspicious.static_if_else"; diff --git a/src/dscanner/analysis/unmodified.d b/src/dscanner/analysis/unmodified.d index 01a1c1f..f93f09a 100644 --- a/src/dscanner/analysis/unmodified.d +++ b/src/dscanner/analysis/unmodified.d @@ -5,6 +5,7 @@ module dscanner.analysis.unmodified; import dscanner.analysis.base; +import dscanner.utils : safeAccess; import dsymbol.scope_ : Scope; import std.container; import dparse.ast; @@ -217,12 +218,9 @@ private: bool initializedFromNew(const Initializer initializer) { - if (initializer && initializer.nonVoidInitializer && - initializer.nonVoidInitializer.assignExpression && - cast(UnaryExpression) initializer.nonVoidInitializer.assignExpression) + if (const UnaryExpression ue = cast(UnaryExpression) safeAccess(initializer) + .nonVoidInitializer.assignExpression) { - const UnaryExpression ue = - cast(UnaryExpression) initializer.nonVoidInitializer.assignExpression; return ue.newExpression !is null; } return false; diff --git a/src/dscanner/analysis/useless_initializer.d b/src/dscanner/analysis/useless_initializer.d index 648d995..ccc9077 100644 --- a/src/dscanner/analysis/useless_initializer.d +++ b/src/dscanner/analysis/useless_initializer.d @@ -5,6 +5,7 @@ module dscanner.analysis.useless_initializer; import dscanner.analysis.base; +import dscanner.utils : safeAccess; import containers.dynamicarray; import containers.hashmap; import dparse.ast; @@ -147,7 +148,7 @@ public: !declarator.initializer.nonVoidInitializer || declarator.comment !is null) { - continue; + continue; } version(unittest) @@ -171,15 +172,14 @@ public: bool isStr, isSzInt; Token customType; - if (decl.type.type2.typeIdentifierPart && - decl.type.type2.typeIdentifierPart.typeIdentifierPart is null) + if (const TypeIdentifierPart tip = safeAccess(decl).type.type2.typeIdentifierPart) { - const IdentifierOrTemplateInstance idt = - decl.type.type2.typeIdentifierPart.identifierOrTemplateInstance; - - customType = idt.identifier; - isStr = customType.text.among("string", "wstring", "dstring") != 0; - isSzInt = customType.text.among("size_t", "ptrdiff_t") != 0; + if (!tip.typeIdentifierPart) + { + customType = tip.identifierOrTemplateInstance.identifier; + isStr = customType.text.among("string", "wstring", "dstring") != 0; + isSzInt = customType.text.among("size_t", "ptrdiff_t") != 0; + } } // --- 'BasicType/Symbol AssignExpression' ---// @@ -230,16 +230,18 @@ public: } } - // Symbol s = Symbol.init - else if (ue && customType != tok!"" && ue.unaryExpression && ue.unaryExpression.primaryExpression && - ue.unaryExpression.primaryExpression.identifierOrTemplateInstance && - ue.unaryExpression.primaryExpression.identifierOrTemplateInstance.identifier == customType && - ue.identifierOrTemplateInstance && ue.identifierOrTemplateInstance.identifier.text == "init") + else if (const IdentifierOrTemplateInstance iot = safeAccess(ue) + .unaryExpression.primaryExpression.identifierOrTemplateInstance) { - if (customType.text in _structCanBeInit) + // Symbol s = Symbol.init + if (ue && customType != tok!"" && iot.identifier == customType && + ue.identifierOrTemplateInstance && ue.identifierOrTemplateInstance.identifier.text == "init") { - if (!_structCanBeInit[customType.text]) - mixin(warn); + if (customType.text in _structCanBeInit) + { + if (!_structCanBeInit[customType.text]) + mixin(warn); + } } } diff --git a/src/dscanner/analysis/vcall_in_ctor.d b/src/dscanner/analysis/vcall_in_ctor.d index 753672f..efd1a44 100644 --- a/src/dscanner/analysis/vcall_in_ctor.d +++ b/src/dscanner/analysis/vcall_in_ctor.d @@ -5,6 +5,7 @@ module dscanner.analysis.vcall_in_ctor; import dscanner.analysis.base; +import dscanner.utils; import dparse.ast, dparse.lexer; import std.algorithm: among; import std.algorithm.iteration : filter; @@ -220,16 +221,12 @@ public: override void visit(const(UnaryExpression) exp) { + if (isInCtor) // get function identifier for a call, only for this member (so no ident chain) - if (isInCtor && exp.functionCallExpression && - exp.functionCallExpression.unaryExpression && - exp.functionCallExpression.unaryExpression.primaryExpression && - exp.functionCallExpression.unaryExpression.primaryExpression - .identifierOrTemplateInstance) + if (const IdentifierOrTemplateInstance iot = safeAccess(exp) + .functionCallExpression.unaryExpression.primaryExpression.identifierOrTemplateInstance) { - const Token t = exp.functionCallExpression.unaryExpression - .primaryExpression.identifierOrTemplateInstance.identifier; - + const Token t = iot.identifier; if (t != tok!"") { _ctorCalls[$-1] ~= t; diff --git a/src/dscanner/imports.d b/src/dscanner/imports.d index a46b660..b2b6fcc 100644 --- a/src/dscanner/imports.d +++ b/src/dscanner/imports.d @@ -12,7 +12,7 @@ import dparse.rollback_allocator; import std.stdio; import std.container.rbtree; import std.functional : toDelegate; -import dscanner.readers; +import dscanner.utils; /** * AST visitor that collects modules imported to an R-B tree. diff --git a/src/dscanner/main.d b/src/dscanner/main.d index d635176..8ab02cc 100644 --- a/src/dscanner/main.d +++ b/src/dscanner/main.d @@ -31,7 +31,7 @@ import dscanner.symbol_finder; import dscanner.analysis.run; import dscanner.analysis.config; import dscanner.dscanner_version; -import dscanner.readers; +import dscanner.utils; import inifiled; diff --git a/src/dscanner/readers.d b/src/dscanner/readers.d deleted file mode 100644 index 931ed17..0000000 --- a/src/dscanner/readers.d +++ /dev/null @@ -1,71 +0,0 @@ -module dscanner.readers; - -import std.array : appender, uninitializedArray; -import std.stdio : stdin, stderr, File; -import std.conv : to; -import std.file : exists; - -ubyte[] readStdin() -{ - auto sourceCode = appender!(ubyte[])(); - ubyte[4096] buf; - while (true) - { - auto b = stdin.rawRead(buf); - if (b.length == 0) - break; - sourceCode.put(b); - } - return sourceCode.data; -} - -ubyte[] readFile(string fileName) -{ - if (fileName == "stdin") - return readStdin(); - if (!exists(fileName)) - { - stderr.writefln("%s does not exist", fileName); - return []; - } - File f = File(fileName); - if (f.size == 0) - return []; - ubyte[] sourceCode = uninitializedArray!(ubyte[])(to!size_t(f.size)); - f.rawRead(sourceCode); - return sourceCode; -} - -string[] expandArgs(string[] args) -{ - import std.file : isFile, FileException, dirEntries, SpanMode; - import std.algorithm.iteration : map; - import std.algorithm.searching : endsWith; - - // isFile can throw if it's a broken symlink. - bool isFileSafe(T)(T a) - { - try - return isFile(a); - catch (FileException) - return false; - } - - string[] rVal; - if (args.length == 1) - args ~= "."; - foreach (arg; args[1 .. $]) - { - if (arg == "stdin" || isFileSafe(arg)) - rVal ~= arg; - else - foreach (item; dirEntries(arg, SpanMode.breadth).map!(a => a.name)) - { - if (isFileSafe(item) && (item.endsWith(`.d`) || item.endsWith(`.di`))) - rVal ~= item; - else - continue; - } - } - return rVal; -} diff --git a/src/dscanner/utils.d b/src/dscanner/utils.d new file mode 100644 index 0000000..7cbcbcc --- /dev/null +++ b/src/dscanner/utils.d @@ -0,0 +1,194 @@ +module dscanner.utils; + +import std.array : appender, uninitializedArray; +import std.stdio : stdin, stderr, File; +import std.conv : to; +import std.file : exists; + +ubyte[] readStdin() +{ + auto sourceCode = appender!(ubyte[])(); + ubyte[4096] buf; + while (true) + { + auto b = stdin.rawRead(buf); + if (b.length == 0) + break; + sourceCode.put(b); + } + return sourceCode.data; +} + +ubyte[] readFile(string fileName) +{ + if (fileName == "stdin") + return readStdin(); + if (!exists(fileName)) + { + stderr.writefln("%s does not exist", fileName); + return []; + } + File f = File(fileName); + if (f.size == 0) + return []; + ubyte[] sourceCode = uninitializedArray!(ubyte[])(to!size_t(f.size)); + f.rawRead(sourceCode); + return sourceCode; +} + +string[] expandArgs(string[] args) +{ + import std.file : isFile, FileException, dirEntries, SpanMode; + import std.algorithm.iteration : map; + import std.algorithm.searching : endsWith; + + // isFile can throw if it's a broken symlink. + bool isFileSafe(T)(T a) + { + try + return isFile(a); + catch (FileException) + return false; + } + + string[] rVal; + if (args.length == 1) + args ~= "."; + foreach (arg; args[1 .. $]) + { + if (arg == "stdin" || isFileSafe(arg)) + rVal ~= arg; + else + foreach (item; dirEntries(arg, SpanMode.breadth).map!(a => a.name)) + { + if (isFileSafe(item) && (item.endsWith(`.d`) || item.endsWith(`.di`))) + rVal ~= item; + else + continue; + } + } + return rVal; +} + +/** + * Allows to build access chains of class members as done with the $(D ?.) operator + * in other languages. In the chain, any $(D null) member that is a class instance + * or that returns one, has for effect to shortcut the complete evaluation. + * + * This function is copied from https://github.com/BBasile/iz to avoid a new submodule. + * Any change made to this copy should also be applied to the origin. + * + * Params: + * M = The class type of the chain entry point. + * + * Bugs: + * Assigning a member only works with $(D unwrap). + * + */ +struct SafeAccess(M) +if (is(M == class)) +{ + M m; + + @disable this(); + + /** + * Instantiate. + * + * Params: + * m = An instance of the entry point type. It is usually only + * $(D null) when the constructor is used internally, to build + * the chain. + */ + this(M m) + { + this.m = m; + } + + alias m this; + /// Unprotect the class instance. + alias unwrap = m; + + /// Handles safe access. + auto ref opDispatch(string member, A...)(auto ref A a) + { + import std.traits : ReturnType; + alias T = typeof(__traits(getMember, m, member)); + static if (is(T == class)) + { + return (!m || !__traits(getMember, m, member)) + ? SafeAccess!T(null) + : SafeAccess!T(__traits(getMember, m, member)); + } + else + { + import std.traits : ReturnType, Parameters, isFunction; + static if (isFunction!T) + { + // otherwise there's a missing return statement. + alias R = ReturnType!T; + static if (!is(R == void) && + !(is(R == class) && Parameters!T.length == 0)) + pragma(msg, __FILE__ ~ "(" ~ __LINE__.stringof ~ "): error, " ~ + "only `void function`s or `class` getters can be called without unwrap"); + + static if (is(R == class)) + { + return (m is null) + ? SafeAccess!R(null) + : SafeAccess!R(__traits(getMember, m, member)(a)); + } + else + { + if (m) + __traits(getMember, m, member)(a); + } + } + else + { + if (m) + __traits(getMember, m, member) = a; + } + } + } +} +/// General usage +@safe unittest +{ + class LongLineOfIdent3{int foo; void setFoo(int v) @safe{foo = v;}} + class LongLineOfIdent2{LongLineOfIdent3 longLineOfIdent3;} + class LongLineOfIdent1{LongLineOfIdent2 longLineOfIdent2;} + class Root {LongLineOfIdent1 longLineOfIdent1;} + + SafeAccess!Root sar = SafeAccess!Root(new Root); + // without the SafeAccess we would receive a SIGSEGV here + sar.longLineOfIdent1.longLineOfIdent2.longLineOfIdent3.setFoo(0xDEADBEEF); + + bool notAccessed = true; + // the same with `&&` whould be much longer + if (LongLineOfIdent3 a = sar.longLineOfIdent1.longLineOfIdent2.longLineOfIdent3) + { + notAccessed = false; + } + assert(notAccessed); + + // checks that forwarding actually works + sar.m.longLineOfIdent1 = new LongLineOfIdent1; + sar.m.longLineOfIdent1.longLineOfIdent2 = new LongLineOfIdent2; + sar.m.longLineOfIdent1.longLineOfIdent2.longLineOfIdent3 = new LongLineOfIdent3; + + sar.longLineOfIdent1.longLineOfIdent2.longLineOfIdent3.setFoo(42); + assert(sar.longLineOfIdent1.longLineOfIdent2.longLineOfIdent3.unwrap.foo == 42); +} + +/** + * IFTI helper for $(D SafeAccess). + * + * Returns: + * $(D m) with the ability to safely access its members that are class + * instances. + */ +auto ref safeAccess(M)(M m) +{ + return SafeAccess!M(m); +} From 75274faedb783f5798d4ec0149d0f61deecaaebc Mon Sep 17 00:00:00 2001 From: Sebastian Wilzbach Date: Mon, 2 Apr 2018 02:16:32 +0200 Subject: [PATCH 07/10] Fix #268 - Use the User's profile directory under Windows --- src/dscanner/main.d | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/src/dscanner/main.d b/src/dscanner/main.d index d635176..0007edf 100644 --- a/src/dscanner/main.d +++ b/src/dscanner/main.d @@ -407,24 +407,28 @@ version (OSX) version = useXDG; */ string getDefaultConfigurationLocation() { + import std.process : environment; + import std.exception : enforce; version (useXDG) { - import std.process : environment; - string configDir = environment.get("XDG_CONFIG_HOME", null); if (configDir is null) { configDir = environment.get("HOME", null); - if (configDir is null) - throw new Exception("Both $XDG_CONFIG_HOME and $HOME are unset"); + enforce(configDir !is null, "Both $XDG_CONFIG_HOME and $HOME are unset"); configDir = buildPath(configDir, ".config", "dscanner", CONFIG_FILE_NAME); } else configDir = buildPath(configDir, "dscanner", CONFIG_FILE_NAME); return configDir; } - else version (Windows) - return CONFIG_FILE_NAME; + else version(Windows) + { + string configDir = environment.get("APPDATA", null); + enforce(configDir !is null, "%APPDATA% is unset"); + configDir = buildPath(configDir, "dscanner", CONFIG_FILE_NAME); + return configDir; + } } /** From 6ca45d8b3f98be1f116ccef37e44996c74f00016 Mon Sep 17 00:00:00 2001 From: Sebastian Wilzbach Date: Tue, 3 Apr 2018 06:26:56 +0200 Subject: [PATCH 08/10] Fix #583 - False negative for missing parameter in Params --- .../properly_documented_public_functions.d | 109 ++++++++++++------ 1 file changed, 73 insertions(+), 36 deletions(-) diff --git a/src/dscanner/analysis/properly_documented_public_functions.d b/src/dscanner/analysis/properly_documented_public_functions.d index 68e9793..704fb87 100644 --- a/src/dscanner/analysis/properly_documented_public_functions.d +++ b/src/dscanner/analysis/properly_documented_public_functions.d @@ -166,8 +166,11 @@ private: import std.array : array; const comment = parseComment(commentText, null); - if (!comment.isDitto && !withinTemplate) - { + if (withinTemplate) { + const paramSection = comment.sections.find!(s => s.name == "Params"); + if (!paramSection.empty) + lastSeenFun.ddocParams ~= paramSection[0].mapping.map!(a => a[0]).array; + } else if (!comment.isDitto) { // check old function for invalid ddoc params if (lastSeenFun.active) postCheckSeenDdocParams(); @@ -188,7 +191,7 @@ private: } void checkDdocParams(size_t line, size_t column, const Parameters params, - const TemplateParameters templateParameters = null) + const TemplateParameters templateParameters = null) { import std.array : array; import std.algorithm.searching : canFind, countUntil; @@ -263,13 +266,13 @@ private: { if (p.templateTypeParameter) return p.templateTypeParameter.identifier.text; - if (p.templateValueParameter) + if (p.templateValueParameter) return p.templateValueParameter.identifier.text; - if (p.templateAliasParameter) + if (p.templateAliasParameter) return p.templateAliasParameter.identifier.text; - if (p.templateTupleParameter) + if (p.templateTupleParameter) return p.templateTupleParameter.identifier.text; - if (p.templateThisParameter) + if (p.templateThisParameter) return p.templateThisParameter.templateTypeParameter.identifier.text; return null; @@ -280,35 +283,35 @@ private: import std.meta : AliasSeq; alias properties = AliasSeq!( "aliasDeclaration", - "aliasThisDeclaration", - "anonymousEnumDeclaration", - "attributeDeclaration", - "classDeclaration", - "conditionalDeclaration", - "constructor", - "debugSpecification", - "destructor", - "enumDeclaration", - "eponymousTemplateDeclaration", - "functionDeclaration", - "importDeclaration", - "interfaceDeclaration", - "invariant_", - "mixinDeclaration", - "mixinTemplateDeclaration", - "postblit", - "pragmaDeclaration", - "sharedStaticConstructor", - "sharedStaticDestructor", - "staticAssertDeclaration", - "staticConstructor", - "staticDestructor", - "structDeclaration", - "templateDeclaration", - "unionDeclaration", - "unittest_", - "variableDeclaration", - "versionSpecification", + "aliasThisDeclaration", + "anonymousEnumDeclaration", + "attributeDeclaration", + "classDeclaration", + "conditionalDeclaration", + "constructor", + "debugSpecification", + "destructor", + "enumDeclaration", + "eponymousTemplateDeclaration", + "functionDeclaration", + "importDeclaration", + "interfaceDeclaration", + "invariant_", + "mixinDeclaration", + "mixinTemplateDeclaration", + "postblit", + "pragmaDeclaration", + "sharedStaticConstructor", + "sharedStaticDestructor", + "staticAssertDeclaration", + "staticConstructor", + "staticDestructor", + "structDeclaration", + "templateDeclaration", + "unionDeclaration", + "unittest_", + "variableDeclaration", + "versionSpecification", ); if (decl.declarations !is null) return false; @@ -796,6 +799,40 @@ string bar(P, R)(R r){}// [warn]: %s }c.format( ProperlyDocumentedPublicFunctions.MISSING_TEMPLATE_PARAMS_MESSAGE.format("P") ), sac); +} + +// https://github.com/dlang-community/D-Scanner/issues/583 +unittest +{ + StaticAnalysisConfig sac = disabledConfig; + sac.properly_documented_public_functions = Check.enabled; + + assertAnalyzerWarnings(q{ + /++ + Implements the homonym function (also known as `accumulate`) + + Returns: + the accumulated `result` + + Params: + fun = one or more functions + +/ + template reduce(fun...) + if (fun.length >= 1) + { + /++ + No-seed version. The first element of `r` is used as the seed's value. + + Params: + r = an iterable value as defined by `isIterable` + + Returns: + the final result of the accumulator applied to the iterable + +/ + auto reduce(R)(R r){} + } + }c.format( + ), sac); stderr.writeln("Unittest for ProperlyDocumentedPublicFunctions passed."); } From b9f5dbe22bd219e00ad934e775923ceeac809fa1 Mon Sep 17 00:00:00 2001 From: Sebastian Wilzbach Date: Tue, 3 Apr 2018 06:49:10 +0200 Subject: [PATCH 09/10] Don't use the dscanner prefix for the ModuleFilter in the config INI --- src/dscanner/analysis/config.d | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/dscanner/analysis/config.d b/src/dscanner/analysis/config.d index c750f68..436a60b 100644 --- a/src/dscanner/analysis/config.d +++ b/src/dscanner/analysis/config.d @@ -213,7 +213,7 @@ private template ModuleFiltersMixin(A) }(); } -@INI("ModuleFilters. +std.,-std.internal") +@INI("ModuleFilters for selectively enabling (+std) and disabling (-std.internal) individual checks", "analysis.config.ModuleFilters") struct ModuleFilters { mixin(ModuleFiltersMixin!int); From 873c8f506a6aedf1d06c00210e51b0d2b085f515 Mon Sep 17 00:00:00 2001 From: Basile Burg Date: Tue, 3 Apr 2018 07:34:10 +0200 Subject: [PATCH 10/10] fix #588 - trust_too_much false positive with alias decl --- src/dscanner/analysis/trust_too_much.d | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/dscanner/analysis/trust_too_much.d b/src/dscanner/analysis/trust_too_much.d index 84a13e1..67ce8da 100644 --- a/src/dscanner/analysis/trust_too_much.d +++ b/src/dscanner/analysis/trust_too_much.d @@ -61,6 +61,15 @@ public: d.accept(this); checkAtAttribute = oldCheckAtAttribute; } + + // issue #588 + override void visit(const AliasDeclaration d) + { + const oldCheckAtAttribute = checkAtAttribute; + checkAtAttribute = false; + d.accept(this); + checkAtAttribute = oldCheckAtAttribute; + } } unittest @@ -132,5 +141,9 @@ unittest } }c , sac); + assertAnalyzerWarnings(q{ + alias nothrow @trusted uint F4(); + }c , sac); + stderr.writeln("Unittest for TrustTooMuchCheck passed."); }