Godot4.4dev7: Using @warning_ignore annotation at function signature level is broken

[MikeSchulze]

Tested versions

• Reproducible in v4.4.dev7.mono.official [46c8f8c]
• Works on v4.4.dev6.mono.official [1f47e4c]

System information

Windows 11

Issue description

I updated my CI-PR workflow to Godot4.4dev7 and it fails now
It reports now the warning and is ignoring the @warning_ignore("unsafe_method_access")
[Ignore]Line 13 (UNSAFE_METHOD_ACCESS):The method "line_number()" is not present on the inferred type "Node" (but may be present on a subtype).
The annotation was working before on function signature level but is now ignored, this is a regression

Steps to reproduce

extends Node


@warning_ignore("unsafe_method_access")
func check(test_case :Node) -> void:
	#@warning_ignore("unsafe_method_access")
	test_case.line_number()

Minimal reproduction project (MRP)

n/a

[MikeSchulze]

Looks like @warning_ignore is broken, in general using at function signature.
With introducing the #76020 the annotation is broken,

The introduction of @warning_ignore_start and @warning_ignore_restore makes sense, but is also very cumbersome and generates code noise.

The @warning_ignore should work at class, method and line level. To exclude defined blocks, @warning_ignore_start and @warning_ignore_restore make sense, but not as a replacement for entire methods or classes.
This used to work well on earlier Godot version, so this is a really bad step.

Running the scripts via command line shows the warning as errors.

Run Test Suite res://addons/gdUnit4/test/network/GdUnitTcpServerTest.gd
        Run Test: res://addons/gdUnit4/test/network/GdUnitTcpServerTest.gd > test_read_next_data_packages :STARTED
Loading resource: res://addons/gdUnit4/src/mocking/GdUnitMockBuilder.gd
ERROR: 'The method "size()" is not present on the inferred type "Variant" (but may be present on a subtype).'
ERROR: 'The method "is_match()" is not present on the inferred type "Variant" (but may be present on a subtype).'
ERROR: 'The method "keys()" is not present on the inferred type "Variant" (but may be present on a subtype).'
ERROR: 'The method "keys()" is not present on the inferred type "Variant" (but may be present on a subtype).'
ERROR: 'Casting "Variant" to "PackedScene" is unsafe.'
ERROR: 'The method "ends_with()" is not present on the inferred type "Variant" (but may be present on a subtype).'
ERROR: 'Casting "Variant" to "String" is unsafe.'
ERROR: 'The method "__set_script()" is not present on the inferred type "Variant" (but may be present on a subtype).'
ERROR: 'The method "__set_singleton()" is not present on the inferred type "Variant" (but may be present on a subtype).'
ERROR: 'The method "__set_mode()" is not present on the inferred type "Variant" (but may be present on a subtype).'
ERROR: 'Casting "Variant" to "Object" is unsafe.'
ERROR: 'The method "new()" is not present on the inferred type "Variant" (but may be present on a subtype).'
ERROR: 'The method "ends_with()" is not present on the inferred type "Variant" (but may be present on a subtype).'
ERROR: 'Casting "Variant" to "String" is unsafe.'
ERROR: 'Casting "Variant" to "String" is unsafe.'
ERROR: 'Casting "Variant" to "String" is unsafe.'
ERROR: 'The method "get_path()" is not present on the inferred type "Variant" (but may be present on a subtype).'
ERROR: 'The method "__set_singleton()" is not present on the inferred type "Node" (but may be present on a subtype).'
ERROR: 'The method "__set_mode()" is not present on the inferred type "Node" (but may be present on a subtype).'
Register singleton 'GdUnitDefaultValueDecoders:<Object#112206022248>'
ERROR: 'The method "size()" is not present on the inferred type "Variant" (but may be present on a subtype).'
ERROR: 'The method "is_match()" is not present on the inferred type "Variant" (but may be present on a subtype).'
ERROR: 'The method "keys()" is not present on the inferred type "Variant" (but may be present on a subtype).'
ERROR: 'The method "keys()" is not present on the inferred type "Variant" (but may be present on a subtype).'
ERROR: 'The argument 1 of the function "listen()" requires the subtype "int" but the supertype "Variant" was provided.'

And in the editor as warnings

[AThousandShips]

This functionality was:

• Undocumented
• Not really intended

See:

• GDScript: Fix @warning_ignore annotation issues #83037 (comment)

This change was intentional

[MikeSchulze]

There has been a correction #75620 do not sound as originally intended.

This change is a step backwards and only makes it more cumbersome. It had worked fine for many versions and feels broken.
It is definitely a regression whether intentional or not!

And it was possible to ignore all in 4.1 # warning-ignore-all:warning-id
https://docs.godotengine.org/en/4.1/tutorials/scripting/gdscript/warning_system.html
But since changing to annotation there was removed, but it was working initial at class level.

[AThousandShips]

#75620 is for the specific case, the method declaration, it's not for the whole function, at least the issue it solved, can you point to where in that PR that part of this was touched on?

The PR is about the scope of shadowing not the scope of the warning,

[MikeSchulze]

#75620 is for the specific case, the method declaration, it's not for the whole function, at least the issue it solved, can you point to where in that PR that part of this was touched on?

The PR is about the scope of shadowing not the scope of the warning,

From the issue description, #75208 is related to the warning, I would say.

Still, I don't see why this should be a problem at the class or method level.
Sure, it may hide future problems, but that may well be intentional, we're talking about warnings here.
If you don't want that, you can define the blocks with the new annotation.

I think that forcing developers to switch to the new annotations after the fact is the wrong way to go.
This only creates frustration, may not be a problem for smaller projects, but for larger projects it only creates unnecessary effort, especially as it used to work well.

[AThousandShips]

It working this way was never documented, and was not by design given the discussions, but regardless the removal of it was entirely intentional

[MikeSchulze]

It working this way was never documented, and was not by design given the discussions, but regardless the removal of it was entirely intentional

Yes, I understood that, but who says it was bad?
Please understand that it's a real problem when you maintain a plugin that has to be backwards compatible.
I can't change everything to the new annotations at once because they are not available in older versions and would cause compiler errors.
To handle this so strictly afterwards is simply not right and also unnecessary.
Both functionalities can coexist and would not break anything.

[AThousandShips]

See the discussion I linked for the background, this was undocumented behaviour that was never officially supported, so therefore there's no breakage

I can't change everything to the new annotations at once because they are not available in older versions and would cause compiler errors.

Use local ignore annotations I'd say, that was the supported usage

[dalexeev]

It working this way was never documented, and was not by design given the discussions, but regardless the removal of it was entirely intentional

Yes, I understood that, but who says it was bad?

This is bad because the user can accidentally ignore existing and future warnings in the function body without actually intending to do so, see the video in the comment.

The problem with @warning_ignore on functions is that the scope of the annotation is unclear. Some might expect it to apply only to the next line, some might expect it to apply only to the function header, and some might expect it to apply to the entire function, including its body.

If we were committed to never breaking compatibility even for undocumented and unintended behavior, we could solve this by introducing a new annotation like @warning_ignore_once and deprecating @warning_ignore. This would be quite sad, given that @warning_ignore works well in most cases and we essentially have to rename it because of an unfortunate mistake in the past.

We can't guarantee absolute compatibility, because otherwise we wouldn't be able to fix any bugs. General rule: don't rely on bad behavior, fixing a bug is not considered breaking compatibility. The problem is that some behaviors are not obviously bad. They can be controversial: convenient in some cases and confusing in others.

In this particular situation, we decided to remove the behavior because it is:

1. Undocumented.
2. Unintentional.
3. Can be confusing for users and lead to unintentional ignoring of warnings.
4. For new engine versions, there are @warning_ignore_start and @warning_ignore_restore annotations. If you want to maintain compatibility with older engine versions, you can use single @warning_ignores.
5. We expect that this change should not significantly affect most projects. Only individual projects that relied on this behavior and don't want to use the new annotations to maintain compatibility with older engine versions within a single project version.

I'm sorry to hear that this has caused inconvenience to your plugin, but I don't see enough justification to revert this change. In my opinion, the only reason we should reconsider this is if the assumption in p.5 turns out to be wrong. If many users report serious problems, even though they didn't rely on this behavior and are not constrained by the need to maintain compatibility with older engine versions.

As for addons, I think the problem may be that you want to have one version of the addon compatible with many engine versions. You could adapt the engine workflow: have a master/main branch of the plugin and several maintenance branches for older engine versions (4.3, 4.2, etc.). And you could cherry-pick compatible changes into maintenance branches.

Note that using any new engine feature makes the addon incompatible with older engine versions. For example, 4.4 introduces typed dictionaries. If you use them, this will make your addon incompatible with 4.3, just like using @warning_ignore_start and @warning_ignore_restore.

Also note that this change has a positive consequence. Users who unintentionally ignored warnings for an entire function will now see them. From this perspective, the change was a bugfix, not a regression.

[MikeSchulze]

@dalexeev thanks for the very detailed explanation.

Also note that this change has a positive consequence. Users who unintentionally ignored warnings for an entire function will now see them. From this perspective, the change was a bugfix, not a regression.

I am with you on this reasoning, as much as it hurts. I found the option to disable the warnings of the entire function or class quite practical, but yes, it is stricter this way, and you are now “forced” to take a closer look at the problem areas.

Nevertheless, I see a bug here, as the editor console output differs from the system console and is even logged as an error.
I have set the level to warning and the editor console shows it as warnings

But running at system console, it is reported as errors.

[MikeSchulze]

@dalexeev I fixed the warnings with success, but I run now into a compile error on v4.2.2.stable.official [15073af]

Just for info, looks like I need a fix customized for Godot2.2.x, all higher versions has no issues.
Or better, I set the minimum required Godot version to 4.3.+ for my next release

[dalexeev]

• @MikeSchulze @warning_ignore support for match branches was added in GDScript: Fix @warning_ignore annotation issues #83037 (i.e. 4.3).