Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

C#: Add check for API breaking compat changes #77183

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

raulsntos
Copy link
Member

@raulsntos raulsntos commented May 17, 2023

Footnotes

  1. I don't actually know if they are all intentional, please check.

@neikeq
Copy link
Contributor

neikeq commented May 18, 2023

I didn't know you could add supressions. I would still like to review those breaking changes, to see if there's a way to avoid them.

@raulsntos
Copy link
Member Author

I think some of them could be avoided.

For example, the methods IsMovieMakerEnabled and SetMovieMakerEnabled disappear because Godot now exposes it as a property MovieMakerEnabled. This happens because we now make the property methods internal (#71516), we could revert that PR and go back to exposing the property methods (maybe we could use [EditorBrowsable(EditorBrowsableState.Never)] to hide them).

As for removed methods or methods that change signature, as long as Godot has a compatibility method in ClassDB we should be able to retrieve it with ClassDB::get_method_with_compatibility (we currently use ClassDB::get_method).

However, this would not be enough because we still need to generate the overloads in C# and currently we generate whatever methods we retrieve from ClassDB::get_method_list and it doesn't include the compatibility methods. But if there was a way for us to retrieve the compatibility methods and keep generating them we should be able to not break compat in this case either.

@raulsntos
Copy link
Member Author

I have tried generating the compat methods in C# (see https://github.com/raulsntos/godot/tree/class-db-compat-test), to test this I used the methods from #76577. However, I'm running into issues with some of the compat methods.

Some of the compat methods only change the return type (e.g.: Object::get_meta_list), and some of them only change the const qualifier (e.g.: EditorInterface::get_edited_scene_root). Generating those methods results in invalid C# that can't be compiled:

// This is the current method signature.
public Godot.Collections.Array<StringName> GetMetaList() { ... }

// This is the generated compat method.
// Only the return type has changed.
public string[] GetMetaList() { ... }
// This is the current method signature.
public Node GetEditedSceneRoot() { ... }

// This is the generated compat method.
// The C++ `const` qualifier is ignored when generating C# methods so the overload is the same
// but when retrieving the method from ClassDB the methods have a different hash.
public Node GetEditedSceneRoot() { ... }

@raulsntos raulsntos force-pushed the dotnet/apicompat branch 3 times, most recently from 0e4c7f4 to 42a03fa Compare June 9, 2023 15:17
@raulsntos
Copy link
Member Author

Since I'm having issues generating valid C# API from the compat methods added in #76577 (and also it seems it won't be merged for 4.1). I thought I'd try to manually add compatibility overloads when possible to avoid breaking compatibility in C#.

With the compat overloads, the only API that breaks compat in C# now is:

  • Navigation APIs (this API was marked as experimental in Godot's documentation anyway, so it was expected that it'd break compat).
  • Editor APIs (most changes are in EditorInterface which changes its inheritance chain).
  • Area2D.Priority property which changes its type from float to int.
  • Area3D.Priority property which changes its type from float to int.
  • WorkerThreadPool.WaitForTaskCompletion method which changes its return type from void to Error.
  • GodotObject.GetMetaList method which changes its return type from string[] to Array<StringName>.
  • AnimationNodeStateMachinePlayback.GetTravelPath method which changes its return type from string[] to Array<StringName>.
  • RenderingServer.GlobalShaderParameterGetList method which changes its return type from string[] to Array<StringName>.
  • RDShaderFile.GetVersionList method which changes its return type from string[] to Array<StringName>.
  • PhysicsDirectSpaceState2D.CollideShape method which changes its return type from Array<Vector2[]> to Array<Vector2>.
  • PhysicsDirectSpaceState3D.CollideShape method which changes its return type from Array<Vector3[]> to Array<Vector3>.

Other than the Navigation APIs and Editor APIs, the other APIs only changes the return type so an overload can't be provided to avoid breaking compatibility.

@dsnopek
Copy link
Contributor

dsnopek commented Jun 9, 2023

Since I'm having issues generating valid C# API from the compat methods added in #76577 (and also it seems it won't be merged for 4.1). I thought I'd try to manually add compatibility overloads when possible to avoid breaking compatibility in C#.

Is the plan that the sort of compat methods from #76577 would help in C# as well? Because, if so, that'd be a compelling reason to continue with #76577 for 4.1. I had thought those were only for GDExtension, which is why I figured they'd never get called (because GDExtensions for 4.0 can't be used with 4.1 anymore anyway).

@raulsntos
Copy link
Member Author

I originally intended to automatically generate the C# compat methods from the GDExtension ones so that we could reuse the same system, but some of the compat methods resulted in invalid C# syntax (as explained in #77183 (comment)).

I think for now the easiest solution is to add the compat methods manually to C#, but they could be replaced in the future with autogenerated ones from the GDExtension system. So, in my opinion, there's no need to rush #76577 for C#'s sake.

@RedworkDE
Copy link
Member

  • I would probably throw all the compat methods into one file, IMO they are small enough and it makes it easier to add them for non-mono developers.
  • In general I am somewhat concerned about the impact of the package validation, given that it fails the CI and the syntax for adding suppressions (or compat methods for non C# devs) is not really obvious.

I would probably split the added compat methods into a safe PR and then leave this for some more thoughts on how to best keep them up-to-date.

@raulsntos
Copy link
Member Author

raulsntos commented Jun 19, 2023

I would probably throw all the compat methods into one file

That's what I was doing originally but I thought it may be convenient to have them in separate files. At this point there aren't that many methods so either option is fine to me.

In general I am somewhat concerned about the impact of the package validation, given that it fails the CI and the syntax for adding suppressions (or compat methods for non C# devs) is not really obvious.

That's true but we are not really supposed to break compat anymore, so it shouldn't really be an issue. And if you are breaking compat I think it's preferable to make it very obvious. The suppressions are not supposed to be the norm but the exception. I'm only adding suppressions because it seems breaking compat in these APIs was unavoidable. All the breaking compat changes were caused by 14 PRs, so it's not a common occurrence.

If contributors need to break compatibility, I can try to provide the necessary changes to make CI pass. This doesn't really scale, but in the future I hope we can use the GDExtension system to automatically generate the overloads. This means contributors will only need to learn how to provide GDExtension compat methods. We may even be able to automatically generate the suppressions from https://github.com/godotengine/godot/blob/764193629ff0bac11c4a6ddfa946f1dd3d841799/misc/extension_api_validation/4.0-stable.expected.

I would probably split the added compat methods into a safe PR and then leave this for some more thoughts on how to best keep them up-to-date.

Considering how late we are in the 4.1 dev cycle that makes sense to me. It's probably better to ensure the compat methods get added to 4.1 and not rush the CI validation changes.

@raulsntos raulsntos marked this pull request as draft August 11, 2023 16:50
@raulsntos
Copy link
Member Author

raulsntos commented Aug 11, 2023

I'm marking this as a draft because, as RedworkDE said, enabling package validation means the CI will fail and adding suppressions or compat methods to C# may not be easy for non-C# contributors.

As I said in #77183 (comment), I'll try to generate the C# compat methods automatically from the GDExtension system so that contributors don't have to add them manually. Then, the only issue will be adding suppressions, but hopefully that won't be a common occurence because adding a suppression means we're breaking compatibility so it should be rare.

When that's done I'll mark this as ready again.

@AThousandShips AThousandShips modified the milestones: 4.2, 4.3 Oct 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants