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

"_hasBeenSet" private bool should not be generated if unused #52

Closed
BenediktMagnus opened this issue Mar 29, 2023 · 4 comments · Fixed by #54
Closed

"_hasBeenSet" private bool should not be generated if unused #52

BenediktMagnus opened this issue Mar 29, 2023 · 4 comments · Fixed by #54

Comments

@BenediktMagnus
Copy link
Contributor

Currently, for an OnReadyGet like this:

[OnReadyGet(Export = true)]
private Texture MyTexture { get; set; } = null!;

the generated code looks like that:

[Export] public global::Godot.Texture MyTextureResource
{
    get => MyTexture;
    set { _hasBeenSetMyTexture = true; MyTexture = value; }
}
private bool _hasBeenSetMyTexture;

The code responsible for that is this here:

g.BlockBrace(() =>
{
g.Line("get => ", Member.Name, ";");
g.Line("set { _hasBeenSet", Member.Name, " = true; ", Member.Name, " = value; }");
});
g.Line("private bool _hasBeenSet", Member.Name, ";");

But _hasBeenSetMyTexture is never used. (I noticed this because I got several CS0414 warnings when compiling.)
It would only be used if the generator had to generate the assignment by itself as in here:

if (IsGeneratingAssignment)
{
g.Line("if (!_hasBeenSet", Member.Name, ")");
g.BlockBrace(() =>
{
WriteAssignment(g);
});
}


I couldn't find any documentation regarding the possible usage of _hasBeenSet outside the generated code (i.e. by the user) and cannot imagine a sensible use case.
Therefore I suggest to only generate _hasBeenSet if IsGeneratingAssignment is set, so that it is only generated if it is actually used.

@31
Copy link
Owner

31 commented Mar 29, 2023

Thanks, this is a reasonable request. I just haven't ended up encountering it because I've never used e.g. [OnReadyGet] public PackedScene scene;--all it offers over [Export] public PackedScene scene; is a null check. 😄 Useful, but not something that has come to mind to take advantage of.

It looks like you've done some digging and understand how the generator works, would you be interested in submitting a PR? I'd be happy to accept it and build/publish a new version.

(I've stopped using GodotOnReady in 4.0 because it doesn't work anymore, but glad to do small things like this for continued 3.x usage.)

@31
Copy link
Owner

31 commented Mar 29, 2023

Export = true made me realize this doc has a typo: If true, always generates a property with [Export], even if Path isn't set.. "Isn't" should be "is". 😞 This pairs with the constructor/Path doc which is the accurate side:

		/// <param name="path">
		/// The path that will be loaded when the node is ready. If not set, a property is generated
		/// with [Export], so the path can be set in the Godot editor.
		/// </param>

@BenediktMagnus
Copy link
Contributor Author

Export = true made me realize this doc has a typo: If true, always generates a property with [Export], even if Path isn't set.. "Isn't" should be "is".

Interesting, this could be the cause for quite some confusion on my side regarding the workings of Export/OnReadyGet. Great the documentation could be improved this way! (And my code simplified a bit.)

I've never used e.g. [OnReadyGet] public PackedScene scene;--all it offers over [Export] public PackedScene scene; is a null check. Useful, but not something that has come to mind to take advantage of.

It is in fact the null check that's interesting for me. I have built small utility system to auto-generate in-editor warnings (via _GetConfigurationWarning) for fields marked with OnReadyGet that are missing. Based on this I use Export for optional nodes/resources and OnReadyGet for mandatory ones. In doing so I can use non-nullables while guaranteeing that the null-suppressed null assignments won't shoot me in the foot at runtime.
It may be only a small thing, but useful indeed.

It looks like you've done some digging and understand how the generator works, would you be interested in submitting a PR? I'd be happy to accept it and build/publish a new version.

I will open a pull request with the suggested change.

@31
Copy link
Owner

31 commented Mar 31, 2023

Version 1.2.4 is now pushed with these changes. Thanks again! 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants