Add warning for deprecated identifiers in GDScript

[Meorge]

Closes godotengine/godot-proposals#12105 .

This PR adds a new warning, DEPRECATED_IDENTIFIER, to GDScript. As the name suggests, it activates when a line contains an identifier that has been marked as deprecated in the documentation.

Tested cases

• User-defined method call
• User-defined method call in inner class
• Native method call
• Main class of file as type annotation
• Main class of file for constructors, static methods, etc
• Inner class as type annotation
• Inner class for constructors, static methods, etc
• Native class as type annotation
• Native class for constructors, static methods, etc
• Autoload with global name as type annotation
• Autoload with global name for constructors, methods, etc
• User-defined class constant
• User-defined class constant in inner class
• User-defined local constant
• User-defined named enum
• User-defined named enum in inner class
• Individual value in user-defined anonymous enum
• Individual value in user-defined anonymous enum in inner class
• Individual value in user-defined named enum
• Individual value in user-defined named enum in inner class
• Individual value in native enum
• Constant in global scope
• Local variable
• User-defined property
• User-defined property in inner class
• Native property
• User-defined signal
• User-defined signal in inner class
• Native signal

To Do

• Get all the cases working!
• Make the message more professional
• Add context to the message (for example, instead of "identifier" say "variable", "method", etc)
• Clean up some code that goes against Godot's code style (I think I used an auto or two)
• Re-run tests to generate new warning output
  • Add tests to detect use of deprecated identifiers?

[Mickeon]

I foresee that if this is implemented, users would rather the warning be a strikethrough on the deprecated subject (even if currently CodeEdit cannot support it)

[Meorge]

Another PR of mine, #100019, adds a strikethrough to deprecated items in the code completion list, which is similar at least 🙂

[Meorge]

There are still a few cases to figure out, and then the other tasks to complete as well, but I think this is close enough to complete now that it'd make sense to open it for review. I'm also keeping the commits separate for now (i.e., not squashing) since I'm trying to include one type of detection per commit so I can tell them apart easier. Once it's ready for merging, I'll squash and force push.

[Mickeon]

There's a warning here but I bring it up to attention as well. You're calling get_doc_data here even though it's already done above at line 1991

[Meorge]

I don't think I saw that in my terminal output, so thanks for catching that! I'll have to take a look at it again and confirm it's working properly.

[Meorge]

All of my test cases appear to be working now!

I would really appreciate it if someone more familiar with the GDScript parser could take a look at gdscript_analyzer.cpp:4033. The current way I'm distinguishing native and user-defined enums feels a bit hacky, so if there's a better way to do it then I'd be all for it.

I've also noticed that as a little quirk of the identifier recognition, accessing a deprecated property from within its getter or setter causes those lines to trigger the warning.

Ultimately, it does make sense, I guess - you are using the deprecated identifier - but in this specific context, it's a bit funny for it to complain about.

[Meorge]

Warnings now display information about what exactly is deprecated!

The only case that doesn't seem to work right is when an autoload's global variable name is used as a type specifier:

var my_autoload: SomeAutoload

I'm having trouble extracting the name SomeAutoload to display in the warning. But I'm not entirely sure if that should be allowed. In any case, for right now it just defaults to "The class is deprecated".

I tried regenerating the GDScript tests, but one of them is causing a hard crash. I'll have to look more at it over the next couple of days when I have time.

[ydeltastar]

I'm getting a crash while typing an enum name.

extends Node2D

enum MyEnum {
	OPTION1,
	OPTION2
}

# Type MyEnum here to crash.
var my_enum:

I would really appreciate it if someone more familiar with the GDScript parser could take a look at gdscript_analyzer.cpp:4033. The current way I'm distinguishing native and user-defined enums feels a bit hacky, so if there's a better way to do it then I'd be all for it.

There is CoreConstants::is_global_enum(name).

I'm having trouble extracting the name SomeAutoload to display in the warning. But I'm not entirely sure if that should be allowed. In any case, for right now it just defaults to "The class is deprecated".

Autoloads are more a project setting than a class type but the parser shows the global name and path when there are errors. You can find the name like this p_assignable->datatype_specifier->type_chain[0]->name.

By the way, you can build with warnings=extra werror=yes strict_checks=yes to catch most of these CI issues locally.

[Calinou]

Tested locally, it works as expected. Code looks good to me. Great work on the comprehensive integration tests 🙂

I've looked at the TODOs in code comments but can't think of a better way to implement these right now.

PS: I noticed --script --check-only does not print any warnings when the script contains warnings. We should look into implementing this in a separate PR to make it more viable for CI use, along with an additional --warnings-as-errors CLI argument (which would complement #99254 nicely).

[Meorge]

Thank you very much! 😄 I had that script for testing on my side, and realized it would make for a nice comprehensive unit test file.

It seems I have run into a bit of a pickle with the tests, though. It looks like at least the RenderingServer may not be constructed when running tests - trying to instantiate AnimatedTexture or TileMap in a test file causes the test to fail with a SIGSEGV error. When I add the following to the beginning of AnimatedTexture's constructor, on the master branch:

ERR_FAIL_NULL_MSG(RS::get_singleton(), "RenderingServer is null, so AnimatedTexture can't set itself up");

It prints out in the test results.

I've changed it to use VisualShaderNodeComment for a native class and AStarGrid2D.new().size for a native property, which seem to partially work.
AnimatedTexture is deprecated, of course, so I don't know how much effort should be put into updating it to work with tests. But this also happens with Control.new(), so that should probably be looked into further.

But tests on some other platforms appear to still be failing 🙁

modules\gdscript\tests\gdscript_test_runner.cpp(217): ERROR: CHECK( result.passed ) is NOT correct!
  values: CHECK( false )
  logged: D:/a/godot/godot/modules/gdscript/tests/scripts/runtime/features/call_native_static_method_validated.gd
          GDTEST_OK
false
166362874999 - Stray Node:  (Type: AnimationPlayer)