diff --git a/docs/sdk/script/Diagnostics.xml b/docs/sdk/script/Diagnostics.xml index 246bd90da..290eb1f9a 100644 --- a/docs/sdk/script/Diagnostics.xml +++ b/docs/sdk/script/Diagnostics.xml @@ -229,6 +229,24 @@ func g(a, ...) { + + suspicious_assignment + + + An assignment was found where an expression was expected. While + an assignment returns its own value, usually a comparison was + intended instead. + + + if (a = b + 1) { /* Do something */} + + + If the assignment is intentional, make this more obvious by + extracting it to a separate statement, or explicitly handle the + boolean conversion by adding a comparison operator. + + + diff --git a/src/script/C4AulCompiler.cpp b/src/script/C4AulCompiler.cpp index 1e2a95725..e76d02187 100644 --- a/src/script/C4AulCompiler.cpp +++ b/src/script/C4AulCompiler.cpp @@ -2,7 +2,7 @@ * OpenClonk, http://www.openclonk.org * * Copyright (c) 2001-2009, RedWolf Design GmbH, http://www.clonk.de/ - * Copyright (c) 2009-2016, The OpenClonk Team and contributors + * Copyright (c) 2009-2018, The OpenClonk Team and contributors * * Distributed under the terms of the ISC license; see accompanying file * "COPYING" for details. @@ -255,6 +255,14 @@ class C4AulCompiler::CodegenAstVisitor : public ::aul::DefaultRecursiveVisitor } }; + void WarnOnAssignment(const ::aul::ast::ExprPtr &n) const + { + if (dynamic_cast(n.get()) != nullptr) + { + Warn(target_host, host, n.get(), Fn, C4AulWarningId::suspicious_assignment); + } + } + public: CodegenAstVisitor(C4ScriptHost *host, C4ScriptHost *source_host) : target_host(host), host(source_host) {} explicit CodegenAstVisitor(C4AulScriptFunc *func) : Fn(func), target_host(func->pOrgScript), host(target_host) {} @@ -1415,6 +1423,7 @@ void C4AulCompiler::CodegenAstVisitor::visit(const ::aul::ast::Return *n) { StackGuard g(this, 0); + WarnOnAssignment(n->value); SafeVisit(n->value); AddBCC(n->loc, AB_RETURN); } @@ -1442,6 +1451,7 @@ void C4AulCompiler::CodegenAstVisitor::visit(const ::aul::ast::ForLoop *n) PushLoop(); if (n->cond) { + WarnOnAssignment(n->cond); cond = AddJumpTarget(); SafeVisit(n->cond); active_loops.top().breaks.push_back(AddBCC(n->cond->loc, AB_CONDN)); @@ -1560,6 +1570,7 @@ void C4AulCompiler::CodegenAstVisitor::visit(const ::aul::ast::WhileLoop *n) { int cond = AddJumpTarget(); PushLoop(); + WarnOnAssignment(n->cond); SafeVisit(n->cond); active_loops.top().breaks.push_back(AddBCC(n->cond->loc, AB_CONDN)); if (SafeVisit(n->body)) @@ -1583,6 +1594,7 @@ void C4AulCompiler::CodegenAstVisitor::visit(const ::aul::ast::Continue *n) void C4AulCompiler::CodegenAstVisitor::visit(const ::aul::ast::If *n) { + WarnOnAssignment(n->cond); SafeVisit(n->cond); int jump = AddBCC(n->loc, AB_CONDN); // Warn if we're controlling a no-op ("if (...);") diff --git a/src/script/C4AulWarnings.h b/src/script/C4AulWarnings.h index bea1236e2..02bd78ede 100644 --- a/src/script/C4AulWarnings.h +++ b/src/script/C4AulWarnings.h @@ -1,7 +1,7 @@ /* * OpenClonk, http://www.openclonk.org * -* Copyright (c) 2017, The OpenClonk Team and contributors +* Copyright (c) 2017-2018, The OpenClonk Team and contributors * * Distributed under the terms of the ISC license; see accompanying file * "COPYING" for details. @@ -38,5 +38,6 @@ DIAG(undeclared_varargs, "use of '%s' in a function forces it to take variable a DIAG(arg_count_mismatch, "call to '%s' passes %u arguments, of which only %u are used", true) DIAG(arg_type_mismatch, "parameter %u of call to '%s' passes %s (%s expected)", true) DIAG(empty_if, "empty controlled statement (use '{}' if this is intentional)", true) +DIAG(suspicious_assignment, "suspicious assignment (was a comparison intended here?)", true) #pragma pop_macro("DIAG") diff --git a/tests/aul/AulDiagnosticsTest.cpp b/tests/aul/AulDiagnosticsTest.cpp index d27801670..751d5b165 100644 --- a/tests/aul/AulDiagnosticsTest.cpp +++ b/tests/aul/AulDiagnosticsTest.cpp @@ -1,7 +1,7 @@ /* * OpenClonk, http://www.openclonk.org * - * Copyright (c) 2015-2017, The OpenClonk Team and contributors + * Copyright (c) 2015-2018, The OpenClonk Team and contributors * * Distributed under the terms of the ISC license; see accompanying file * "COPYING" for details. @@ -85,6 +85,50 @@ TEST_F(AulDiagnosticsTest, empty_if) } } +TEST_F(AulDiagnosticsTest, suspicious_assignment) +{ + { + EXPECT_WARNING(suspicious_assignment); + RunCode("var a = 0; if (a = 0) {}"); + } + { + EXPECT_WARNING(suspicious_assignment).Times(0); + RunCode("var a = 0; if (a == 1) {}"); + RunCode("var a = 0; if (a += 1) {}"); + } + { + EXPECT_WARNING(suspicious_assignment); + RunCode("for (var a = 0; a = 0;) {}"); + } + { + EXPECT_WARNING(suspicious_assignment).Times(0); + RunCode("for (var a = 0; a == 1;) {}"); + RunCode("for (var a = 0; a != 0;) {}"); + } + { + EXPECT_WARNING(suspicious_assignment); + RunCode("var a = 0; while (a = 0) {}"); + } + { + EXPECT_WARNING(suspicious_assignment).Times(0); + RunCode("var a = 0; while (a == 1) {}"); + RunCode("var a = 0; while (a *= 1) {}"); + } + { + EXPECT_WARNING(suspicious_assignment); + RunCode("var a = 0; return a = 0;"); + } + { + EXPECT_WARNING(suspicious_assignment); + RunCode("var a = 0; return (a = 0);"); + } + { + EXPECT_WARNING(suspicious_assignment).Times(0); + RunCode("var a = 0; return a == 0;"); + RunCode("var a = 0; return (a = 0) == 0;"); + } +} + TEST_F(AulDiagnosticsTest, arg_count_mismatch) { {