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 #[init(...)] to be used by generated init function #199

Closed
ttencate opened this issue Mar 22, 2023 · 5 comments · Fixed by #205
Closed

Add #[init(...)] to be used by generated init function #199

ttencate opened this issue Mar 22, 2023 · 5 comments · Fixed by #205
Labels
c: register Register classes, functions and other symbols to GDScript feature Adds functionality to the library

Comments

@ttencate
Copy link
Contributor

ttencate commented Mar 22, 2023

Right now, the init function created by #[class(init)] invokes Default::default() for each field. This is already somewhat useful, but not sufficient if there are fields with types like Gd<Node>.

I was tempted to implement Default for Gd, but this would completely hide the allocation and very likely lead to the user forgetting to manually free() it.

So it would be nice if we could specify a different default instead. Proposal:

  1. If annotated with #[default] or not annotated at all, use Default::default().
  2. If annotated with #[default(...)], use the ... as the initializer. I think we can support arbitrary expressions here because parentheses are always balanced, but would have to see how far we can get with this. This would support the use case above: #[default(Node::new_alloc())] my_node: Gd<Node> makes the allocation visible.

Note that #[default] is orthogonal to #[export], which is why we don't write #[export(default = ...)].

@Bromeon
Copy link
Member

Bromeon commented Mar 22, 2023

Thanks!

All attribute APIs currently follow this schema:

#[attribute(key, key = value, ...)]

And it would be nice to keep it that way, because:

  • it's more consistent for a user
  • it's easier for proc-macro implementations, because attributes are always maps
  • such attributes are extensible in non-breaking ways

Maybe we can additionally consider #[attribute = value] later, if there are compelling reasons.


Arbitrary expressions have some other issues: scope/visibility, order of initializations, side effects.

I don't think we need to support them from the start. Even just #[default] and something like #[default(alloc)] (note alloc being a key here) would be nice!

Also, why is #[default] without args needed at all?


Brainstorming alternative syntaxes:

#[init(default)]
#[init(default = value)]
#[init(default_alloc)]

@ttencate
Copy link
Contributor Author

Note that even -1 is already an expression, and so is GodotString::from("foo"), so allowing only literals is pretty limiting.

Scope is clear: same as where the expr is written, since it lives in the same module as the generated impl.

Initialization order is also clear. And not usually relevant.

Side effects are possible but I don't see the problem with that.

@ttencate
Copy link
Contributor Author

I like #[init(default = value)] btw, since it repeats the init from class and thus ties the two together.

@Bromeon
Copy link
Member

Bromeon commented Mar 22, 2023

Note that even -1 is already an expression, and so is GodotString::from("foo"), so allowing only literals is pretty limiting.

No, negatives as well as bool are already supported:

#[test]
fn test_parse_kv_key_lit() {
expect_parsed(
quote! {
#[attr(key="string", pos=32, neg=-32, bool=true, float=3.4)]
},
hash_map!(
"key".to_string() => KvValue::Lit("\"string\"".to_string()),
"pos".to_string() => KvValue::Lit("32".to_string()),
"neg".to_string() => KvValue::Lit("-32".to_string()),
"bool".to_string() => KvValue::Lit("true".to_string()),
"float".to_string() => KvValue::Lit("3.4".to_string()),
),
);
}

It's still limiting, but could serve as a start. For string literals we should definitely have a way to let "string" represent a GodotString.

Further extending KvParser should then probably be a separate PR.


I like #[init(default = value)] btw, since it repeats the init from class and thus ties the two together.

One thought was that #[init] could possibly be extended to incorporate related features like #130:

#[init(onready)]
lateinit_field: Option<...>,

However, that may also be confusing, as "init" explicitly refers to the constructor otherwise. Anyway, can be revisited later...


Scope is clear: same as where the expr is written, since it lives in the same module as the generated impl.

That's nice, then we don't need to fully qualify anything and use works right away 👍


Side effects are possible but I don't see the problem with that.

Yeah, it would need to be quite deliberate. I also think it's going to be fine -- wouldn't really be possible to prevent.

@ttencate
Copy link
Contributor Author

For string literals we should definitely have a way to let "string" represent a GodotString.

That would be good. Could be a simple matter of unconditionally calling .into() on the result of the expression.

Further extending KvParser should then probably be a separate PR.

Agreed. I think it'll be doable, because a comma is never at the top level of an expression. And it never will be, because this would give ambiguities when parsing function arguments as well. So we just run through the tokens, pushing and popping matching brackets onto a stack, until we find a comma while the stack is empty.

@ttencate ttencate changed the title Add #[default(...)] to be used by generated init function Add #[init(...)] to be used by generated init function Mar 23, 2023
bors bot added a commit that referenced this issue Mar 27, 2023
203: Make `get` and `set` arguments take identifiers instead of strings r=Bromeon a=ttencate

Refactors `KvParser` to accept some expressions, as long as they don't
contain a comma at the top level outside any pair of `[]`, `()` or `{}`.

Also tightens up spans in error messages quite a bit.

Prelude for #199.

Co-authored-by: Thomas ten Cate <[email protected]>
@bors bors bot closed this as completed in c6ebd3d Mar 27, 2023
@Bromeon Bromeon added feature Adds functionality to the library c: register Register classes, functions and other symbols to GDScript labels Mar 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: register Register classes, functions and other symbols to GDScript feature Adds functionality to the library
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants