From 010ac1d3b7234700782a27625ab350ddf88c3538 Mon Sep 17 00:00:00 2001 From: lucica28 <57060141+lucica28@users.noreply.github.com> Date: Fri, 25 Nov 2022 12:57:48 +0200 Subject: [PATCH] replace libdparse in constructor check (#43) --- changelog/dscanner.struct-ctor-check.dd | 19 ++++ src/dscanner/analysis/constructors.d | 142 +++++++++--------------- src/dscanner/analysis/run.d | 7 +- 3 files changed, 74 insertions(+), 94 deletions(-) create mode 100644 changelog/dscanner.struct-ctor-check.dd diff --git a/changelog/dscanner.struct-ctor-check.dd b/changelog/dscanner.struct-ctor-check.dd new file mode 100644 index 0000000..27940c8 --- /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/src/dscanner/analysis/constructors.d b/src/dscanner/analysis/constructors.d index c87f36b..84765d0 100644 --- a/src/dscanner/analysis/constructors.d +++ b/src/dscanner/analysis/constructors.d @@ -1,104 +1,63 @@ module dscanner.analysis.constructors; -import dparse.ast; -import dparse.lexer; import std.stdio; -import std.typecons : Rebindable; import dscanner.analysis.base; import dscanner.analysis.helpers; -import dsymbol.scope_ : Scope; -final class ConstructorCheck : BaseAnalyzer +extern(C++) class ConstructorCheck(AST) : BaseAnalyzerDmd { - alias visit = BaseAnalyzer.visit; - + alias visit = BaseAnalyzerDmd.visit; mixin AnalyzerInfo!"constructor_check"; - this(BaseAnalyzerArguments args) + extern(D) this(string fileName) { - super(args); + super(fileName); } - override void visit(const ClassDeclaration classDeclaration) + override void visit(AST.ClassDeclaration d) { - const oldHasDefault = hasDefaultArgConstructor; - const oldHasNoArg = hasNoArgConstructor; - hasNoArgConstructor = null; - hasDefaultArgConstructor = null; - immutable State prev = state; - state = State.inClass; - classDeclaration.accept(this); + bool hasDefaultArgConstructor = false; + bool hasNoArgConstructor = false; + + if (d.members) + { + foreach (s; *d.members) + { + if (auto cd = s.isCtorDeclaration()) + { + auto tf = cd.type.isTypeFunction(); + + if (tf) + { + if (tf.parameterList.parameters.length == 0) + hasNoArgConstructor = true; + else + { + // Check if all parameters have a default value + hasDefaultArgConstructor = true; + + foreach (param; *tf.parameterList.parameters) + if (param.defaultArg is null) + hasDefaultArgConstructor = false; + } + } + } + + s.accept(this); + } + } + if (hasNoArgConstructor && hasDefaultArgConstructor) { - addErrorMessage( - Message.Diagnostic.from(fileName, classDeclaration.name, - "This class has a zero-argument constructor as well as a" - ~ " constructor with one default argument. This can be confusing."), - [ - Message.Diagnostic.from(fileName, hasNoArgConstructor, "zero-argument constructor defined here"), - Message.Diagnostic.from(fileName, hasDefaultArgConstructor, "default argument constructor defined here") - ], - "dscanner.confusing.constructor_args" - ); - } - hasDefaultArgConstructor = oldHasDefault; - hasNoArgConstructor = oldHasNoArg; - state = prev; - } - - override void visit(const StructDeclaration structDeclaration) - { - immutable State prev = state; - state = State.inStruct; - structDeclaration.accept(this); - state = prev; - } - - override void visit(const Constructor constructor) - { - final switch (state) - { - case State.inStruct: - if (constructor.parameters.parameters.length == 1 - && constructor.parameters.parameters[0].default_ !is null) - { - const(Token)[] tokens = constructor.parameters.parameters[0].default_.tokens; - assert(tokens.length); - // we extend the token range to the `=` sign, since it's continuous - tokens = (tokens.ptr - 1)[0 .. tokens.length + 1]; - addErrorMessage(tokens, - "dscanner.confusing.struct_constructor_default_args", - "This struct constructor can never be called with its " - ~ "default argument."); - } - break; - case State.inClass: - if (constructor.parameters.parameters.length == 1 - && constructor.parameters.parameters[0].default_ !is null) - { - hasDefaultArgConstructor = constructor; - } - else if (constructor.parameters.parameters.length == 0) - hasNoArgConstructor = constructor; - break; - case State.ignoring: - break; + addErrorMessage(cast(ulong) d.loc.linnum, + cast(ulong) d.loc.charnum, KEY, MESSAGE); } } - + private: - - enum State : ubyte - { - ignoring, - inClass, - inStruct - } - - State state; - - Rebindable!(const Constructor) hasNoArgConstructor; - Rebindable!(const Constructor) hasDefaultArgConstructor; + 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 @@ -107,20 +66,23 @@ unittest StaticAnalysisConfig sac = disabledConfig(); sac.constructor_check = Check.enabled; - // TODO: test supplemental diagnostics - assertAnalyzerWarnings(q{ - class Cat /+ - ^^^ [warn]: This class has a zero-argument constructor as well as a constructor with one default argument. This can be confusing. +/ + 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 = "doggie") {} /+ - ^^^^^^^^^^ [warn]: This struct constructor can never be called with its default argument. +/ + this(string name = "kittie", int x = 2) {} + } + + class Cat + { + this() {} + this(string name = "kittie", int x) {} } }c, sac); diff --git a/src/dscanner/analysis/run.d b/src/dscanner/analysis/run.d index 4459fc5..be6fb41 100644 --- a/src/dscanner/analysis/run.d +++ b/src/dscanner/analysis/run.d @@ -840,10 +840,6 @@ private BaseAnalyzer[] getAnalyzersForModuleAndConfig(string fileName, checks ~= new CommaExpressionCheck(args.setSkipTests( analysisConfig.comma_expression_check == Check.skipTests && !ut)); - if (moduleName.shouldRun!ConstructorCheck(analysisConfig)) - checks ~= new ConstructorCheck(args.setSkipTests( - analysisConfig.constructor_check == Check.skipTests && !ut)); - if (moduleName.shouldRun!UnmodifiedFinder(analysisConfig)) checks ~= new UnmodifiedFinder(args.setSkipTests( analysisConfig.could_be_immutable_check == Check.skipTests && !ut)); @@ -1324,6 +1320,9 @@ MessageSet analyzeDmd(string fileName, ASTBase.Module m, const char[] moduleName if (moduleName.shouldRunDmd!(ExplicitlyAnnotatedUnittestCheck!ASTBase)(config)) visitors ~= new ExplicitlyAnnotatedUnittestCheck!ASTBase(fileName); + if (moduleName.shouldRunDmd!(ConstructorCheck!ASTBase)(config)) + visitors ~= new ConstructorCheck!ASTBase(fileName); + foreach (visitor; visitors) { m.accept(visitor);