Macro to use when deprecating methods, properties and signals

[akien-mga]

It would be nice to have a C++ macro that could be used when deprecating methods. It should:

• Display a user-defined warning in the terminal
• Optionally, run the method that replaces the deprecated method if compatible (e.g. if the method was just renamed, the old name could be deprecated this way and redirect to the new name)

It could be something like:

void OS::foo(int p_bar) {
    WARN_DEPRECATED("This method has been deprecated, redirecting to the equivalent OS.bar(bar)", OS::bar(p_bar));
}

Or maybe better, assuming that we'll mostly want to deprecate bindings and not directly C++ APIs (those APIs should be transparent to the end users), we could have a ObjectTypeDB::bind_method_deprecated method to replace ObjectTypeDB::bind_method where relevant. For the above example, it could be something like:

ObjectTypeDB::bind_method_deprecated(_MD("foo", "bar"),"This method has been deprecated, redirecting to the equivalent OS.bar(bar)",&OS::bar);

WDYT?

[akien-mga]

BTW the macro and/or method would also output the name of the original method and relevant C++ code line to the warning, just like is done already for warnings and error messages. Something like:

WARNING: Deprecated method OS::foo(int p_bar), core/os/os.cpp:1234
    This method has been deprecated, redirecting to the equivalent OS.bar(bar)

If it could even output the file and line number where the method was called (in the GDScript), it would be even better.

[akien-mga]

We could even add an optional parameter to indicate in which Godot version the deprecated method will be removed altogether.

[neikeq]

I prefer the second option: ObjectTypeDB::bind_method_deprecated. However, I don't think we should redirect to a different method. Printing a warning is enough. We could then mention the alternative method in the deprecated method description.
If this option is implemented, it would also be good to include this information in classes.xml and display it in the editor help (something like a @deprecated annotation :P).

BTW, do we want to deprecate only methods? or may we also need to deprecate constants or signals (or even properties) in the future?

[neikeq]

A different way is to use ObjectTypeDB::bind_method normally and call ObjectTypeDB::add_deprecated_method( get_type_static() , method_name ) after that. This would do the same as the ObjectTypeDB::bind_method_deprecated option but following the same style as virtual methods:

#ifdef TOOLS_ENABLED
#define BIND_VMETHOD(m_method)\
    ObjectTypeDB::add_virtual_method( get_type_static() , m_method );
#else
// Virtual methods just seems to be an annotation for the editor help.
/* We probably want to replace the above `#ifdef TOOLS_ENABLED` with `#ifdef DEBUG_ENABLED`
   since we also want to print the warning in debug builds. */
#define BIND_VMETHOD(m_method)
#endif

void ObjectTypeDB::add_virtual_method(const StringName& p_type, const MethodInfo& p_method , bool p_virtual) {
    ERR_FAIL_COND(!types.has(p_type));

#ifdef DEBUG_METHODS_ENABLED
    MethodInfo mi=p_method;
    if (p_virtual)
        mi.flags|=METHOD_FLAG_VIRTUAL;
    types[p_type].virtual_methods.push_back(mi);

#endif
}

[akien-mga]

BTW, do we want to deprecate only methods? or may we also need to deprecate constants or signals (or even properties) in the future?

Yeah we should also have the possibility to deprecate constants, signals and properties (see e.g. #4109 which would require a way to deprecate the old property).

ObjectTypeDB::add_deprecated_method( get_type_static() , method_name ) sounds like a very good solution indeed.

[reduz]

Sounds ok to me, though I'm not sure where would it be used

[akien-mga]

After discussing this with reduz on IRC, we noted that this new method or macro would likely imply that every function call will have to check a flag, thus potentially slowing down GDScript a bit. So it should be implemented in a way that only impacts TOOLS_ENABLED builds, it should be transparent for the export templates.

[akien-mga]

Not critical for the upcoming 2.1, so moving to the next milestone.

[akien-mga]

For 3.0, so much stuff was deprecated that it makes no sense to try to keep compatibility bindings. For 3.1 and later, the idea described here will become relevant again though, I still think it would be a nice improvement.

[neikeq]

I volunteer to implement this. It will be happen in 3.1 though, because I'm busy with other stuff right now and, as you said, it makes no sense in 3.0.

[akien-mga]

Not only relevant for methods, but also properties and signals. (And I guess enums? Not sure we should bother with those until an actual need arises).

[akien-mga]

@reduz implemented 8fc298be5, but it's too simple IMO, it still implies that we need to keep deprecated methods around, just with this additional macro to easily print an undefined warning. I want a system that prevents unnecessary code duplication, gives more information to the user regarding what method to use instead, and prevents deprecated stuff from showing up in the documentation and autocomplete.

I'll give it a try myself I guess :)

[vnen]

I think it should still be shown in documentation though (with a clear deprecation warning). If one is left with the task of updating a project, they should have a way to know what the deprecated method was used for.

[akien-mga]

Fair point, but the deprecation warning should be added automatically to the docs then, we don't want to have to remember to change the docs of deprecated methods manually each time.

[neikeq]

#23023 already tackles most issues. If it's merged, we would only be missing deprecation for things outside of ClassDB:

• Built-in types (Vector2, String, etc).
• @GDScript. Methods are declared in GDScriptFunctions. Constants are added as globals in GDScriptLanguage::init().
• @Global Scope. Constants are declared in GlobalConstants.
  It also contains properties that point to singletons. Those don't need to be deprecated since that's implicit if the class of the singleton they point to is deprecated.

[akien-mga]

We have WARN_DEPRECATED already. A more thorough framework for deprecating methods and define aliases could be useful, but this issue is quite old and this should probably be re-thought and re-opened as a proposal on https://github.com/godotengine/godot-proposals.