diff --git a/.travis.yml b/.travis.yml index 2a33498..3535085 100644 --- a/.travis.yml +++ b/.travis.yml @@ -16,10 +16,10 @@ script: "./.travis.sh" jobs: include: - stage: GitHub Release - if: tag IS present - d: ldc + #if: tag IS present + d: ldc-1.8.0 os: linux - script: echo "Deploying to GitHub releases ..." && make release + script: echo "Deploying to GitHub releases ..." && ./release.sh deploy: provider: releases api_key: $GH_REPO_TOKEN @@ -30,10 +30,10 @@ jobs: repo: dlang-community/D-Scanner tags: true - stage: GitHub Release - if: tag IS present - d: ldc + #if: tag IS present + d: ldc-1.8.0 os: osx - script: echo "Deploying to GitHub releases ..." && make release + script: echo "Deploying to GitHub releases ..." && ./release.sh deploy: provider: releases api_key: $GH_REPO_TOKEN @@ -43,3 +43,6 @@ jobs: on: repo: dlang-community/D-Scanner tags: true +stages: + - name: test + if: type = pull_request or (type = push and branch = master) diff --git a/README.md b/README.md index 66c4fc6..ea0533b 100644 --- a/README.md +++ b/README.md @@ -142,6 +142,7 @@ Note that the "--skipTests" option is the equivalent of changing each * 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. +* Redundant storage class attributes #### Wishlist @@ -228,7 +229,7 @@ the given source file to standard output in XML format. ```sh $ dscanner --ast helloworld.d ``` - + ```xml diff --git a/dsymbol b/dsymbol index 5b90412..239b137 160000 --- a/dsymbol +++ b/dsymbol @@ -1 +1 @@ -Subproject commit 5b90412457ac5f1d67c04e4da01587edfd529ad5 +Subproject commit 239b137b280c06864b73fcc1d00b75e06568d4c2 diff --git a/dub.json b/dub.json index 295b7e2..71039ba 100644 --- a/dub.json +++ b/dub.json @@ -12,9 +12,9 @@ "StdLoggerDisableWarning" ], "dependencies" : { - "libdparse": "~>0.8.0-alpha.5", - "dsymbol" : "~>0.3.0-alpha.3", - "inifiled" : "~>1.2.0", + "libdparse": "~>0.8.0", + "dsymbol" : "~>0.3.0", + "inifiled" : "~>1.3.0", "emsi_containers" : "~>0.6.0", "libddoc" : "~>0.3.0-beta.1", "stdx-allocator" : "~>2.77.0" diff --git a/inifiled b/inifiled index 971c535..9d7333e 160000 --- a/inifiled +++ b/inifiled @@ -1 +1 @@ -Subproject commit 971c5356388a73ebbf69e32f7f5e97cfc06cdcff +Subproject commit 9d7333ec17f116a05712a24df139ff2f212a9867 diff --git a/libdparse b/libdparse index ee0fa01..970efe3 160000 --- a/libdparse +++ b/libdparse @@ -1 +1 @@ -Subproject commit ee0fa01ab74b6bf27bed3c7bdb9d6fb789963342 +Subproject commit 970efe34e66fc7b3cb93a6ec59984099908070c5 diff --git a/makefile b/makefile index fb96f99..638c736 100644 --- a/makefile +++ b/makefile @@ -27,6 +27,8 @@ VERSIONS = DEBUG_VERSIONS = -version=dparse_verbose DMD_FLAGS = -w -inline -release -O -J. -od${OBJ_DIR} -version=StdLoggerDisableWarning -fPIC DMD_TEST_FLAGS = -w -g -J. -version=StdLoggerDisableWarning +override LDC_FLAGS += -O5 -release -oq +override GDC_FLAGS += -O3 -frelease SHELL:=/bin/bash all: dmdbuild @@ -46,11 +48,11 @@ dmdbuild: githash $(SRC) gdcbuild: githash mkdir -p bin - ${GDC} -O3 -frelease -obin/dscanner ${VERSIONS} ${INCLUDE_PATHS} ${SRC} -J. + ${GDC} ${GDC_FLAGS} -obin/dscanner ${VERSIONS} ${INCLUDE_PATHS} ${SRC} -J. ldcbuild: githash mkdir -p bin - ${LDC} -O5 -release -oq -of=bin/dscanner ${VERSIONS} ${INCLUDE_PATHS} ${SRC} -J. + ${LDC} ${LDC_FLAGS} -of=bin/dscanner ${VERSIONS} ${INCLUDE_PATHS} ${SRC} -J. # compile the dependencies separately, s.t. their unittests don't get executed bin/dscanner-unittest-lib.a: ${LIB_SRC} @@ -74,26 +76,5 @@ 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 + ./release.sh diff --git a/release.sh b/release.sh new file mode 100755 index 0000000..775849b --- /dev/null +++ b/release.sh @@ -0,0 +1,23 @@ +#!/usr/bin/env bash +set -eux -o pipefail +VERSION=$(git describe --abbrev=0 --tags) +ARCH="${ARCH:-64}" +LDC_FLAGS=() +unameOut="$(uname -s)" +case "$unameOut" in + Linux*) OS=linux; LDC_FLAGS=("-flto=full" "-linker=gold" "-static") ;; + Darwin*) OS=osx; LDC_FLAGS+=("-L-macosx_version_min" "-L10.7" "-L-lcrt1.o"); ;; + *) 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:-make} ldcbuild LDC_FLAGS="${LDC_FLAGS[*]}" +tar cvfz "bin/$archiveName" -C bin dscanner diff --git a/src/dscanner/analysis/config.d b/src/dscanner/analysis/config.d index 436a60b..b269a1c 100644 --- a/src/dscanner/analysis/config.d +++ b/src/dscanner/analysis/config.d @@ -197,6 +197,9 @@ struct StaticAnalysisConfig @INI("Check for @trusted applied to a bigger scope than a single function") string trust_too_much = Check.enabled; + @INI("Check for redundant storage classes on variable declarations") + string redundant_storage_classes = Check.enabled; + @INI("Module-specific filters") ModuleFilters filters; } diff --git a/src/dscanner/analysis/opequals_without_tohash.d b/src/dscanner/analysis/opequals_without_tohash.d index 13df894..5575e32 100644 --- a/src/dscanner/analysis/opequals_without_tohash.d +++ b/src/dscanner/analysis/opequals_without_tohash.d @@ -76,12 +76,6 @@ class OpEqualsWithoutToHashCheck : BaseAnalyzer string message = "'" ~ name.text ~ "' has method 'toHash', but not 'opEquals'."; addErrorMessage(name.line, name.column, KEY, message); } - - if (hasOpCmp && !hasOpEquals) - { - addErrorMessage(name.line, name.column, KEY, - "'" ~ name.text ~ "' has method 'opCmp', but not 'opEquals'."); - } } enum string KEY = "dscanner.suspicious.incomplete_operator_overloading"; @@ -108,6 +102,15 @@ unittest } } + // AA would use default equal and default toHash + struct Bee + { + int opCmp(Bee) const + { + return true; + } + } + // Fail on class opEquals class Rabbit // [warn]: 'Rabbit' has method 'opEquals', but not 'toHash'. { diff --git a/src/dscanner/analysis/properly_documented_public_functions.d b/src/dscanner/analysis/properly_documented_public_functions.d index 704fb87..59b1ea5 100644 --- a/src/dscanner/analysis/properly_documented_public_functions.d +++ b/src/dscanner/analysis/properly_documented_public_functions.d @@ -72,12 +72,13 @@ class ProperlyDocumentedPublicFunctions : BaseAnalyzer if (islastSeenVisibilityLabelPublic || decl.attributes.map!`a.attribute`.any!(x => x == tokPublic)) { - if (decl.functionDeclaration !is null || - decl.templateDeclaration !is null || + // Don't complain about non-documented function declarations + if ((decl.functionDeclaration !is null && decl.functionDeclaration.comment.ptr !is null) || + (decl.templateDeclaration !is null && decl.templateDeclaration.comment.ptr !is null) || decl.mixinTemplateDeclaration !is null || - decl.classDeclaration !is null || - decl.structDeclaration !is null) - decl.accept(this); + (decl.classDeclaration !is null && decl.classDeclaration.comment.ptr !is null) || + (decl.structDeclaration !is null && decl.structDeclaration.comment.ptr !is null)) + decl.accept(this); } } @@ -112,7 +113,7 @@ class ProperlyDocumentedPublicFunctions : BaseAnalyzer override void visit(const FunctionDeclaration decl) { - import std.algorithm.searching : any; + import std.algorithm.searching : all, any; // ignore header declaration for now if (decl.functionBody is null) @@ -453,8 +454,10 @@ unittest Some text */ private void foo(int k){} + /// public int bar(){} // [warn]: %s public: + /// int foobar(){} // [warn]: %s }c.format( ProperlyDocumentedPublicFunctions.MISSING_RETURNS_MESSAGE, @@ -468,8 +471,10 @@ unittest Some text */ private template foo(int k){} + /// public template bar(T){} // [warn]: %s public: + /// template foobar(T){} // [warn]: %s }c.format( ProperlyDocumentedPublicFunctions.MISSING_TEMPLATE_PARAMS_MESSAGE.format("T"), @@ -483,8 +488,10 @@ unittest Some text */ private struct foo(int k){} + /// public struct bar(T){} // [warn]: %s public: + /// struct foobar(T){} // [warn]: %s }c.format( ProperlyDocumentedPublicFunctions.MISSING_TEMPLATE_PARAMS_MESSAGE.format("T"), @@ -761,9 +768,10 @@ unittest * Returns: bar */ template abcde(Args ...) { - auto abcde(T, U...)(T t, U varargs) { - /// .... - } + /// + auto abcde(T, U...)(T t, U varargs) { + /// .... + } } }c, sac); } @@ -801,6 +809,21 @@ string bar(P, R)(R r){}// [warn]: %s ), sac); } +// https://github.com/dlang-community/D-Scanner/issues/601 +unittest +{ + StaticAnalysisConfig sac = disabledConfig; + sac.properly_documented_public_functions = Check.enabled; + + assertAnalyzerWarnings(q{ + void put(Range)(Range items) if (canPutConstRange!Range) + { + alias p = put!(Unqual!Range); + p(items); + } + }, sac); +} + // https://github.com/dlang-community/D-Scanner/issues/583 unittest { diff --git a/src/dscanner/analysis/redundant_storage_class.d b/src/dscanner/analysis/redundant_storage_class.d new file mode 100644 index 0000000..f9bb2be --- /dev/null +++ b/src/dscanner/analysis/redundant_storage_class.d @@ -0,0 +1,102 @@ +// Copyright (c) 2018, dlang-community +// 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.redundant_storage_class; + +import std.stdio; +import std.string; +import dparse.ast; +import dparse.lexer; +import dscanner.analysis.base; +import dscanner.analysis.helpers; +import dsymbol.scope_ : Scope; + +/** + * Checks for redundant storage classes such immutable and __gshared, static and __gshared + */ +class RedundantStorageClassCheck : BaseAnalyzer +{ + alias visit = BaseAnalyzer.visit; + enum string REDUNDANT_VARIABLE_ATTRIBUTES = "Variable declaration for `%s` has redundant attributes (%-(`%s`%|, %))."; + + this(string fileName, bool skipTests = false) + { + super(fileName, null, skipTests); + } + + override void visit(const Declaration node) + { + checkAttributes(node); + node.accept(this); + } + + void checkAttributes(const Declaration node) + { + if (node.variableDeclaration !is null && node.attributes !is null) + checkVariableDeclaration(node.variableDeclaration, node.attributes); + } + + void checkVariableDeclaration(const VariableDeclaration vd, const Attribute[] attributes) + { + import std.algorithm.comparison : among; + import std.algorithm.searching: all; + + string[] globalAttributes; + foreach (attrib; attributes) + { + if (attrib.attribute.type.among(tok!"shared", tok!"static", tok!"__gshared", tok!"immutable")) + globalAttributes ~= attrib.attribute.type.str; + } + if (globalAttributes.length > 1) + { + if (globalAttributes.length == 2 && ( + globalAttributes.all!(a => a.among("shared", "static")) || + globalAttributes.all!(a => a.among("static", "immutable")) + )) + 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); + } + } +} + +unittest +{ + import dscanner.analysis.config : StaticAnalysisConfig, Check, disabledConfig; + + StaticAnalysisConfig sac = disabledConfig(); + sac.redundant_storage_classes = Check.enabled; + + // https://github.com/dlang-community/D-Scanner/issues/438 + assertAnalyzerWarnings(q{ + immutable int a; + + immutable shared int a; // [warn]: %s + shared immutable int a; // [warn]: %s + + immutable __gshared int a; // [warn]: %s + __gshared immutable int a; // [warn]: %s + + __gshared static int a; // [warn]: %s + + shared static int a; + static shared int a; + static immutable int a; + immutable static int a; + + enum int a; + extern(C++) immutable int a; + immutable int function(immutable int, shared int) a; + }c.format( + RedundantStorageClassCheck.REDUNDANT_VARIABLE_ATTRIBUTES.format("a", ["immutable", "shared"]), + RedundantStorageClassCheck.REDUNDANT_VARIABLE_ATTRIBUTES.format("a", ["shared", "immutable"]), + RedundantStorageClassCheck.REDUNDANT_VARIABLE_ATTRIBUTES.format("a", ["immutable", "__gshared"]), + RedundantStorageClassCheck.REDUNDANT_VARIABLE_ATTRIBUTES.format("a", ["__gshared", "immutable"]), + RedundantStorageClassCheck.REDUNDANT_VARIABLE_ATTRIBUTES.format("a", ["__gshared", "static"]), + ), sac); + + stderr.writeln("Unittest for RedundantStorageClassCheck passed."); +} diff --git a/src/dscanner/analysis/run.d b/src/dscanner/analysis/run.d index 653d775..01e586c 100644 --- a/src/dscanner/analysis/run.d +++ b/src/dscanner/analysis/run.d @@ -73,6 +73,7 @@ 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 dscanner.analysis.redundant_storage_class; import dsymbol.string_interning : internString; import dsymbol.scope_; @@ -516,6 +517,10 @@ MessageSet analyze(string fileName, const Module m, const StaticAnalysisConfig a checks ~= new TrustTooMuchCheck(fileName, analysisConfig.trust_too_much == Check.skipTests && !ut); + if (moduleName.shouldRun!"redundant_storage_classes"(analysisConfig)) + checks ~= new RedundantStorageClassCheck(fileName, + analysisConfig.redundant_storage_classes == Check.skipTests && !ut); + version (none) if (moduleName.shouldRun!"redundant_if_check"(analysisConfig)) checks ~= new IfStatementCheck(fileName, moduleScope, diff --git a/src/dscanner/main.d b/src/dscanner/main.d index 7441ba9..7aa1417 100644 --- a/src/dscanner/main.d +++ b/src/dscanner/main.d @@ -104,6 +104,11 @@ else stderr.writeln(e.msg); return 1; } + catch (GetOptException e) + { + stderr.writeln(e.msg); + return 1; + } if (muffin) { @@ -143,8 +148,8 @@ else return 0; } - if (!errorFormat.length) - errorFormat = defaultErrorFormat; + if (!errorFormat.length) + errorFormat = defaultErrorFormat; const(string[]) absImportPaths = importPaths.map!(a => a.absolutePath() .buildNormalizedPath()).array(); @@ -234,8 +239,8 @@ else string s = configLocation is null ? getConfigurationLocation() : configLocation; if (s.exists()) readINIFile(config, s); - if (skipTests) - config.enabled2SkipTests; + if (skipTests) + config.enabled2SkipTests; if (report) generateReport(expandArgs(args), config, cache, moduleCache); else @@ -422,13 +427,13 @@ string getDefaultConfigurationLocation() configDir = buildPath(configDir, "dscanner", CONFIG_FILE_NAME); return configDir; } - 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; - } + 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; + } } /** diff --git a/src/dscanner/utils.d b/src/dscanner/utils.d index 7cbcbcc..35e0896 100644 --- a/src/dscanner/utils.d +++ b/src/dscanner/utils.d @@ -3,7 +3,43 @@ module dscanner.utils; import std.array : appender, uninitializedArray; import std.stdio : stdin, stderr, File; import std.conv : to; -import std.file : exists; +import std.encoding : BOM, BOMSeq, EncodingException, getBOM; +import std.format : format; +import std.file : exists, read; + +private void processBOM(ubyte[] sourceCode, string fname) +{ + enum spec = "D-Scanner does not support %s-encoded files (%s)"; + const BOMSeq bs = sourceCode.getBOM; + with(BOM) switch (bs.schema) + { + case none, utf8: + break; + default: + throw new EncodingException(spec.format(bs.schema, fname)); + } + sourceCode = sourceCode[bs.sequence.length .. $]; +} + +unittest +{ + import std.exception : assertThrown, assertNotThrown; + import std.encoding : bomTable; + import std.traits : EnumMembers; + + foreach(m ; EnumMembers!BOM) + { + auto sc = bomTable[m].sequence.dup; + if (m != BOM.none && m != BOM.utf8) + { + assertThrown!(EncodingException)(processBOM(sc, "")); + } + else + { + assertNotThrown!(EncodingException)(processBOM(sc, "")); + } + } +} ubyte[] readStdin() { @@ -16,6 +52,7 @@ ubyte[] readStdin() break; sourceCode.put(b); } + sourceCode.data.processBOM("stdin"); return sourceCode.data; } @@ -29,10 +66,9 @@ ubyte[] readFile(string fileName) return []; } File f = File(fileName); - if (f.size == 0) - return []; - ubyte[] sourceCode = uninitializedArray!(ubyte[])(to!size_t(f.size)); - f.rawRead(sourceCode); + ubyte[] sourceCode; + sourceCode = cast(ubyte[]) fileName.read(); + sourceCode.processBOM(fileName); return sourceCode; }