godot icon indicating copy to clipboard operation
godot copied to clipboard

[GDExtension] Incorrect signature or documentation in generated gdextension_interface.h

Open JimTwn opened this issue 1 year ago • 3 comments

Tested versions

Reproducible in 4.2.1.stable.mono.official.b09f793f5

System information

Godot v4.2.1.stable.mono - Windows 10.0.22631 - Vulkan (Forward+) - dedicated NVIDIA GeForce RTX 3070 (NVIDIA; 31.0.15.4665) - AMD Ryzen 7 5800X3D 8-Core Processor (16 Threads)

Issue description

The generated gdextension_interface.h file contains a function signature that incorrectly drops the const qualifier from its return value.

typedef GDExtensionTypePtr (*GDExtensionInterfacePackedColorArrayOperatorIndexConst)(
	GDExtensionConstTypePtr p_self, 
	GDExtensionInt p_index);

This function should return GDExtensionConstTypePtr to preserve the const qualifier on the input p_self. Additionally, this is the expected behaviour described in its documentation:

/**
 * @name packed_color_array_operator_index_const
 * @since 4.1
 *
 * Gets a __const__ pointer to a color in a PackedColorArray.
 *
 * @param p_self A const pointer to a const PackedColorArray object.
 * @param p_index The index of the Color to get.
 *
 * @return A __const__ pointer to the requested Color.
 */

There are a number of similar functions operating on various PackedArray types which do correctly return const qualified values. E.g.:

  • packed_float32_array_operator_index_const
  • packed_float64_array_operator_index_const
  • packed_int32_array_operator_index_const
  • etc.

Steps to reproduce

Generate the file using:

godot --dump-gdextension-interface --headless

Minimal reproduction project (MRP)

N/A

JimTwn avatar Mar 19 '24 16:03 JimTwn

Unsure why this was the case originally, but the change would have to be limited to changing the documentation, anything else would break compatibility

AThousandShips avatar Mar 19 '24 16:03 AThousandShips

The same goes for:

  • packed_string_array_operator_index_const. It should return GDExtensionConstStringPtr .
  • packed_vector2_array_operator_index_const which should both return GDExtensionConstTypePtr .
  • packed_vector3_array_operator_index_const which should both return GDExtensionConstTypePtr .
  • array_operator_index_const which should return GDExtensionConstVariantPtr.
  • dictionary_operator_index_const which should return GDExtensionConstVariantPtr.
/**
 * @name packed_string_array_operator_index_const
 * @since 4.1
 *
 * Gets a const pointer to a string in a PackedStringArray.
 *
 * @param p_self A const pointer to a PackedStringArray object.
 * @param p_index The index of the String to get.
 *
 * @return A const pointer to the requested String.
 */
typedef GDExtensionStringPtr (*GDExtensionInterfacePackedStringArrayOperatorIndexConst)(
	GDExtensionConstTypePtr p_self, 
	GDExtensionInt p_index);


/**
 * @name packed_vector2_array_operator_index_const
 * @since 4.1
 *
 * Gets a const pointer to a Vector2 in a PackedVector2Array.
 *
 * @param p_self A const pointer to a PackedVector2Array object.
 * @param p_index The index of the Vector2 to get.
 *
 * @return A const pointer to the requested Vector2.
 */
typedef GDExtensionTypePtr (*GDExtensionInterfacePackedVector2ArrayOperatorIndexConst)(
	GDExtensionConstTypePtr p_self, 
	GDExtensionInt p_index);


/**
 * @name packed_vector3_array_operator_index_const
 * @since 4.1
 *
 * Gets a const pointer to a Vector3 in a PackedVector3Array.
 *
 * @param p_self A const pointer to a PackedVector3Array object.
 * @param p_index The index of the Vector3 to get.
 *
 * @return A const pointer to the requested Vector3.
 */
typedef GDExtensionTypePtr (*GDExtensionInterfacePackedVector3ArrayOperatorIndexConst)(
		GDExtensionConstTypePtr p_self,
		GDExtensionInt p_index);

/**
 * @name array_operator_index_const
 * @since 4.1
 *
 * Gets a const pointer to a Variant in an Array.
 *
 * @param p_self A const pointer to an Array object.
 * @param p_index The index of the Variant to get.
 *
 * @return A const pointer to the requested Variant.
 */
typedef GDExtensionVariantPtr (*GDExtensionInterfaceArrayOperatorIndexConst)(
	GDExtensionConstTypePtr p_self, 
	GDExtensionInt p_index);


/**
 * @name dictionary_operator_index_const
 * @since 4.1
 *
 * Gets a const pointer to a Variant in a Dictionary with the given key.
 *
 * @param p_self A const pointer to a Dictionary object.
 * @param p_key A pointer to a Variant representing the key.
 *
 * @return A const pointer to a Variant representing the value at the given key.
 */
typedef GDExtensionVariantPtr (*GDExtensionInterfaceDictionaryOperatorIndexConst)(
	GDExtensionConstTypePtr p_self, 
	GDExtensionConstVariantPtr p_key);

JimTwn avatar Mar 19 '24 17:03 JimTwn

Unsure why this was the case originally, but the change would have to be limited to changing the documentation, anything else would break compatibility

Yeah, changing this would likely have an effect on source compatibility. We don't protect source compatibility quite as stringently as binary compatibility, but we should still try avoid breaking source compatibility unless we have a good reason.

I wonder if proposal https://github.com/godotengine/godot-proposals/issues/9560 could help with this?

If we generated gdextension_interface.h from a machine-readable format, we could maybe encode "legacy types" into the data, which would allow generating a "legacy" version of the header, or one updated with more correct types?

dsnopek avatar Apr 24 '24 21:04 dsnopek