diff --git a/.github/workflows/default.yml b/.github/workflows/default.yml index 510d68c3..3b7b103b 100644 --- a/.github/workflows/default.yml +++ b/.github/workflows/default.yml @@ -6,13 +6,14 @@ on: push: branches: - master + - replace_libdparse jobs: main: name: Run all tests # Only run for the main repository - not forks - if: ${{ github.repository == 'dlang-community/D-Scanner' }} + if: ${{ github.repository == 'Dlang-UPB/D-Scanner' }} # Run permutations of common os + host compilers strategy: @@ -72,6 +73,10 @@ jobs: submodules: 'recursive' fetch-depth: 0 + # Uncomment to get a ssh connection inside the GH Actions runner + #- name: Setup upterm session + # uses: lhotari/action-upterm@v1 + # Install the host compiler (DMD or LDC) # Also grabs DMD for GDC to include dub + rdmd - name: Install ${{ matrix.compiler.version }} @@ -87,6 +92,10 @@ jobs: sudo apt-get install gdc-12 -y gdc-12 --version + # - name: Setup upterm session + # if: ${{ matrix.build.type == 'make' && matrix.host == 'macos-latest'}} + # uses: lhotari/action-upterm@v1 + # Compile D-Scanner and execute all tests without dub - name: Build and test without dub if: ${{ matrix.build.type == 'make' }} @@ -99,7 +108,8 @@ jobs: ./build.bat ./build.bat test else - make "-j$(nproc)" all test + NUM_PROC=$(nproc || getconf _NPROCESSORS_ONLN || 1) + make "-j$((NUM_PROC / 2))" all test fi # Compile D-Scanner and execute all tests using a specific dependency version @@ -128,6 +138,11 @@ jobs: fi "./bin/dscanner$EXE" --config .dscanner.ini --styleCheck src + - name: Run style checks + if: ${{ matrix.compiler.dmd == 'dmd' && matrix.build.type == 'make' }} + run: | + make style + # Parse phobos to check for failures / crashes / ... - name: Checkout Phobos uses: actions/checkout@v2 diff --git a/.gitignore b/.gitignore index 12324736..05bffbc9 100755 --- a/.gitignore +++ b/.gitignore @@ -11,6 +11,9 @@ # Sublime Text 2 *.sublime-workspace +# Idea stuff +.idea/ + # Subversion .svn/ diff --git a/.gitmodules b/.gitmodules index 12d4fee0..52ede99b 100644 --- a/.gitmodules +++ b/.gitmodules @@ -17,3 +17,6 @@ [submodule "DCD"] path = DCD url = https://github.com/dlang-community/DCD.git +[submodule "dmd"] + path = dmd + url = git@github.com:dlang/dmd.git diff --git a/DCD b/DCD index 5c529f30..218d0477 160000 --- a/DCD +++ b/DCD @@ -1 +1 @@ -Subproject commit 5c529f300d3a64d9ce8729ff99e3007922719bc8 +Subproject commit 218d0477606aff1ff13cb0720474237752d09413 diff --git a/README.md b/README.md index 422be370..8683d8d3 100644 --- a/README.md +++ b/README.md @@ -1,13 +1,17 @@ -# D-Scanner - -[![CI status](https://travis-ci.org/dlang-community/D-Scanner.svg?branch=master)](https://travis-ci.org/dlang-community/D-Scanner/) -[![Latest version](https://img.shields.io/dub/v/dscanner.svg)](http://code.dlang.org/packages/dscanner) -[![License](https://img.shields.io/dub/l/dscanner.svg)](http://code.dlang.org/packages/dscanner) +# D-Scanner fork that uses dmd as a library (WIP) D-Scanner is a tool for analyzing D source code ### Building and installing -First make sure that you have all the source code. Run ```git submodule update --init --recursive``` + +First, make sure that you have fetched the upstream: git@github.com:dlang-community/D-Scanner.git + +``` +git remote add upstream git@github.com:dlang-community/D-Scanner.git +git fetch upstream +``` + +Secondly, make sure that you have all the source code. Run ```git submodule update --init --recursive``` after cloning the project. To build D-Scanner, run ```make``` (or the build.bat file on Windows). diff --git a/build.bat b/build.bat index 44b58100..8da9fd00 100644 --- a/build.bat +++ b/build.bat @@ -18,8 +18,8 @@ if %githashsize% == 0 ( move /y bin\githash_.txt bin\githash.txt ) -set DFLAGS=-O -release -version=StdLoggerDisableWarning -Jbin %MFLAGS% -set TESTFLAGS=-g -w -version=StdLoggerDisableWarning -Jbin +set DFLAGS=-O -release -version=StdLoggerDisableWarning -version=CallbackAPI -version=DMDLIB -version=MARS -Jbin -Jdmd -Jdmd\compiler\src\dmd\res %MFLAGS% +set TESTFLAGS=-g -w -version=StdLoggerDisableWarning -version=CallbackAPI -version=DMDLIB -version=MARS -Jbin -Jdmd -Jdmd\compiler\src\dmd\res set CORE= set LIBDPARSE= set STD= @@ -29,6 +29,33 @@ set DSYMBOL= set CONTAINERS= set LIBDDOC= +SET DMD_FRONTEND_SRC=objc_glue.obj clone.obj transitivevisitor.obj iasm.obj iasmdmd.obj canthrow.obj tokens.obj optimize.obj func.obj semantic2.obj dvarstats.obj ph2.obj code.obj cdef.obj xmm.obj out.obj elfobj.obj glocal.obj dvec.obj code_x86.obj iasm2.obj string2.obj file2.obj obj.obj go.obj inliner.obj cc.obj bcomplex.obj mscoffobj.obj ptrntab.obj dlist.obj pdata.obj fp.obj cod3.obj os.obj cgelem.obj dcode.obj disasm86.obj exh.obj blockopt.obj aarray.obj cg.obj newman.obj dwarfdbginf.obj codebuilder.obj var.obj cod2.obj machobj.obj cgobj.obj cod4.obj dtype.obj cv4.obj backend.obj el.obj cgcod.obj cv8.obj dwarf.obj evalu8.obj ty.obj mem.obj cgxmm.obj gdag.obj gother.obj goh.obj cgcv.obj debugprint.obj cgsched.obj dwarfeh.obj cgreg.obj backconfig.obj gloop.obj divcoeff.obj cod5.obj dwarf2.obj cg87.obj nteh.obj dcgcv.obj util2.obj compress.obj type.obj elpicpie.obj gsroa.obj cgcs.obj ee.obj symbol.obj barray.obj melf.obj oper.obj cgcse.obj rtlsym.obj mscoff.obj drtlsym.obj symtab.obj dt.obj mach.obj cod1.obj global.obj filespec.obj gflow.obj elem.obj cgen.obj md5.obj chkformat.obj argtypes_sysv_x64.obj sideeffect.obj denum.obj apply.obj e2ir.obj typinf.obj statement.obj arraytypes.obj blockexit.obj init.obj scanomf.obj utils.obj parsetimevisitor.obj errorsink.obj scanmscoff.obj initsem.obj arrayop.obj nogc.obj dsymbol.obj hdrgen.obj dmangle.obj astenums.obj libmscoff.obj compiler.obj foreachvar.obj scanmach.obj dcast.obj tocsym.obj tocvdebug.obj semantic3.obj builtin.obj sapply.obj printast.obj dtemplate.obj importc.obj file_manager.obj dclass.obj argtypes_x86.obj glue.obj statement_rewrite_walker.obj target.obj aggregate.obj stringtable.obj ctfloat.obj response.obj strtold.obj port.obj aav.obj env.obj optional.obj filename.obj man.obj rootobject.obj complex.obj hash.obj region.obj utf.obj speller.obj rmem.obj array.obj longdouble.obj bitarray.obj eh.obj strictvisitor.obj permissivevisitor.obj lambdacomp.obj ctfeexpr.obj cparse.obj imphint.obj delegatize.obj access.obj identifier.obj todt.obj dmsc.obj entity.obj impcnvtab.obj dimport.obj lexer.obj dinifile.obj libomf.obj vsoptions.obj dstruct.obj aliasthis.obj ctorflow.obj errors.obj astcodegen.obj mtype.obj dtoh.obj argtypes_aarch64.obj cpreprocess.obj dmdparams.obj lib.obj id.obj parse.obj doc.obj scanelf.obj iasmgcc.obj cppmanglewin.obj stmtstate.obj ob.obj expression.obj declaration.obj location.obj dinterpret.obj inline.obj bitfields.obj string.obj int128.obj file.obj outbuffer.obj nspace.obj gluelayer.obj json.obj toir.obj intrange.obj cond.obj constfold.obj dversion.obj staticassert.obj dmodule.obj traits.obj opover.obj link.obj toctype.obj staticcond.obj statementsem.obj globals.obj libmach.obj toobj.obj s2ir.obj inlinecost.obj objc.obj visitor.obj asttypename.obj mustuse.obj dsymbolsem.obj frontend.obj safe.obj dscope.obj attrib.obj ast_node.obj escape.obj cli.obj templateparamsem.obj libelf.obj console.obj cppmangle.obj astbase.obj dmacro.obj typesem.obj expressionsem.obj + +set DMD_ROOT_SRC= +for %%x in (dmd\compiler\src\dmd\common\*.d) do set DMD_ROOT_SRC=!DMD_ROOT_SRC! %%x +for %%x in (dmd\compiler\src\dmd\root\*.d) do set DMD_ROOT_SRC=!DMD_ROOT_SRC! %%x + +set DMD_LEXER_SRC=^ + dmd\compiler\src\dmd\console.d ^ + dmd\compiler\src\dmd\entity.d ^ + dmd\compiler\src\dmd\errors.d ^ + dmd\compiler\src\dmd\file_manager.d ^ + dmd\compiler\src\dmd\globals.d ^ + dmd\compiler\src\dmd\id.d ^ + dmd\compiler\src\dmd\identifier.d ^ + dmd\compiler\src\dmd\lexer.d ^ + dmd\compiler\src\dmd\tokens.d ^ + dmd\compiler\src\dmd\utils.d + +set DMD_PARSER_SRC=^ + dmd\compiler\src\dmd\astbase.d ^ + dmd\compiler\src\dmd\parse.d ^ + dmd\compiler\src\dmd\parsetimevisitor.d ^ + dmd\compiler\src\dmd\transitivevisitor.d ^ + dmd\compiler\src\dmd\permissivevisitor.d ^ + dmd\compiler\src\dmd\strictvisitor.d ^ + dmd\compiler\src\dmd\astenums.d + for %%x in (src\dscanner\*.d) do set CORE=!CORE! %%x for %%x in (src\dscanner\analysis\*.d) do set ANALYSIS=!ANALYSIS! %%x for %%x in (libdparse\src\dparse\*.d) do set LIBDPARSE=!LIBDPARSE! %%x @@ -42,17 +69,76 @@ for %%x in (DCD\dsymbol\src\dsymbol\conversion\*.d) do set DSYMBOL=!DSYMBOL! %%x for %%x in (containers\src\containers\*.d) do set CONTAINERS=!CONTAINERS! %%x for %%x in (containers\src\containers\internal\*.d) do set CONTAINERS=!CONTAINERS! %%x +for %%x in (dmd\compiler\src\dmd\common\*.d) do %DC% %DFLAGS% -c %%x -od. -I"dmd\compiler\src" +for %%x in (dmd\compiler\src\dmd\root\*.d) do %DC% %DFLAGS% -c %%x -od. -I"dmd\compiler\src" +for %%x in (dmd\compiler\src\dmd\backend\*.d) do %DC% %DFLAGS% -c %%x -od. -I"dmd\compiler\src" +for %%x in (dmd\compiler\src\dmd\*.d) do %DC% %DFLAGS% -c %%x -od. -I"dmd\compiler\src" + +%DC% %DFLAGS% -c dmd\compiler\src\dmd\backend\iasm.d -od. -ofiasm2.obj -I"dmd\compiler\src" +%DC% %DFLAGS% -c dmd\compiler\src\dmd\common\string.d -od. -ofstring2.obj -I"dmd\compiler\src" +%DC% %DFLAGS% -c dmd\compiler\src\dmd\common\file.d -od. -offile2.obj -I"dmd\compiler\src" + if "%1" == "test" goto test_cmd @echo on -%DC% %MFLAGS% %CORE% %STD% %LIBDPARSE% %LIBDDOC% %ANALYSIS% %INIFILED% %DSYMBOL% %CONTAINERS% %DFLAGS% -I"libdparse\src" -I"DCD\dsymbol\src" -I"containers\src" -I"libddoc\src" -I"libddoc\common\source" -ofbin\dscanner.exe +dir +echo %DMD_FRONTEND_SRC% + +%DC% %MFLAGS%^ + %CORE%^ + %STD%^ + %LIBDPARSE%^ + %LIBDDOC%^ + %ANALYSIS%^ + %INIFILED%^ + %DSYMBOL%^ + %CONTAINERS%^ + %DMD_FRONTEND_SRC%^ + %DFLAGS%^ + -I"libdparse\src"^ + -I"DCD\dsymbol\src"^ + -I"containers\src"^ + -I"libddoc\src"^ + -I"libddoc\common\source"^ + -I"dmd\compiler\src"^ + -ofbin\dscanner.exe goto eof :test_cmd @echo on set TESTNAME="bin\dscanner-unittest" -%DC% %MFLAGS% %STD% %LIBDPARSE% %LIBDDOC% %INIFILED% %DSYMBOL% %CONTAINERS% -I"libdparse\src" -I"DCD\dsymbol\src" -I"containers\src" -I"libddoc\src" -lib %TESTFLAGS% -of%TESTNAME%.lib -if exist %TESTNAME%.lib %DC% %MFLAGS% %CORE% %ANALYSIS% %TESTNAME%.lib -I"src" -I"inifiled\source" -I"libdparse\src" -I"DCD\dsymbol\src" -I"containers\src" -I"libddoc\src" -I"libddoc\common\source" -unittest %TESTFLAGS% -of%TESTNAME%.exe +%DC% %MFLAGS% ^ + %STD%^ + %LIBDPARSE%^ + %LIBDDOC%^ + %INIFILED%^ + %DSYMBOL%^ + %CONTAINERS%^ + %DMD_FRONTEND_SRC%^ + -I"libdparse\src"^ + -I"DCD\dsymbol\src"^ + -I"containers\src"^ + -I"libddoc\src"^ + -I"dmd\compiler\src"^ + -I"dmd\compiler\src\dmd\res"^ + -lib %TESTFLAGS%^ + -of%TESTNAME%.lib +if exist %TESTNAME%.lib %DC% %MFLAGS%^ + %CORE%^ + %ANALYSIS%^ + %TESTNAME%.lib^ + -I"src"^ + -I"inifiled\source"^ + -I"libdparse\src"^ + -I"DCD\dsymbol\src"^ + -I"containers\src"^ + -I"libddoc\src"^ + -I"libddoc\common\source"^ + -I"dmd\compiler\src"^ + -I"dmd\compiler\src\dmd\res"^ + -unittest^ + %TESTFLAGS%^ + -of%TESTNAME%.exe if exist %TESTNAME%.exe %TESTNAME%.exe if exist %TESTNAME%.obj del %TESTNAME%.obj diff --git a/changelog/dscanner.assert-without-message.dd b/changelog/dscanner.assert-without-message.dd new file mode 100644 index 00000000..2ea35b66 --- /dev/null +++ b/changelog/dscanner.assert-without-message.dd @@ -0,0 +1 @@ +Avoid checking `enforce` calls as it is phobos specific. \ No newline at end of file diff --git a/changelog/dscanner.struct-ctor-check.dd b/changelog/dscanner.struct-ctor-check.dd new file mode 100644 index 00000000..27940c86 --- /dev/null +++ b/changelog/dscanner.struct-ctor-check.dd @@ -0,0 +1,19 @@ +Remove the check regarding structs with no arguments constructors. + +The check is implemented in constructors.d and it warns against the usage +of both constructors with all parameters with default values and constructors +without any arguments, as this might be confusing. This scenario, for structs, +is no longer D valid code and that's why it is being deprecated. + +Let's consider the following code: + +--- +struct Dog +{ + this() {} + this(string name = "doggie") {} // [warn]: This struct constructor can never be called with its default argument. +} +--- + +D-Scanner would throw and error for this particular struct, but this code +does not compile anymore hence this check is not needed anymore/ \ No newline at end of file diff --git a/containers b/containers index 116a0287..c25a0eba 160000 --- a/containers +++ b/containers @@ -1 +1 @@ -Subproject commit 116a02872039efbd0289828cd5eeff6f60bdf539 +Subproject commit c25a0ebabbd110a7d937aee0c2ded24f69b377ae diff --git a/dmd b/dmd new file mode 160000 index 00000000..a4220358 --- /dev/null +++ b/dmd @@ -0,0 +1 @@ +Subproject commit a4220358ecfcffe7ea38ab4a1996ffc5a5331f22 diff --git a/dub.json b/dub.json index 1d481a7b..f1cc19ec 100644 --- a/dub.json +++ b/dub.json @@ -16,7 +16,11 @@ "dcd:dsymbol" : "~>0.15.0-beta.1", "inifiled" : "~>1.3.1", "emsi_containers" : "~>0.9.0", - "libddoc" : "~>0.8.0" + "libddoc" : "~>0.8.0", + "dmd": { + "repository": "git+https://github.com/dlang/dmd.git", + "version": "a4220358ecfcffe7ea38ab4a1996ffc5a5331f22" + } }, "targetPath" : "bin", "stringImportPaths" : [ diff --git a/inifiled b/inifiled index cecaff80..7ca12692 160000 --- a/inifiled +++ b/inifiled @@ -1 +1 @@ -Subproject commit cecaff8037a60db2a51c9bded4802c87d938a44e +Subproject commit 7ca1269254aff06197829cdff643356030c8f5bc diff --git a/libdparse b/libdparse index c3fa4e6e..d7ffe45c 160000 --- a/libdparse +++ b/libdparse @@ -1 +1 @@ -Subproject commit c3fa4e6eb3720c6aad9e9a772a919ccee2cf1215 +Subproject commit d7ffe45c71ecaafcadb04b354c6e7cc950c38c62 diff --git a/makefile b/makefile index 58fd93a9..befb0c08 100644 --- a/makefile +++ b/makefile @@ -1,10 +1,46 @@ .PHONY: all test clean +.DEFAULT_GOAL := all + DC ?= dmd GIT ?= git DMD := $(DC) GDC := gdc LDC := ldc2 +DMD_ROOT_SRC := \ + $(shell find dmd/compiler/src/dmd/common -name "*.d")\ + $(shell find dmd/compiler/src/dmd/root -name "*.d")\ + +DMD_FRONTEND_SRC := \ + $(shell find dmd/compiler/src/dmd/common -name "*.d")\ + $(shell find dmd/compiler/src/dmd/root -name "*.d")\ + $(shell find dmd/compiler/src/dmd/backend -name "*.d")\ + $(shell find dmd/compiler/src/dmd -maxdepth 1 -name "*.d" ! -name "mars.d" ) + +DMD_LEXER_SRC := \ + dmd/compiler/src/dmd/console.d \ + dmd/compiler/src/dmd/entity.d \ + dmd/compiler/src/dmd/errors.d \ + dmd/compiler/src/dmd/errorsink.d \ + dmd/compiler/src/dmd/file_manager.d \ + dmd/compiler/src/dmd/globals.d \ + dmd/compiler/src/dmd/id.d \ + dmd/compiler/src/dmd/identifier.d \ + dmd/compiler/src/dmd/lexer.d \ + dmd/compiler/src/dmd/tokens.d \ + dmd/compiler/src/dmd/utils.d \ + dmd/compiler/src/dmd/location.d \ + $(DMD_ROOT_SRC) + +DMD_PARSER_SRC := \ + dmd/compiler/src/dmd/astbase.d \ + dmd/compiler/src/dmd/parse.d \ + dmd/compiler/src/dmd/parsetimevisitor.d \ + dmd/compiler/src/dmd/transitivevisitor.d \ + dmd/compiler/src/dmd/permissivevisitor.d \ + dmd/compiler/src/dmd/strictvisitor.d \ + dmd/compiler/src/dmd/astenums.d \ + $(DMD_LEXER_SRC) LIB_SRC := \ $(shell find containers/src -name "*.d")\ @@ -13,7 +49,9 @@ LIB_SRC := \ $(shell find libdparse/src/std/experimental/ -name "*.d")\ $(shell find libdparse/src/dparse/ -name "*.d")\ $(shell find libddoc/src -name "*.d") \ - $(shell find libddoc/common/source -name "*.d") + $(shell find libddoc/common/source -name "*.d") \ + $(DMD_FRONTEND_SRC) + PROJECT_SRC := $(shell find src/ -name "*.d") SRC := $(LIB_SRC) $(PROJECT_SRC) @@ -42,26 +80,27 @@ INCLUDE_PATHS = \ -IDCD/dsymbol/src \ -Icontainers/src \ -Ilibddoc/src \ - -Ilibddoc/common/source + -Ilibddoc/common/source \ + -Idmd/compiler/src -DMD_VERSIONS = -version=StdLoggerDisableWarning +DMD_VERSIONS = -version=StdLoggerDisableWarning -version=CallbackAPI -version=DMDLIB -version=MARS DMD_DEBUG_VERSIONS = -version=dparse_verbose -LDC_VERSIONS = -d-version=StdLoggerDisableWarning +LDC_VERSIONS = -d-version=StdLoggerDisableWarning -d-version=CallbackAPI -d-version=DMDLIB -d-version=MARS LDC_DEBUG_VERSIONS = -d-version=dparse_verbose -GDC_VERSIONS = -fversion=StdLoggerDisableWarning +GDC_VERSIONS = -fversion=StdLoggerDisableWarning -fversion=CallbackAPI -fversion=DMDLIB -fversion=MARS GDC_DEBUG_VERSIONS = -fversion=dparse_verbose -DC_FLAGS += -Jbin +DC_FLAGS += -Jbin -Jdmd -Jdmd/compiler/src/dmd/res override DMD_FLAGS += $(DFLAGS) -w -release -O -od${OBJ_DIR} override LDC_FLAGS += $(DFLAGS) -O5 -release -oq override GDC_FLAGS += $(DFLAGS) -O3 -frelease -fall-instantiations override GDC_TEST_FLAGS += -fall-instantiations -DC_TEST_FLAGS += -g -Jbin +DC_TEST_FLAGS += -g -Jbin -Jdmd -Jdmd/compiler/src/dmd/res override DMD_TEST_FLAGS += -w -DC_DEBUG_FLAGS := -g -Jbin +DC_DEBUG_FLAGS := -g -Jbin -Jdmd -Jdmd/compiler/src/dmd/res ifeq ($(DC), $(filter $(DC), dmd ldmd2 gdmd)) VERSIONS := $(DMD_VERSIONS) @@ -82,11 +121,20 @@ else ifneq (,$(findstring gdc, $(DC))) DC_TEST_FLAGS += $(GDC_TEST_FLAGS) -funittest WRITE_TO_TARGET_NAME = -o $@ endif - SHELL:=/usr/bin/env bash +ifneq (, $(findstring $(GDC), $(DC))) + CONFIG_CMD := $(DC) dmd/config.d -o config && ./config bin VERSION /etc && rm config; +else + CONFIG_CMD := $(DC) -run dmd/config.d bin VERSION /etc; +endif + GITHASH = bin/githash.txt +DC_CONFIG_FILE := bin/SYSCONFDIR.imp + +$(DC_CONFIG_FILE): | $(DSCANNER_BIN_DIR) + $(CONFIG_CMD) $(OBJ_DIR)/$(DC)/%.o: %.d ${DC} ${DC_FLAGS} ${VERSIONS} ${INCLUDE_PATHS} -c $< ${WRITE_TO_TARGET_NAME} @@ -94,7 +142,7 @@ $(OBJ_DIR)/$(DC)/%.o: %.d $(UT_OBJ_DIR)/$(DC)/%.o: %.d ${DC} ${DC_TEST_FLAGS} ${VERSIONS} ${INCLUDE_PATHS} -c $< ${WRITE_TO_TARGET_NAME} -${DSCANNER_BIN}: ${GITHASH} ${OBJ_BY_DC} | ${DSCANNER_BIN_DIR} +${DSCANNER_BIN}: $(DC_CONFIG_FILE) ${GITHASH} ${OBJ_BY_DC} | ${DSCANNER_BIN_DIR} ${DC} ${OBJ_BY_DC} ${WRITE_TO_TARGET_NAME} ${OBJ_BY_DC}: | ${OBJ_BY_DC_DIR} @@ -122,7 +170,7 @@ githash: ${GITHASH} ${GITHASH}: mkdir -p bin && ${GIT} describe --tags --always > ${GITHASH} -debug: ${GITHASH} +debug: $(DC_CONFIG_FILE) ${GITHASH} ${DC} -w -g -Jbin -ofdsc ${VERSIONS} ${DEBUG_VERSIONS} ${INCLUDE_PATHS} ${SRC} # compile the dependencies separately, s.t. their unittests don't get executed @@ -131,7 +179,7 @@ ${UT_DSCANNER_LIB}: ${LIB_SRC} | ${UT_DSCANNER_LIB_DIR} test: ${UT_DSCANNER_BIN} -${UT_DSCANNER_BIN}: ${UT_DSCANNER_LIB} ${GITHASH} ${UT_OBJ_BY_DC} | ${DSCANNER_BIN_DIR} +${UT_DSCANNER_BIN}: $(DC_CONFIG_FILE) ${UT_DSCANNER_LIB} ${GITHASH} ${UT_OBJ_BY_DC} | ${DSCANNER_BIN_DIR} ${DC} ${UT_DSCANNER_LIB} ${UT_OBJ_BY_DC} ${WRITE_TO_TARGET_NAME} ./${UT_DSCANNER_BIN} @@ -150,3 +198,42 @@ report: all release: ./release.sh + +# Add source files here as we transition to DMD-as-a-library +STYLE_CHECKED_SRC := \ + src/dscanner/imports.d \ + src/dscanner/main.d + +style: + @echo "Check for trailing whitespace" + grep -nr '[[:blank:]]$$' ${STYLE_CHECKED_SRC}; test $$? -eq 1 + + @echo "Enforce whitespace before opening parenthesis" + grep -nrE "\<(for|foreach|foreach_reverse|if|while|switch|catch|version)\(" ${STYLE_CHECKED_SRC} ; test $$? -eq 1 + + @echo "Enforce no whitespace after opening parenthesis" + grep -nrE "\<(version) \( " ${STYLE_CHECKED_SRC} ; test $$? -eq 1 + + @echo "Enforce whitespace between colon(:) for import statements (doesn't catch everything)" + grep -nr 'import [^/,=]*:.*;' ${STYLE_CHECKED_SRC} | grep -vE "import ([^ ]+) :\s"; test $$? -eq 1 + + @echo "Check for package wide std.algorithm imports" + grep -nr 'import std.algorithm : ' ${STYLE_CHECKED_SRC} ; test $$? -eq 1 + + @echo "Enforce Allman style" + grep -nrE '(if|for|foreach|foreach_reverse|while|unittest|switch|else|version) .*{$$' ${STYLE_CHECKED_SRC}; test $$? -eq 1 + + @echo "Enforce do { to be in Allman style" + grep -nr 'do *{$$' ${STYLE_CHECKED_SRC} ; test $$? -eq 1 + + @echo "Enforce no space between assert and the opening brace, i.e. assert(" + grep -nrE 'assert +\(' ${STYLE_CHECKED_SRC} ; test $$? -eq 1 + + @echo "Enforce space after cast(...)" + grep -nrE '[^"]cast\([^)]*?\)[[:alnum:]]' ${STYLE_CHECKED_SRC} ; test $$? -eq 1 + + @echo "Enforce space between a .. b" + grep -nrE '[[:alnum:]][.][.][[:alnum:]]|[[:alnum:]] [.][.][[:alnum:]]|[[:alnum:]][.][.] [[:alnum:]]' ${STYLE_CHECKED_SRC}; test $$? -eq 1 + + @echo "Enforce space between binary operators" + grep -nrE "[[:alnum:]](==|!=|<=|<<|>>|>>>|^^)[[:alnum:]]|[[:alnum:]] (==|!=|<=|<<|>>|>>>|^^)[[:alnum:]]|[[:alnum:]](==|!=|<=|<<|>>|>>>|^^) [[:alnum:]]" ${STYLE_CHECKED_SRC}; test $$? -eq 1 diff --git a/src/dscanner/analysis/alias_syntax_check.d b/src/dscanner/analysis/alias_syntax_check.d index e8c1ab25..7629a496 100644 --- a/src/dscanner/analysis/alias_syntax_check.d +++ b/src/dscanner/analysis/alias_syntax_check.d @@ -5,43 +5,73 @@ module dscanner.analysis.alias_syntax_check; -import dparse.ast; -import dparse.lexer; import dscanner.analysis.base; +import dmd.tokens; +import dmd.lexer : Lexer; +import dmd.location : Loc; /** * Checks for uses of the old alias syntax. */ -final class AliasSyntaxCheck : BaseAnalyzer +extern(C++) class AliasSyntaxCheck(AST) : BaseAnalyzerDmd { - alias visit = BaseAnalyzer.visit; - mixin AnalyzerInfo!"alias_syntax_check"; + alias visit = BaseAnalyzerDmd.visit; - this(string fileName, bool skipTests = false) + extern(D) this(string fileName) { - super(fileName, null, skipTests); + super(fileName); } - override void visit(const AliasDeclaration ad) + override void visit(AST.AliasDeclaration ad) { - if (ad.declaratorIdentifierList is null) - 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, - "Prefer the new \"'alias' identifier '=' type ';'\" syntax" - ~ " to the old \"'alias' type identifier ';'\" syntax."); + import dscanner.utils: readFile; + import dmd.errorsink : ErrorSinkNull; + import dmd.globals : global; + + __gshared ErrorSinkNull errorSinkNull; + if (!errorSinkNull) + errorSinkNull = new ErrorSinkNull; + + auto bytes = readFile(fileName); + bool foundEq = false; + Loc idLoc; + + bytes ~= '\0'; + bytes = bytes[ad.loc.fileOffset .. $]; + + scope lexer = new Lexer(null, cast(char*) bytes, 0, bytes.length, 0, 0, errorSinkNull, &global.compileEnv); + TOK nextTok; + lexer.nextToken(); + + do + { + if (lexer.token.value == TOK.assign) + foundEq = true; + + if (lexer.token.value == TOK.identifier) + idLoc = lexer.token.loc; + + nextTok = lexer.nextToken; + } + while(nextTok != TOK.semicolon && nextTok != TOK.endOfFile); + + if (!foundEq) + // Re-lexing is done based on offsets, so the alias appears to be at line 1. + // Fix this by computing the initial location. + addErrorMessage(cast(ulong) (ad.loc.linnum + idLoc.linnum - 1), cast(ulong) idLoc.charnum, KEY, + "Prefer the new \"'alias' identifier '=' type ';'\" syntax" + ~ " to the old \"'alias' type identifier ';'\" syntax."); } + private: enum KEY = "dscanner.style.alias_syntax"; } unittest { - import dscanner.analysis.helpers : assertAnalyzerWarnings; + import dscanner.analysis.helpers : assertAnalyzerWarnings = assertAnalyzerWarningsDMD; import dscanner.analysis.config : StaticAnalysisConfig, Check, disabledConfig; import std.stdio : stderr; diff --git a/src/dscanner/analysis/asm_style.d b/src/dscanner/analysis/asm_style.d index de54c773..ab83596d 100644 --- a/src/dscanner/analysis/asm_style.d +++ b/src/dscanner/analysis/asm_style.d @@ -6,37 +6,58 @@ module dscanner.analysis.asm_style; import std.stdio; -import dparse.ast; -import dparse.lexer; import dscanner.analysis.base; import dscanner.analysis.helpers; -import dsymbol.scope_ : Scope; +import dmd.tokens; /** * Checks for confusing asm expressions. * See_also: $(LINK https://issues.dlang.org/show_bug.cgi?id=9738) */ -final class AsmStyleCheck : BaseAnalyzer +extern (C++) class AsmStyleCheck(AST) : BaseAnalyzerDmd { - alias visit = BaseAnalyzer.visit; - + alias visit = BaseAnalyzerDmd.visit; mixin AnalyzerInfo!"asm_style_check"; - this(string fileName, const(Scope)* sc, bool skipTests = false) + extern (D) this(string fileName, bool skipTests = false) { - super(fileName, sc, skipTests); + super(fileName, skipTests); } - override void visit(const AsmBrExp brExp) + override void visit(AST.AsmStatement asmStatement) { - if (brExp.asmBrExp !is null && brExp.asmBrExp.asmUnaExp !is null - && brExp.asmBrExp.asmUnaExp.asmPrimaryExp !is null) + for (Token* token = asmStatement.tokens; token !is null; token = token.next) { - addErrorMessage(brExp.line, brExp.column, "dscanner.confusing.brexp", - "This is confusing because it looks like an array index. Rewrite a[1] as [a + 1] to clarify."); + if (isConfusingStatement(token)) + { + auto lineNum = cast(ulong) token.loc.linnum; + auto charNum = cast(ulong) token.loc.charnum; + addErrorMessage(lineNum, charNum, KEY, MESSAGE); + } } - brExp.accept(this); } + + private bool isConfusingStatement(Token* token) + { + if (token.next is null) + return false; + + if (token.next.next is null) + return false; + + TOK tok1 = token.value; + TOK tok2 = token.next.value; + TOK tok3 = token.next.next.value; + + if (tok1 == TOK.leftBracket && tok2 == TOK.int32Literal && tok3 == TOK.rightBracket) + return true; + + return false; + } + +private: + enum string KEY = "dscanner.confusing.brexp"; + enum string MESSAGE = "This is confusing because it looks like an array index. Rewrite a[1] as [a + 1] to clarify."; } unittest @@ -45,7 +66,7 @@ unittest StaticAnalysisConfig sac = disabledConfig(); sac.asm_style_check = Check.enabled; - assertAnalyzerWarnings(q{ + assertAnalyzerWarningsDMD(q{ void testAsm() { asm diff --git a/src/dscanner/analysis/assert_without_msg.d b/src/dscanner/analysis/assert_without_msg.d index e24c9891..3cf49d96 100644 --- a/src/dscanner/analysis/assert_without_msg.d +++ b/src/dscanner/analysis/assert_without_msg.d @@ -5,150 +5,69 @@ module dscanner.analysis.assert_without_msg; import dscanner.analysis.base; -import dscanner.utils : safeAccess; -import dsymbol.scope_ : Scope; -import dparse.lexer; -import dparse.ast; - import std.stdio; -import std.algorithm; /** * Check that all asserts have an explanatory message. */ -final class AssertWithoutMessageCheck : BaseAnalyzer +extern(C++) class AssertWithoutMessageCheck(AST) : BaseAnalyzerDmd { - enum string KEY = "dscanner.style.assert_without_msg"; - enum string MESSAGE = "An assert should have an explanatory message"; mixin AnalyzerInfo!"assert_without_msg"; + alias visit = BaseAnalyzerDmd.visit; /// - this(string fileName, const(Scope)* sc, bool skipTests = false) - { - super(fileName, sc, skipTests); - } - - override void visit(const AssertExpression expr) + extern(D) this(string fileName, bool skipTests = false) { - if (expr.assertArguments && expr.assertArguments.message is null) - addErrorMessage(expr.line, expr.column, KEY, MESSAGE); + super(fileName, skipTests); } - override void visit(const FunctionCallExpression expr) + // Avoid visiting in/out contracts for this check + override void visitFuncBody(AST.FuncDeclaration f) { - if (!isStdExceptionImported) - return; - - if (const IdentifierOrTemplateInstance iot = safeAccess(expr) - .unaryExpression.primaryExpression.identifierOrTemplateInstance) - { - 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); - } + if (f.fbody) + { + f.fbody.accept(this); + } } - override void visit(const SingleImport sImport) + override void visit(AST.AssertExp ae) { - static immutable stdException = ["std", "exception"]; - if (sImport.identifierChain.identifiers.map!(a => a.text).equal(stdException)) - isStdExceptionImported = true; + if (!ae.msg) + addErrorMessage(ae.loc.linnum, ae.loc.charnum, KEY, MESSAGE); } - // revert the stack after new scopes - override void visit(const Declaration decl) + override void visit(AST.StaticAssert ae) { - // be careful - ImportDeclarations don't introduce a new scope - if (decl.importDeclaration is null) - { - bool tmp = isStdExceptionImported; - scope(exit) isStdExceptionImported = tmp; - decl.accept(this); - } - else - decl.accept(this); + if (!ae.msgs) + addErrorMessage(ae.loc.linnum, ae.loc.charnum, KEY, MESSAGE); } - mixin ScopedVisit!IfStatement; - mixin ScopedVisit!BlockStatement; - - alias visit = BaseAnalyzer.visit; private: - bool isStdExceptionImported; - - template ScopedVisit(NodeType) - { - override void visit(const NodeType n) - { - bool tmp = isStdExceptionImported; - scope(exit) isStdExceptionImported = tmp; - n.accept(this); - } - } + enum string KEY = "dscanner.style.assert_without_msg"; + enum string MESSAGE = "An assert should have an explanatory message"; } unittest { import std.stdio : stderr; - import std.format : format; import dscanner.analysis.config : StaticAnalysisConfig, Check, disabledConfig; - import dscanner.analysis.helpers : assertAnalyzerWarnings; + import dscanner.analysis.helpers : assertAnalyzerWarningsDMD; StaticAnalysisConfig sac = disabledConfig(); sac.assert_without_msg = Check.enabled; - assertAnalyzerWarnings(q{ + assertAnalyzerWarningsDMD(q{ unittest { assert(0, "foo bar"); - assert(0); // [warn]: %s + assert(0); // [warn]: An assert should have an explanatory message } - }c.format( - AssertWithoutMessageCheck.MESSAGE, - ), sac); + }c, sac); - assertAnalyzerWarnings(q{ + assertAnalyzerWarningsDMD(q{ unittest { static assert(0, "foo bar"); - static assert(0); // [warn]: %s - } - }c.format( - AssertWithoutMessageCheck.MESSAGE, - ), sac); - - // check for std.exception.enforce - assertAnalyzerWarnings(q{ - 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 - } - }c.format( - AssertWithoutMessageCheck.MESSAGE, - ), sac); - - // check for std.exception.enforce - assertAnalyzerWarnings(q{ - unittest { - import exception; - class C { - import std.exception; - } - enforce(0); // std.exception not imported yet - could be a user-defined symbol - struct S { - import std.exception; - } - enforce(0); // std.exception not imported yet - could be a user-defined symbol - if (false) { - import std.exception; - } - enforce(0); // std.exception not imported yet - could be a user-defined symbol - { - import std.exception; - } - enforce(0); // std.exception not imported yet - could be a user-defined symbol + static assert(0); // [warn]: An assert should have an explanatory message } }c, sac); diff --git a/src/dscanner/analysis/auto_ref_assignment.d b/src/dscanner/analysis/auto_ref_assignment.d index dbd57609..69d35a6f 100644 --- a/src/dscanner/analysis/auto_ref_assignment.d +++ b/src/dscanner/analysis/auto_ref_assignment.d @@ -5,131 +5,97 @@ module dscanner.analysis.auto_ref_assignment; -import dparse.lexer; -import dparse.ast; import dscanner.analysis.base; /** * Checks for assignment to auto-ref function parameters. */ -final class AutoRefAssignmentCheck : BaseAnalyzer +extern(C++) class AutoRefAssignmentCheck(AST) : BaseAnalyzerDmd { mixin AnalyzerInfo!"auto_ref_assignment_check"; + alias visit = BaseAnalyzerDmd.visit; - /// - this(string fileName, bool skipTests = false) - { - super(fileName, null, skipTests); - } + mixin ScopedVisit!(AST.ClassDeclaration); + mixin ScopedVisit!(AST.StructDeclaration); + mixin ScopedVisit!(AST.FuncDeclaration); + mixin ScopedVisit!(AST.InterfaceDeclaration); + mixin ScopedVisit!(AST.UnionDeclaration); + mixin ScopedVisit!(AST.ScopeStatement); - override void visit(const Module m) + /// + extern(D) this(string fileName) { - pushScope(); - m.accept(this); - popScope(); + super(fileName); } - override void visit(const FunctionDeclaration func) + override void visit(AST.TemplateDeclaration td) { - if (func.parameters is null || func.parameters.parameters.length == 0) - return; - pushScope(); - scope (exit) - popScope(); - func.accept(this); + auto autoRefParamsOld = autoRefParams; + autoRefParams = []; + auto temp = inTemplateScope; + inTemplateScope = true; + + super.visit(td); + + inTemplateScope = temp; + autoRefParams = autoRefParamsOld; } - override void visit(const Parameter param) + override void visit(AST.Parameter p) { - import std.algorithm.searching : canFind; + import dmd.astenums : STC; - immutable bool isAuto = param.parameterAttributes.canFind!(a => a.idType == cast(ubyte) tok!"auto"); - immutable bool isRef = param.parameterAttributes.canFind!(a => a.idType == cast(ubyte) tok!"ref"); - if (!isAuto || !isRef) - return; - addSymbol(param.name.text); + if (p.storageClass & STC.auto_ && p.storageClass & STC.ref_ && p.ident) + autoRefParams ~= p.ident.toString(); } - override void visit(const AssignExpression assign) + override void visit(AST.AssignExp ae) { - if (assign.operator == tok!"" || scopes.length == 0) - return; - interest++; - assign.ternaryExpression.accept(this); - interest--; - } + import std.algorithm: canFind; - override void visit(const IdentifierOrTemplateInstance ioti) - { - import std.algorithm.searching : canFind; + auto ie = ae.e1.isIdentifierExp(); - if (ioti.identifier == tok!"" || interest <= 0) - return; - if (scopes[$ - 1].canFind(ioti.identifier.text)) - addErrorMessage(ioti.identifier.line, ioti.identifier.column, KEY, MESSAGE); + if (ie && inTemplateScope && autoRefParams.canFind(ie.ident.toString())) + addErrorMessage(cast(ulong) ae.loc.linnum, cast(ulong) ae.loc.charnum, KEY, + "Assignment to auto-ref function parameter."); } - override void visit(const IdentifierChain ic) + template ScopedVisit(NodeType) { - import std.algorithm.searching : canFind; - - if (ic.identifiers.length == 0 || interest <= 0) - return; - if (scopes[$ - 1].canFind(ic.identifiers[0].text)) - addErrorMessage(ic.identifiers[0].line, ic.identifiers[0].column, KEY, MESSAGE); + override void visit(NodeType n) + { + auto temp = inTemplateScope; + inTemplateScope = false; + super.visit(n); + inTemplateScope = temp; + } } - alias visit = BaseAnalyzer.visit; - private: + const(char[])[] autoRefParams; + bool inTemplateScope; - enum string MESSAGE = "Assignment to auto-ref function parameter."; - enum string KEY = "dscanner.suspicious.auto_ref_assignment"; - - invariant - { - assert(interest >= 0); - } - - int interest; - - void addSymbol(string symbolName) - { - scopes[$ - 1] ~= symbolName; - } - - void pushScope() - { - scopes.length++; - } - - void popScope() - { - scopes = scopes[0 .. $ - 1]; - } - - string[][] scopes; + enum KEY = "dscanner.suspicious.object_const"; } unittest { import std.stdio : stderr; - import std.format : format; import dscanner.analysis.config : StaticAnalysisConfig, Check, disabledConfig; - import dscanner.analysis.helpers : assertAnalyzerWarnings; + import dscanner.analysis.helpers : assertAnalyzerWarnings = assertAnalyzerWarningsDMD; StaticAnalysisConfig sac = disabledConfig(); sac.auto_ref_assignment_check = Check.enabled; assertAnalyzerWarnings(q{ int doStuff(T)(auto ref int a) { - a = 10; // [warn]: %s + a = 10; // [warn]: Assignment to auto-ref function parameter. } int doStuff(T)(ref int a) { a = 10; } - }c.format(AutoRefAssignmentCheck.MESSAGE), sac); + }c, sac); stderr.writeln("Unittest for AutoRefAssignmentCheck passed."); } diff --git a/src/dscanner/analysis/base.d b/src/dscanner/analysis/base.d index 0b92a5c2..63b93bdf 100644 --- a/src/dscanner/analysis/base.d +++ b/src/dscanner/analysis/base.d @@ -5,6 +5,12 @@ import std.string; import dparse.ast; import std.array; import dsymbol.scope_ : Scope; +import dmd.transitivevisitor; +import core.stdc.string; +import std.conv : to; + +import dmd.visitor; +import dmd.func; struct Message { @@ -26,11 +32,15 @@ enum comparitor = q{ a.line < b.line || (a.line == b.line && a.column < b.column alias MessageSet = RedBlackTree!(Message, comparitor, true); +/** + * Should be present in all visitors to specify the name of the check + * done by a patricular visitor + */ mixin template AnalyzerInfo(string checkName) { enum string name = checkName; - override protected string getName() + extern(D) override protected string getName() { return name; } @@ -100,3 +110,56 @@ protected: MessageSet _messages; } + +/** + * Visitor that implements the AST traversal logic. + * Supports collecting error messages + */ +extern(C++) class BaseAnalyzerDmd : SemanticTimeTransitiveVisitor +{ + alias visit = SemanticTimeTransitiveVisitor.visit; + + extern(D) this(string fileName, bool skipTests = false) + { + this.fileName = fileName; + this.skipTests = skipTests; + _messages = new MessageSet; + } + + /** + * Ensures that template AnalyzerInfo is instantiated in all classes + * deriving from this class + */ + extern(D) protected string getName() + { + assert(0); + } + + extern(D) Message[] messages() + { + return _messages[].array; + } + + override void visit(UnitTestDeclaration ud) + { + if (!skipTests) + super.visit(ud); + } + + +protected: + + extern(D) void addErrorMessage(size_t line, size_t column, string key, string message) + { + _messages.insert(Message(fileName, line, column, key, message, getName())); + } + + extern(D) bool skipTests; + + /** + * The file name + */ + extern(D) string fileName; + + extern(D) MessageSet _messages; +} diff --git a/src/dscanner/analysis/builtin_property_names.d b/src/dscanner/analysis/builtin_property_names.d index cadc2209..cc5a69b7 100644 --- a/src/dscanner/analysis/builtin_property_names.d +++ b/src/dscanner/analysis/builtin_property_names.d @@ -5,14 +5,7 @@ module dscanner.analysis.builtin_property_names; -import std.stdio; -import std.regex; -import dparse.ast; -import dparse.lexer; import dscanner.analysis.base; -import dscanner.analysis.helpers; -import dsymbol.scope_; -import std.algorithm : map; /** * The following code should be killed with fire: @@ -27,63 +20,64 @@ import std.algorithm : map; * } * --- */ -final class BuiltinPropertyNameCheck : BaseAnalyzer -{ - alias visit = BaseAnalyzer.visit; +extern(C++) class BuiltinPropertyNameCheck(AST) : BaseAnalyzerDmd +{ + alias visit = BaseAnalyzerDmd.visit; mixin AnalyzerInfo!"builtin_property_names_check"; - this(string fileName, const(Scope)* sc, bool skipTests = false) + extern(D) this(string fileName) { - super(fileName, sc, skipTests); + super(fileName); } - override void visit(const FunctionDeclaration fd) - { - if (depth > 0 && isBuiltinProperty(fd.name.text)) - { - addErrorMessage(fd.name.line, fd.name.column, KEY, generateErrorMessage(fd.name.text)); - } - fd.accept(this); - } + mixin AggregateVisit!(AST.StructDeclaration); + mixin AggregateVisit!(AST.ClassDeclaration); + mixin AggregateVisit!(AST.InterfaceDeclaration); + mixin AggregateVisit!(AST.UnionDeclaration); - override void visit(const FunctionBody functionBody) + override void visit(AST.VarDeclaration vd) { - immutable int d = depth; - scope (exit) - depth = d; - depth = 0; - functionBody.accept(this); + if (inAggregate && isBuiltinProperty(vd.ident.toString())) + addErrorMessage(cast(ulong) vd.loc.linnum, cast(ulong) vd.loc.charnum, + KEY, generateErrorMessage(vd.ident.toString())); } - override void visit(const AutoDeclaration ad) + override void visit(AST.FuncDeclaration fd) { - if (depth > 0) - foreach (i; ad.parts.map!(a => a.identifier)) - { - if (isBuiltinProperty(i.text)) - addErrorMessage(i.line, i.column, KEY, generateErrorMessage(i.text)); - } + if (inAggregate && isBuiltinProperty(fd.ident.toString())) + addErrorMessage(cast(ulong) fd.loc.linnum, cast(ulong) fd.loc.charnum, + KEY, generateErrorMessage(fd.ident.toString())); } - override void visit(const Declarator d) + override void visit(AST.AliasDeclaration ad) { - if (depth > 0 && isBuiltinProperty(d.name.text)) - addErrorMessage(d.name.line, d.name.column, KEY, generateErrorMessage(d.name.text)); + if (inAggregate && isBuiltinProperty(ad.ident.toString())) + addErrorMessage(cast(ulong) ad.loc.linnum, cast(ulong) ad.loc.charnum, + KEY, generateErrorMessage(ad.ident.toString())); } - override void visit(const StructBody sb) + override void visit(AST.TemplateDeclaration td) { - depth++; - sb.accept(this); - depth--; + if (inAggregate && isBuiltinProperty(td.ident.toString())) + addErrorMessage(cast(ulong) td.loc.linnum, cast(ulong) td.loc.charnum, + KEY, generateErrorMessage(td.ident.toString())); } private: - enum string KEY = "dscanner.confusing.builtin_property_names"; - string generateErrorMessage(string name) + template AggregateVisit(NodeType) + { + override void visit(NodeType n) + { + inAggregate++; + super.visit(n); + inAggregate--; + } + } + + extern(D) string generateErrorMessage(const(char)[] name) { import std.string : format; @@ -91,7 +85,7 @@ private: ~ " confuse code that depends on the '.%s' property of a type.", name, name); } - bool isBuiltinProperty(string name) + extern(D) bool isBuiltinProperty(const(char)[] name) { import std.algorithm : canFind; @@ -99,12 +93,14 @@ private: } enum string[] BuiltinProperties = ["init", "sizeof", "mangleof", "alignof", "stringof"]; - int depth; + int inAggregate; } unittest { import dscanner.analysis.config : StaticAnalysisConfig, Check, disabledConfig; + import dscanner.analysis.helpers : assertAnalyzerWarnings = assertAnalyzerWarningsDMD; + import std.stdio : stderr; StaticAnalysisConfig sac = disabledConfig(); sac.builtin_property_names_check = Check.enabled; @@ -117,5 +113,5 @@ class SomeClass } }c, sac); - stderr.writeln("Unittest for NumberStyleCheck passed."); + stderr.writeln("Unittest for BuiltinPropertyNamesCheck passed."); } diff --git a/src/dscanner/analysis/constructors.d b/src/dscanner/analysis/constructors.d index 2a65e069..84765d03 100644 --- a/src/dscanner/analysis/constructors.d +++ b/src/dscanner/analysis/constructors.d @@ -1,93 +1,63 @@ module dscanner.analysis.constructors; -import dparse.ast; -import dparse.lexer; import std.stdio; import dscanner.analysis.base; import dscanner.analysis.helpers; -import dsymbol.scope_ : Scope; -final class ConstructorCheck : BaseAnalyzer +extern(C++) class ConstructorCheck(AST) : BaseAnalyzerDmd { - alias visit = BaseAnalyzer.visit; - + alias visit = BaseAnalyzerDmd.visit; mixin AnalyzerInfo!"constructor_check"; - this(string fileName, const(Scope)* sc, bool skipTests = false) - { - super(fileName, sc, skipTests); - } - - override void visit(const ClassDeclaration classDeclaration) + extern(D) this(string fileName) { - immutable bool oldHasDefault = hasDefaultArgConstructor; - immutable bool oldHasNoArg = hasNoArgConstructor; - hasNoArgConstructor = false; - hasDefaultArgConstructor = false; - immutable State prev = state; - state = State.inClass; - classDeclaration.accept(this); - if (hasNoArgConstructor && hasDefaultArgConstructor) - { - addErrorMessage(classDeclaration.name.line, - classDeclaration.name.column, "dscanner.confusing.constructor_args", - "This class has a zero-argument constructor as well as a" - ~ " constructor with one default argument. This can be confusing."); - } - hasDefaultArgConstructor = oldHasDefault; - hasNoArgConstructor = oldHasNoArg; - state = prev; + super(fileName); } - override void visit(const StructDeclaration structDeclaration) + override void visit(AST.ClassDeclaration d) { - immutable State prev = state; - state = State.inStruct; - structDeclaration.accept(this); - state = prev; - } + bool hasDefaultArgConstructor = false; + bool hasNoArgConstructor = false; - override void visit(const Constructor constructor) - { - final switch (state) + if (d.members) { - case State.inStruct: - if (constructor.parameters.parameters.length == 1 - && constructor.parameters.parameters[0].default_ !is null) - { - addErrorMessage(constructor.line, constructor.column, - "dscanner.confusing.struct_constructor_default_args", - "This struct constructor can never be called with its " - ~ "default argument."); - } - break; - case State.inClass: - if (constructor.parameters.parameters.length == 1 - && constructor.parameters.parameters[0].default_ !is null) + foreach (s; *d.members) { - hasDefaultArgConstructor = true; - } - else if (constructor.parameters.parameters.length == 0) - hasNoArgConstructor = true; - break; - case State.ignoring: - break; - } - } + if (auto cd = s.isCtorDeclaration()) + { + auto tf = cd.type.isTypeFunction(); -private: + if (tf) + { + if (tf.parameterList.parameters.length == 0) + hasNoArgConstructor = true; + else + { + // Check if all parameters have a default value + hasDefaultArgConstructor = true; - enum State : ubyte - { - ignoring, - inClass, - inStruct - } + foreach (param; *tf.parameterList.parameters) + if (param.defaultArg is null) + hasDefaultArgConstructor = false; + } + } + } - State state; + s.accept(this); + } + } - bool hasNoArgConstructor; - bool hasDefaultArgConstructor; + if (hasNoArgConstructor && hasDefaultArgConstructor) + { + addErrorMessage(cast(ulong) d.loc.linnum, + cast(ulong) d.loc.charnum, KEY, MESSAGE); + } + } + +private: + enum MESSAGE = "This class has a zero-argument constructor as well as a" + ~ " constructor with default arguments. This can be confusing."; + enum KEY = "dscanner.confusing.constructor_args"; } unittest @@ -96,17 +66,23 @@ unittest StaticAnalysisConfig sac = disabledConfig(); sac.constructor_check = Check.enabled; - 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. + assertAnalyzerWarningsDMD(q{ + class Cat // [warn]: This class has a zero-argument constructor as well as a constructor with default arguments. This can be confusing. { this() {} this(string name = "kittie") {} } - struct Dog + class Cat // [warn]: This class has a zero-argument constructor as well as a constructor with default arguments. This can be confusing. + { + this() {} + this(string name = "kittie", int x = 2) {} + } + + class Cat { this() {} - this(string name = "doggie") {} // [warn]: This struct constructor can never be called with its default argument. + this(string name = "kittie", int x) {} } }c, sac); diff --git a/src/dscanner/analysis/del.d b/src/dscanner/analysis/del.d index 99ca6408..abc60fff 100644 --- a/src/dscanner/analysis/del.d +++ b/src/dscanner/analysis/del.d @@ -6,30 +6,29 @@ module dscanner.analysis.del; import std.stdio; -import dparse.ast; -import dparse.lexer; import dscanner.analysis.base; +import dscanner.analysis.helpers; import dsymbol.scope_; /** * Checks for use of the deprecated 'delete' keyword */ -final class DeleteCheck : BaseAnalyzer +extern(C++) class DeleteCheck(AST) : BaseAnalyzerDmd { - alias visit = BaseAnalyzer.visit; - + // alias visit = BaseAnalyzerDmd!AST.visit; + alias visit = BaseAnalyzerDmd.visit; mixin AnalyzerInfo!"delete_check"; - this(string fileName, const(Scope)* sc, bool skipTests = false) + extern(D) this(string fileName) { - super(fileName, sc, skipTests); + super(fileName); } - override void visit(const DeleteExpression d) + override void visit(AST.DeleteExp d) { - addErrorMessage(d.line, d.column, "dscanner.deprecated.delete_keyword", + addErrorMessage(cast(ulong) d.loc.linnum, cast(ulong) d.loc.charnum, "dscanner.deprecated.delete_keyword", "Avoid using the 'delete' keyword."); - d.accept(this); + super.visit(d); } } @@ -40,7 +39,7 @@ unittest StaticAnalysisConfig sac = disabledConfig(); sac.delete_check = Check.enabled; - assertAnalyzerWarnings(q{ + assertAnalyzerWarningsDMD(q{ void testDelete() { int[int] data = [1 : 2]; diff --git a/src/dscanner/analysis/enumarrayliteral.d b/src/dscanner/analysis/enumarrayliteral.d index c52c7261..fce6ab77 100644 --- a/src/dscanner/analysis/enumarrayliteral.d +++ b/src/dscanner/analysis/enumarrayliteral.d @@ -5,53 +5,33 @@ module dscanner.analysis.enumarrayliteral; -import dparse.ast; -import dparse.lexer; import dscanner.analysis.base; -import std.algorithm : canFind, map; -import dsymbol.scope_ : Scope; -void doNothing(string, size_t, size_t, string, bool) +extern(C++) class EnumArrayVisitor(AST) : BaseAnalyzerDmd { -} - -final class EnumArrayLiteralCheck : BaseAnalyzer -{ - alias visit = BaseAnalyzer.visit; - mixin AnalyzerInfo!"enum_array_literal_check"; + alias visit = BaseAnalyzerDmd.visit; - this(string fileName, const(Scope)* sc, bool skipTests = false) + extern(D) this(string fileName) { - super(fileName, sc, skipTests); + super(fileName); } - bool looking; - - mixin visitTemplate!ClassDeclaration; - mixin visitTemplate!InterfaceDeclaration; - mixin visitTemplate!UnionDeclaration; - mixin visitTemplate!StructDeclaration; + override void visit(AST.VarDeclaration vd) + { + import dmd.astenums : STC, InitKind; + import std.string : toStringz; - override void visit(const AutoDeclaration autoDec) - { - if (autoDec.storageClasses.canFind!(a => a.token == tok!"enum")) - { - foreach (part; autoDec.parts) - { - if (part.initializer is null) - continue; - if (part.initializer.nonVoidInitializer is null) - continue; - if (part.initializer.nonVoidInitializer.arrayInitializer is null) - continue; - addErrorMessage(part.identifier.line, part.identifier.column, - "dscanner.performance.enum_array_literal", - "This enum may lead to unnecessary allocation at run-time." + string message = "This enum may lead to unnecessary allocation at run-time." ~ " Use 'static immutable " - ~ part.identifier.text ~ " = [ ...' instead."); - } - } - autoDec.accept(this); + ~ vd.ident.toString().idup() ~ " = [ ...' instead."; + + if (!vd.type && vd._init.kind == InitKind.array && vd.storage_class & STC.manifest) + addErrorMessage(cast(ulong) vd.loc.linnum, + cast(ulong) vd.loc.charnum, KEY, + message); + super.visit(vd); } -} + + private enum KEY = "dscanner.performance.enum_array_literal"; +} \ No newline at end of file diff --git a/src/dscanner/analysis/explicitly_annotated_unittests.d b/src/dscanner/analysis/explicitly_annotated_unittests.d index c5afc018..b26441c4 100644 --- a/src/dscanner/analysis/explicitly_annotated_unittests.d +++ b/src/dscanner/analysis/explicitly_annotated_unittests.d @@ -4,53 +4,36 @@ module dscanner.analysis.explicitly_annotated_unittests; -import dparse.lexer; -import dparse.ast; import dscanner.analysis.base; - -import std.stdio; +import dscanner.analysis.helpers; /** * Requires unittests to be explicitly annotated with either @safe or @system */ -final class ExplicitlyAnnotatedUnittestCheck : BaseAnalyzer +extern(C++) class ExplicitlyAnnotatedUnittestCheck(AST) : BaseAnalyzerDmd { - enum string KEY = "dscanner.style.explicitly_annotated_unittest"; - enum string MESSAGE = "A unittest should be annotated with at least @safe or @system"; mixin AnalyzerInfo!"explicitly_annotated_unittests"; + alias visit = BaseAnalyzerDmd.visit; - /// - this(string fileName, bool skipTests = false) + extern(D) this(string fileName) { - super(fileName, null, skipTests); + super(fileName); } - override void visit(const Declaration decl) + override void visit(AST.UnitTestDeclaration d) { - if (decl.unittest_ !is null) - { - bool isSafeOrSystem; - if (decl.attributes !is null) - foreach (attribute; decl.attributes) - { - if (attribute.atAttribute !is null) - { - const token = attribute.atAttribute.identifier.text; - if (token == "safe" || token == "system") - { - isSafeOrSystem = true; - break; - } - } - } - if (!isSafeOrSystem) - addErrorMessage(decl.unittest_.line, decl.unittest_.column, KEY, MESSAGE); - } - decl.accept(this); - } + import dmd.astenums : STC; - alias visit = BaseAnalyzer.visit; + if (!(d.storage_class & STC.safe || d.storage_class & STC.system)) + addErrorMessage(cast(ulong) d.loc.linnum, cast(ulong) d.loc.charnum, + KEY, MESSAGE); + super.visit(d); + } + +private: + enum string KEY = "dscanner.style.explicitly_annotated_unittest"; + enum string MESSAGE = "A unittest should be annotated with at least @safe or @system"; } unittest @@ -63,32 +46,29 @@ unittest StaticAnalysisConfig sac = disabledConfig(); sac.explicitly_annotated_unittests = Check.enabled; - assertAnalyzerWarnings(q{ + assertAnalyzerWarningsDMD(q{ + + @disable foo() {} + @safe unittest {} @system unittest {} pure nothrow @system @nogc unittest {} - unittest {} // [warn]: %s - pure nothrow @nogc unittest {} // [warn]: %s - }c.format( - ExplicitlyAnnotatedUnittestCheck.MESSAGE, - ExplicitlyAnnotatedUnittestCheck.MESSAGE, - ), sac); + unittest {} // [warn]: A unittest should be annotated with at least @safe or @system + pure nothrow @nogc unittest {} // [warn]: A unittest should be annotated with at least @safe or @system + }c, sac); // nested - assertAnalyzerWarnings(q{ + assertAnalyzerWarningsDMD(q{ struct Foo { @safe unittest {} @system unittest {} - unittest {} // [warn]: %s - pure nothrow @nogc unittest {} // [warn]: %s + unittest {} // [warn]: A unittest should be annotated with at least @safe or @system + pure nothrow @nogc unittest {} // [warn]: A unittest should be annotated with at least @safe or @system } - }c.format( - ExplicitlyAnnotatedUnittestCheck.MESSAGE, - ExplicitlyAnnotatedUnittestCheck.MESSAGE, - ), sac); + }c, sac); stderr.writeln("Unittest for ExplicitlyAnnotatedUnittestCheck passed."); -} +} \ No newline at end of file diff --git a/src/dscanner/analysis/final_attribute.d b/src/dscanner/analysis/final_attribute.d index 8df2adbf..3f880b3e 100644 --- a/src/dscanner/analysis/final_attribute.d +++ b/src/dscanner/analysis/final_attribute.d @@ -7,36 +7,22 @@ module dscanner.analysis.final_attribute; import dscanner.analysis.base; import dscanner.analysis.helpers; -import dparse.ast; -import dparse.lexer; +import std.string : format; +import std.stdio; +import dmd.dsymbol; +import dmd.astcodegen; /** * Checks for useless usage of the final attribute. * * There are several cases where the compiler allows them even if it's a noop. */ -final class FinalAttributeChecker : BaseAnalyzer +extern(C++) class FinalAttributeChecker(AST) : BaseAnalyzerDmd { -private: - - enum string KEY = "dscanner.useless.final"; - enum string MSGB = "Useless final attribute, %s"; - - static struct MESSAGE - { - static immutable struct_i = "structs cannot be subclassed"; - static immutable union_i = "unions cannot be subclassed"; - static immutable class_t = "templated functions declared within a class are never virtual"; - static immutable class_p = "private functions declared within a class are never virtual"; - static immutable class_f = "functions declared within a final class are never virtual"; - static immutable class_s = "static functions are never virtual"; - static immutable interface_t = "templated functions declared within an interface are never virtual"; - static immutable struct_f = "functions declared within a struct are never virtual"; - static immutable union_f = "functions declared within an union are never virtual"; - static immutable func_n = "nested functions are never virtual"; - static immutable func_g = "global functions are never virtual"; - } + mixin AnalyzerInfo!"final_attribute_check"; + // alias visit = BaseAnalyzerDmd!AST.visit; + alias visit = BaseAnalyzerDmd.visit; enum Parent { @@ -49,254 +35,245 @@ private: } bool _private; - bool _finalAggregate; + bool _inFinalClass; bool _alwaysStatic; bool _blockStatic; + bool _blockFinal; Parent _parent = Parent.module_; - void addError(T)(T t, string msg) + enum pushPopPrivate = q{ + const bool wasPrivate = _private; + _private = false; + scope (exit) _private = wasPrivate; + }; + + extern(D) this(string fileName) { - 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)); + super(fileName); } -public: + override void visit(AST.StorageClassDeclaration scd) + { + import dmd.astenums : STC; - alias visit = BaseAnalyzer.visit; + if (scd.stc & STC.static_) + _blockStatic = true; - mixin AnalyzerInfo!"final_attribute_check"; + scope (exit) _blockStatic = false; - enum pushPopPrivate = q{ - const bool wasPrivate = _private; - _private = false; - scope (exit) _private = wasPrivate; - }; + if (scd.stc & STC.final_) + _blockFinal = true; - /// - this(string fileName, bool skipTests = false) + scope (exit) _blockFinal = false; + + if (!scd.decl) + return; + + foreach (member; *scd.decl) + { + auto sd = member.isStructDeclaration(); + auto ud = member.isUnionDeclaration(); + + if (!ud && sd && scd.stc & STC.final_) + { + addErrorMessage(cast(ulong) sd.loc.linnum, cast(ulong) sd.loc.charnum, KEY, + MSGB.format(FinalAttributeChecker.MESSAGE.struct_i)); + } + + if (ud && scd.stc & STC.final_) + { + addErrorMessage(cast(ulong) ud.loc.linnum, cast(ulong) ud.loc.charnum, KEY, + MSGB.format(FinalAttributeChecker.MESSAGE.union_i)); + } + + member.accept(this); + } + } + + override void visit(AST.TemplateDeclaration td) { - super(fileName, null, skipTests); + import dmd.astenums : STC; + + if (!td.members) + return; + + foreach (member; *td.members) + { + auto fd = member.isFuncDeclaration(); + + if (fd) + { + if (_parent == Parent.class_ && fd.storage_class & STC.final_) + addErrorMessage(cast(ulong) fd.loc.linnum, cast(ulong) fd.loc.charnum, KEY, + MSGB.format(FinalAttributeChecker.MESSAGE.class_t)); + + if (_parent == Parent.interface_ && fd.storage_class & STC.final_) + addErrorMessage(cast(ulong) fd.loc.linnum, cast(ulong) fd.loc.charnum, KEY, + MSGB.format(FinalAttributeChecker.MESSAGE.interface_t)); + } + } + } - override void visit(const(StructDeclaration) sd) + override void visit(AST.ClassDeclaration cd) { + if (_blockFinal && !_inFinalClass) + _inFinalClass = true; + else if (_inFinalClass) + _inFinalClass = false; + _blockStatic = false; + mixin (pushPopPrivate); const Parent saved = _parent; - _parent = Parent.struct_; - _alwaysStatic = false; - sd.accept(this); + _parent = Parent.class_; + super.visit(cd); _parent = saved; + _inFinalClass = false; } - override void visit(const(InterfaceDeclaration) id) + override void visit(AST.FuncDeclaration fd) { + import dmd.astenums : STC; + + if (_parent == Parent.class_ && _private && fd.storage_class & STC.final_) + addErrorMessage(cast(ulong) fd.loc.linnum, cast(ulong) fd.loc.charnum, KEY, + MSGB.format(FinalAttributeChecker.MESSAGE.class_p)); + + else if (fd.storage_class & STC.final_ && (fd.storage_class & STC.static_ || _blockStatic)) + addErrorMessage(cast(ulong) fd.loc.linnum, cast(ulong) fd.loc.charnum, KEY, + MSGB.format(FinalAttributeChecker.MESSAGE.class_s)); + + else if (_parent == Parent.class_ && _inFinalClass && fd.storage_class & STC.final_) + addErrorMessage(cast(ulong) fd.loc.linnum, cast(ulong) fd.loc.charnum, KEY, + MSGB.format(FinalAttributeChecker.MESSAGE.class_f)); + + if (_parent == Parent.struct_ && fd.storage_class & STC.final_) + addErrorMessage(cast(ulong) fd.loc.linnum, cast(ulong) fd.loc.charnum, KEY, + MSGB.format(FinalAttributeChecker.MESSAGE.struct_f)); + + if (_parent == Parent.union_ && fd.storage_class & STC.final_) + addErrorMessage(cast(ulong) fd.loc.linnum, cast(ulong) fd.loc.charnum, KEY, + MSGB.format(FinalAttributeChecker.MESSAGE.union_f)); + + if (_parent == Parent.module_ && fd.storage_class & STC.final_) + addErrorMessage(cast(ulong) fd.loc.linnum, cast(ulong) fd.loc.charnum, KEY, + MSGB.format(FinalAttributeChecker.MESSAGE.func_g)); + + if (_parent == Parent.function_ && fd.storage_class & STC.final_) + addErrorMessage(cast(ulong) fd.loc.linnum, cast(ulong) fd.loc.charnum, KEY, + MSGB.format(FinalAttributeChecker.MESSAGE.func_n)); + + _blockStatic = false; mixin (pushPopPrivate); const Parent saved = _parent; - _parent = Parent.interface_; - _alwaysStatic = false; - id.accept(this); + _parent = Parent.function_; + super.visit(fd); _parent = saved; } - override void visit(const(UnionDeclaration) ud) + override void visit(AST.InterfaceDeclaration id) { + _blockStatic = false; mixin (pushPopPrivate); const Parent saved = _parent; - _parent = Parent.union_; - _alwaysStatic = false; - ud.accept(this); + _parent = Parent.interface_; + super.visit(id); _parent = saved; } - override void visit(const(ClassDeclaration) cd) + override void visit(AST.UnionDeclaration ud) { + _blockStatic = false; mixin (pushPopPrivate); const Parent saved = _parent; - _parent = Parent.class_; - _alwaysStatic = false; - cd.accept(this); + _parent = Parent.union_; + super.visit(ud); _parent = saved; } - override void visit(const(MixinTemplateDeclaration) mtd) - { - // can't really know where it'll be mixed (class |final class | struct ?) - } - - override void visit(const(TemplateDeclaration) mtd) + override void visit(AST.StructDeclaration sd) { - // regular template are also mixable + _blockStatic = false; + mixin (pushPopPrivate); + const Parent saved = _parent; + _parent = Parent.struct_; + super.visit(sd); + _parent = saved; } - override void visit(const(AttributeDeclaration) decl) + override void visit(AST.VisibilityDeclaration vd) { - if (_parent == Parent.class_ && decl.attribute && - decl.attribute.attribute == tok!"static") - _alwaysStatic = true; + if (vd.visibility.kind == Visibility.Kind.private_) + _private = true; + else + _private = false; + + super.visit(vd); + _private = false; } - override void visit(const(Declaration) d) + enum KEY = "dscanner.useless.final"; + enum string MSGB = "Useless final attribute, %s"; + extern(D) static struct MESSAGE { - import std.algorithm.iteration : filter; - import std.algorithm.searching : canFind; - - const Parent savedParent = _parent; - - bool undoBlockStatic; - if (_parent == Parent.class_ && d.attributes && - d.attributes.canFind!(a => a.attribute == tok!"static")) - { - _blockStatic = true; - undoBlockStatic = true; - } - - const bool wasFinalAggr = _finalAggregate; - scope(exit) - { - d.accept(this); - _parent = savedParent; - if (undoBlockStatic) - _blockStatic = false; - _finalAggregate = wasFinalAggr; - } - - if (!d.attributeDeclaration && - !d.classDeclaration && - !d.structDeclaration && - !d.unionDeclaration && - !d.interfaceDeclaration && - !d.functionDeclaration) - return; - - if (d.attributeDeclaration && d.attributeDeclaration.attribute) - { - const tp = d.attributeDeclaration.attribute.attribute.type; - _private = isProtection(tp) & (tp == tok!"private"); - } - - const bool isFinal = d.attributes - .canFind!(a => a.attribute.type == tok!"final"); - - const bool isStaticOnce = d.attributes - .canFind!(a => a.attribute.type == tok!"static"); - - // determine if private - const bool changeProtectionOnce = d.attributes - .canFind!(a => a.attribute.type.isProtection); - - const bool isPrivateOnce = d.attributes - .canFind!(a => a.attribute.type == tok!"private"); - - bool isPrivate; - - if (isPrivateOnce) - isPrivate = true; - else if (_private && !changeProtectionOnce) - isPrivate = true; - - // check final aggregate type - if (d.classDeclaration || d.structDeclaration || d.unionDeclaration) - { - _finalAggregate = isFinal; - if (_finalAggregate && savedParent == Parent.module_) - { - if (d.structDeclaration) - addError(d.structDeclaration, MESSAGE.struct_i); - else if (d.unionDeclaration) - addError(d.unionDeclaration, MESSAGE.union_i); - } - } - - if (!d.functionDeclaration) - return; - - // check final functions - _parent = Parent.function_; - const(FunctionDeclaration) fd = d.functionDeclaration; - - if (isFinal) final switch(savedParent) - { - case Parent.class_: - if (fd.templateParameters) - addError(fd, MESSAGE.class_t); - if (isPrivate) - addError(fd, MESSAGE.class_p); - else if (isStaticOnce || _alwaysStatic || _blockStatic) - addError(fd, MESSAGE.class_s); - else if (_finalAggregate) - addError(fd, MESSAGE.class_f); - break; - case Parent.interface_: - if (fd.templateParameters) - addError(fd, MESSAGE.interface_t); - break; - case Parent.struct_: - addError(fd, MESSAGE.struct_f); - break; - case Parent.union_: - addError(fd, MESSAGE.union_f); - break; - case Parent.function_: - addError(fd, MESSAGE.func_n); - break; - case Parent.module_: - addError(fd, MESSAGE.func_g); - break; - } + static immutable struct_i = "structs cannot be subclassed"; + static immutable union_i = "unions cannot be subclassed"; + static immutable class_t = "templated functions declared within a class are never virtual"; + static immutable class_p = "private functions declared within a class are never virtual"; + static immutable class_f = "functions declared within a final class are never virtual"; + static immutable class_s = "static functions are never virtual"; + static immutable interface_t = "templated functions declared within an interface are never virtual"; + static immutable struct_f = "functions declared within a struct are never virtual"; + static immutable union_f = "functions declared within an union are never virtual"; + static immutable func_n = "nested functions are never virtual"; + static immutable func_g = "global functions are never virtual"; } } @system unittest { import dscanner.analysis.config : StaticAnalysisConfig, Check, disabledConfig; - import dscanner.analysis.helpers : assertAnalyzerWarnings; - import std.stdio : stderr; - import std.format : format; StaticAnalysisConfig sac = disabledConfig(); sac.final_attribute_check = Check.enabled; - - // pass - - assertAnalyzerWarnings(q{ + + assertAnalyzerWarningsDMD(q{ void foo(){} }, sac); - assertAnalyzerWarnings(q{ + assertAnalyzerWarningsDMD(q{ void foo(){void foo(){}} }, sac); - assertAnalyzerWarnings(q{ + assertAnalyzerWarningsDMD(q{ struct S{} }, sac); - assertAnalyzerWarnings(q{ + assertAnalyzerWarningsDMD(q{ union U{} }, sac); - assertAnalyzerWarnings(q{ + assertAnalyzerWarningsDMD(q{ class Foo{public final void foo(){}} }, sac); - assertAnalyzerWarnings(q{ + assertAnalyzerWarningsDMD(q{ final class Foo{static struct Bar{}} }, sac); - assertAnalyzerWarnings(q{ + assertAnalyzerWarningsDMD(q{ class Foo{private: public final void foo(){}} }, sac); - assertAnalyzerWarnings(q{ + assertAnalyzerWarningsDMD(q{ class Foo{private: public: final void foo(){}} }, sac); - assertAnalyzerWarnings(q{ + assertAnalyzerWarningsDMD(q{ class Foo{private: public: final void foo(){}} }, sac); - assertAnalyzerWarnings(q{ + assertAnalyzerWarningsDMD(q{ class Impl { private: @@ -307,7 +284,7 @@ public: } }, sac); - assertAnalyzerWarnings(q{ + assertAnalyzerWarningsDMD(q{ mixin template Impl() { protected final void mixin_template_can() {} @@ -316,87 +293,87 @@ public: // fail - assertAnalyzerWarnings(q{ + assertAnalyzerWarningsDMD(q{ final void foo(){} // [warn]: %s }c.format( - FinalAttributeChecker.MSGB.format(FinalAttributeChecker.MESSAGE.func_g) + (FinalAttributeChecker!ASTCodegen).MSGB.format((FinalAttributeChecker!ASTCodegen).MESSAGE.func_g) ), sac); - assertAnalyzerWarnings(q{ + assertAnalyzerWarningsDMD(q{ void foo(){final void foo(){}} // [warn]: %s }c.format( - FinalAttributeChecker.MSGB.format(FinalAttributeChecker.MESSAGE.func_n) + (FinalAttributeChecker!ASTCodegen).MSGB.format((FinalAttributeChecker!ASTCodegen).MESSAGE.func_n) ), sac); - assertAnalyzerWarnings(q{ + assertAnalyzerWarningsDMD(q{ void foo() { static if (true) final class A{ private: final protected void foo(){}} // [warn]: %s } }c.format( - FinalAttributeChecker.MSGB.format(FinalAttributeChecker.MESSAGE.class_f) + (FinalAttributeChecker!ASTCodegen).MSGB.format((FinalAttributeChecker!ASTCodegen).MESSAGE.class_f) ), sac); - assertAnalyzerWarnings(q{ + assertAnalyzerWarningsDMD(q{ final struct Foo{} // [warn]: %s }c.format( - FinalAttributeChecker.MSGB.format(FinalAttributeChecker.MESSAGE.struct_i) + (FinalAttributeChecker!ASTCodegen).MSGB.format((FinalAttributeChecker!ASTCodegen).MESSAGE.struct_i) ), sac); - assertAnalyzerWarnings(q{ + assertAnalyzerWarningsDMD(q{ final union Foo{} // [warn]: %s }c.format( - FinalAttributeChecker.MSGB.format(FinalAttributeChecker.MESSAGE.union_i) + (FinalAttributeChecker!ASTCodegen).MSGB.format((FinalAttributeChecker!ASTCodegen).MESSAGE.union_i) ), sac); - assertAnalyzerWarnings(q{ + assertAnalyzerWarningsDMD(q{ class Foo{private final void foo(){}} // [warn]: %s }c.format( - FinalAttributeChecker.MSGB.format(FinalAttributeChecker.MESSAGE.class_p) + (FinalAttributeChecker!ASTCodegen).MSGB.format((FinalAttributeChecker!ASTCodegen).MESSAGE.class_p) ), sac); - assertAnalyzerWarnings(q{ + assertAnalyzerWarningsDMD(q{ class Foo{private: final void foo(){}} // [warn]: %s }c.format( - FinalAttributeChecker.MSGB.format(FinalAttributeChecker.MESSAGE.class_p) + (FinalAttributeChecker!ASTCodegen).MSGB.format((FinalAttributeChecker!ASTCodegen).MESSAGE.class_p) ), sac); - assertAnalyzerWarnings(q{ + assertAnalyzerWarningsDMD(q{ interface Foo{final void foo(T)(){}} // [warn]: %s }c.format( - FinalAttributeChecker.MSGB.format(FinalAttributeChecker.MESSAGE.interface_t) + (FinalAttributeChecker!ASTCodegen).MSGB.format((FinalAttributeChecker!ASTCodegen).MESSAGE.interface_t) ), sac); - assertAnalyzerWarnings(q{ + assertAnalyzerWarningsDMD(q{ final class Foo{final void foo(){}} // [warn]: %s }c.format( - FinalAttributeChecker.MSGB.format(FinalAttributeChecker.MESSAGE.class_f) + (FinalAttributeChecker!ASTCodegen).MSGB.format((FinalAttributeChecker!ASTCodegen).MESSAGE.class_f) ), sac); - assertAnalyzerWarnings(q{ + assertAnalyzerWarningsDMD(q{ private: final class Foo {public: private final void foo(){}} // [warn]: %s }c.format( - FinalAttributeChecker.MSGB.format(FinalAttributeChecker.MESSAGE.class_p) + (FinalAttributeChecker!ASTCodegen).MSGB.format((FinalAttributeChecker!ASTCodegen).MESSAGE.class_p) ), sac); - assertAnalyzerWarnings(q{ + assertAnalyzerWarningsDMD(q{ class Foo {final static void foo(){}} // [warn]: %s }c.format( - FinalAttributeChecker.MSGB.format(FinalAttributeChecker.MESSAGE.class_s) + (FinalAttributeChecker!ASTCodegen).MSGB.format((FinalAttributeChecker!ASTCodegen).MESSAGE.class_s) ), sac); - assertAnalyzerWarnings(q{ + assertAnalyzerWarningsDMD(q{ class Foo { void foo(){} static: final void foo(){} // [warn]: %s } }c.format( - FinalAttributeChecker.MSGB.format(FinalAttributeChecker.MESSAGE.class_s) + (FinalAttributeChecker!ASTCodegen).MSGB.format((FinalAttributeChecker!ASTCodegen).MESSAGE.class_s) ), sac); - assertAnalyzerWarnings(q{ + assertAnalyzerWarningsDMD(q{ class Foo { void foo(){} @@ -404,11 +381,11 @@ public: void foo(){} } }c.format( - FinalAttributeChecker.MSGB.format(FinalAttributeChecker.MESSAGE.class_s) + (FinalAttributeChecker!ASTCodegen).MSGB.format((FinalAttributeChecker!ASTCodegen).MESSAGE.class_s) ), sac); - assertAnalyzerWarnings(q{ + assertAnalyzerWarningsDMD(q{ class Statement { final class UsesEH{} @@ -417,4 +394,4 @@ public: }, sac); stderr.writeln("Unittest for FinalAttributeChecker passed."); -} +} \ No newline at end of file diff --git a/src/dscanner/analysis/fish.d b/src/dscanner/analysis/fish.d deleted file mode 100644 index 0417e486..00000000 --- a/src/dscanner/analysis/fish.d +++ /dev/null @@ -1,67 +0,0 @@ -// Copyright Brian Schott (Hackerpilot) 2014. -// 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.fish; - -import std.stdio; -import dparse.ast; -import dparse.lexer; -import dscanner.analysis.base; -import dscanner.analysis.helpers; -import dsymbol.scope_ : Scope; - -/** - * Checks for use of the deprecated floating point comparison operators. - */ -final class FloatOperatorCheck : BaseAnalyzer -{ - alias visit = BaseAnalyzer.visit; - - enum string KEY = "dscanner.deprecated.floating_point_operators"; - mixin AnalyzerInfo!"float_operator_check"; - - this(string fileName, const(Scope)* sc, bool skipTests = false) - { - super(fileName, sc, skipTests); - } - - override void visit(const RelExpression r) - { - if (r.operator == tok!"<>" || r.operator == tok!"<>=" - || r.operator == tok!"!<>" || r.operator == tok!"!>" - || r.operator == tok!"!<" || r.operator == tok!"!<>=" - || r.operator == tok!"!>=" || r.operator == tok!"!<=") - { - addErrorMessage(r.line, r.column, KEY, - "Avoid using the deprecated floating-point operators."); - } - r.accept(this); - } -} - -unittest -{ - import dscanner.analysis.config : StaticAnalysisConfig, Check, disabledConfig; - - StaticAnalysisConfig sac = disabledConfig(); - sac.float_operator_check = Check.enabled; - assertAnalyzerWarnings(q{ - void testFish() - { - 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. - } - }c, sac); - - stderr.writeln("Unittest for FloatOperatorCheck passed."); -} diff --git a/src/dscanner/analysis/helpers.d b/src/dscanner/analysis/helpers.d index 6886844f..83ea7957 100644 --- a/src/dscanner/analysis/helpers.d +++ b/src/dscanner/analysis/helpers.d @@ -19,6 +19,11 @@ import dscanner.analysis.base; import std.experimental.allocator.mallocator; import std.experimental.allocator; +import dmd.parse : Parser; +import dmd.astbase : ASTBase; +import dmd.astcodegen; +import dmd.frontend; + S between(S)(S value, S before, S after) if (isSomeString!S) { return value.after(before).before(after); @@ -136,3 +141,121 @@ void assertAnalyzerWarnings(string code, const StaticAnalysisConfig config, throw new AssertError(message, file, line); } } + +void assertAnalyzerWarningsDMD(string code, const StaticAnalysisConfig config, bool semantic = false, + string file = __FILE__, size_t line = __LINE__) +{ + import dmd.globals : global; + import dscanner.utils : getModuleName; + import std.file : remove, exists; + import std.stdio : File; + import std.path : dirName; + import dmd.arraytypes : Strings; + + import std.stdio : File; + import std.file : exists, remove; + + auto deleteme = "test.txt"; + File f = File(deleteme, "w"); + scope(exit) + { + assert(exists(deleteme)); + remove(deleteme); + } + + f.write(code); + f.close(); + + auto dmdParentDir = dirName(dirName(dirName(dirName(__FILE_FULL_PATH__)))); + + global.params.useUnitTests = true; + global.path = new Strings(); + global.path.push((dmdParentDir ~ "/dmd" ~ "\0").ptr); + global.path.push((dmdParentDir ~ "/dmd/druntime/src" ~ "\0").ptr); + + initDMD(); + + auto input = cast(char[]) code; + input ~= '\0'; + auto t = dmd.frontend.parseModule(cast(const(char)[]) file, cast(const (char)[]) input); + if (semantic) + t.module_.fullSemantic(); + + MessageSet rawWarnings = analyzeDmd("test.txt", t.module_, getModuleName(t.module_.md), config); + + string[] codeLines = code.splitLines(); + + // Get the warnings ordered by line + string[size_t] warnings; + foreach (rawWarning; rawWarnings[]) + { + // Skip the warning if it is on line zero + immutable size_t rawLine = rawWarning.line; + if (rawLine == 0) + { + stderr.writefln("!!! Skipping warning because it is on line zero:\n%s", + rawWarning.message); + continue; + } + + size_t warnLine = line - 1 + rawLine; + warnings[warnLine] = format("[warn]: %s", rawWarning.message); + } + + // Get all the messages from the comments in the code + string[size_t] messages; + 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; + + // Get the line of this code line + size_t lineNo = i + line; + + // Get the message + messages[lineNo] = commentPart; + } + + // Throw an assert error if any messages are not listed in the warnings + foreach (lineNo, message; messages) + { + // No warning + if (lineNo !in warnings) + { + immutable string errors = "Expected warning:\n%s\nFrom source code at (%s:?):\n%s".format(messages[lineNo], + lineNo, codeLines[lineNo - line]); + throw new AssertError(errors, file, lineNo); + } + // Different warning + else if (warnings[lineNo] != messages[lineNo]) + { + 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); + } + } + + // Throw an assert error if there were any warnings that were not expected + string[] unexpectedWarnings; + foreach (lineNo, warning; warnings) + { + // Unexpected warning + if (lineNo !in messages) + { + unexpectedWarnings ~= "%s\nFrom source code at (%s:?):\n%s".format(warning, + lineNo, codeLines[lineNo - line]); + } + } + if (unexpectedWarnings.length) + { + immutable string message = "Unexpected warnings:\n" ~ unexpectedWarnings.join("\n"); + throw new AssertError(message, file, line); + } +} diff --git a/src/dscanner/analysis/imports_sortedness.d b/src/dscanner/analysis/imports_sortedness.d index 4f33fd4b..6238215d 100644 --- a/src/dscanner/analysis/imports_sortedness.d +++ b/src/dscanner/analysis/imports_sortedness.d @@ -5,89 +5,100 @@ module dscanner.analysis.imports_sortedness; import dscanner.analysis.base; -import dparse.lexer; -import dparse.ast; - -import std.stdio; /** * Checks the sortedness of module imports */ -final class ImportSortednessCheck : BaseAnalyzer +extern(C++) class ImportSortednessCheck(AST) : BaseAnalyzerDmd { enum string KEY = "dscanner.style.imports_sortedness"; enum string MESSAGE = "The imports are not sorted in alphabetical order"; mixin AnalyzerInfo!"imports_sortedness"; + alias visit = BaseAnalyzerDmd.visit; + // alias visit = BaseAnalyzerDmd!AST.visit; /// - this(string fileName, bool skipTests = false) + extern(D) this(string fileName) { - super(fileName, null, skipTests); + super(fileName); } - mixin ScopedVisit!Module; - mixin ScopedVisit!Statement; - mixin ScopedVisit!BlockStatement; - mixin ScopedVisit!StructBody; - mixin ScopedVisit!IfStatement; - mixin ScopedVisit!TemplateDeclaration; - mixin ScopedVisit!ConditionalDeclaration; + mixin ScopedVisit!(AST.StructDeclaration); + mixin ScopedVisit!(AST.FuncDeclaration); + mixin ScopedVisit!(AST.InterfaceDeclaration); + mixin ScopedVisit!(AST.UnionDeclaration); + mixin ScopedVisit!(AST.TemplateDeclaration); + mixin ScopedVisit!(AST.IfStatement); + mixin ScopedVisit!(AST.WhileStatement); + mixin ScopedVisit!(AST.ForStatement); + mixin ScopedVisit!(AST.ForeachStatement); + mixin ScopedVisit!(AST.ScopeStatement); + mixin ScopedVisit!(AST.ConditionalDeclaration); + - override void visit(const VariableDeclaration id) + override void visit(AST.VarDeclaration vd) { imports[level] = []; } - override void visit(const ImportDeclaration id) + override void visit(AST.Import i) { - import std.algorithm.iteration : map; + import std.algorithm : map; import std.array : join; - import std.string : strip; + import std.conv : to; - if (id.importBindings is null || id.importBindings.importBinds.length == 0) - { - foreach (singleImport; id.singleImports) - { - string importModuleName = singleImport.identifierChain.identifiers.map!`a.text`.join("."); - addImport(importModuleName, singleImport); - } - } + string importModuleName = i.packages.map!(a => a.toString().dup).join("."); + + if (importModuleName != "") + importModuleName ~= "." ~ i.id.toString(); else - { - string importModuleName = id.importBindings.singleImport.identifierChain.identifiers.map!`a.text`.join("."); + importModuleName ~= i.id.toString(); - foreach (importBind; id.importBindings.importBinds) + if (i.names.length) + { + foreach (name; i.names) { - addImport(importModuleName ~ "-" ~ importBind.left.text, id.importBindings.singleImport); + string aux = to!string(importModuleName ~ "-" ~ name.toString()); + addImport(aux, i); } } + else addImport(importModuleName, i); } - alias visit = BaseAnalyzer.visit; - private: - + enum maxDepth = 20; int level; string[][int] imports; + bool[maxDepth] levelAvailable; template ScopedVisit(NodeType) { - override void visit(const NodeType n) + override void visit(NodeType n) { + if (level >= maxDepth) + return; + + imports[level] = []; imports[++level] = []; - n.accept(this); + levelAvailable[level] = true; + super.visit(n); level--; } } - void addImport(string importModuleName, const SingleImport singleImport) + extern(D) void addImport(string importModuleName, AST.Import i) { import std.uni : sicmp; - if (imports[level].length > 0 && imports[level][$ -1].sicmp(importModuleName) > 0) + if (!levelAvailable[level]) + { + imports[level] = []; + levelAvailable[level] = true; + } + + if (imports[level].length > 0 && imports[level][$ - 1].sicmp(importModuleName) > 0) { - addErrorMessage(singleImport.identifierChain.identifiers[0].line, - singleImport.identifierChain.identifiers[0].column, KEY, MESSAGE); + addErrorMessage(cast(ulong) i.loc.linnum, cast(ulong) i.loc.charnum, KEY, MESSAGE); } else { @@ -99,9 +110,8 @@ private: unittest { import std.stdio : stderr; - import std.format : format; import dscanner.analysis.config : StaticAnalysisConfig, Check, disabledConfig; - import dscanner.analysis.helpers : assertAnalyzerWarnings; + import dscanner.analysis.helpers : assertAnalyzerWarnings = assertAnalyzerWarningsDMD; StaticAnalysisConfig sac = disabledConfig(); sac.imports_sortedness = Check.enabled; @@ -113,42 +123,30 @@ unittest assertAnalyzerWarnings(q{ import foo.bar; - import bar.foo; // [warn]: %s - }c.format( - ImportSortednessCheck.MESSAGE, - ), sac); + import bar.foo; // [warn]: The imports are not sorted in alphabetical order + }c, sac); assertAnalyzerWarnings(q{ import c; import c.b; - import c.a; // [warn]: %s + import c.a; // [warn]: The imports are not sorted in alphabetical order import d.a; - import d; // [warn]: %s - }c.format( - ImportSortednessCheck.MESSAGE, - ImportSortednessCheck.MESSAGE, - ), sac); + import d; // [warn]: The imports are not sorted in alphabetical order + }c, 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 - }c.format( - ImportSortednessCheck.MESSAGE, - ImportSortednessCheck.MESSAGE, - ImportSortednessCheck.MESSAGE, - ), sac); + import a.b, a.d, a.c; // [warn]: The imports are not sorted in alphabetical order + import a.c, a.b, a.c; // [warn]: The imports are not sorted in alphabetical order + import foo.bar, bar.foo; // [warn]: The imports are not sorted in alphabetical order + }c, sac); // multiple items out of order assertAnalyzerWarnings(q{ import foo.bar; - import bar.foo; // [warn]: %s - import bar.bar.foo; // [warn]: %s - }c.format( - ImportSortednessCheck.MESSAGE, - ImportSortednessCheck.MESSAGE, - ), sac); + import bar.foo; // [warn]: The imports are not sorted in alphabetical order + import bar.bar.foo; // [warn]: The imports are not sorted in alphabetical order + }c, sac); assertAnalyzerWarnings(q{ import test : bar; @@ -158,38 +156,28 @@ unittest // selective imports assertAnalyzerWarnings(q{ import test : foo; - import test : bar; // [warn]: %s - }c.format( - ImportSortednessCheck.MESSAGE, - ), sac); + import test : bar; // [warn]: The imports are not sorted in alphabetical order + }c, sac); // selective imports assertAnalyzerWarnings(q{ - import test : foo, bar; // [warn]: %s - }c.format( - ImportSortednessCheck.MESSAGE, - ), sac); + import test : foo, bar; // [warn]: The imports are not sorted in alphabetical order + }c, sac); assertAnalyzerWarnings(q{ import b; import c : foo; - import c : bar; // [warn]: %s - import a; // [warn]: %s - }c.format( - ImportSortednessCheck.MESSAGE, - ImportSortednessCheck.MESSAGE, - ), sac); + import c : bar; // [warn]: The imports are not sorted in alphabetical order + import a; // [warn]: The imports are not sorted in alphabetical order + }c, sac); assertAnalyzerWarnings(q{ import c; import c : bar; import d : bar; - import d; // [warn]: %s - import a : bar; // [warn]: %s - }c.format( - ImportSortednessCheck.MESSAGE, - ImportSortednessCheck.MESSAGE, - ), sac); + import d; // [warn]: The imports are not sorted in alphabetical order + import a : bar; // [warn]: The imports are not sorted in alphabetical order + }c, sac); assertAnalyzerWarnings(q{ import t0; @@ -199,21 +187,18 @@ unittest assertAnalyzerWarnings(q{ import t1 : a, b = foo; - import t1 : b, a = foo; // [warn]: %s - import t0 : a, b = foo; // [warn]: %s - }c.format( - ImportSortednessCheck.MESSAGE, - ImportSortednessCheck.MESSAGE, - ), sac); + import t1 : b, a = foo; // [warn]: The imports are not sorted in alphabetical order + import t0 : a, b = foo; // [warn]: The imports are not sorted in alphabetical order + }c, sac); // local imports in functions assertAnalyzerWarnings(q{ import t2; - import t1; // [warn]: %s + import t1; // [warn]: The imports are not sorted in alphabetical order void foo() { import f2; - import f1; // [warn]: %s + import f1; // [warn]: The imports are not sorted in alphabetical order import f3; } void bar() @@ -221,23 +206,20 @@ unittest import f1; import f2; } - }c.format( - ImportSortednessCheck.MESSAGE, - ImportSortednessCheck.MESSAGE, - ), sac); + }c, sac); // local imports in scopes assertAnalyzerWarnings(q{ import t2; - import t1; // [warn]: %s + import t1; // [warn]: The imports are not sorted in alphabetical order void foo() { import f2; - import f1; // [warn]: %s + import f1; // [warn]: The imports are not sorted in alphabetical order import f3; { import f2; - import f1; // [warn]: %s + import f1; // [warn]: The imports are not sorted in alphabetical order import f3; } { @@ -246,24 +228,20 @@ unittest import f3; } } - }c.format( - ImportSortednessCheck.MESSAGE, - ImportSortednessCheck.MESSAGE, - ImportSortednessCheck.MESSAGE, - ), sac); + }c, sac); // local imports in functions assertAnalyzerWarnings(q{ import t2; - import t1; // [warn]: %s + import t1; // [warn]: The imports are not sorted in alphabetical order void foo() { import f2; - import f1; // [warn]: %s + import f1; // [warn]: The imports are not sorted in alphabetical order import f3; while (true) { import f2; - import f1; // [warn]: %s + import f1; // [warn]: The imports are not sorted in alphabetical order import f3; } for (;;) { @@ -273,58 +251,47 @@ unittest } foreach (el; arr) { import f2; - import f1; // [warn]: %s + import f1; // [warn]: The imports are not sorted in alphabetical order import f3; } } - }c.format( - ImportSortednessCheck.MESSAGE, - ImportSortednessCheck.MESSAGE, - ImportSortednessCheck.MESSAGE, - ImportSortednessCheck.MESSAGE, - ), sac); + }c, sac); // nested scopes assertAnalyzerWarnings(q{ import t2; - import t1; // [warn]: %s + import t1; // [warn]: The imports are not sorted in alphabetical order void foo() { import f2; - import f1; // [warn]: %s + import f1; // [warn]: The imports are not sorted in alphabetical order import f3; { import f2; - import f1; // [warn]: %s + import f1; // [warn]: The imports are not sorted in alphabetical order import f3; { import f2; - import f1; // [warn]: %s + import f1; // [warn]: The imports are not sorted in alphabetical order import f3; { import f2; - import f1; // [warn]: %s + import f1; // [warn]: The imports are not sorted in alphabetical order import f3; } } } } - }c.format( - ImportSortednessCheck.MESSAGE, - ImportSortednessCheck.MESSAGE, - ImportSortednessCheck.MESSAGE, - ImportSortednessCheck.MESSAGE, - ImportSortednessCheck.MESSAGE, - ), sac); + }c, sac); // local imports in functions assertAnalyzerWarnings(q{ import t2; - import t1; // [warn]: %s + import t1; // [warn]: The imports are not sorted in alphabetical order struct foo() { import f2; - import f1; // [warn]: %s + import f1; // [warn]: The imports are not sorted in alphabetical order import f3; } class bar() @@ -332,10 +299,7 @@ unittest import f1; import f2; } - }c.format( - ImportSortednessCheck.MESSAGE, - ImportSortednessCheck.MESSAGE, - ), sac); + }c, sac); // issue 422 - sorted imports with : assertAnalyzerWarnings(q{ @@ -384,4 +348,4 @@ unittest }, sac); stderr.writeln("Unittest for ImportSortednessCheck passed."); -} +} \ No newline at end of file diff --git a/src/dscanner/analysis/incorrect_infinite_range.d b/src/dscanner/analysis/incorrect_infinite_range.d index 055f05bc..a9e312dd 100644 --- a/src/dscanner/analysis/incorrect_infinite_range.d +++ b/src/dscanner/analysis/incorrect_infinite_range.d @@ -13,85 +13,85 @@ import dparse.lexer; /** * Checks for incorrect infinite range definitions */ -final class IncorrectInfiniteRangeCheck : BaseAnalyzer +extern(C++) class IncorrectInfiniteRangeCheck(AST) : BaseAnalyzerDmd { - alias visit = BaseAnalyzer.visit; + // alias visit = BaseAnalyzerDmd!AST.visit; + alias visit = BaseAnalyzerDmd.visit; mixin AnalyzerInfo!"incorrect_infinite_range_check"; /// - this(string fileName, bool skipTests = false) + extern(D) this(string fileName) { - super(fileName, null, skipTests); + super(fileName); } - override void visit(const StructBody structBody) + override void visit(AST.StructDeclaration sd) { - inStruct++; - structBody.accept(this); - inStruct--; + inAggregate++; + super.visit(sd); + inAggregate--; } - override void visit(const FunctionDeclaration fd) + override void visit(AST.ClassDeclaration cd) { - if (inStruct > 0 && fd.name.text == "empty") - { - line = fd.name.line; - column = fd.name.column; - fd.accept(this); - } + inAggregate++; + super.visit(cd); + inAggregate--; } - override void visit(const FunctionBody fb) + override void visit(AST.FuncDeclaration fd) { - if (fb.specifiedFunctionBody && fb.specifiedFunctionBody.blockStatement !is null) - visit(fb.specifiedFunctionBody.blockStatement); - else if (fb.shortenedFunctionBody && fb.shortenedFunctionBody.expression !is null) - visitReturnExpression(fb.shortenedFunctionBody.expression); - } + import dmd.astenums : Tbool; - override void visit(const BlockStatement bs) - { - if (bs.declarationsAndStatements is null) + if (!inAggregate) return; - if (bs.declarationsAndStatements.declarationsAndStatements is null) - return; - if (bs.declarationsAndStatements.declarationsAndStatements.length != 1) - return; - visit(bs.declarationsAndStatements); - } - override void visit(const ReturnStatement rs) - { - if (inStruct == 0 || line == size_t.max) // not within a struct yet + if (!fd.ident || fd.ident.toString() != "empty") return; - visitReturnExpression(rs.expression); - } - void visitReturnExpression(const Expression expression) - { - if (!expression || expression.items.length != 1) - return; - UnaryExpression unary = cast(UnaryExpression) expression.items[0]; - if (unary is null) - return; - if (unary.primaryExpression is null) + AST.TypeFunction tf = fd.type.isTypeFunction(); + + if (!tf || !tf.next || !tf.next.ty) return; - if (unary.primaryExpression.primary != tok!"false") + + AST.ReturnStatement rs = fd.fbody ? fd.fbody.isReturnStatement() : null; + + if (rs) + { + AST.IntegerExp ie = cast(AST.IntegerExp) rs.exp; + + if (ie && ie.getInteger() == 0) + addErrorMessage(cast(ulong) fd.loc.linnum, cast(ulong) fd.loc.charnum, KEY, + "Use `enum bool empty = false;` to define an infinite range."); + } + + AST.CompoundStatement cs = fd.fbody ? fd.fbody.isCompoundStatement() : null; + + if (!cs || (*cs.statements).length == 0) return; - addErrorMessage(line, column, KEY, MESSAGE); + + if (auto rs1 = (*cs.statements)[0].isReturnStatement()) + { + AST.IntegerExp ie = cast(AST.IntegerExp) rs1.exp; + + if (ie && ie.getInteger() == 0) + addErrorMessage(cast(ulong) fd.loc.linnum, cast(ulong) fd.loc.charnum, KEY, + "Use `enum bool empty = false;` to define an infinite range."); + } + + super.visit(fd); } - override void visit(const Unittest u) + override void visit(AST.UnitTestDeclaration ud) { + } private: - uint inStruct; + uint inAggregate; 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; } unittest @@ -102,9 +102,9 @@ unittest StaticAnalysisConfig sac = disabledConfig(); sac.incorrect_infinite_range_check = Check.enabled; - assertAnalyzerWarnings(q{struct InfiniteRange + assertAnalyzerWarningsDMD(q{struct InfiniteRange { - bool empty() // [warn]: %1$s + bool empty() // [warn]: Use `enum bool empty = false;` to define an infinite range. { return false; } @@ -128,7 +128,7 @@ unittest struct InfiniteRange { - bool empty() => false; // [warn]: %1$s + bool empty() => false; // [warn]: Use `enum bool empty = false;` to define an infinite range. bool stuff() => false; unittest { @@ -143,10 +143,9 @@ struct InfiniteRange } bool empty() { return false; } -class C { bool empty() { return false; } } // [warn]: %1$s +class C { bool empty() { return false; } } // [warn]: Use `enum bool empty = false;` to define an infinite range. -}c - .format(IncorrectInfiniteRangeCheck.MESSAGE), sac); +}c, sac); } // test for https://github.com/dlang-community/D-Scanner/issues/656 @@ -167,7 +166,7 @@ unittest StaticAnalysisConfig sac = disabledConfig(); sac.incorrect_infinite_range_check = Check.enabled; - assertAnalyzerWarnings(q{ + assertAnalyzerWarningsDMD(q{ enum isAllZeroBits = () { if (true) @@ -177,4 +176,4 @@ unittest }(); }, sac); stderr.writeln("Unittest for IncorrectInfiniteRangeCheck passed."); -} +} \ No newline at end of file diff --git a/src/dscanner/analysis/length_subtraction.d b/src/dscanner/analysis/length_subtraction.d index a788ec76..04dd8d66 100644 --- a/src/dscanner/analysis/length_subtraction.d +++ b/src/dscanner/analysis/length_subtraction.d @@ -7,8 +7,6 @@ module dscanner.analysis.length_subtraction; import std.stdio; -import dparse.ast; -import dparse.lexer; import dscanner.analysis.base; import dscanner.analysis.helpers; import dsymbol.scope_; @@ -16,46 +14,33 @@ import dsymbol.scope_; /** * Checks for subtraction from a .length property. This is usually a bug. */ -final class LengthSubtractionCheck : BaseAnalyzer +extern(C++) class LengthSubtractionCheck(AST) : BaseAnalyzerDmd { - alias visit = BaseAnalyzer.visit; + // alias visit = BaseAnalyzerDmd!AST.visit; + alias visit = BaseAnalyzerDmd.visit; mixin AnalyzerInfo!"length_subtraction_check"; - this(string fileName, const(Scope)* sc, bool skipTests = false) + extern(D) this(string fileName) { - super(fileName, sc, skipTests); + super(fileName); } - override void visit(const AddExpression addExpression) + override void visit(AST.BinExp be) { - if (addExpression.operator == tok!"-") + import dmd.tokens : EXP; + + if (auto de = be.e1.isDotIdExp()) { - 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", - "Avoid subtracting from '.length' as it may be unsigned."); + if (be.op == EXP.min && de.ident.toString() == "length") + addErrorMessage(cast(ulong) de.loc.linnum, cast(ulong) de.loc.charnum + 1, KEY, + "Avoid subtracting from '.length' as it may be unsigned."); } - end: - addExpression.accept(this); + + super.visit(be); } + + private enum KEY = "dscanner.suspicious.length_subtraction"; } unittest @@ -64,7 +49,7 @@ unittest StaticAnalysisConfig sac = disabledConfig(); sac.length_subtraction_check = Check.enabled; - assertAnalyzerWarnings(q{ + assertAnalyzerWarningsDMD(q{ void testSizeT() { if (i < a.length - 1) // [warn]: Avoid subtracting from '.length' as it may be unsigned. diff --git a/src/dscanner/analysis/local_imports.d b/src/dscanner/analysis/local_imports.d index 4489ce7d..6a040005 100644 --- a/src/dscanner/analysis/local_imports.d +++ b/src/dscanner/analysis/local_imports.d @@ -5,99 +5,101 @@ module dscanner.analysis.local_imports; -import std.stdio; -import dparse.ast; -import dparse.lexer; import dscanner.analysis.base; import dscanner.analysis.helpers; -import dsymbol.scope_; + +import std.stdio : writeln; /** * Checks for local imports that import all symbols. * See_also: $(LINK https://issues.dlang.org/show_bug.cgi?id=10378) */ -final class LocalImportCheck : BaseAnalyzer +extern(C++) class LocalImportCheck(AST) : BaseAnalyzerDmd { - alias visit = BaseAnalyzer.visit; - mixin AnalyzerInfo!"local_import_check"; + alias visit = BaseAnalyzerDmd.visit; - /** - * Construct with the given file name. - */ - this(string fileName, const(Scope)* sc, bool skipTests = false) + mixin ScopedVisit!(AST.FuncDeclaration); + mixin ScopedVisit!(AST.IfStatement); + mixin ScopedVisit!(AST.WhileStatement); + mixin ScopedVisit!(AST.ForStatement); + mixin ScopedVisit!(AST.ForeachStatement); + mixin ScopedVisit!(AST.ClassDeclaration); + mixin ScopedVisit!(AST.StructDeclaration); + + extern(D) this(string fileName) { - super(fileName, sc, skipTests); + super(fileName); + this.localImport = false; } - mixin visitThing!StructBody; - mixin visitThing!BlockStatement; - - override void visit(const Declaration dec) + override void visit(AST.Import i) { - if (dec.importDeclaration is null) + // Look for import foo.bar : x or foo.bar : y = x + if (!i.isstatic && localImport && i.names.length == 0 && !i.aliasId) { - dec.accept(this); - return; + addErrorMessage(cast(ulong) i.loc.linnum, cast(ulong) i.loc.charnum, KEY, MESSAGE); } - foreach (attr; dec.attributes) - { - if (attr.attribute == tok!"static") - isStatic = true; - } - dec.accept(this); - isStatic = false; } - override void visit(const ImportDeclaration id) + // Skip unittests for now + override void visit(AST.UnitTestDeclaration ud) { - if ((!isStatic && interesting) && (id.importBindings is null - || id.importBindings.importBinds.length == 0)) - { - foreach (singleImport; id.singleImports) - { - if (singleImport.rename.text.length == 0) - { - addErrorMessage(singleImport.identifierChain.identifiers[0].line, - singleImport.identifierChain.identifiers[0].column, - "dscanner.suspicious.local_imports", "Local imports should specify" - ~ " the symbols being imported to avoid hiding local symbols."); - } - } - } + return; } private: - - mixin template visitThing(T) + template ScopedVisit(NodeType) { - override void visit(const T thing) + override void visit(NodeType n) { - const b = interesting; - interesting = true; - thing.accept(this); - interesting = b; + bool prevState = localImport; + localImport = true; + super.visit(n); + localImport = prevState; } } - bool interesting; - bool isStatic; + bool localImport; + enum KEY = "dscanner.suspicious.local_imports"; + enum MESSAGE = "Local imports should specify the symbols being imported to avoid hiding local symbols."; } unittest { import dscanner.analysis.config : StaticAnalysisConfig, Check, disabledConfig; + import std.stdio : stderr; StaticAnalysisConfig sac = disabledConfig(); sac.local_import_check = Check.enabled; - assertAnalyzerWarnings(q{ - void testLocalImport() + + assertAnalyzerWarningsDMD(q{ + import std.experimental; + + void foo() { 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; + + if (1) + { + import foo.bar; // [warn]: Local imports should specify the symbols being imported to avoid hiding local symbols. + } + else + { + import foo.bar; // [warn]: Local imports should specify the symbols being imported to avoid hiding local symbols. + } + + foreach (i; [1, 2, 3]) + { + import foo.bar; // [warn]: Local imports should specify the symbols being imported to avoid hiding local symbols. + import std.stdio : writeln; + } } + + import std.experimental.dragon; }c, sac); stderr.writeln("Unittest for LocalImportCheck passed."); -} +} \ No newline at end of file diff --git a/src/dscanner/analysis/logic_precedence.d b/src/dscanner/analysis/logic_precedence.d index 35d36338..c7f17c1e 100644 --- a/src/dscanner/analysis/logic_precedence.d +++ b/src/dscanner/analysis/logic_precedence.d @@ -5,12 +5,8 @@ module dscanner.analysis.logic_precedence; -import std.stdio; -import dparse.ast; -import dparse.lexer; import dscanner.analysis.base; import dscanner.analysis.helpers; -import dsymbol.scope_; /** * Checks for code with confusing && and || operator precedence @@ -19,41 +15,57 @@ import dsymbol.scope_; * if (a && (b || c)) // good * --- */ -final class LogicPrecedenceCheck : BaseAnalyzer +extern(C++) class LogicPrecedenceCheck(AST) : BaseAnalyzerDmd { - alias visit = BaseAnalyzer.visit; - enum string KEY = "dscanner.confusing.logical_precedence"; mixin AnalyzerInfo!"logical_precedence_check"; + alias visit = BaseAnalyzerDmd.visit; - this(string fileName, const(Scope)* sc, bool skipTests = false) + extern(D) this(string fileName, bool skipTests = false) { - super(fileName, sc, skipTests); + super(fileName, skipTests); } - override void visit(const OrOrExpression orOr) + override void visit(AST.LogicalExp le) { - if (orOr.left is null || orOr.right is null) - return; - const AndAndExpression left = cast(AndAndExpression) orOr.left; - const AndAndExpression right = cast(AndAndExpression) orOr.right; - if (left is null && right is null) - return; - if ((left !is null && left.right is null) && (right !is null && right.right is null)) - return; - addErrorMessage(orOr.line, orOr.column, KEY, + import dmd.tokens : EXP; + + auto left = le.e1.isLogicalExp(); + auto right = le.e2.isLogicalExp(); + + if (left) + left = left.op == EXP.andAnd ? left : null; + if (right) + right = right.op == EXP.andAnd ? right : null; + + if (le.op != EXP.orOr) + goto END; + + if (!left && !right) + goto END; + + if ((left && left.parens) || (right && right.parens)) + goto END; + + if ((left !is null && left.e2 is null) && (right !is null && right.e2 is null)) + goto END; + + addErrorMessage(cast(ulong) le.loc.linnum, cast(ulong) le.loc.charnum, KEY, "Use parenthesis to clarify this expression."); - orOr.accept(this); + +END: + super.visit(le); } } unittest { import dscanner.analysis.config : StaticAnalysisConfig, Check, disabledConfig; + import std.stdio : stderr; StaticAnalysisConfig sac = disabledConfig(); sac.logical_precedence_check = Check.enabled; - assertAnalyzerWarnings(q{ + assertAnalyzerWarningsDMD(q{ void testFish() { if (a && b || c) {} // [warn]: Use parenthesis to clarify this expression. @@ -63,4 +75,4 @@ unittest } }c, sac); stderr.writeln("Unittest for LogicPrecedenceCheck passed."); -} +} \ No newline at end of file diff --git a/src/dscanner/analysis/objectconst.d b/src/dscanner/analysis/objectconst.d index 7de61824..cc865e6d 100644 --- a/src/dscanner/analysis/objectconst.d +++ b/src/dscanner/analysis/objectconst.d @@ -5,100 +5,107 @@ module dscanner.analysis.objectconst; -import std.stdio; -import std.regex; -import dparse.ast; -import dparse.lexer; import dscanner.analysis.base; import dscanner.analysis.helpers; -import dsymbol.scope_ : Scope; +import std.stdio; -/** - * Checks that opEquals, opCmp, toHash, 'opCast', and toString are either const, - * immutable, or inout. - */ -final class ObjectConstCheck : BaseAnalyzer +extern(C++) class ObjectConstCheck(AST) : BaseAnalyzerDmd { - alias visit = BaseAnalyzer.visit; - mixin AnalyzerInfo!"object_const_check"; + alias visit = BaseAnalyzerDmd.visit; - /// - this(string fileName, const(Scope)* sc, bool skipTests = false) + extern(D) this(string fileName) { - super(fileName, sc, skipTests); + super(fileName); } - mixin visitTemplate!ClassDeclaration; - mixin visitTemplate!InterfaceDeclaration; - mixin visitTemplate!UnionDeclaration; - mixin visitTemplate!StructDeclaration; - - override void visit(const AttributeDeclaration d) + void visitAggregate(AST.AggregateDeclaration ad) { - if (d.attribute.attribute == tok!"const" && inAggregate) - { - constColon = true; - } - d.accept(this); - } + import dmd.astenums : MODFlags, STC; - override void visit(const Declaration d) - { - import std.algorithm : any; - bool setConstBlock; - if (inAggregate && d.attributes && d.attributes.any!(a => a.attribute == tok!"const")) - { - setConstBlock = true; - constBlock = true; - } + if (!ad.members) + return; - bool containsDisable(A)(const A[] attribs) + foreach(member; *ad.members) { - import std.algorithm.searching : canFind; - return attribs.canFind!(a => a.atAttribute !is null && - a.atAttribute.identifier.text == "disable"); - } - - if (const FunctionDeclaration fd = d.functionDeclaration) - { - const isDeclationDisabled = containsDisable(d.attributes) || - containsDisable(fd.memberFunctionAttributes); - - if (inAggregate && !constColon && !constBlock && !isDeclationDisabled - && isInteresting(fd.name.text) && !hasConst(fd.memberFunctionAttributes)) + if (auto fd = member.isFuncDeclaration()) { - addErrorMessage(d.functionDeclaration.name.line, - d.functionDeclaration.name.column, "dscanner.suspicious.object_const", - "Methods 'opCmp', 'toHash', 'opEquals', 'opCast', and/or 'toString' are non-const."); + if (isInteresting(fd.ident.toString()) && !isConstFunc(fd) && + !(fd.storage_class & STC.disable)) + addErrorMessage(cast(ulong) fd.loc.linnum, cast(ulong) fd.loc.charnum, KEY, + "Methods 'opCmp', 'toHash', 'opEquals', 'opCast', and/or 'toString' are non-const."); + + member.accept(this); } + else if (auto scd = member.isStorageClassDeclaration()) + { + foreach (smember; *scd.decl) + { + if (auto fd2 = smember.isFuncDeclaration()) + { + if (isInteresting(fd2.ident.toString()) && !isConstFunc(fd2, scd) && + !(fd2.storage_class & STC.disable)) + addErrorMessage(cast(ulong) fd2.loc.linnum, cast(ulong) fd2.loc.charnum, KEY, + "Methods 'opCmp', 'toHash', 'opEquals', 'opCast', and/or 'toString' are non-const."); + + smember.accept(this); + } + else + smember.accept(this); + } + } + else + member.accept(this); } + } - d.accept(this); + override void visit(AST.ClassDeclaration cd) + { + visitAggregate(cd); + } - if (!inAggregate) - constColon = false; - if (setConstBlock) - constBlock = false; + override void visit(AST.StructDeclaration sd) + { + visitAggregate(sd); } - private static bool hasConst(const MemberFunctionAttribute[] attributes) + override void visit(AST.InterfaceDeclaration id) { - import std.algorithm : any; + visitAggregate(id); + } - return attributes.any!(a => a.tokenType == tok!"const" - || a.tokenType == tok!"immutable" || a.tokenType == tok!"inout"); + override void visit(AST.UnionDeclaration ud) + { + visitAggregate(ud); } - private static bool isInteresting(string name) + extern(D) private static bool isInteresting(const char[] name) { return name == "opCmp" || name == "toHash" || name == "opEquals" || name == "toString" || name == "opCast"; } - private bool constBlock; - private bool constColon; + /** + * Checks if a function has either one of attributes `const`, `immutable`, `inout` + */ + private bool isConstFunc(AST.FuncDeclaration fd, AST.StorageClassDeclaration scd = null) + { + import dmd.astenums : MODFlags, STC; + import std.stdio : writeln; + + if (scd && (scd.stc & STC.const_ || scd.stc & STC.immutable_ || scd.stc & STC.wild)) + return true; + + if(fd.type && (fd.type.mod == MODFlags.const_ || + fd.type.mod == MODFlags.immutable_ || fd.type.mod == MODFlags.wild)) + return true; + + return false; + } + private enum KEY = "dscanner.suspicious.object_const"; + + AST.AggregateDeclaration deleteme; } unittest @@ -107,7 +114,7 @@ unittest StaticAnalysisConfig sac = disabledConfig(); sac.object_const_check = Check.enabled; - assertAnalyzerWarnings(q{ + assertAnalyzerWarningsDMD(q{ void testConsts() { // Will be ok because all are declared const/immutable @@ -123,7 +130,7 @@ unittest return 1; } - const hash_t toHash() // ok + immutable hash_t toHash() // ok { return 0; } @@ -141,7 +148,7 @@ unittest class Fox { - const{ override string toString() { return "foo"; }} // ok + inout { override string toString() { return "foo"; } } // ok } class Rat diff --git a/src/dscanner/analysis/opequals_without_tohash.d b/src/dscanner/analysis/opequals_without_tohash.d index f3b854d9..292bf730 100644 --- a/src/dscanner/analysis/opequals_without_tohash.d +++ b/src/dscanner/analysis/opequals_without_tohash.d @@ -5,104 +5,101 @@ 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; /** * Checks for when a class/struct has the method opEquals without toHash, or * toHash without opEquals. */ -final class OpEqualsWithoutToHashCheck : BaseAnalyzer +extern(C++) class OpEqualsWithoutToHashCheck(AST) : BaseAnalyzerDmd { - alias visit = BaseAnalyzer.visit; - mixin AnalyzerInfo!"opequals_tohash_check"; + alias visit = BaseAnalyzerDmd.visit; - this(string fileName, const(Scope)* sc, bool skipTests = false) + extern(D) this(string fileName, bool skipTests = false) { - super(fileName, sc, skipTests); + super(fileName, skipTests); } - override void visit(const ClassDeclaration node) + override void visit(AST.ClassDeclaration cd) { - actualCheck(node.name, node.structBody); - node.accept(this); + visitBaseClasses(cd); + visitAggregate(cd); } - override void visit(const StructDeclaration node) + override void visit(AST.StructDeclaration sd) { - actualCheck(node.name, node.structBody); - node.accept(this); + visitAggregate(sd); } - private void actualCheck(const Token name, const StructBody structBody) + private void isInteresting(AST.FuncDeclaration fd, ref bool hasOpEquals, ref bool hasToHash) { - bool hasOpEquals; - bool hasToHash; - bool hasOpCmp; + import dmd.astenums : STC; - // Just return if missing children - if (!structBody || !structBody.declarations || name is Token.init) - return; + if (!(fd.storage_class & STC.disable) && fd.ident.toString() == "opEquals") + hasOpEquals = true; - // Check all the function declarations - foreach (declaration; structBody.declarations) - { - // Skip if not a function declaration - if (!declaration || !declaration.functionDeclaration) - continue; + if (!(fd.storage_class & STC.disable) && fd.ident.toString() == "toHash") + hasToHash = true; + } - bool containsDisable(A)(const A[] attribs) + private void visitAggregate(AST.AggregateDeclaration ad) + { + bool hasOpEquals, hasToHash; + + if (!ad.members) + return; + + foreach(member; *ad.members) + { + if (auto fd = member.isFuncDeclaration()) { - import std.algorithm.searching : canFind; - return attribs.canFind!(a => a.atAttribute !is null && - a.atAttribute.identifier.text == "disable"); + isInteresting(fd, hasOpEquals, hasToHash); + member.accept(this); } - - const isDeclationDisabled = containsDisable(declaration.attributes) || - containsDisable(declaration.functionDeclaration.memberFunctionAttributes); - - if (isDeclationDisabled) - continue; - - // Check if opEquals or toHash - immutable string methodName = declaration.functionDeclaration.name.text; - if (methodName == "opEquals") - hasOpEquals = true; - else if (methodName == "toHash") - hasToHash = true; - else if (methodName == "opCmp") - hasOpCmp = true; + else if (auto scd = member.isStorageClassDeclaration()) + { + foreach (smember; *scd.decl) + { + if (auto fd2 = smember.isFuncDeclaration()) + { + isInteresting(fd2, hasOpEquals, hasToHash); + smember.accept(this); + } + else + smember.accept(this); + } + } + else + member.accept(this); } - // 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); + string message = ad.ident.toString().dup; + message = "'" ~ message ~ "' has method 'opEquals', but not 'toHash'."; + addErrorMessage(cast(ulong) ad.loc.linnum, cast(ulong) ad.loc.charnum, KEY, message); } - // 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); + string message = ad.ident.toString().dup; + message = "'" ~ message ~ "' has method 'toHash', but not 'opEquals'."; + addErrorMessage(cast(ulong) ad.loc.linnum, cast(ulong) ad.loc.charnum, KEY, message); } } - enum string KEY = "dscanner.suspicious.incomplete_operator_overloading"; + private enum KEY = "dscanner.suspicious.incomplete_operator_overloading"; } unittest { import dscanner.analysis.config : StaticAnalysisConfig, Check, disabledConfig; + import std.stdio : stderr; StaticAnalysisConfig sac = disabledConfig(); sac.opequals_tohash_check = Check.enabled; - assertAnalyzerWarnings(q{ + assertAnalyzerWarningsDMD(q{ // Success because it has opEquals and toHash class Chimp { @@ -171,4 +168,4 @@ unittest }c, sac); stderr.writeln("Unittest for OpEqualsWithoutToHashCheck passed."); -} +} \ No newline at end of file diff --git a/src/dscanner/analysis/pokemon.d b/src/dscanner/analysis/pokemon.d index c3dbb114..a588379b 100644 --- a/src/dscanner/analysis/pokemon.d +++ b/src/dscanner/analysis/pokemon.d @@ -6,11 +6,8 @@ module dscanner.analysis.pokemon; import std.stdio; -import dparse.ast; -import dparse.lexer; import dscanner.analysis.base; import dscanner.analysis.helpers; -import dsymbol.scope_ : Scope; /** * Checks for Pokémon exception handling, i.e. "gotta' catch 'em all". @@ -23,64 +20,27 @@ import dsymbol.scope_ : Scope; * } * --- */ -final class PokemonExceptionCheck : BaseAnalyzer +extern(C++) class PokemonExceptionCheck(AST) : BaseAnalyzerDmd { - enum MESSAGE = "Catching Error or Throwable is almost always a bad idea."; - enum string KEY = "dscanner.suspicious.catch_em_all"; mixin AnalyzerInfo!"exception_check"; + alias visit = BaseAnalyzerDmd.visit; - alias visit = BaseAnalyzer.visit; - - this(string fileName, const(Scope)* sc, bool skipTests = false) + extern(D) this(string fileName, bool skipTests = false) { - super(fileName, sc, skipTests); + super(fileName, skipTests); } - override void visit(const LastCatch lc) + override void visit(AST.Catch c) { - addErrorMessage(lc.line, lc.column, KEY, MESSAGE); - lc.accept(this); + if (c.type.isTypeIdentifier().ident.toString() == "Error" || + c.type.isTypeIdentifier().ident.toString() == "Throwable") + addErrorMessage(cast(ulong) c.loc.linnum, cast(ulong) c.loc.charnum, + KEY, MESSAGE); } - bool ignoreType = true; - - override void visit(const Catch c) - { - ignoreType = false; - c.type.accept(this); - ignoreType = true; - - c.accept(this); - } - - override void visit(const Type2 type2) - { - if (ignoreType) - return; - - if (type2.type !is null) - { - type2.type.accept(this); - return; - } - - if (type2.typeIdentifierPart.typeIdentifierPart !is null) - { - return; - } - const identOrTemplate = type2.typeIdentifierPart.identifierOrTemplateInstance; - if (identOrTemplate.templateInstance !is null) - { - return; - } - if (identOrTemplate.identifier.text == "Throwable" - || identOrTemplate.identifier.text == "Error") - { - immutable column = identOrTemplate.identifier.column; - immutable line = identOrTemplate.identifier.line; - addErrorMessage(line, column, KEY, MESSAGE); - } - } +private: + enum MESSAGE = "Catching Error or Throwable is almost always a bad idea."; + enum string KEY = "dscanner.suspicious.catch_em_all"; } unittest @@ -89,7 +49,7 @@ unittest StaticAnalysisConfig sac = disabledConfig(); sac.exception_check = Check.enabled; - assertAnalyzerWarnings(q{ + assertAnalyzerWarningsDMD(q{ void testCatch() { try @@ -119,13 +79,9 @@ unittest 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. - { - } } }c, sac); stderr.writeln("Unittest for PokemonExceptionCheck passed."); -} +} \ No newline at end of file diff --git a/src/dscanner/analysis/properly_documented_public_functions.d b/src/dscanner/analysis/properly_documented_public_functions.d index 3f7b3c90..ba7a9894 100644 --- a/src/dscanner/analysis/properly_documented_public_functions.d +++ b/src/dscanner/analysis/properly_documented_public_functions.d @@ -4,15 +4,12 @@ module dscanner.analysis.properly_documented_public_functions; -import dparse.lexer; -import dparse.ast; -import dparse.formatter : astFmt = format; import dscanner.analysis.base; -import dscanner.utils : safeAccess; - import std.format : format; import std.range.primitives; -import std.stdio; +import std.conv : to; +import std.algorithm.searching : canFind, any, find; +import dmd.astcodegen; /** * Requires each public function to contain the following ddoc sections @@ -22,7 +19,7 @@ import std.stdio; - Ddoc params entries without a parameter trigger warnings as well - RETURNS: (except if it's void, only functions) */ -final class ProperlyDocumentedPublicFunctions : BaseAnalyzer +extern(C++) class ProperlyDocumentedPublicFunctions(AST) : BaseAnalyzerDmd { enum string MISSING_PARAMS_KEY = "dscanner.style.doc_missing_params"; enum string MISSING_PARAMS_MESSAGE = "Parameter %s isn't documented in the `Params` section."; @@ -39,257 +36,183 @@ final class ProperlyDocumentedPublicFunctions : BaseAnalyzer enum string MISSING_THROW_MESSAGE = "An instance of `%s` is thrown but not documented in the `Throws` section"; mixin AnalyzerInfo!"properly_documented_public_functions"; + alias visit = BaseAnalyzerDmd.visit; - /// - this(string fileName, bool skipTests = false) + extern(D) this(string fileName, bool skipTests = false) { - super(fileName, null, skipTests); + super(fileName, skipTests); } - override void visit(const Module mod) + override void visit(AST.Module m) { - islastSeenVisibilityLabelPublic = true; - mod.accept(this); + super.visit(m); postCheckSeenDdocParams(); } - override void visit(const UnaryExpression decl) + override void visit(AST.Catch c) { - import std.algorithm.searching : canFind; - - const IdentifierOrTemplateInstance iot = safeAccess(decl) - .functionCallExpression.unaryExpression.primaryExpression - .identifierOrTemplateInstance; + import std.algorithm.iteration : filter; + import std.array : array; - Type newNamedType(N)(N name) - { - Type t = new Type; - t.type2 = new Type2; - t.type2.typeIdentifierPart = new TypeIdentifierPart; - t.type2.typeIdentifierPart.identifierOrTemplateInstance = new IdentifierOrTemplateInstance; - t.type2.typeIdentifierPart.identifierOrTemplateInstance.identifier = name; - return t; - } + thrown = thrown.filter!(a => a != to!string(c.type.toChars())).array; + super.visit(c); + } - if (inThrowExpression && decl.newExpression && decl.newExpression.type && - !thrown.canFind!(a => a == decl.newExpression.type)) - { - thrown ~= decl.newExpression.type; - } - // enforce(condition); - if (iot && iot.identifier.text == "enforce") - { - thrown ~= newNamedType(Token(tok!"identifier", "Exception", 0, 0, 0)); - } - else if (iot && iot.templateInstance && iot.templateInstance.identifier.text == "enforce") - { - // enforce!Type(condition); - if (const TemplateSingleArgument tsa = safeAccess(iot.templateInstance) - .templateArguments.templateSingleArgument) - { - thrown ~= newNamedType(tsa.token); - } - // enforce!(Type)(condition); - else if (const TemplateArgumentList tal = safeAccess(iot.templateInstance) - .templateArguments.templateArgumentList) - { - if (tal.items.length && tal.items[0].type) - thrown ~= tal.items[0].type; - } - } - decl.accept(this); + override void visit(AST.ThrowStatement t) + { + AST.NewExp ne = t.exp.isNewExp(); + if (ne) + thrown ~= to!string(ne.newtype.toChars()); + + super.visit(t); } - override void visit(const Declaration decl) + override void visit(AST.FuncDeclaration d) { - import std.algorithm.searching : any; - import std.algorithm.iteration : map; + nestedFunc++; + scope (exit) + nestedFunc--; - // skip private symbols - enum tokPrivate = tok!"private", - tokProtected = tok!"protected", - tokPackage = tok!"package", - tokPublic = tok!"public"; + import std.stdio : writeln, writefln; + import std.conv : to; + import std.algorithm.searching : canFind, any, find; + import dmd.dsymbol : Visibility; + import dmd.mtype : Type; + import ddoc.comments : parseComment; + import std.algorithm.iteration : map; + import std.array : array; - // Nested funcs for `Throws` - bool decNestedFunc; - if (decl.functionDeclaration) - { - nestedFuncs++; - decNestedFunc = true; - } - scope(exit) + if (d.comment is null || d.fbody is null || d.visibility.kind != Visibility.Kind.public_) { - if (decNestedFunc) - nestedFuncs--; - } - if (nestedFuncs > 1) - { - decl.accept(this); + super.visit(d); return; } - if (decl.attributes.length > 0) + if (nestedFunc == 1) { - const bool isPublic = !decl.attributes.map!`a.attribute`.any!(x => x == tokPrivate || - x == tokProtected || - x == tokPackage); - // recognize label blocks - if (!hasDeclaration(decl)) - islastSeenVisibilityLabelPublic = isPublic; - - if (!isPublic) - return; - } - - if (islastSeenVisibilityLabelPublic || decl.attributes.map!`a.attribute`.any!(x => x == tokPublic)) - { - // 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.classDeclaration.comment.ptr !is null) || - (decl.structDeclaration !is null && decl.structDeclaration.comment.ptr !is null)) - decl.accept(this); - } - } - - override void visit(const TemplateDeclaration decl) - { - setLastDdocParams(decl.name.line, decl.name.column, decl.comment); - checkDdocParams(decl.name.line, decl.name.column, decl.templateParameters); - - withinTemplate = true; - scope(exit) withinTemplate = false; - decl.accept(this); - } + thrown.length = 0; + string[] params; - override void visit(const MixinTemplateDeclaration decl) - { - decl.accept(this); - } + if (d.parameters) foreach (p; *d.parameters) + params ~= to!string(p.ident.toString()); - override void visit(const StructDeclaration decl) - { - setLastDdocParams(decl.name.line, decl.name.column, decl.comment); - checkDdocParams(decl.name.line, decl.name.column, decl.templateParameters); - decl.accept(this); - } + auto comment = setLastDdocParams(d.loc.linnum, d.loc.charnum, to!string(d.comment)); + checkDdocParams(d.loc.linnum, d.loc.charnum, params, null); - override void visit(const ClassDeclaration decl) - { - setLastDdocParams(decl.name.line, decl.name.column, decl.comment); - checkDdocParams(decl.name.line, decl.name.column, decl.templateParameters); - decl.accept(this); + auto tf = d.type.isTypeFunction(); + if (tf && tf.next != Type.tvoid && d.comment + && !comment.isDitto && !comment.sections.any!(s => s.name == "Returns")) + addErrorMessage(cast(ulong) d.loc.linnum, cast(ulong) d.loc.charnum, + MISSING_RETURNS_KEY, MISSING_RETURNS_MESSAGE); + } + + super.visit(d); + if (nestedFunc == 1) + foreach (t; thrown) + if (!hasThrowSection(to!string(d.comment))) + addErrorMessage(cast(ulong) d.loc.linnum, cast(ulong) d.loc.charnum, + MISSING_THROW_KEY, MISSING_THROW_MESSAGE.format(t)); } - override void visit(const FunctionDeclaration decl) + override void visit(AST.TemplateDeclaration d) { - import std.algorithm.searching : all, any; - import std.array : Appender; + import dmd.dsymbol : Visibility; + import ddoc.comments : parseComment; + import std.algorithm.iteration : map, filter; + import std.algorithm.searching : find, canFind; + import std.array : array; - // ignore header declaration for now - if (!decl.functionBody || (!decl.functionBody.specifiedFunctionBody - && !decl.functionBody.shortenedFunctionBody)) + if (d.comment is null) return; - if (nestedFuncs == 1) - thrown.length = 0; - // detect ThrowExpression only if not nothrow - if (!decl.attributes.any!(a => a.attribute.text == "nothrow")) - { - decl.accept(this); - if (nestedFuncs == 1 && !hasThrowSection(decl.comment)) - foreach(t; thrown) - { - Appender!(char[]) app; - astFmt(&app, t); - addErrorMessage(decl.name.line, decl.name.column, MISSING_THROW_KEY, - MISSING_THROW_MESSAGE.format(app.data)); - } - } + // A `template` inside another public `template` declaration will have visibility undefined + // Check that as well as it's part of the public template + if ((d.visibility.kind != Visibility.Kind.public_) + && !(d.visibility.kind == Visibility.Kind.undefined && withinTemplate)) + return; - if (nestedFuncs == 1) + if (d.visibility.kind == Visibility.Kind.public_) { - auto comment = setLastDdocParams(decl.name.line, decl.name.column, decl.comment); - checkDdocParams(decl.name.line, decl.name.column, 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); + setLastDdocParams(d.loc.linnum, d.loc.charnum, to!string(d.comment)); + withinTemplate = true; + funcParams.length = 0; + templateParams.length = 0; } - } - - // remove thrown Type that are caught - override void visit(const TryStatement ts) - { - import std.algorithm.iteration : filter; - import std.algorithm.searching : canFind; - import std.array : array; - ts.accept(this); + foreach (p; *d.origParameters) + if (!canFind(templateParams, to!string(p.ident.toString()))) + templateParams ~= to!string(p.ident.toString()); - if (ts.catches) - thrown = thrown.filter!(a => !ts.catches.catches - .canFind!(b => b.type == a)) - .array; - } + super.visit(d); - override void visit(const ThrowExpression ts) - { - const wasInThrowExpression = inThrowExpression; - inThrowExpression = true; - scope (exit) - inThrowExpression = wasInThrowExpression; - ts.accept(this); - inThrowExpression = false; + if (d.visibility.kind == Visibility.Kind.public_) + { + withinTemplate = false; + checkDdocParams(d.loc.linnum, d.loc.charnum, funcParams, templateParams); + } } - alias visit = BaseAnalyzer.visit; + /** + * Look for: foo(T)(T x) + * In that case, T does not have to be documented, because x must be. + */ + override bool visitEponymousMember(AST.TemplateDeclaration d) + { + import ddoc.comments : parseComment; + import std.algorithm.searching : canFind, any, find; + import std.algorithm.iteration : map, filter; + import std.array : array; -private: - bool islastSeenVisibilityLabelPublic; - bool withinTemplate; - size_t nestedFuncs; + if (!d.members || d.members.length != 1) + return false; + AST.Dsymbol onemember = (*d.members)[0]; + if (onemember.ident != d.ident) + return false; - static struct Function - { - bool active; - size_t line, column; - const(string)[] ddocParams; - bool[string] params; - } - Function lastSeenFun; + if (AST.FuncDeclaration fd = onemember.isFuncDeclaration()) + { + const comment = parseComment(to!string(d.comment), null); + const paramSection = comment.sections.find!(s => s.name == "Params"); + auto tf = fd.type.isTypeFunction(); - bool inThrowExpression; - const(Type)[] thrown; + if (tf) + foreach (idx, p; tf.parameterList) + { - // find invalid ddoc parameters (i.e. they don't occur in a function declaration) - void postCheckSeenDdocParams() - { - import std.format : format; + if (!paramSection.empty && + !canFind(paramSection[0].mapping.map!(a => a[0]).array, to!string(p.ident.toString())) && + !canFind(funcParams, to!string(p.ident.toString()))) + funcParams ~= to!string(p.ident.toString()); - if (lastSeenFun.active) - foreach (p; lastSeenFun.ddocParams) - if (p !in lastSeenFun.params) - addErrorMessage(lastSeenFun.line, lastSeenFun.column, NON_EXISTENT_PARAMS_KEY, - NON_EXISTENT_PARAMS_MESSAGE.format(p)); + lastSeenFun.params[to!string(p.ident.toString())] = true; - lastSeenFun.active = false; - } + auto ti = p.type.isTypeIdentifier(); + if (ti is null) + continue; - bool hasThrowSection(string commentText) - { - import std.algorithm.searching : canFind; - import ddoc.comments : parseComment; - - const comment = parseComment(commentText, null); - return comment.isDitto || comment.sections.canFind!(s => s.name == "Throws"); - } + templateParams = templateParams.filter!(a => a != to!string(ti.ident.toString())).array; + lastSeenFun.params[to!string(ti.ident.toString())] = true; + } + return true; + } + + if (AST.AggregateDeclaration ad = onemember.isAggregateDeclaration()) + return true; + + if (AST.VarDeclaration vd = onemember.isVarDeclaration()) + { + if (d.constraint) + return false; + + if (vd._init) + return true; + } + + return false; + } - auto setLastDdocParams(size_t line, size_t column, string commentText) + extern(D) auto setLastDdocParams(size_t line, size_t column, string commentText) { import ddoc.comments : parseComment; import std.algorithm.searching : find; @@ -324,8 +247,16 @@ private: return comment; } - void checkDdocParams(size_t line, size_t column, const Parameters params, - const TemplateParameters templateParameters = null) + /** + * + * Params: + * line = Line of the public declaration verified + * column = Column of the public declaration verified + * params = Funcion parameters that must be documented + * templateParams = Template parameters that must be documented. + * Can be null if we are looking at a regular FuncDeclaration + */ + extern(D) void checkDdocParams(size_t line, size_t column, string[] params, string[] templateParams) { import std.array : array; import std.algorithm.searching : canFind, countUntil; @@ -333,136 +264,73 @@ private: import std.algorithm.mutation : remove; import std.range : indexed, iota; - // convert templateParameters into a string[] for faster access - const(TemplateParameter)[] templateList; - if (const tp = templateParameters) - if (const tpl = tp.templateParameterList) - templateList = tpl.items; - string[] tlList = templateList.map!(a => templateParamName(a)).array; - - // make a copy of all parameters and remove the seen ones later during the loop - size_t[] unseenTemplates = templateList.length.iota.array; - - if (lastSeenFun.active && params !is null) - foreach (p; params.parameters) + if (lastSeenFun.active && !params.empty) + foreach (p; params) { - string templateName; - - if (auto iot = safeAccess(p).type.type2 - .typeIdentifierPart.identifierOrTemplateInstance.unwrap) - { - templateName = iot.identifier.text; - } - else if (auto iot = safeAccess(p).type.type2.type.type2 - .typeIdentifierPart.identifierOrTemplateInstance.unwrap) - { - templateName = iot.identifier.text; - } - const idx = tlList.countUntil(templateName); - if (idx >= 0) - { - unseenTemplates = unseenTemplates.remove(idx); - tlList = tlList.remove(idx); - // documenting template parameter should be allowed - lastSeenFun.params[templateName] = true; - } - - if (!lastSeenFun.ddocParams.canFind(p.name.text)) + if (!lastSeenFun.ddocParams.canFind(p)) addErrorMessage(line, column, MISSING_PARAMS_KEY, - format(MISSING_PARAMS_MESSAGE, p.name.text)); + format(MISSING_PARAMS_MESSAGE, p)); else - lastSeenFun.params[p.name.text] = true; + lastSeenFun.params[p] = true; } - // now check the remaining, not used template parameters - auto unseenTemplatesArr = templateList.indexed(unseenTemplates).array; - checkDdocParams(line, column, unseenTemplatesArr); + checkDdocParams(line, column, templateParams); } - void checkDdocParams(size_t line, size_t column, const TemplateParameters templateParams) - { - if (lastSeenFun.active && templateParams !is null && - templateParams.templateParameterList !is null) - checkDdocParams(line, column, templateParams.templateParameterList.items); - } - - void checkDdocParams(size_t line, size_t column, const TemplateParameter[] templateParams) + extern(D) void checkDdocParams(size_t line, size_t column, string[] 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)) + if (!lastSeenFun.ddocParams.canFind(p)) addErrorMessage(line, column, MISSING_PARAMS_KEY, - format(MISSING_TEMPLATE_PARAMS_MESSAGE, name)); + format(MISSING_TEMPLATE_PARAMS_MESSAGE, p)); else - lastSeenFun.params[name] = true; + lastSeenFun.params[p] = true; } } - static string templateParamName(const TemplateParameter p) + extern(D) bool hasThrowSection(string commentText) + { + import std.algorithm.searching : canFind; + import ddoc.comments : parseComment; + + const comment = parseComment(commentText, null); + return comment.isDitto || comment.sections.canFind!(s => s.name == "Throws"); + } + + void postCheckSeenDdocParams() { - if (p.templateTypeParameter) - return p.templateTypeParameter.identifier.text; - if (p.templateValueParameter) - return p.templateValueParameter.identifier.text; - if (p.templateAliasParameter) - return p.templateAliasParameter.identifier.text; - if (p.templateTupleParameter) - return p.templateTupleParameter.identifier.text; - if (p.templateThisParameter) - return p.templateThisParameter.templateTypeParameter.identifier.text; - - return null; + import std.format : format; + + if (lastSeenFun.active) + foreach (p; lastSeenFun.ddocParams) + if (p !in lastSeenFun.params) + addErrorMessage(lastSeenFun.line, lastSeenFun.column, NON_EXISTENT_PARAMS_KEY, + NON_EXISTENT_PARAMS_MESSAGE.format(p)); + + lastSeenFun.active = false; } - bool hasDeclaration(const Declaration decl) + private enum KEY = "dscanner.performance.enum_array_literal"; + int nestedFunc; + int withinTemplate; + + extern(D) string[] funcParams; + extern(D) string[] templateParams; + extern(D) string[] thrown; + + static struct Function { - 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", - ); - if (decl.declarations !is null) - return false; - - auto isNull = true; - foreach (property; properties) - if (mixin("decl." ~ property ~ " !is null")) - isNull = false; - - return !isNull; + bool active; + size_t line, column; + // All params documented + const(string)[] ddocParams; + // Stores actual function params that are also documented + bool[string] params; } + Function lastSeenFun; } version(unittest) @@ -470,7 +338,7 @@ version(unittest) import std.stdio : stderr; import std.format : format; import dscanner.analysis.config : StaticAnalysisConfig, Check, disabledConfig; - import dscanner.analysis.helpers : assertAnalyzerWarnings; + import dscanner.analysis.helpers : assertAnalyzerWarnings = assertAnalyzerWarningsDMD; } // missing params @@ -485,8 +353,8 @@ unittest */ void foo(int k){} // [warn]: %s }c.format( - ProperlyDocumentedPublicFunctions.MISSING_PARAMS_MESSAGE.format("k") - ), sac); + (ProperlyDocumentedPublicFunctions!ASTCodegen).MISSING_PARAMS_MESSAGE.format("k") + ), sac, true); assertAnalyzerWarnings(q{ /** @@ -494,8 +362,8 @@ unittest */ void foo(int K)(){} // [warn]: %s }c.format( - ProperlyDocumentedPublicFunctions.MISSING_TEMPLATE_PARAMS_MESSAGE.format("K") - ), sac); + (ProperlyDocumentedPublicFunctions!ASTCodegen).MISSING_TEMPLATE_PARAMS_MESSAGE.format("K") + ), sac, true); assertAnalyzerWarnings(q{ /** @@ -503,8 +371,8 @@ unittest */ struct Foo(Bar){} // [warn]: %s }c.format( - ProperlyDocumentedPublicFunctions.MISSING_TEMPLATE_PARAMS_MESSAGE.format("Bar") - ), sac); + (ProperlyDocumentedPublicFunctions!ASTCodegen).MISSING_TEMPLATE_PARAMS_MESSAGE.format("Bar") + ), sac, true); assertAnalyzerWarnings(q{ /** @@ -512,8 +380,8 @@ unittest */ class Foo(Bar){} // [warn]: %s }c.format( - ProperlyDocumentedPublicFunctions.MISSING_TEMPLATE_PARAMS_MESSAGE.format("Bar") - ), sac); + (ProperlyDocumentedPublicFunctions!ASTCodegen).MISSING_TEMPLATE_PARAMS_MESSAGE.format("Bar") + ), sac, true); assertAnalyzerWarnings(q{ /** @@ -521,30 +389,30 @@ unittest */ template Foo(Bar){} // [warn]: %s }c.format( - ProperlyDocumentedPublicFunctions.MISSING_TEMPLATE_PARAMS_MESSAGE.format("Bar") - ), sac); + (ProperlyDocumentedPublicFunctions!ASTCodegen).MISSING_TEMPLATE_PARAMS_MESSAGE.format("Bar") + ), sac, true); // test no parameters assertAnalyzerWarnings(q{ /** Some text */ void foo(){} - }c, sac); + }c, sac, true); assertAnalyzerWarnings(q{ /** Some text */ struct Foo(){} - }c, sac); + }c, sac, true); assertAnalyzerWarnings(q{ /** Some text */ class Foo(){} - }c, sac); + }c, sac, true); assertAnalyzerWarnings(q{ /** Some text */ template Foo(){} - }c, sac); + }c, sac, true); } @@ -558,19 +426,19 @@ unittest /** Some text */ - int foo(){} // [warn]: %s + int foo(){ return 0; } // [warn]: %s }c.format( - ProperlyDocumentedPublicFunctions.MISSING_RETURNS_MESSAGE, - ), sac); + (ProperlyDocumentedPublicFunctions!ASTCodegen).MISSING_RETURNS_MESSAGE, + ), sac, true); assertAnalyzerWarnings(q{ /** Some text */ - auto foo(){} // [warn]: %s + auto foo(){ return 0; } // [warn]: %s }c.format( - ProperlyDocumentedPublicFunctions.MISSING_RETURNS_MESSAGE, - ), sac); + (ProperlyDocumentedPublicFunctions!ASTCodegen).MISSING_RETURNS_MESSAGE, + ), sac, true); } // ignore private @@ -584,7 +452,7 @@ unittest Some text */ private void foo(int k){} - }c, sac); + }c, sac, true); // with block assertAnalyzerWarnings(q{ @@ -594,14 +462,14 @@ unittest */ private void foo(int k){} /// - public int bar(){} // [warn]: %s + public int bar(){ return 0; } // [warn]: %s public: /// - int foobar(){} // [warn]: %s + int foobar(){ return 0; } // [warn]: %s }c.format( - ProperlyDocumentedPublicFunctions.MISSING_RETURNS_MESSAGE, - ProperlyDocumentedPublicFunctions.MISSING_RETURNS_MESSAGE, - ), sac); + (ProperlyDocumentedPublicFunctions!ASTCodegen).MISSING_RETURNS_MESSAGE, + (ProperlyDocumentedPublicFunctions!ASTCodegen).MISSING_RETURNS_MESSAGE, + ), sac, true); // with block (template) assertAnalyzerWarnings(q{ @@ -616,9 +484,9 @@ unittest /// template foobar(T){} // [warn]: %s }c.format( - ProperlyDocumentedPublicFunctions.MISSING_TEMPLATE_PARAMS_MESSAGE.format("T"), - ProperlyDocumentedPublicFunctions.MISSING_TEMPLATE_PARAMS_MESSAGE.format("T"), - ), sac); + (ProperlyDocumentedPublicFunctions!ASTCodegen).MISSING_TEMPLATE_PARAMS_MESSAGE.format("T"), + (ProperlyDocumentedPublicFunctions!ASTCodegen).MISSING_TEMPLATE_PARAMS_MESSAGE.format("T"), + ), sac, true); // with block (struct) assertAnalyzerWarnings(q{ @@ -633,9 +501,9 @@ unittest /// struct foobar(T){} // [warn]: %s }c.format( - ProperlyDocumentedPublicFunctions.MISSING_TEMPLATE_PARAMS_MESSAGE.format("T"), - ProperlyDocumentedPublicFunctions.MISSING_TEMPLATE_PARAMS_MESSAGE.format("T"), - ), sac); + (ProperlyDocumentedPublicFunctions!ASTCodegen).MISSING_TEMPLATE_PARAMS_MESSAGE.format("T"), + (ProperlyDocumentedPublicFunctions!ASTCodegen).MISSING_TEMPLATE_PARAMS_MESSAGE.format("T"), + ), sac, true); } // test parameter names @@ -653,10 +521,10 @@ unittest * Returns: * A long description. */ -int foo(int k){} // [warn]: %s +int foo(int k){ return 0; } // [warn]: %s }c.format( - ProperlyDocumentedPublicFunctions.MISSING_PARAMS_MESSAGE.format("k") - ), sac); + (ProperlyDocumentedPublicFunctions!ASTCodegen).MISSING_PARAMS_MESSAGE.format("k") + ), sac, true); assertAnalyzerWarnings(q{ /** @@ -669,8 +537,8 @@ int foo(int k){} // [warn]: %s */ int foo(int k) => k; // [warn]: %s }c.format( - ProperlyDocumentedPublicFunctions.MISSING_PARAMS_MESSAGE.format("k") - ), sac); + (ProperlyDocumentedPublicFunctions!ASTCodegen).MISSING_PARAMS_MESSAGE.format("k") + ), sac, true); assertAnalyzerWarnings(q{ /** @@ -683,10 +551,10 @@ k = A stupid parameter Returns: A long description. */ -int foo(int k){} // [warn]: %s +int foo(int k){ return 0; } // [warn]: %s }c.format( - ProperlyDocumentedPublicFunctions.NON_EXISTENT_PARAMS_MESSAGE.format("val") - ), sac); + (ProperlyDocumentedPublicFunctions!ASTCodegen).NON_EXISTENT_PARAMS_MESSAGE.format("val") + ), sac, true); assertAnalyzerWarnings(q{ /** @@ -697,10 +565,10 @@ Params: Returns: A long description. */ -int foo(int k){} // [warn]: %s +int foo(int k){ return 0; } // [warn]: %s }c.format( - ProperlyDocumentedPublicFunctions.MISSING_PARAMS_MESSAGE.format("k") - ), sac); + (ProperlyDocumentedPublicFunctions!ASTCodegen).MISSING_PARAMS_MESSAGE.format("k") + ), sac, true); assertAnalyzerWarnings(q{ /** @@ -714,10 +582,10 @@ foobar = A stupid parameter Returns: A long description. */ -int foo(int foo, int foobar){} // [warn]: %s +int foo(int foo, int foobar){ return 0; } // [warn]: %s }c.format( - ProperlyDocumentedPublicFunctions.NON_EXISTENT_PARAMS_MESSAGE.format("bad") - ), sac); + (ProperlyDocumentedPublicFunctions!ASTCodegen).NON_EXISTENT_PARAMS_MESSAGE.format("bad") + ), sac, true); assertAnalyzerWarnings(q{ /** @@ -733,8 +601,8 @@ A long description. */ struct foo(int foo, int foobar){} // [warn]: %s }c.format( - ProperlyDocumentedPublicFunctions.NON_EXISTENT_PARAMS_MESSAGE.format("bad") - ), sac); + (ProperlyDocumentedPublicFunctions!ASTCodegen).NON_EXISTENT_PARAMS_MESSAGE.format("bad") + ), sac, true); // properly documented assertAnalyzerWarnings(q{ @@ -748,8 +616,8 @@ bar = A stupid parameter Returns: A long description. */ -int foo(int foo, int bar){} - }c, sac); +int foo(int foo, int bar){ return 0; } + }c, sac, true); assertAnalyzerWarnings(q{ /** @@ -763,7 +631,7 @@ Returns: A long description. */ struct foo(int foo, int bar){} - }c, sac); + }c, sac, true); } // support ditto @@ -782,11 +650,11 @@ unittest * Returns: * A long description. */ -int foo(int k){} +int foo(int k){ return 0; } /// ditto -int bar(int k){} - }c, sac); +int bar(int k){ return 0; } + }c, sac, true); assertAnalyzerWarnings(q{ /** @@ -799,11 +667,11 @@ int bar(int k){} * Returns: * A long description. */ -int foo(int k){} +int foo(int k){ return 0; } /// ditto struct Bar(K){} - }c, sac); + }c, sac, true); assertAnalyzerWarnings(q{ /** @@ -816,11 +684,11 @@ struct Bar(K){} * Returns: * A long description. */ -int foo(int k){} +int foo(int k){ return 0; } /// ditto -int bar(int f){} - }c, sac); +int bar(int f){ return 0; } + }c, sac, true); assertAnalyzerWarnings(q{ /** @@ -832,13 +700,13 @@ int bar(int f){} * Returns: * A long description. */ -int foo(int k){} +int foo(int k){ return 0; } /// ditto -int bar(int bar){} // [warn]: %s +int bar(int bar){ return 0; } // [warn]: %s }c.format( - ProperlyDocumentedPublicFunctions.MISSING_PARAMS_MESSAGE.format("bar") - ), sac); + (ProperlyDocumentedPublicFunctions!ASTCodegen).MISSING_PARAMS_MESSAGE.format("bar") + ), sac, true); assertAnalyzerWarnings(q{ /** @@ -854,13 +722,13 @@ int bar(int bar){} // [warn]: %s * See_Also: * $(REF takeExactly, std,range) */ -int foo(int k){} // [warn]: %s +int foo(int k){ return 0; } // [warn]: %s /// ditto -int bar(int bar){} +int bar(int bar){ return 0; } }c.format( - ProperlyDocumentedPublicFunctions.NON_EXISTENT_PARAMS_MESSAGE.format("f") - ), sac); + (ProperlyDocumentedPublicFunctions!ASTCodegen).NON_EXISTENT_PARAMS_MESSAGE.format("f") + ), sac, true); } // check correct ddoc headers @@ -880,8 +748,8 @@ unittest Returns: Awesome values. +/ -string bar(string val){} - }c, sac); +string bar(string val){ return ""; } + }c, sac, true); assertAnalyzerWarnings(q{ /++ @@ -895,7 +763,7 @@ string bar(string val){} Returns: Awesome values. +/ template bar(string val){} - }c, sac); + }c, sac, true); } @@ -926,7 +794,7 @@ template abcde(Args ...) { /// .... } } - }c, sac); + }c, sac, true); } // Don't force the documentation of the template parameter if it's a used type in the parameter list @@ -945,7 +813,7 @@ Params: Returns: Awesome values. +/ string bar(R)(R r){} - }c, sac); + }c, sac, true); assertAnalyzerWarnings(q{ /++ @@ -958,8 +826,8 @@ Returns: Awesome values. +/ string bar(P, R)(R r){}// [warn]: %s }c.format( - ProperlyDocumentedPublicFunctions.MISSING_TEMPLATE_PARAMS_MESSAGE.format("P") - ), sac); + (ProperlyDocumentedPublicFunctions!ASTCodegen).MISSING_TEMPLATE_PARAMS_MESSAGE.format("P") + ), sac, true); } // https://github.com/dlang-community/D-Scanner/issues/601 @@ -974,7 +842,7 @@ unittest alias p = put!(Unqual!Range); p(items); } - }, sac); + }, sac, true); } unittest @@ -993,7 +861,7 @@ unittest +/ void put(Range)(const(Range) items) if (canPutConstRange!Range) {} - }, sac); + }, sac, true); } unittest @@ -1002,206 +870,75 @@ unittest sac.properly_documented_public_functions = Check.enabled; assertAnalyzerWarnings(q{ +class AssertError : Error +{ + this(string msg) { super(msg); } +} + /++ Throw but likely catched. +/ -void bar(){ +void bar1(){ try{throw new Exception("bla");throw new Error("bla");} catch(Exception){} catch(Error){}} - }c, sac); -} -unittest -{ - StaticAnalysisConfig sac = disabledConfig; - sac.properly_documented_public_functions = Check.enabled; - - assertAnalyzerWarnings(q{ /++ Simple case +/ -void bar(){throw new Exception("bla");} // [warn]: %s - }c.format( - ProperlyDocumentedPublicFunctions.MISSING_THROW_MESSAGE.format("Exception") - ), sac); -} + void bar2(){throw new Exception("bla");} // [warn]: %s -unittest -{ - StaticAnalysisConfig sac = disabledConfig; - sac.properly_documented_public_functions = Check.enabled; - - assertAnalyzerWarnings(q{ /++ Supposed to be documented Throws: Exception if... +/ -void bar(){throw new Exception("bla");} - }c.format( - ), sac); -} +void bar3(){throw new Exception("bla");} -unittest -{ - StaticAnalysisConfig sac = disabledConfig; - sac.properly_documented_public_functions = Check.enabled; - - assertAnalyzerWarnings(q{ /++ rethrow +/ -void bar(){try throw new Exception("bla"); catch(Exception) throw new Error();} // [warn]: %s - }c.format( - ProperlyDocumentedPublicFunctions.MISSING_THROW_MESSAGE.format("Error") - ), sac); -} +void bar4(){try throw new Exception("bla"); catch(Exception) throw new Error("bla");} // [warn]: %s -unittest -{ - StaticAnalysisConfig sac = disabledConfig; - sac.properly_documented_public_functions = Check.enabled; - - assertAnalyzerWarnings(q{ /++ trust nothrow before everything +/ -void bar() nothrow {try throw new Exception("bla"); catch(Exception) assert(0);} - }c, sac); -} +void bar5() nothrow {try throw new Exception("bla"); catch(Exception) assert(0);} -unittest -{ - StaticAnalysisConfig sac = disabledConfig; - sac.properly_documented_public_functions = Check.enabled; - - assertAnalyzerWarnings(q{ /++ case of throw in nested func +/ -void bar() // [warn]: %s +void bar6() // [warn]: %s { void foo(){throw new AssertError("bla");} foo(); } - }c.format( - ProperlyDocumentedPublicFunctions.MISSING_THROW_MESSAGE.format("AssertError") - ), sac); -} - -unittest -{ - StaticAnalysisConfig sac = disabledConfig; - sac.properly_documented_public_functions = Check.enabled; - assertAnalyzerWarnings(q{ /++ case of throw in nested func but caught +/ -void bar() +void bar7() { void foo(){throw new AssertError("bla");} try foo(); catch (AssertError){} } - }c, sac); -} -unittest -{ - StaticAnalysisConfig sac = disabledConfig; - sac.properly_documented_public_functions = Check.enabled; - - assertAnalyzerWarnings(q{ /++ case of double throw in nested func but only 1 caught +/ -void bar() // [warn]: %s +void bar8() // [warn]: %s { void foo(){throw new AssertError("bla");throw new Error("bla");} try foo(); catch (Error){} -} - }c.format( - ProperlyDocumentedPublicFunctions.MISSING_THROW_MESSAGE.format("AssertError") - ), sac); -} - -unittest -{ - StaticAnalysisConfig sac = disabledConfig; - sac.properly_documented_public_functions = Check.enabled; - - assertAnalyzerWarnings(q{ -/++ -enforce -+/ -void bar() // [warn]: %s -{ - enforce(condition); -} - }c.format( - ProperlyDocumentedPublicFunctions.MISSING_THROW_MESSAGE.format("Exception") - ), sac); -} - -unittest -{ - StaticAnalysisConfig sac = disabledConfig; - sac.properly_documented_public_functions = Check.enabled; - - assertAnalyzerWarnings(q{ -/++ -enforce -+/ -void bar() // [warn]: %s -{ - enforce!AssertError(condition); -} - }c.format( - ProperlyDocumentedPublicFunctions.MISSING_THROW_MESSAGE.format("AssertError") - ), sac); -} - -unittest -{ - StaticAnalysisConfig sac = disabledConfig; - sac.properly_documented_public_functions = Check.enabled; - - assertAnalyzerWarnings(q{ -/++ -enforce -+/ -void bar() // [warn]: %s -{ - enforce!(AssertError)(condition); -} - }c.format( - ProperlyDocumentedPublicFunctions.MISSING_THROW_MESSAGE.format("AssertError") - ), sac); -} - -unittest -{ - StaticAnalysisConfig sac = disabledConfig; - sac.properly_documented_public_functions = Check.enabled; - - assertAnalyzerWarnings(q{ -/++ -enforce -+/ -void foo() // [warn]: %s -{ - void bar() - { - enforce!AssertError(condition); - } - bar(); -} - - }c.format( - ProperlyDocumentedPublicFunctions.MISSING_THROW_MESSAGE.format("AssertError") - ), sac); +}}c.format( + (ProperlyDocumentedPublicFunctions!ASTCodegen).MISSING_THROW_MESSAGE.format("object.Exception"), + (ProperlyDocumentedPublicFunctions!ASTCodegen).MISSING_THROW_MESSAGE.format("object.Error"), + (ProperlyDocumentedPublicFunctions!ASTCodegen).MISSING_THROW_MESSAGE + .format("properly_documented_public_functions.AssertError"), + (ProperlyDocumentedPublicFunctions!ASTCodegen).MISSING_THROW_MESSAGE + .format("properly_documented_public_functions.AssertError") + ), sac, true); } // https://github.com/dlang-community/D-Scanner/issues/583 @@ -1213,10 +950,8 @@ unittest assertAnalyzerWarnings(q{ /++ Implements the homonym function (also known as `accumulate`) - Returns: the accumulated `result` - Params: fun = one or more functions +/ @@ -1225,17 +960,15 @@ unittest { /++ 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); + ), sac, true); stderr.writeln("Unittest for ProperlyDocumentedPublicFunctions passed."); } diff --git a/src/dscanner/analysis/range.d b/src/dscanner/analysis/range.d index f7a6f653..2790787d 100644 --- a/src/dscanner/analysis/range.d +++ b/src/dscanner/analysis/range.d @@ -6,20 +6,17 @@ module dscanner.analysis.range; import std.stdio; -import dparse.ast; -import dparse.lexer; import dscanner.analysis.base; import dscanner.analysis.helpers; -import dsymbol.scope_ : Scope; +import std.string : format; /** * Checks for .. expressions where the left side is larger than the right. This * is almost always a mistake. */ -final class BackwardsRangeCheck : BaseAnalyzer +extern(C++) class BackwardsRangeCheck(AST) : BaseAnalyzerDmd { - alias visit = BaseAnalyzer.visit; - + alias visit = BaseAnalyzerDmd.visit; mixin AnalyzerInfo!"backwards_range_check"; /// Key for this check in the report output @@ -29,132 +26,37 @@ final class BackwardsRangeCheck : BaseAnalyzer * Params: * fileName = the name of the file being analyzed */ - this(string fileName, const Scope* sc, bool skipTests = false) - { - super(fileName, sc, skipTests); - } - - override void visit(const ForeachStatement foreachStatement) - { - if (foreachStatement.low !is null && foreachStatement.high !is null) - { - import std.string : format; - - state = State.left; - visit(foreachStatement.low); - state = State.right; - visit(foreachStatement.high); - state = State.ignore; - if (hasLeft && hasRight && left > right) - { - 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); - } - hasLeft = false; - hasRight = false; - } - foreachStatement.accept(this); - } - - override void visit(const AddExpression add) - { - immutable s = state; - state = State.ignore; - add.accept(this); - state = s; - } - - override void visit(const UnaryExpression unary) + extern(D) this(string fileName, bool skipTests = false) { - if (state != State.ignore && unary.primaryExpression is null) - return; - else - unary.accept(this); + super(fileName, skipTests); } - override void visit(const PrimaryExpression primary) + override void visit(AST.IntervalExp ie) { - import std.conv : to, ConvException; + auto lwr = ie.lwr.isIntegerExp(); + auto upr = ie.upr.isIntegerExp(); - if (state == State.ignore || !isNumberLiteral(primary.primary.type)) - return; - if (state == State.left) + if (lwr && upr && lwr.getInteger() > upr.getInteger()) { - line = primary.primary.line; - this.column = primary.primary.column; - - try - left = parseNumber(primary.primary.text); - catch (ConvException e) - return; - hasLeft = true; - } - else - { - try - right = parseNumber(primary.primary.text); - catch (ConvException e) - return; - hasRight = true; - } - } - - override void visit(const Index index) - { - if (index.low !is null && index.high !is null) - { - state = State.left; - visit(index.low); - state = State.right; - visit(index.high); - state = State.ignore; - if (hasLeft && hasRight && left > right) - { - import std.string : format; - - string message = format("%d is larger than %d. This slice is likely incorrect.", - left, right); - addErrorMessage(line, this.column, KEY, message); - } - hasLeft = false; - hasRight = false; + string message = format("%d is larger than %d. This slice is likely incorrect.", + lwr.getInteger(), upr.getInteger()); + addErrorMessage(cast(ulong) ie.loc.linnum, cast(ulong) ie.loc.charnum, KEY, message); } - index.accept(this); + } -private: - bool hasLeft; - bool hasRight; - long left; - long right; - size_t column; - size_t line; - enum State + override void visit(AST.ForeachRangeStatement s) { - ignore, - left, - right - } - - State state = State.ignore; + auto lwr = s.lwr.isIntegerExp(); + auto upr = s.upr.isIntegerExp(); - long parseNumber(string te) - { - import std.conv : to; - import std.regex : ctRegex, replaceAll; - - enum re = ctRegex!("[_uUlL]", ""); - string t = te.replaceAll(re, ""); - if (t.length > 2) + if (lwr && upr && lwr.getInteger() > upr.getInteger()) { - if (t[1] == 'x' || t[1] == 'X') - return to!long(t[2 .. $], 16); - if (t[1] == 'b' || t[1] == 'B') - return to!long(t[2 .. $], 2); + string message = format( + "%d is larger than %d. Did you mean to use 'foreach_reverse( ... ; %d .. %d)'?", + lwr.getInteger(), upr.getInteger(), upr.getInteger(), lwr.getInteger()); + addErrorMessage(cast(ulong) s.loc.linnum, cast(ulong) s.loc.charnum, KEY, message); } - return to!long(t); } } @@ -164,7 +66,7 @@ unittest StaticAnalysisConfig sac = disabledConfig(); sac.backwards_range_check = Check.enabled; - assertAnalyzerWarnings(q{ + assertAnalyzerWarningsDMD(q{ void testRange() { a = node.tupleof[2..T.length+1]; // ok diff --git a/src/dscanner/analysis/redundant_attributes.d b/src/dscanner/analysis/redundant_attributes.d index 0b3a467f..6c17aa5a 100644 --- a/src/dscanner/analysis/redundant_attributes.d +++ b/src/dscanner/analysis/redundant_attributes.d @@ -4,174 +4,60 @@ module dscanner.analysis.redundant_attributes; -import dparse.ast; -import dparse.lexer; -import dsymbol.scope_ : Scope; import dscanner.analysis.base; import dscanner.analysis.helpers; -import std.algorithm; -import std.conv : to, text; -import std.range : empty, front, walkLength; +import dmd.dsymbol; +import std.string : format; /** * Checks for redundant attributes. At the moment only visibility attributes. */ -final class RedundantAttributesCheck : BaseAnalyzer +extern(C++) class RedundantAttributesCheck(AST) : BaseAnalyzerDmd { mixin AnalyzerInfo!"redundant_attributes_check"; + alias visit = BaseAnalyzerDmd.visit; + + Visibility.Kind currVisibility; + uint currLine; - this(string fileName, const(Scope)* sc, bool skipTests = false) + extern(D) this(string fileName) { - super(fileName, sc, skipTests); - stack.length = 0; - } - - override void visit(const Declaration decl) - { - - // labels, e.g. private: - if (auto attr = decl.attributeDeclaration) - { - if (filterAttributes(attr.attribute)) - { - addAttribute(attr.attribute); - } - } - - auto attributes = decl.attributes.filter!(a => filterAttributes(a)); - if (attributes.walkLength > 0) { - - // new scope: private { } - if (decl.declarations.length > 0) - { - const prev = currentAttributes[]; - // append to current scope and reset once block is left - foreach (attr; attributes) - addAttribute(attr); - - scope(exit) currentAttributes = prev; - decl.accept(this); - } // declarations, e.g. private int ... - else - { - foreach (attr; attributes) - checkAttribute(attr); - - decl.accept(this); - } - } - else - { - decl.accept(this); - } - } - - alias visit = BaseAnalyzer.visit; - - mixin ScopedVisit!Module; - mixin ScopedVisit!BlockStatement; - mixin ScopedVisit!StructBody; - mixin ScopedVisit!CaseStatement; - mixin ScopedVisit!ForStatement; - mixin ScopedVisit!IfStatement; - mixin ScopedVisit!TemplateDeclaration; - mixin ScopedVisit!ConditionalDeclaration; - -private: - - alias ConstAttribute = const Attribute; - alias CurrentScope = ConstAttribute[]; - ref CurrentScope currentAttributes() - { - return stack[$ - 1]; - } - - CurrentScope[] stack; - - void addAttribute(const Attribute attr) - { - removeOverwrite(attr); - if (checkAttribute(attr)) - { - currentAttributes ~= attr; - } - } - - bool checkAttribute(const Attribute attr) - { - auto match = currentAttributes.find!(a => a.attribute.type == attr.attribute.type); - if (!match.empty) - { - auto token = attr.attribute; - addErrorMessage(token.line, token.column, KEY, - text("same visibility attribute used as defined on line ", - match.front.attribute.line.to!string, ".")); - return false; - } - return true; - } - - void removeOverwrite(const Attribute attr) - { - import std.array : array; - const group = getAttributeGroup(attr); - if (currentAttributes.filter!(a => getAttributeGroup(a) == group - && !isIdenticalAttribute(a, attr)).walkLength > 0) - { - currentAttributes = currentAttributes.filter!(a => getAttributeGroup(a) != group - || isIdenticalAttribute(a, attr)).array; - } - } - - bool filterAttributes(const Attribute attr) - { - return isAccessSpecifier(attr); - } - - static int getAttributeGroup(const Attribute attr) - { - if (isAccessSpecifier(attr)) - return 1; - - // TODO: not implemented - return attr.attribute.type; - } - - static bool isAccessSpecifier(const Attribute attr) - { - auto type = attr.attribute.type; - return type.among(tok!"private", tok!"protected", tok!"public", tok!"package", tok!"export") > 0; - } - - static bool isIdenticalAttribute(const Attribute a, const Attribute b) - { - return a.attribute.type == b.attribute.type; - } - - auto attributesString() - { - return currentAttributes.map!(a => a.attribute.type.str).joiner(",").to!string; + super(fileName); } template ScopedVisit(NodeType) { - override void visit(const NodeType n) + override void visit(NodeType n) { - pushScope(); - n.accept(this); - popScope(); + Visibility.Kind prevVisibility = currVisibility; + currVisibility = Visibility.Kind.undefined; + super.visit(n); + currVisibility = prevVisibility; } } - void pushScope() - { - stack.length++; - } + mixin ScopedVisit!(AST.StructDeclaration); + mixin ScopedVisit!(AST.ClassDeclaration); + mixin ScopedVisit!(AST.InterfaceDeclaration); + mixin ScopedVisit!(AST.UnionDeclaration); + mixin ScopedVisit!(AST.StaticIfCondition); + mixin ScopedVisit!(AST.StaticIfDeclaration); + mixin ScopedVisit!(AST.TemplateDeclaration); + mixin ScopedVisit!(AST.ConditionalDeclaration); - void popScope() + override void visit(AST.VisibilityDeclaration vd) { - stack.length--; + if (currVisibility == vd.visibility.kind) + addErrorMessage(cast(ulong) vd.loc.linnum, cast(ulong) vd.loc.charnum, KEY, + "Same visibility attribute used as defined on line %u.".format(currLine)); + Visibility.Kind prevVisibility = currVisibility; + uint prevLine = currLine; + currVisibility = vd.visibility.kind; + currLine = vd.loc.linnum; + super.visit(vd); + currVisibility = prevVisibility; + currLine = prevLine; } enum string KEY = "dscanner.suspicious.redundant_attributes"; @@ -190,72 +76,72 @@ unittest sac.redundant_attributes_check = Check.enabled; // test labels vs. block attributes - assertAnalyzerWarnings(q{ -unittest + assertAnalyzerWarningsDMD(q{ +class C { 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 6. } - 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 - assertAnalyzerWarnings(q{ -unittest + assertAnalyzerWarningsDMD(q{ +class C { 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 6. + public // [warn]: Same visibility attribute used as defined on line 6. { int c; } }}c, sac); // test scopes - assertAnalyzerWarnings(q{ -unittest + assertAnalyzerWarningsDMD(q{ +class C { 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; } }}c, sac); // check duplicated visibility attributes - assertAnalyzerWarnings(q{ -unittest + assertAnalyzerWarningsDMD(q{ +class C { 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 - assertAnalyzerWarnings(q{ -unittest + assertAnalyzerWarningsDMD(q{ +class C { 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); // test scopes - assertAnalyzerWarnings(q{ -unittest + assertAnalyzerWarningsDMD(q{ +class C { public: - if (1 == 1) + static if (1 == 1) { private int b; } @@ -263,7 +149,7 @@ 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); } @@ -274,8 +160,8 @@ unittest sac.redundant_attributes_check = Check.enabled; // test labels vs. block attributes - assertAnalyzerWarnings(q{ -unittest + assertAnalyzerWarningsDMD(q{ +class C { @safe: @safe void foo(); diff --git a/src/dscanner/analysis/redundant_parens.d b/src/dscanner/analysis/redundant_parens.d index 8f2c56ff..9b8168e5 100644 --- a/src/dscanner/analysis/redundant_parens.d +++ b/src/dscanner/analysis/redundant_parens.d @@ -5,62 +5,65 @@ module dscanner.analysis.redundant_parens; -import dparse.ast; -import dparse.lexer; import dscanner.analysis.base; -import dsymbol.scope_ : Scope; /** * Checks for redundant parenthesis */ -final class RedundantParenCheck : BaseAnalyzer +extern(C++) class RedundantParenCheck(AST) : BaseAnalyzerDmd { - alias visit = BaseAnalyzer.visit; - + alias visit = BaseAnalyzerDmd.visit; mixin AnalyzerInfo!"redundant_parens_check"; /// - this(string fileName, const(Scope)* sc, bool skipTests = false) - { - super(fileName, sc, skipTests); - } - - override void visit(const IfStatement statement) + extern(D) this(string fileName, bool skipTests = false) { - UnaryExpression unary; - if (statement.expression is null || statement.expression.items.length != 1) - goto end; - unary = cast(UnaryExpression) statement.expression.items[0]; - if (unary is null) - goto end; - if (unary.primaryExpression is null) - goto end; - if (unary.primaryExpression.expression is null) - goto end; - addErrorMessage(unary.primaryExpression.expression.line, - unary.primaryExpression.expression.column, KEY, "Redundant parenthesis."); - end: - statement.accept(this); + super(fileName, skipTests); } - override void visit(const PrimaryExpression primaryExpression) + override void visit(AST.IfStatement s) { - UnaryExpression unary; - if (primaryExpression.expression is null) - goto end; - unary = cast(UnaryExpression) primaryExpression.expression.items[0]; - if (unary is null) - goto end; - if (unary.primaryExpression is null) - goto end; - if (unary.primaryExpression.expression is null) - goto end; - addErrorMessage(primaryExpression.expression.line, - primaryExpression.expression.column, KEY, "Redundant parenthesis."); - end: - primaryExpression.accept(this); + if (s.condition.parens) + addErrorMessage(cast(ulong) s.loc.linnum, cast(ulong) s.loc.charnum, + KEY, MESSAGE); } private: enum string KEY = "dscanner.suspicious.redundant_parens"; + enum string MESSAGE = "Redundant parenthesis."; } + +unittest +{ + import dscanner.analysis.config : StaticAnalysisConfig, Check, disabledConfig; + import std.stdio : stderr; + import dscanner.analysis.helpers : assertAnalyzerWarnings = assertAnalyzerWarningsDMD; + + StaticAnalysisConfig sac = disabledConfig(); + sac.redundant_parens_check = Check.enabled; + + assertAnalyzerWarnings(q{ + void testRedundantParens() + { + int a = 0; + bool b = true; + + if ((a + 2 == 3)) // [warn]: Redundant parenthesis. + { + + } + + if ((b)) // [warn]: Redundant parenthesis. + { + + } + + if (b) { } + + if (a * 2 == 0) { } + } + }c, sac); + + stderr.writeln("Unittest for RedundantParenthesis passed."); + +} \ No newline at end of file diff --git a/src/dscanner/analysis/run.d b/src/dscanner/analysis/run.d index bb2b5542..57f5942b 100644 --- a/src/dscanner/analysis/run.d +++ b/src/dscanner/analysis/run.d @@ -12,7 +12,6 @@ import std.array; import std.conv; import std.algorithm; import std.range; -import std.array; import std.functional : toDelegate; import std.file : mkdirRecurse; import std.path : dirName; @@ -33,7 +32,6 @@ import dscanner.analysis.style; import dscanner.analysis.enumarrayliteral; import dscanner.analysis.pokemon; import dscanner.analysis.del; -import dscanner.analysis.fish; import dscanner.analysis.numbers; import dscanner.analysis.objectconst; import dscanner.analysis.range; @@ -93,8 +91,19 @@ import dsymbol.modulecache : ModuleCache; import dscanner.utils; import dscanner.reports : DScannerJsonReporter, SonarQubeGenericIssueDataReporter; +import dmd.astbase : ASTBase; +import dmd.parse : Parser; + +import dmd.frontend; +import dmd.astcodegen; + bool first = true; +version (unittest) + enum ut = true; +else + enum ut = false; + private alias ASTAllocator = CAllocatorImpl!( AllocatorList!(n => Region!Mallocator(1024 * 128), Mallocator)); @@ -229,10 +238,34 @@ void generateSonarQubeGenericIssueDataReport(string[] fileNames, const StaticAna bool analyze(string[] fileNames, const StaticAnalysisConfig config, string errorFormat, ref StringCache cache, ref ModuleCache moduleCache, bool staticAnalyze = true) { + import dmd.parse : Parser; + import dmd.astbase : ASTBase; + import dmd.id : Id; + import dmd.globals : global; + import dmd.identifier : Identifier; + import std.string : toStringz; + import dmd.arraytypes : Strings; + bool hasErrors; foreach (fileName; fileNames) { + + auto dmdParentDir = dirName(dirName(dirName(dirName(__FILE_FULL_PATH__)))); + + global.params.useUnitTests = true; + global.path = new Strings(); + global.path.push((dmdParentDir ~ "/dmd" ~ "\0").ptr); + global.path.push((dmdParentDir ~ "/dmd/druntime/src" ~ "\0").ptr); + + initDMD(); + auto code = readFile(fileName); + auto input = cast(char[]) code; + input ~= '\0'; + + auto t = dmd.frontend.parseModule(cast(const(char)[]) fileName, cast(const (char)[]) input); + // t.module_.fullSemantic(); + // Skip files that could not be read and continue with the rest if (code.length == 0) continue; @@ -246,6 +279,11 @@ bool analyze(string[] fileNames, const StaticAnalysisConfig config, string error if (errorCount > 0 || (staticAnalyze && warningCount > 0)) hasErrors = true; MessageSet results = analyze(fileName, m, config, moduleCache, tokens, staticAnalyze); + MessageSet resultsDmd = analyzeDmd(fileName, t.module_, getModuleName(t.module_.md), config); + foreach (result; resultsDmd[]) + { + results.insert(result); + } if (results is null) continue; foreach (result; results[]) @@ -325,6 +363,46 @@ bool shouldRun(check : BaseAnalyzer)(string moduleName, const ref StaticAnalysis return true; } +/** + * Checks whether a module is part of a user-specified include/exclude list. + * + * The user can specify a comma-separated list of filters, everyone needs to start with + * either a '+' (inclusion) or '-' (exclusion). + * + * If no includes are specified, all modules are included. +*/ +bool shouldRunDmd(check : BaseAnalyzerDmd)(const char[] moduleName, const ref StaticAnalysisConfig config) +{ + enum string a = check.name; + + if (mixin("config." ~ a) == Check.disabled) + return false; + + // By default, run the check + if (!moduleName.length) + return true; + + auto filters = mixin("config.filters." ~ a); + + // Check if there are filters are defined + // filters starting with a comma are invalid + if (filters.length == 0 || filters[0].length == 0) + return true; + + auto includers = filters.filter!(f => f[0] == '+').map!(f => f[1..$]); + auto excluders = filters.filter!(f => f[0] == '-').map!(f => f[1..$]); + + // exclusion has preference over inclusion + if (!excluders.empty && excluders.any!(s => moduleName.canFind(s))) + return false; + + if (!includers.empty) + return includers.any!(s => moduleName.canFind(s)); + + // by default: include all modules + return true; +} + /// unittest { @@ -335,7 +413,7 @@ unittest config.asm_style_check = Check.enabled; // this is done automatically by inifiled config.filters.asm_style_check = filters.split(","); - return shouldRun!AsmStyleCheck(moduleName, config); + return moduleName.shouldRunDmd!(AsmStyleCheck!ASTCodegen)(config); } // test inclusion @@ -368,11 +446,6 @@ MessageSet analyze(string fileName, const Module m, const StaticAnalysisConfig a if (!staticAnalyze) return null; - version (unittest) - enum ut = true; - else - enum ut = false; - string moduleName; if (m !is null && m.moduleDeclaration !is null && m.moduleDeclaration.moduleName !is null && @@ -391,50 +464,18 @@ MessageSet analyze(string fileName, const Module m, const StaticAnalysisConfig a GC.disable; - if (moduleName.shouldRun!AsmStyleCheck(analysisConfig)) - checks ~= new AsmStyleCheck(fileName, moduleScope, - analysisConfig.asm_style_check == Check.skipTests && !ut); - - if (moduleName.shouldRun!BackwardsRangeCheck(analysisConfig)) - checks ~= new BackwardsRangeCheck(fileName, moduleScope, - analysisConfig.backwards_range_check == Check.skipTests && !ut); - - if (moduleName.shouldRun!BuiltinPropertyNameCheck(analysisConfig)) - checks ~= new BuiltinPropertyNameCheck(fileName, moduleScope, - analysisConfig.builtin_property_names_check == Check.skipTests && !ut); - if (moduleName.shouldRun!CommaExpressionCheck(analysisConfig)) checks ~= new CommaExpressionCheck(fileName, moduleScope, analysisConfig.comma_expression_check == Check.skipTests && !ut); - if (moduleName.shouldRun!ConstructorCheck(analysisConfig)) - checks ~= new ConstructorCheck(fileName, moduleScope, - analysisConfig.constructor_check == Check.skipTests && !ut); - if (moduleName.shouldRun!UnmodifiedFinder(analysisConfig)) checks ~= new UnmodifiedFinder(fileName, moduleScope, analysisConfig.could_be_immutable_check == Check.skipTests && !ut); - if (moduleName.shouldRun!DeleteCheck(analysisConfig)) - checks ~= new DeleteCheck(fileName, moduleScope, - analysisConfig.delete_check == Check.skipTests && !ut); - if (moduleName.shouldRun!DuplicateAttributeCheck(analysisConfig)) checks ~= new DuplicateAttributeCheck(fileName, moduleScope, analysisConfig.duplicate_attribute == Check.skipTests && !ut); - if (moduleName.shouldRun!EnumArrayLiteralCheck(analysisConfig)) - checks ~= new EnumArrayLiteralCheck(fileName, moduleScope, - analysisConfig.enum_array_literal_check == Check.skipTests && !ut); - - if (moduleName.shouldRun!PokemonExceptionCheck(analysisConfig)) - checks ~= new PokemonExceptionCheck(fileName, moduleScope, - analysisConfig.exception_check == Check.skipTests && !ut); - - if (moduleName.shouldRun!FloatOperatorCheck(analysisConfig)) - checks ~= new FloatOperatorCheck(fileName, moduleScope, - analysisConfig.float_operator_check == Check.skipTests && !ut); - if (moduleName.shouldRun!FunctionAttributeCheck(analysisConfig)) checks ~= new FunctionAttributeCheck(fileName, moduleScope, analysisConfig.function_attribute_check == Check.skipTests && !ut); @@ -447,18 +488,6 @@ MessageSet analyze(string fileName, const Module m, const StaticAnalysisConfig a checks ~= new LabelVarNameCheck(fileName, moduleScope, analysisConfig.label_var_same_name_check == Check.skipTests && !ut); - if (moduleName.shouldRun!LengthSubtractionCheck(analysisConfig)) - checks ~= new LengthSubtractionCheck(fileName, moduleScope, - analysisConfig.length_subtraction_check == Check.skipTests && !ut); - - if (moduleName.shouldRun!LocalImportCheck(analysisConfig)) - checks ~= new LocalImportCheck(fileName, moduleScope, - analysisConfig.local_import_check == Check.skipTests && !ut); - - if (moduleName.shouldRun!LogicPrecedenceCheck(analysisConfig)) - checks ~= new LogicPrecedenceCheck(fileName, moduleScope, - analysisConfig.logical_precedence_check == Check.skipTests && !ut); - if (moduleName.shouldRun!MismatchedArgumentCheck(analysisConfig)) checks ~= new MismatchedArgumentCheck(fileName, moduleScope, analysisConfig.mismatched_args_check == Check.skipTests && !ut); @@ -467,18 +496,6 @@ MessageSet analyze(string fileName, const Module m, const StaticAnalysisConfig a checks ~= new NumberStyleCheck(fileName, moduleScope, analysisConfig.number_style_check == Check.skipTests && !ut); - if (moduleName.shouldRun!ObjectConstCheck(analysisConfig)) - checks ~= new ObjectConstCheck(fileName, moduleScope, - analysisConfig.object_const_check == Check.skipTests && !ut); - - if (moduleName.shouldRun!OpEqualsWithoutToHashCheck(analysisConfig)) - checks ~= new OpEqualsWithoutToHashCheck(fileName, moduleScope, - analysisConfig.opequals_tohash_check == Check.skipTests && !ut); - - if (moduleName.shouldRun!RedundantParenCheck(analysisConfig)) - checks ~= new RedundantParenCheck(fileName, moduleScope, - analysisConfig.redundant_parens_check == Check.skipTests && !ut); - if (moduleName.shouldRun!StyleChecker(analysisConfig)) checks ~= new StyleChecker(fileName, moduleScope, analysisConfig.style_check == Check.skipTests && !ut); @@ -487,10 +504,6 @@ MessageSet analyze(string fileName, const Module m, const StaticAnalysisConfig a checks ~= new UndocumentedDeclarationCheck(fileName, moduleScope, analysisConfig.undocumented_declaration_check == Check.skipTests && !ut); - if (moduleName.shouldRun!UnusedLabelCheck(analysisConfig)) - checks ~= new UnusedLabelCheck(fileName, moduleScope, - analysisConfig.unused_label_check == Check.skipTests && !ut); - if (moduleName.shouldRun!UnusedVariableCheck(analysisConfig)) checks ~= new UnusedVariableCheck(fileName, moduleScope, analysisConfig.unused_variable_check == Check.skipTests && !ut); @@ -504,26 +517,6 @@ MessageSet analyze(string fileName, const Module m, const StaticAnalysisConfig a analysisConfig.max_line_length, analysisConfig.long_line_check == Check.skipTests && !ut); - if (moduleName.shouldRun!AutoRefAssignmentCheck(analysisConfig)) - checks ~= new AutoRefAssignmentCheck(fileName, - analysisConfig.auto_ref_assignment_check == Check.skipTests && !ut); - - if (moduleName.shouldRun!IncorrectInfiniteRangeCheck(analysisConfig)) - checks ~= new IncorrectInfiniteRangeCheck(fileName, - analysisConfig.incorrect_infinite_range_check == Check.skipTests && !ut); - - if (moduleName.shouldRun!UselessAssertCheck(analysisConfig)) - checks ~= new UselessAssertCheck(fileName, - analysisConfig.useless_assert_check == Check.skipTests && !ut); - - if (moduleName.shouldRun!AliasSyntaxCheck(analysisConfig)) - checks ~= new AliasSyntaxCheck(fileName, - analysisConfig.alias_syntax_check == Check.skipTests && !ut); - - if (moduleName.shouldRun!StaticIfElse(analysisConfig)) - checks ~= new StaticIfElse(fileName, - analysisConfig.static_if_else_check == Check.skipTests && !ut); - if (moduleName.shouldRun!LambdaReturnCheck(analysisConfig)) checks ~= new LambdaReturnCheck(fileName, analysisConfig.lambda_return_check == Check.skipTests && !ut); @@ -532,22 +525,6 @@ MessageSet analyze(string fileName, const Module m, const StaticAnalysisConfig a checks ~= new AutoFunctionChecker(fileName, analysisConfig.auto_function_check == Check.skipTests && !ut); - if (moduleName.shouldRun!ImportSortednessCheck(analysisConfig)) - checks ~= new ImportSortednessCheck(fileName, - analysisConfig.imports_sortedness == Check.skipTests && !ut); - - if (moduleName.shouldRun!ExplicitlyAnnotatedUnittestCheck(analysisConfig)) - checks ~= new ExplicitlyAnnotatedUnittestCheck(fileName, - analysisConfig.explicitly_annotated_unittests == Check.skipTests && !ut); - - if (moduleName.shouldRun!ProperlyDocumentedPublicFunctions(analysisConfig)) - checks ~= new ProperlyDocumentedPublicFunctions(fileName, - analysisConfig.properly_documented_public_functions == Check.skipTests && !ut); - - if (moduleName.shouldRun!FinalAttributeChecker(analysisConfig)) - checks ~= new FinalAttributeChecker(fileName, - analysisConfig.final_attribute_check == Check.skipTests && !ut); - if (moduleName.shouldRun!VcallCtorChecker(analysisConfig)) checks ~= new VcallCtorChecker(fileName, analysisConfig.vcall_in_ctor == Check.skipTests && !ut); @@ -560,26 +537,14 @@ MessageSet analyze(string fileName, const Module m, const StaticAnalysisConfig a checks ~= new AllManCheck(fileName, tokens, analysisConfig.allman_braces_check == Check.skipTests && !ut); - if (moduleName.shouldRun!RedundantAttributesCheck(analysisConfig)) - checks ~= new RedundantAttributesCheck(fileName, moduleScope, - analysisConfig.redundant_attributes_check == Check.skipTests && !ut); - if (moduleName.shouldRun!HasPublicExampleCheck(analysisConfig)) checks ~= new HasPublicExampleCheck(fileName, moduleScope, analysisConfig.has_public_example == Check.skipTests && !ut); - if (moduleName.shouldRun!AssertWithoutMessageCheck(analysisConfig)) - checks ~= new AssertWithoutMessageCheck(fileName, moduleScope, - analysisConfig.assert_without_msg == Check.skipTests && !ut); - if (moduleName.shouldRun!IfConstraintsIndentCheck(analysisConfig)) checks ~= new IfConstraintsIndentCheck(fileName, tokens, analysisConfig.if_constraints_indent == Check.skipTests && !ut); - if (moduleName.shouldRun!TrustTooMuchCheck(analysisConfig)) - checks ~= new TrustTooMuchCheck(fileName, - analysisConfig.trust_too_much == Check.skipTests && !ut); - if (moduleName.shouldRun!RedundantStorageClassCheck(analysisConfig)) checks ~= new RedundantStorageClassCheck(fileName, analysisConfig.redundant_storage_classes == Check.skipTests && !ut); @@ -610,3 +575,133 @@ MessageSet analyze(string fileName, const Module m, const StaticAnalysisConfig a return set; } + +MessageSet analyzeDmd(string fileName, ASTCodegen.Module m, const char[] moduleName, const StaticAnalysisConfig config) +{ + MessageSet set = new MessageSet; + BaseAnalyzerDmd[] visitors; + + if (moduleName.shouldRunDmd!(ObjectConstCheck!ASTCodegen)(config)) + visitors ~= new ObjectConstCheck!ASTCodegen(fileName); + + if (moduleName.shouldRunDmd!(EnumArrayVisitor!ASTCodegen)(config)) + visitors ~= new EnumArrayVisitor!ASTCodegen(fileName); + + if (moduleName.shouldRunDmd!(DeleteCheck!ASTCodegen)(config)) + visitors ~= new DeleteCheck!ASTCodegen(fileName); + + if (moduleName.shouldRunDmd!(FinalAttributeChecker!ASTCodegen)(config)) + visitors ~= new FinalAttributeChecker!ASTCodegen(fileName); + + if (moduleName.shouldRunDmd!(ImportSortednessCheck!ASTCodegen)(config)) + visitors ~= new ImportSortednessCheck!ASTCodegen(fileName); + + if (moduleName.shouldRunDmd!(IncorrectInfiniteRangeCheck!ASTCodegen)(config)) + visitors ~= new IncorrectInfiniteRangeCheck!ASTCodegen(fileName); + + if (moduleName.shouldRunDmd!(RedundantAttributesCheck!ASTCodegen)(config)) + visitors ~= new RedundantAttributesCheck!ASTCodegen(fileName); + + if (moduleName.shouldRunDmd!(LengthSubtractionCheck!ASTCodegen)(config)) + visitors ~= new LengthSubtractionCheck!ASTCodegen(fileName); + + if (moduleName.shouldRunDmd!(AliasSyntaxCheck!ASTCodegen)(config)) + visitors ~= new AliasSyntaxCheck!ASTCodegen(fileName); + + if (moduleName.shouldRunDmd!(ExplicitlyAnnotatedUnittestCheck!ASTCodegen)(config)) + visitors ~= new ExplicitlyAnnotatedUnittestCheck!ASTCodegen(fileName); + + if (moduleName.shouldRunDmd!(ConstructorCheck!ASTCodegen)(config)) + visitors ~= new ConstructorCheck!ASTCodegen(fileName); + + if (moduleName.shouldRunDmd!(AssertWithoutMessageCheck!ASTCodegen)(config)) + visitors ~= new AssertWithoutMessageCheck!ASTCodegen( + fileName, + config.assert_without_msg == Check.skipTests && !ut + ); + + if (moduleName.shouldRunDmd!(LocalImportCheck!ASTCodegen)(config)) + visitors ~= new LocalImportCheck!ASTCodegen(fileName); + + if (moduleName.shouldRunDmd!(OpEqualsWithoutToHashCheck!ASTCodegen)(config)) + visitors ~= new OpEqualsWithoutToHashCheck!ASTCodegen( + fileName, + config.opequals_tohash_check == Check.skipTests && !ut + ); + + if (moduleName.shouldRunDmd!(TrustTooMuchCheck!ASTCodegen)(config)) + visitors ~= new TrustTooMuchCheck!ASTCodegen( + fileName, + config.trust_too_much == Check.skipTests && !ut + ); + + if (moduleName.shouldRunDmd!(AutoRefAssignmentCheck!ASTCodegen)(config)) + visitors ~= new AutoRefAssignmentCheck!ASTCodegen(fileName); + + if (moduleName.shouldRunDmd!(LogicPrecedenceCheck!ASTCodegen)(config)) + visitors ~= new LogicPrecedenceCheck!ASTCodegen( + fileName, + config.logical_precedence_check == Check.skipTests && !ut + ); + + if (moduleName.shouldRunDmd!(UnusedLabelCheck!ASTCodegen)(config)) + visitors ~= new UnusedLabelCheck!ASTCodegen( + fileName, + config.unused_label_check == Check.skipTests && !ut + ); + + if (moduleName.shouldRunDmd!(BuiltinPropertyNameCheck!ASTCodegen)(config)) + visitors ~= new BuiltinPropertyNameCheck!ASTCodegen(fileName); + + if (moduleName.shouldRunDmd!(PokemonExceptionCheck!ASTCodegen)(config)) + visitors ~= new PokemonExceptionCheck!ASTCodegen( + fileName, + config.exception_check == Check.skipTests && !ut + ); + + if (moduleName.shouldRunDmd!(BackwardsRangeCheck!ASTCodegen)(config)) + visitors ~= new BackwardsRangeCheck!ASTCodegen( + fileName, + config.backwards_range_check == Check.skipTests && !ut + ); + + if (moduleName.shouldRunDmd!(ProperlyDocumentedPublicFunctions!ASTCodegen)(config)) + visitors ~= new ProperlyDocumentedPublicFunctions!ASTCodegen( + fileName, + config.properly_documented_public_functions == Check.skipTests && !ut + ); + + if (moduleName.shouldRunDmd!(RedundantParenCheck!ASTCodegen)(config)) + visitors ~= new RedundantParenCheck!ASTCodegen( + fileName, + config.redundant_parens_check == Check.skipTests && !ut + ); + + if (moduleName.shouldRunDmd!(StaticIfElse!ASTCodegen)(config)) + visitors ~= new StaticIfElse!ASTCodegen( + fileName, + config.static_if_else_check == Check.skipTests && !ut + ); + + if (moduleName.shouldRunDmd!(UselessAssertCheck!ASTCodegen)(config)) + visitors ~= new UselessAssertCheck!ASTCodegen( + fileName, + config.useless_assert_check == Check.skipTests && !ut + ); + + if (moduleName.shouldRunDmd!(AsmStyleCheck!ASTCodegen)(config)) + visitors ~= new AsmStyleCheck!ASTCodegen( + fileName, + config.asm_style_check == Check.skipTests && !ut + ); + + foreach (visitor; visitors) + { + m.accept(visitor); + + foreach (message; visitor.messages) + set.insert(message); + } + + return set; +} diff --git a/src/dscanner/analysis/static_if_else.d b/src/dscanner/analysis/static_if_else.d index 17966dab..61f47b1f 100644 --- a/src/dscanner/analysis/static_if_else.d +++ b/src/dscanner/analysis/static_if_else.d @@ -5,10 +5,8 @@ module dscanner.analysis.static_if_else; -import dparse.ast; -import dparse.lexer; import dscanner.analysis.base; -import dscanner.utils : safeAccess; +import std.stdio; /** * Checks for potentially mistaken static if / else if. @@ -22,43 +20,49 @@ import dscanner.utils : safeAccess; * * However, it's more likely that this is a mistake. */ -final class StaticIfElse : BaseAnalyzer +extern(C++) class StaticIfElse(AST) : BaseAnalyzerDmd { - alias visit = BaseAnalyzer.visit; - + alias visit = BaseAnalyzerDmd.visit; mixin AnalyzerInfo!"static_if_else_check"; - this(string fileName, bool skipTests = false) + extern(D) this(string fileName, bool skipTests = false) { - super(fileName, null, skipTests); + super(fileName, skipTests); } - override void visit(const ConditionalStatement cc) + override void visit(AST.ConditionalStatement s) { - cc.accept(this); - if (cc.falseStatement is null) + import dmd.astenums : STMT; + + if (!s.condition.isStaticIfCondition()) { + super.visit(s); return; } - const(IfStatement) ifStmt = getIfStatement(cc); - if (!ifStmt) + + s.condition.accept(this); + + if (s.ifbody) + s.ifbody.accept(this); + + if (s.elsebody) { - return; + if (s.elsebody.stmt == STMT.If) + addErrorMessage(cast(ulong) s.elsebody.loc.linnum, cast(ulong) s.elsebody.loc.charnum, + KEY, MESSAGE); + + s.elsebody.accept(this); } - addErrorMessage(ifStmt.line, ifStmt.column, KEY, "Mismatched static if. Use 'else static if' here."); - } - - const(IfStatement) getIfStatement(const ConditionalStatement cc) - { - return safeAccess(cc).falseStatement.statement.statementNoCaseNoDefault.ifStatement; } +private: enum KEY = "dscanner.suspicious.static_if_else"; + enum MESSAGE = "Mismatched static if. Use 'else static if' here."; } unittest { - import dscanner.analysis.helpers : assertAnalyzerWarnings; + import dscanner.analysis.helpers : assertAnalyzerWarnings = assertAnalyzerWarningsDMD; import dscanner.analysis.config : StaticAnalysisConfig, Check, disabledConfig; import std.stdio : stderr; diff --git a/src/dscanner/analysis/trust_too_much.d b/src/dscanner/analysis/trust_too_much.d index f9eddf4c..e589136c 100644 --- a/src/dscanner/analysis/trust_too_much.d +++ b/src/dscanner/analysis/trust_too_much.d @@ -5,88 +5,50 @@ module dscanner.analysis.trust_too_much; -import std.stdio; -import dparse.ast; -import dparse.lexer; import dscanner.analysis.base; -import dsymbol.scope_; +import dmd.astenums : STC; /** * Checks that `@trusted` is only applied to a a single function */ -final class TrustTooMuchCheck : BaseAnalyzer +extern(C++) class TrustTooMuchCheck(AST) : BaseAnalyzerDmd { -private: + mixin AnalyzerInfo!"trust_too_much"; + alias visit = BaseAnalyzerDmd.visit; - static immutable MESSAGE = "Trusting a whole scope is a bad idea, " ~ +private: + extern(D) static immutable MESSAGE = "Trusting a whole scope is a bad idea, " ~ "`@trusted` should only be attached to the functions individually"; - static immutable string KEY = "dscanner.trust_too_much"; - - bool checkAtAttribute = true; + extern(D) static immutable string KEY = "dscanner.trust_too_much"; public: - - alias visit = BaseAnalyzer.visit; - - mixin AnalyzerInfo!"trust_too_much"; - /// - this(string fileName, bool skipTests = false) + extern(D) this(string fileName, bool skipTests = false) { - super(fileName, sc, skipTests); + super(fileName, skipTests); } - override void visit(const AtAttribute d) + override void visit(AST.StorageClassDeclaration scd) { - 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.unittest_ is null && - d.constructor is null && d.destructor is null && - d.staticConstructor is null && d.staticDestructor is null && - d.sharedStaticConstructor is null && d.sharedStaticDestructor is null; - d.accept(this); - checkAtAttribute = oldCheckAtAttribute; - } - - // issue #588 - override void visit(const AliasDeclaration d) - { - const oldCheckAtAttribute = checkAtAttribute; - checkAtAttribute = false; - d.accept(this); - checkAtAttribute = oldCheckAtAttribute; + if (scd.stc & STC.trusted) + addErrorMessage(cast(ulong) scd.loc.linnum, cast(ulong) scd.loc.charnum, + KEY, MESSAGE); + + super.visit(scd); } } unittest { import dscanner.analysis.config : StaticAnalysisConfig, Check, disabledConfig; - import dscanner.analysis.helpers : assertAnalyzerWarnings; + import dscanner.analysis.helpers : assertAnalyzerWarnings = assertAnalyzerWarningsDMD; import std.format : format; + import std.stdio : stderr; StaticAnalysisConfig sac = disabledConfig(); sac.trust_too_much = Check.enabled; - const msg = TrustTooMuchCheck.MESSAGE; + const msg = "Trusting a whole scope is a bad idea, " ~ + "`@trusted` should only be attached to the functions individually"; //--- fail cases ---// @@ -157,4 +119,4 @@ unittest }c , sac); stderr.writeln("Unittest for TrustTooMuchCheck passed."); -} +} \ No newline at end of file diff --git a/src/dscanner/analysis/unused.d b/src/dscanner/analysis/unused.d index 9934a37a..7f71f220 100644 --- a/src/dscanner/analysis/unused.d +++ b/src/dscanner/analysis/unused.d @@ -223,7 +223,7 @@ abstract class UnusedIdentifierCheck : BaseAnalyzer else if (idt.templateInstance && idt.templateInstance.identifier != tok!"") variableUsed(idt.templateInstance.identifier.text); } - if (mixinDepth > 0 && primary.primary == tok!"stringLiteral" + if ((mixinDepth > 0 && primary.primary == tok!"stringLiteral") || primary.primary == tok!"wstringLiteral" || primary.primary == tok!"dstringLiteral") { diff --git a/src/dscanner/analysis/unused_label.d b/src/dscanner/analysis/unused_label.d index b70d3986..37a232da 100644 --- a/src/dscanner/analysis/unused_label.d +++ b/src/dscanner/analysis/unused_label.d @@ -5,128 +5,131 @@ module dscanner.analysis.unused_label; import dscanner.analysis.base; -import dscanner.analysis.helpers; -import dparse.ast; -import dparse.lexer; -import dsymbol.scope_ : Scope; -import std.algorithm.iteration : each; +import dmd.tokens; /** * Checks for labels that are never used. */ -final class UnusedLabelCheck : BaseAnalyzer +extern (C++) class UnusedLabelCheck(AST) : BaseAnalyzerDmd { - alias visit = BaseAnalyzer.visit; - + alias visit = BaseAnalyzerDmd.visit; mixin AnalyzerInfo!"unused_label_check"; - /// - this(string fileName, const(Scope)* sc, bool skipTests = false) + extern (D) this(string fileName, bool skipTests = false) { - super(fileName, sc, skipTests); + super(fileName, skipTests); } - override void visit(const Module mod) + override void visit(AST.Module m) { pushScope(); - mod.accept(this); + super.visit(m); popScope(); } - override void visit(const FunctionLiteralExpression flit) + override void visit(AST.LabelStatement ls) { - if (flit.specifiedFunctionBody) + Label* label = ls.ident.toString() in current; + + if (label is null) + { + current[ls.ident.toString()] = Label(ls.ident.toString(), + ls.loc.linnum, ls.loc.charnum, false); + } + else { - pushScope(); - flit.specifiedFunctionBody.accept(this); - popScope(); + label.line = ls.loc.linnum; + label.column = ls.loc.charnum; } + + super.visit(ls); } - override void visit(const FunctionBody functionBody) + override void visit(AST.GotoStatement gs) { - if (functionBody.specifiedFunctionBody !is null) - { - pushScope(); - functionBody.specifiedFunctionBody.accept(this); - popScope(); - } - if (functionBody.missingFunctionBody && functionBody.missingFunctionBody.functionContracts) - functionBody.missingFunctionBody.functionContracts.each!((a){pushScope(); a.accept(this); popScope();}); + if (gs.ident) + labelUsed(gs.ident.toString()); } - override void visit(const LabeledStatement labeledStatement) + override void visit(AST.BreakStatement bs) { - 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); - } - else - { - label.line = token.line; - label.column = token.column; - } - if (labeledStatement.declarationOrStatement !is null) - labeledStatement.declarationOrStatement.accept(this); + if (bs.ident) + labelUsed(bs.ident.toString()); } - override void visit(const ContinueStatement contStatement) + override void visit(AST.StaticForeachStatement s) { - if (contStatement.label.text.length) - labelUsed(contStatement.label.text); + if (s.sfe.aggrfe) + super.visit(s.sfe.aggrfe); + + if (s.sfe.rangefe) + super.visit(s.sfe.rangefe); } - override void visit(const BreakStatement breakStatement) + override void visit(AST.ContinueStatement cs) { - if (breakStatement.label.text.length) - labelUsed(breakStatement.label.text); + if (cs.ident) + labelUsed(cs.ident.toString()); } - override void visit(const GotoStatement gotoStatement) + override void visit(AST.FuncDeclaration fd) { - if (gotoStatement.label.text.length) - labelUsed(gotoStatement.label.text); + pushScope(); + super.visit(fd); + popScope(); } - override void visit(const AsmInstruction instr) + override void visit(AST.FuncLiteralDeclaration fd) { - instr.accept(this); + pushScope(); + super.visit(fd); + popScope(); + } + override void visit(AST.AsmStatement as) + { + if (!as.tokens) + return; + + // Look for jump instructions bool jmp; - if (instr.identifierOrIntegerOrOpcode.text.length) - jmp = instr.identifierOrIntegerOrOpcode.text[0] == 'j'; + if (getFirstLetterOf(cast(char*) as.tokens[0].ptr) == 'j') + jmp = true; - if (!jmp || !instr.operands || instr.operands.operands.length != 1) - return; + // Last argument of the jmp instruction will be the label + Token* label; + for (label = as.tokens; label.next; label = label.next) {} - const AsmExp e = cast(AsmExp) instr.operands.operands[0]; - if (e.left && cast(AsmBrExp) e.left) - { - const AsmBrExp b = cast(AsmBrExp) e.left; - if (b && b.asmUnaExp && b.asmUnaExp.asmPrimaryExp) - { - const AsmPrimaryExp p = b.asmUnaExp.asmPrimaryExp; - if (p && p.identifierChain && p.identifierChain.identifiers.length == 1) - labelUsed(p.identifierChain.identifiers[0].text); - } - } + if (jmp && label.ident) + labelUsed(label.ident.toString()); + } + + private char getFirstLetterOf(char* str) + { + import std.ascii : isAlpha; + + if (str is null) + return '\0'; + + while (str && !isAlpha(*str)) + str++; + + return *str; } private: static struct Label { - string name; + const(char)[] name; size_t line; size_t column; bool used; } - Label[string][] stack; + extern (D) Label[const(char)[]][] stack; - auto ref current() + extern (D) auto ref current() { return stack[$ - 1]; } @@ -138,6 +141,8 @@ private: void popScope() { + import std.conv : to; + foreach (label; current.byValue()) { if (label.line == size_t.max || label.column == size_t.max) @@ -147,13 +152,13 @@ private: else if (!label.used) { addErrorMessage(label.line, label.column, "dscanner.suspicious.unused_label", - "Label \"" ~ label.name ~ "\" is not used."); + "Label \"" ~ to!string(label.name) ~ "\" is not used."); } } stack.length--; } - void labelUsed(string name) + extern (D) void labelUsed(const(char)[] name) { Label* entry = name in current; if (entry is null) @@ -165,12 +170,13 @@ private: unittest { - import dscanner.analysis.config : Check, StaticAnalysisConfig, disabledConfig; + import dscanner.analysis.helpers : assertAnalyzerWarningsDMD; + import dscanner.analysis.config : StaticAnalysisConfig, Check, disabledConfig; import std.stdio : stderr; StaticAnalysisConfig sac = disabledConfig(); sac.unused_label_check = Check.enabled; - assertAnalyzerWarnings(q{ + assertAnalyzerWarningsDMD(q{ int testUnusedLabel() { int x = 0; @@ -209,7 +215,7 @@ unittest } }c, sac); - assertAnalyzerWarnings(q{ + assertAnalyzerWarningsDMD(q{ void testAsm() { asm { jmp lbl;} @@ -217,7 +223,7 @@ unittest } }c, sac); - assertAnalyzerWarnings(q{ + assertAnalyzerWarningsDMD(q{ void testAsm() { asm { mov RAX,1;} @@ -226,7 +232,7 @@ unittest }c, sac); // from std.math - assertAnalyzerWarnings(q{ + assertAnalyzerWarningsDMD(q{ real polyImpl() { asm { jecxz return_ST; @@ -235,7 +241,7 @@ unittest }c, sac); // a label might be hard to find, e.g. in a mixin - assertAnalyzerWarnings(q{ + assertAnalyzerWarningsDMD(q{ real polyImpl() { mixin("return_ST: return 1;"); asm { @@ -244,5 +250,16 @@ unittest } }c, sac); + assertAnalyzerWarningsDMD(q{ + void testAsm() + { + asm nothrow @nogc + { + "movgr2fcsr $r0,%0" : + : "r" (newState & (roundingMask | allExceptions)); + } + } + }c, sac); + stderr.writeln("Unittest for UnusedLabelCheck passed."); } diff --git a/src/dscanner/analysis/useless_assert.d b/src/dscanner/analysis/useless_assert.d index 0b75ff20..efe945fa 100644 --- a/src/dscanner/analysis/useless_assert.d +++ b/src/dscanner/analysis/useless_assert.d @@ -7,94 +7,35 @@ module dscanner.analysis.useless_assert; import dscanner.analysis.base; import dscanner.analysis.helpers; -import dparse.ast; -import dparse.lexer; import std.stdio; -auto filterChars(string chars, S)(S str) -{ - import std.algorithm.comparison : among; - import std.algorithm.iteration : filter; - import std.meta : aliasSeqOf; - return str.filter!(c => !c.among(aliasSeqOf!chars)); -} - /** * Checks for asserts that always succeed */ -final class UselessAssertCheck : BaseAnalyzer +extern(C++) class UselessAssertCheck(AST) : BaseAnalyzerDmd { - alias visit = BaseAnalyzer.visit; - + alias visit = BaseAnalyzerDmd.visit; mixin AnalyzerInfo!"useless_assert_check"; /// - this(string fileName, bool skipTests = false) + extern(D) this(string fileName, bool skipTests = false) { - super(fileName, null, skipTests); + super(fileName, skipTests); } - override void visit(const AssertExpression ae) + override void visit(AST.AssertExp ae) { - import std.conv : to; + auto ie = ae.e1.isIntegerExp(); + if (ie && ie.getInteger() != 0) + addErrorMessage(cast(ulong) ae.loc.linnum, cast(ulong) ae.loc.charnum, KEY, MESSAGE); - UnaryExpression unary = cast(UnaryExpression) ae.assertArguments.assertion; - if (unary is null) - return; - if (unary.primaryExpression is null) - return; - immutable token = unary.primaryExpression.primary; - immutable skipSwitch = unary.primaryExpression.arrayLiteral !is null - || unary.primaryExpression.assocArrayLiteral !is null - || unary.primaryExpression.functionLiteralExpression !is null; - if (!skipSwitch) switch (token.type) - { - case tok!"doubleLiteral": - if (!token.text.filterChars!"Ll".to!double) - return; - break; - case tok!"floatLiteral": - if (!token.text.filterChars!"Ff".to!float) - return; - break; - case tok!"idoubleLiteral": - case tok!"ifloatLiteral": - case tok!"irealLiteral": - return; // `to` doesn't support imaginary numbers - case tok!"intLiteral": - if (!token.text.to!int) - return; - break; - case tok!"longLiteral": - if (!token.text.filterChars!"Ll".to!long) - return; - break; - case tok!"realLiteral": - if (!token.text.to!real) - return; - break; - case tok!"uintLiteral": - if (!token.text.filterChars!"Uu".to!uint) - return; - break; - case tok!"ulongLiteral": - if (!token.text.filterChars!"UuLl".to!ulong) - return; - break; - case tok!"characterLiteral": - if (token.text == `'\0'`) - return; - break; - case tok!"dstringLiteral": - case tok!"stringLiteral": - case tok!"wstringLiteral": - case tok!"true": - break; - default: - return; - } - addErrorMessage(ae.line, ae.column, KEY, MESSAGE); + auto re = ae.e1.isRealExp(); + if (re && re.value != 0) + addErrorMessage(cast(ulong) ae.loc.linnum, cast(ulong) ae.loc.charnum, KEY, MESSAGE); + + if (ae.e1.isStringExp() || ae.e1.isArrayLiteralExp() || ae.e1.isAssocArrayLiteralExp()) + addErrorMessage(cast(ulong) ae.loc.linnum, cast(ulong) ae.loc.charnum, KEY, MESSAGE); } private: @@ -108,20 +49,20 @@ unittest import dscanner.analysis.config : StaticAnalysisConfig, Check, disabledConfig; import std.format : format; + alias assertAnalyzerWarnings = assertAnalyzerWarningsDMD; + StaticAnalysisConfig sac = disabledConfig(); sac.useless_assert_check = Check.enabled; assertAnalyzerWarnings(q{ unittest { - assert(true); // [warn]: %1$s - assert(1); // [warn]: %1$s - assert([10]); // [warn]: %1$s + assert(true); // [warn]: Assert condition is always true. + assert(1); // [warn]: Assert condition is always true. + assert([10]); // [warn]: Assert condition is always true. assert(false); assert(0); assert(0.0L); } - -}c - .format(UselessAssertCheck.MESSAGE), sac); +}c, sac); stderr.writeln("Unittest for UselessAssertCheck passed."); } diff --git a/src/dscanner/imports.d b/src/dscanner/imports.d index b2b6fccc..072eba85 100644 --- a/src/dscanner/imports.d +++ b/src/dscanner/imports.d @@ -5,75 +5,81 @@ module dscanner.imports; -import dparse.ast; -import dparse.lexer; -import dparse.parser; -import dparse.rollback_allocator; import std.stdio; import std.container.rbtree; import std.functional : toDelegate; import dscanner.utils; +import dmd.permissivevisitor; +import dmd.transitivevisitor; +import dmd.tokens; +import dmd.common.outbuffer; +import core.stdc.stdio; +import dmd.parse; +import dmd.astbase; +import dmd.id; +import dmd.globals; +import dmd.identifier; +import core.memory; +import std.stdio; +import std.file; +import std.conv : to; -/** - * AST visitor that collects modules imported to an R-B tree. - */ -class ImportPrinter : ASTVisitor +extern(C++) class ImportVisitor(AST) : ParseTimeTransitiveVisitor!AST { + alias visit = ParseTimeTransitiveVisitor!AST.visit; + this() { imports = new RedBlackTree!string; } - override void visit(const SingleImport singleImport) - { - ignore = false; - singleImport.accept(this); - ignore = true; - } - - override void visit(const IdentifierChain identifierChain) - { - if (ignore) - return; - bool first = true; + override void visit(AST.Import imp) + { + import std.conv : to; string s; - foreach (ident; identifierChain.identifiers) - { - if (!first) - s ~= "."; - s ~= ident.text; - first = false; - } - imports.insert(s); - } - alias visit = ASTVisitor.visit; + foreach (const pid; imp.packages) + s = s ~ to!string(pid.toChars()) ~ "."; - /// Collected imports - RedBlackTree!string imports; + s ~= to!string(imp.id.toChars()); + imports.insert(s); + } -private: - bool ignore = true; + RedBlackTree!string imports; } -private void visitFile(bool usingStdin, string fileName, RedBlackTree!string importedModules, StringCache* cache) +private void visitFile(bool usingStdin, string fileName, RedBlackTree!string importedModules) { - RollbackAllocator rba; - LexerConfig config; - config.fileName = fileName; - config.stringBehavior = StringBehavior.source; - auto visitor = new ImportPrinter; - auto tokens = getTokensForParser(usingStdin ? readStdin() : readFile(fileName), config, cache); - auto mod = parseModule(tokens, fileName, &rba, toDelegate(&doNothing)); - visitor.visit(mod); - importedModules.insert(visitor.imports[]); + import dmd.errorsink : ErrorSinkNull; + + Id.initialize(); + global._init(); + global.params.useUnitTests = true; + ASTBase.Type._init(); + + auto id = Identifier.idPool(fileName); + auto m = new ASTBase.Module(&(fileName.dup)[0], id, false, false); + ubyte[] bytes = usingStdin ? readStdin() : readFile(fileName); + auto input = cast(char[]) bytes; + + __gshared ErrorSinkNull errorSinkNull; + if (!errorSinkNull) + errorSinkNull = new ErrorSinkNull; + + scope p = new Parser!ASTBase(m, input, false, new ErrorSinkNull, null, false); + p.nextToken(); + m.members = p.parseModule(); + + scope vis = new ImportVisitor!ASTBase(); + m.accept(vis); + importedModules.insert(vis.imports[]); } private void doNothing(string, size_t, size_t, string, bool) { } -void printImports(bool usingStdin, string[] args, string[] importPaths, StringCache* cache, bool recursive) +void printImports(bool usingStdin, string[] args, string[] importPaths, bool recursive) { string[] fileNames = usingStdin ? ["stdin"] : expandArgs(args); import std.path : buildPath, dirSeparator; @@ -85,7 +91,7 @@ void printImports(bool usingStdin, string[] args, string[] importPaths, StringCa auto resolvedLocations = new RedBlackTree!(string); auto importedFiles = new RedBlackTree!(string); foreach (name; fileNames) - visitFile(usingStdin, name, importedFiles, cache); + visitFile(usingStdin, name, importedFiles); if (importPaths.empty) { foreach (item; importedFiles[]) @@ -110,7 +116,7 @@ void printImports(bool usingStdin, string[] args, string[] importPaths, StringCa resolvedModules.insert(item); resolvedLocations.insert(alt); if (recursive) - visitFile(false, alt, newlyDiscovered, cache); + visitFile(false, alt, newlyDiscovered); continue itemLoop; } } @@ -126,3 +132,37 @@ void printImports(bool usingStdin, string[] args, string[] importPaths, StringCa foreach (resolved; resolvedLocations[]) writeln(resolved); } + +unittest +{ + import std.stdio : File; + import std.file : exists, remove; + + auto deleteme = "test.txt"; + File file = File(deleteme, "w"); + scope(exit) + { + assert(exists(deleteme)); + remove(deleteme); + } + + file.write(q{ + import std.stdio; + import std.fish : scales, head; + import DAGRON = std.experimental.dragon; + import std.file; + }); + + file.close(); + + auto importedFiles = new RedBlackTree!(string); + visitFile(false, deleteme, importedFiles); + + auto expected = new RedBlackTree!(string); + expected.insert("std.stdio"); + expected.insert("std.fish"); + expected.insert("std.file"); + expected.insert("std.experimental.dragon"); + + assert(expected == importedFiles); +} diff --git a/src/dscanner/main.d b/src/dscanner/main.d index 87de4ae4..e352f7b9 100644 --- a/src/dscanner/main.d +++ b/src/dscanner/main.d @@ -298,7 +298,7 @@ else } else if (imports || recursiveImports) { - printImports(usingStdin, args, importPaths, &cache, recursiveImports); + printImports(usingStdin, args, importPaths, recursiveImports); } else if (ast || outline) { @@ -468,7 +468,7 @@ string getDefaultConfigurationLocation() configDir = buildPath(configDir, "dscanner", CONFIG_FILE_NAME); return configDir; } - else version(Windows) + else version (Windows) { string configDir = environment.get("APPDATA", null); enforce(configDir !is null, "%APPDATA% is unset"); diff --git a/src/dscanner/utils.d b/src/dscanner/utils.d index 4c7edb7d..9ccefcfa 100644 --- a/src/dscanner/utils.d +++ b/src/dscanner/utils.d @@ -7,6 +7,10 @@ import std.encoding : BOM, BOMSeq, EncodingException, getBOM; import std.format : format; import std.file : exists, read; +import dmd.astbase : ASTBase; +import dmd.parse : Parser; +import dmd.astcodegen; + private void processBOM(ref ubyte[] sourceCode, string fname) { enum spec = "D-Scanner does not support %s-encoded files (%s)"; @@ -244,3 +248,24 @@ auto ref safeAccess(M)(M m) { return SafeAccess!M(m); } + +/** + * Return the module name from a ModuleDeclaration instance with the following format: `foo.bar.module` + */ +const(char[]) getModuleName(ASTCodegen.ModuleDeclaration *mdptr) +{ + import std.array : array, join; + + if (mdptr !is null) + { + import std.algorithm : map; + ASTCodegen.ModuleDeclaration md = *mdptr; + + if (md.packages.length != 0) + return join(md.packages.map!(e => e.toString()).array ~ md.id.toString().dup, "."); + else + return md.id.toString(); + } + + return ""; +} \ No newline at end of file