diff --git a/src/dscanner/analysis/unmodified.d b/src/dscanner/analysis/unmodified.d index 6dd3d8c1..b5c1fc92 100644 --- a/src/dscanner/analysis/unmodified.d +++ b/src/dscanner/analysis/unmodified.d @@ -142,11 +142,43 @@ final class UnmodifiedFinder : BaseAnalyzer mixin PartsMightModify!AsmPrimaryExp; mixin PartsMightModify!IndexExpression; - mixin PartsMightModify!FunctionCallExpression; - mixin PartsMightModify!NewExpression; mixin PartsMightModify!IdentifierOrTemplateChain; mixin PartsMightModify!ReturnStatement; + override void visit(const FunctionCallExpression functionCallExpression) + { + interest++; + if (functionCallExpression.type !is null) + functionCallExpression.type.accept(this); + if (functionCallExpression.unaryExpression !is null) + functionCallExpression.unaryExpression.accept(this); + if (functionCallExpression.templateArguments !is null) + functionCallExpression.templateArguments.accept(this); + // Arguments may be bound to `ref`/`out` parameters, which modifies + // them regardless of their type. + callArgument++; + if (functionCallExpression.arguments !is null) + functionCallExpression.arguments.accept(this); + callArgument--; + interest--; + } + + override void visit(const NewExpression newExpression) + { + interest++; + if (newExpression.newAnonClassExpression !is null) + newExpression.newAnonClassExpression.accept(this); + if (newExpression.type !is null) + newExpression.type.accept(this); + callArgument++; + if (newExpression.arguments !is null) + newExpression.arguments.accept(this); + callArgument--; + if (newExpression.assignExpression !is null) + newExpression.assignExpression.accept(this); + interest--; + } + override void visit(const UnaryExpression unary) { if (unary.prefix == tok!"++" || unary.prefix == tok!"--" @@ -161,6 +193,13 @@ final class UnmodifiedFinder : BaseAnalyzer } else unary.accept(this); + + // A member access (`a.b`) may require a mutable `a` although it reads + // like an expression: the member can be a non-const method or property + // (e.g. range accessors) or return a mutable reference. Without + // semantic analysis, const-ness of the base cannot be proven. + if (unary.identifierOrTemplateInstance !is null) + markMemberAccessBase(unary); } override void visit(const ForeachStatement foreachStatement) @@ -210,7 +249,7 @@ private: { size_t index = tree.length - 1; auto vi = VariableInfo(name); - if (guaranteeUse == 0) + if (guaranteeUse == 0 && callArgument == 0) { auto r = tree[index].equalRange(&vi); if (!r.empty && r.front.isValueType && !inAsm) @@ -224,6 +263,23 @@ private: } } + void markMemberAccessBase(const UnaryExpression memberAccess) + { + const UnaryExpression base = memberAccess.unaryExpression; + if (base is null) + return; + if (base.identifierOrTemplateInstance !is null) + { + // chained access `a.b.c`: descend to the leftmost base + markMemberAccessBase(base); + return; + } + if (base.primaryExpression !is null + && base.primaryExpression.identifierOrTemplateInstance !is null) + variableMightBeModified( + base.primaryExpression.identifierOrTemplateInstance.identifier.text); + } + bool initializedFromNew(const Initializer initializer) { if (const UnaryExpression ue = cast(UnaryExpression) safeAccess(initializer) @@ -323,6 +379,8 @@ private: int guaranteeUse; + int callArgument; + int isImmutable; bool inAsm; @@ -387,6 +445,126 @@ bool isValueTypeSimple(const Type type) pure nothrow @nogc } }, sac); + // a value type passed to a function may be bound to a `ref` parameter + + assertAnalyzerWarnings(q{ + void mutate(ref int x) + { + x = 42; + } + + int refMutation() + { + int value = 0; + mutate(value); + return value; + } + }, sac); + + // a value type passed to a function may be bound to an `out` parameter + + assertAnalyzerWarnings(q{ + long produce(out bool createdNow) + { + createdNow = true; + return 1; + } + + long outParam() + { + bool createdNow; + immutable id = produce(createdNow); + return id + (createdNow ? 1 : 0); + } + }, sac); + + // range accessors are methods that may not be const-callable, so the + // range variable cannot always be const or immutable + + assertAnalyzerWarnings(q{ + struct SinglePassRange + { + int i = 0; + + @property bool empty() + { + return i >= 1; + } + + @property int front() + { + return i; + } + } + + int rangeAccessors() + { + auto r = SinglePassRange(); + if (r.empty) + return 0; + return r.front; + } + }, sac); + + // member access on a variable may prevent const or immutable even without + // a visible modification, e.g. Nullable.get on a struct holding an array + + assertAnalyzerWarnings(q{ + struct Document + { + string name; + int[] items; + } + + string nullableWithArray() + { + import std.typecons : Nullable; + + auto doc = Nullable!Document(Document("x", [1, 2])); + if (doc.isNull) + return ""; + Document copy = doc.get; + copy.name = "y"; + return copy.name; + } + }, sac); + + // member access without a call is treated as a potential modification + // because the member can be a non-const method or property + + assertAnalyzerWarnings(q{ + struct Point + { + int x; + } + + int readMember() + { + Point p; + return p.x; + } + }, sac); + + // simple reads of value types are still reported + + assertAnalyzerWarnings(q{ + int simpleRead() + { + int i = 1; /+ + ^ [warn]: Variable i is never modified and could have been declared const or immutable. +/ + return i; + } + }, sac); + + assertAnalyzerWarnings(q{ + int readIndex(int[] arr) + { + int i = 0; /+ + ^ [warn]: Variable i is never modified and could have been declared const or immutable. +/ + return arr[i]; + } + }, sac); + assertAnalyzerWarnings(q{ @("nolint(dscanner.suspicious.unmodified)") void foo(){