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

Add C# source generator for ScriptPathAttribute #46713

Merged
merged 1 commit into from
Mar 7, 2021

Conversation

neikeq
Copy link
Contributor

@neikeq neikeq commented Mar 5, 2021

This source generator adds a newly introduced attribute, ScriptPath to all classes that:

  • Are top-level classes (not inner/nested).
  • Have the partial modifier.
  • Inherit Godot.Object.
  • The class name matches the file name.

A build error is thrown if the generator finds a class that meets these conditions but is not declared partial, unless the class is annotated with the DisableGodotGenerators attribute.

We also generate an AssemblyHasScripts assembly attribute which Godot uses to get all the script classes in the assembly, eliminating the need for Godot to search them. We can also avoid searching in assemblies that don't have this attribute. This will be good for performance in the future once we support multiple assemblies with Godot script classes.

This is an example of what the generated code looks like:

using Godot;
namespace Foo {
	[ScriptPathAttribute("res://Player.cs")]
	// Multiple partial declarations are allowed
	[ScriptPathAttribute("res://Foo/Player.cs")]
	partial class Player {}
}

[assembly:AssemblyHasScripts(new System.Type[] { typeof(Foo.Player) })]

The new attributes replace script metadata which we were generating by determining the namespace of script classes with a very simple parser.
This fixes several issues with the old approach related to parser errors and conditional compilation.
It also makes the task part of the MSBuild project build, rather than a separate step executed by the Godot editor.

@neikeq neikeq added this to the 4.0 milestone Mar 5, 2021
@neikeq neikeq requested a review from a team as a code owner March 5, 2021 23:39
@neikeq neikeq force-pushed the csharp-source-generators-init branch 2 times, most recently from a999c21 to 8541f75 Compare March 6, 2021 00:11
@neikeq
Copy link
Contributor Author

neikeq commented Mar 6, 2021

NOTE: We use CompilerVisibleProperty to expose MSBuild properties to our source generator. Rider doesn't support this yet:
https://youtrack.jetbrains.com/issue/RIDER-55242
https://youtrack.jetbrains.com/issue/RIDER-58080

I found no problems with VSCode's C# (OmniSharp) extension.

@neikeq neikeq force-pushed the csharp-source-generators-init branch 3 times, most recently from b2eb9fd to d0d86d6 Compare March 6, 2021 00:51
@akien-mga
Copy link
Member

To fix the clang-format issues, you likely need to update your local version to clang-format 11 (they changed the logic for alignment of ternary operators in that version, going from "bad" to "still bad but different"...).

@neikeq neikeq force-pushed the csharp-source-generators-init branch from d0d86d6 to b647eff Compare March 6, 2021 18:26
@neikeq
Copy link
Contributor Author

neikeq commented Mar 6, 2021

I don't get it, I have the black format precommit hook (version is the same as in the CI)...

@akien-mga
Copy link
Member

Try to run it manually with black -l 120 path/to/files. I think the pre-commit hook might fail in some circumstance (e.g. git commit --amend).

This source generator adds a newly introduced attribute,
`ScriptPath` to all classes that:

- Are top-level classes (not inner/nested).
- Have the `partial` modifier.
- Inherit `Godot.Object`.
- The class name matches the file name.

A build error is thrown if the generator finds a class that meets these
conditions but is not declared `partial`, unless the class is annotated
with the `DisableGodotGenerators` attribute.

We also generate an `AssemblyHasScripts` assembly attribute which Godot
uses to get all the script classes in the assembly, eliminating the need
for Godot to search them. We can also avoid searching in assemblies that
don't have this attribute. This will be good for performance in the
future once we support multiple assemblies with Godot script classes.

This is an example of what the generated code looks like:

```
using Godot;
namespace Foo {
	[ScriptPathAttribute("res://Player.cs")]
	// Multiple partial declarations are allowed
	[ScriptPathAttribute("res://Foo/Player.cs")]
	partial class Player {}
}

[assembly:AssemblyHasScripts(new System.Type[] { typeof(Foo.Player) })]
```

The new attributes replace script metadata which we were generating by
determining the namespace of script classes with a very simple parser.
This fixes several issues with the old approach related to parser
errors and conditional compilation.
It also makes the task part of the MSBuild project build, rather than
a separate step executed by the Godot editor.
@neikeq neikeq force-pushed the csharp-source-generators-init branch from b647eff to e2afe70 Compare March 6, 2021 20:50
@neikeq
Copy link
Contributor Author

neikeq commented Mar 6, 2021

Try to run it manually with black -l 120 path/to/files.

Thanks, that did it.

I think the pre-commit hook might fail in some circumstance (e.g. git commit --amend).

I was suspicious of git commit --amend being the problem and thus tried a soft reset and re-commit, yet same problem.

Comment on lines +3 to +4
<PackageVersion_Godot_NET_Sdk>4.0.0-dev4</PackageVersion_Godot_NET_Sdk>
<PackageVersion_Godot_SourceGenerators>4.0.0-dev1</PackageVersion_Godot_SourceGenerators>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it intended to have version 4.0.0-dev4 instead of 4.0.0-dev1?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, since these are the versions of each package, dev1 to dev3 have already been used. It does seem a bit strange that they're out of sync, so maybe the source generators package should just start at 4.0.0-dev4.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For stable releases they should be the same version, but during development there will be new additions to the source generator while the Sdk may remain the same. Technically we can release an unchanged new version of the Sdk to keep the version in sync with the other package, but that doesn't feel right. The only case where that would be appropriate is when going from rc to stable without changes in the packages.

@akien-mga akien-mga merged commit 15bd2bf into godotengine:master Mar 7, 2021
@akien-mga
Copy link
Member

Thanks!

@neikeq neikeq deleted the csharp-source-generators-init branch May 13, 2021 23:13
dasg34 added a commit to dasg34/mixed-reality-extension-godot-1 that referenced this pull request Aug 9, 2022
since 4.0, `partial` has been requied for the the subclasses of `Godot.Object`.

For more information, refer to godotengine/godot#46713
dasg34 added a commit to dasg34/mixed-reality-extension-godot-1 that referenced this pull request Aug 10, 2022
since 4.0, `partial` has been requied for the the subclasses of `Godot.Object`.

For more information, refer to godotengine/godot#46713
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.

3 participants