Merge remote-tracking branch 'origin/topic/jsiwek/deprecation-improvements'

* origin/topic/jsiwek/deprecation-improvements:
  Fix wrong frame offsets for locals of alternate event/hook prototypes
  Add deprecation expression to deprecated prototype/parameter messages
  Improve "use of deprecated prototype" warning message
  Emit deprecation warning for use of &deprecated function parameters

CHANGES

@@ -1,4 +1,29 @@
 
+3.2.0-dev.892 | 2020-07-13 12:10:22 -0700
+
+  * Fix wrong frame offsets for locals of alternate event/hook prototypes
+
+    Local frame offsets were being assigned based on number of the alternate
+    prototype's parameters, which may end up having less total parameters
+    than the canonical prototype, causing the local value to incorrectly
+    overwrite an event/hook argument value. (Jon Siwek, Corelight)
+
+  * Add deprecation expression to deprecated prototype/parameter messages (Jon Siwek, Corelight)
+
+  * Improve "use of deprecated prototype" warning message
+
+    The location information now points out the place of the deprecated
+    prototype instead of the location where the ID was initially declared
+    (which may not itself be a deprecated prototype). (Jon Siwek, Corelight)
+
+  * Emit deprecation warning for use of &deprecated function parameters
+
+    Particularly, this is meant for using &deprecated on canonical
+    event/hook prototype parameters to encourage users to create handlers
+    to another, non-deprecated prototype.  i.e. for canonical prototypes,
+    we may not always want to put &deprecated directly on the prototype
+    itself since that signals deprecation of the ID entirely. (Jon Siwek, Corelight)
+
 3.2.0-dev.885 | 2020-07-10 11:20:41 -0700
 
   * Add more error checks to shadow log parsing (Jon Siwek, Corelight)

VERSION

@@ -1 +1 @@
-3.2.0-dev.885
+3.2.0-dev.892

src/Attr.cc

@@ -40,6 +40,18 @@ Attr::Attr(AttrTag t)
 void Attr::SetAttrExpr(ExprPtr e)
 	{ expr = std::move(e); }
 
+std::string Attr::DeprecationMessage() const
+	{
+	if ( tag != ATTR_DEPRECATED )
+		return "";
+
+	if ( ! expr )
+		return "";
+
+	auto ce = static_cast<zeek::detail::ConstExpr*>(expr.get());
+	return ce->Value()->AsStringVal()->CheckString();
+	}
+
 void Attr::Describe(ODesc* d) const
 	{
 	AddTag(d);

src/Attr.h

@@ -3,6 +3,7 @@
 #pragma once
 
 #include <vector>
+#include <string>
 
 #include "Obj.h"
 #include "BroList.h"
@@ -73,6 +74,12 @@ public:
 	void Describe(ODesc* d) const override;
 	void DescribeReST(ODesc* d, bool shorten = false) const;
 
+	/**
+	 * Returns the deprecation string associated with a &deprecated attribute
+	 * or an empty string if this is not such an attribute.
+	 */
+	std::string DeprecationMessage() const;
+
 	bool operator==(const Attr& other) const
 		{
 		if ( tag != other.tag )

src/ID.cc

@@ -294,14 +294,7 @@ std::string ID::GetDeprecationWarning() const
 	const auto& depr_attr = GetAttr(ATTR_DEPRECATED);
 
 	if ( depr_attr )
-		{
+		result = depr_attr->DeprecationMessage();
-		auto expr = static_cast<zeek::detail::ConstExpr*>(depr_attr->GetExpr().get());
-		if ( expr )
-			{
-			StringVal* text = expr->Value()->AsStringVal();
-			result = text->CheckString();
-			}
-		}
 
 	if ( result.empty() )
 		return fmt("deprecated (%s)", Name());

src/Type.cc

@@ -563,7 +563,7 @@ FuncType::FuncType(RecordTypePtr arg_args,
 		offsets[i] = i;
 		}
 
-	prototypes.emplace_back(Prototype{false, args, std::move(offsets)});
+	prototypes.emplace_back(Prototype{false, "", args, std::move(offsets)});
 	}
 
 TypePtr FuncType::ShallowClone()
@@ -1120,14 +1120,7 @@ string RecordType::GetFieldDeprecationWarning(int field, bool has_check) const
 		{
 		string result;
 		if ( const auto& deprecation = decl->GetAttr(zeek::detail::ATTR_DEPRECATED) )
-			{
+			result = deprecation->DeprecationMessage();
-			auto expr = static_cast<zeek::detail::ConstExpr*>(deprecation->GetExpr().get());
-			if ( expr )
-				{
-				StringVal* text = expr->Value()->AsStringVal();
-				result = text->CheckString();
-				}
-			}
 
 		if ( result.empty() )
 			return fmt("deprecated (%s%s$%s)", GetName().c_str(), has_check ? "?" : "",

src/Type.h

@@ -427,7 +427,10 @@ public:
 	 */
 	struct Prototype {
 		bool deprecated;
+		std::string deprecation_msg;
 		RecordTypePtr args;
+		// Maps from parameter index in canonical prototype to
+		// parameter index in this alternate prorotype.
 		std::map<int, int> offsets;
 	};
 

src/Var.cc

@@ -96,17 +96,27 @@ static bool add_prototype(const zeek::detail::IDPtr& id, zeek::Type* t,
 			return false;
 			}
 
-		offsets[i] = o;
+		offsets[o] = i;
 		}
 
 	auto deprecated = false;
+	std::string depr_msg;
 
 	if ( attrs )
 		for ( const auto& a : *attrs )
 			if ( a->Tag() == zeek::detail::ATTR_DEPRECATED )
+				{
 				deprecated = true;
+				depr_msg = a->DeprecationMessage();
+				break;
+				}
+
+	zeek::FuncType::Prototype p;
+	p.deprecated = deprecated;
+	p.deprecation_msg = std::move(depr_msg);
+	p.args = alt_args;
+	p.offsets = std::move(offsets);
 
-	zeek::FuncType::Prototype p{deprecated, alt_args, std::move(offsets)};
 	canon_ft->AddPrototype(std::move(p));
 	return true;
 	}
@@ -450,7 +460,25 @@ static std::optional<zeek::FuncType::Prototype> func_type_check(const zeek::Func
 		return {};
 		}
 
-	return decl->FindPrototype(*impl->Params());
+	auto rval = decl->FindPrototype(*impl->Params());
+
+	if ( rval )
+		for ( auto i = 0; i < rval->args->NumFields(); ++i )
+			if ( auto ad = rval->args->FieldDecl(i)->GetAttr(zeek::detail::ATTR_DEPRECATED) )
+				{
+				auto msg = ad->DeprecationMessage();
+
+				if ( msg.empty() )
+					impl->Warn(fmt("use of deprecated parameter '%s'",
+				                   rval->args->FieldName(i)),
+				               decl, true);
+				else
+					impl->Warn(fmt("use of deprecated parameter '%s': %s",
+				                   rval->args->FieldName(i), msg.data()),
+				               decl, true);
+				}
+
+	return rval;
 	}
 
 static bool canonical_arg_types_match(const zeek::FuncType* decl, const zeek::FuncType* impl)
@@ -523,7 +551,15 @@ void begin_func(zeek::detail::IDPtr id, const char* module_name,
 				}
 
 			if ( prototype->deprecated )
-				t->Warn("use of deprecated prototype", id.get());
+				{
+				if ( prototype->deprecation_msg.empty() )
+					t->Warn(fmt("use of deprecated '%s' prototype", id->Name()),
+					        prototype->args.get(), true);
+				else
+					t->Warn(fmt("use of deprecated '%s' prototype: %s",
+					            id->Name(), prototype->deprecation_msg.data()),
+					        prototype->args.get(), true);
+				}
 			}
 		else
 			{
@@ -568,24 +604,54 @@ void begin_func(zeek::detail::IDPtr id, const char* module_name,
 	else
 		id->SetType(t);
 
+	const auto& args = t->Params();
+	const auto& canon_args = id->GetType()->AsFuncType()->Params();
+
 	zeek::detail::push_scope(std::move(id), std::move(attrs));
 
-	const auto& args = t->Params();
+	for ( int i = 0; i < canon_args->NumFields(); ++i )
-	int num_args = args->NumFields();
-
-	for ( int i = 0; i < num_args; ++i )
 		{
-		zeek::TypeDecl* arg_i = args->FieldDecl(i);
+		zeek::TypeDecl* arg_i;
+		bool hide = false;
+
+		if ( prototype )
+			{
+			auto it = prototype->offsets.find(i);
+
+			if ( it == prototype->offsets.end() )
+				{
+				// Alternate prototype hides this param
+				hide = true;
+				arg_i = canon_args->FieldDecl(i);
+				}
+			else
+				{
+				// Alternate prototype maps this param to another index
+				arg_i = args->FieldDecl(it->second);
+				}
+			}
+		else
+			{
+			if ( i < args->NumFields() )
+				arg_i = args->FieldDecl(i);
+			else
+				break;
+			}
+
 		auto arg_id = zeek::detail::lookup_ID(arg_i->id, module_name);
 
 		if ( arg_id && ! arg_id->IsGlobal() )
 			arg_id->Error("argument name used twice");
 
-		arg_id = zeek::detail::install_ID(arg_i->id, module_name, false, false);
+		const char* local_name = arg_i->id;
-		arg_id->SetType(arg_i->type);
 
-		if ( prototype )
+		if ( hide )
-			arg_id->SetOffset(prototype->offsets[i]);
+			// Note the illegal '-' in hidden name implies we haven't
+			// clobbered any local variable names.
+			local_name = fmt("%s-hidden", local_name);
+
+		arg_id = zeek::detail::install_ID(local_name, module_name, false, false);
+		arg_id->SetType(arg_i->type);
 		}
 
 	if ( zeek::detail::Attr* depr_attr = find_attr(zeek::detail::current_scope()->Attrs().get(),

testing/btest/Baseline/language.alternate-event-hook-prototypes/out

@@ -1,4 +1,4 @@
-warning in /home/jon/pro/zeek/zeek/testing/btest/.tmp/language.alternate-event-hook-prototypes/alternate-event-hook-prototypes.zeek, line 68 and /home/jon/pro/zeek/zeek/testing/btest/.tmp/language.alternate-event-hook-prototypes/alternate-event-hook-prototypes.zeek, line 10: use of deprecated prototype (hook(c:count;) : bool and my_hook)
+warning in /home/jon/pro/zeek/zeek/testing/btest/.tmp/language.alternate-event-hook-prototypes/alternate-event-hook-prototypes.zeek, line 68 and /home/jon/pro/zeek/zeek/testing/btest/.tmp/language.alternate-event-hook-prototypes/alternate-event-hook-prototypes.zeek, line 13: use of deprecated 'my_hook' prototype (hook(c:count;) : bool)
 my_hook, infinite, 13
 my_hook, 13, infinite
 my_hook, infinite

testing/btest/Baseline/language.alternate-prototypes-deprecated-args/hidden-error

@@ -0,0 +1,3 @@
+warning in /home/jon/pro/zeek/zeek/testing/btest/.tmp/language.alternate-prototypes-deprecated-args/alternate-prototypes-deprecated-args.zeek, line 11 and /home/jon/pro/zeek/zeek/testing/btest/.tmp/language.alternate-prototypes-deprecated-args/alternate-prototypes-deprecated-args.zeek, line 7: use of deprecated parameter 'b': Don't use 'b' (event(a:string; b:string; c:string;))
+warning in /home/jon/pro/zeek/zeek/testing/btest/.tmp/language.alternate-prototypes-deprecated-args/alternate-prototypes-deprecated-args.zeek, line 30 and /home/jon/pro/zeek/zeek/testing/btest/.tmp/language.alternate-prototypes-deprecated-args/alternate-prototypes-deprecated-args.zeek, line 9: use of deprecated 'myev' prototype: Don't use this prototype (event(a:string; b:string;))
+error in ./hide.zeek, line 5: unknown identifier b, at or near "b"

testing/btest/Baseline/language.alternate-prototypes-deprecated-args/out

@@ -0,0 +1,8 @@
+warning in /home/jon/pro/zeek/zeek/testing/btest/.tmp/language.alternate-prototypes-deprecated-args/alternate-prototypes-deprecated-args.zeek, line 11 and /home/jon/pro/zeek/zeek/testing/btest/.tmp/language.alternate-prototypes-deprecated-args/alternate-prototypes-deprecated-args.zeek, line 7: use of deprecated parameter 'b': Don't use 'b' (event(a:string; b:string; c:string;))
+warning in /home/jon/pro/zeek/zeek/testing/btest/.tmp/language.alternate-prototypes-deprecated-args/alternate-prototypes-deprecated-args.zeek, line 30 and /home/jon/pro/zeek/zeek/testing/btest/.tmp/language.alternate-prototypes-deprecated-args/alternate-prototypes-deprecated-args.zeek, line 9: use of deprecated 'myev' prototype: Don't use this prototype (event(a:string; b:string;))
+myev (canon), one, two, three
+myev (new), one, three, [1, 2, 3]
+myev (new), one, three, 0
+myev (new), one, three, 1
+myev (new), one, three, 2
+myev (old), one, two

testing/btest/language/alternate-prototypes-deprecated-args.zeek

@@ -0,0 +1,47 @@
+# @TEST-EXEC: zeek -b %INPUT >out 2>&1
+#
+# @TEST-EXEC-FAIL: zeek -b %INPUT hide.zeek >hidden-error 2>&1
+# @TEST-EXEC: TEST_DIFF_CANONIFIER=$SCRIPTS/diff-remove-abspath btest-diff out
+# @TEST-EXEC: TEST_DIFF_CANONIFIER=$SCRIPTS/diff-remove-abspath btest-diff hidden-error
+
+global myev: event(a: string, b: string &deprecated="Don't use 'b'", c: string);
+global myev: event(a: string, c: string);
+global myev: event(a: string, b: string) &deprecated="Don't use this prototype";
+
+event myev(a: string, b: string, c: string) &priority=11
+	{
+	print "myev (canon)", a, b, c;
+	}
+
+event myev(a: string, c: string) &priority = 7
+	{
+	local ddd = vector(1,2,3);
+	print "myev (new)", a, c, ddd;
+	}
+
+global eee = vector(1,2,3);
+
+event myev(a: string, c: string) &priority = 6
+	{
+	for ( o in eee )
+		print "myev (new)", a, c, o;
+	}
+
+event myev(a: string, b: string) &priority = 5
+	{
+	print "myev (old)", a, b;
+	}
+
+event zeek_init()
+	{
+	event myev("one", "two", "three");
+	}
+
+@TEST-START-FILE hide.zeek
+event myev(a: string, c: string) &priority = 7
+	{
+	local ddd = vector(1,2,3);
+	print "myev (new)", a, c, ddd;
+	print b;
+	}
+@TEST-END-FILE