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

Conflict between keywords in Rust and Dart #2066

Closed
dbsxdbsx opened this issue Jun 12, 2024 · 8 comments · Fixed by #2070
Closed

Conflict between keywords in Rust and Dart #2066

dbsxdbsx opened this issue Jun 12, 2024 · 8 comments · Fixed by #2070

Comments

@dbsxdbsx
Copy link
Contributor

dbsxdbsx commented Jun 12, 2024

Issue

For a rust struct without any impl block:

#[derive(Default, Debug, Clone, PartialEq, Serialize, Deserialize)]
pub struct CmsMediaOutLineJson {
    pub code: i64,
    pub msg: String,
    #[serde(deserialize_with = "i64_or_str")]
    pub page: i64,
    #[serde(rename = "pagecount")]
    #[serde(deserialize_with = "i64_or_str")]
    pub page_count: i64,
    #[serde(deserialize_with = "i64_or_str")]
    pub limit: i64,
    #[serde(deserialize_with = "i64_or_str")]
    pub total: i64,
    #[serde(rename = "list")]
    pub out_lines: Vec<MediaOutline>,
    pub class: Vec<MediaClass>,
}

it failed to finish generation, because of the keyword class, some err msgs like these:
column 1 of lib\src\rust\frb_generated.dart: 'class' can't be used as an identifier because it's a keyword.
line 24, column 1 of lib\src\rust\media\cms_media_outline.dart: Can't have modifier 'final' here. ╷ 24 │ final List<MediaClass> class;
, which stuck the generation dart:

class CmsMediaOutLineJson  {
                final PlatformInt64 code;
final String msg;
final PlatformInt64 page;
final PlatformInt64 pageCount;
final PlatformInt64 limit;
final PlatformInt64 total;
final List<MediaOutline> outLines;
final List<MediaClass> class;

                const CmsMediaOutLineJson({required this.code ,required this.msg ,required this.page ,required this.pageCount ,required this.limit ,required this.total ,required this.outLines ,required this.class ,});

Suggestion

This can be worked around by manually changing the field name. But I wonder whether it is possible to do it by generation automatically without ruining the existed rust code---For example, using a new frb macro to replace it like the serde macro #[frb(deserialize_with = "your_dart_field_name")].

With this feature, whenever there is a key word conflicting, there is no need for user to modify the rust part code.

Other Thoughts

Though I know the class is always the keyword of dart, I wonder why the same rust code could be compiled 6 months ago when it was the last time I modified the rust struct with FRB v1.

Besides, I've thought of other ways to work around it, like reflection in Dart side. But it seems that there is not a way good enough for both pure Dart and Fluttter projects.

@fzyzcjy
Copy link
Owner

fzyzcjy commented Jun 12, 2024

Totally agree!

As for API, we already have #[frb(name = ...)] to rename functions. To be consistent, maybe we can use the same attribute to rename fields.

Feel free to PR for this, alternatively I may work on it later!

I wonder why the same rust code could be compiled 6 months ago when it was the last time I modified the rust struct with FRB v1.

I guess it is because v2 overhauled a lot of things...

@dbsxdbsx
Copy link
Contributor Author

dbsxdbsx commented Jun 12, 2024

As for API, we already have #[frb(name = ...)] to rename functions. To be consistent, maybe we can use the same attribute to rename fields.

Looking forward to it.

@fzyzcjy
Copy link
Owner

fzyzcjy commented Jun 12, 2024

Btw, curious what was the generated output in v1 that does not have problem? (Does it auto rename, or do something else)

@dbsxdbsx
Copy link
Contributor Author

dbsxdbsx commented Jun 12, 2024

I guess the direct reason is that the issued rust struct is not directly in use, so it was not generated at that time(I checked the git diff of generated dart files).
In detail, in my case:
image
since the field and methods(recursively ) of struct MediaDoc is the only struct I want generated, the config file is like this:

# rust_input: rust/src/api/**/*.rs
rust_input: crate::api,crate::media::MediaDoc
dart_output: lib/src/rust
# full_dep: true

Semantically, CmsMediaOutLineJson is only needed when fetching something online, which seems to be(I am not sure all details since I've not touched this project for months) an intermediate type. It means that its instance would not be exposed to dart in any form(param, return type, type filed, even recursively).

And when I did it with frb v1(the version I modified),
the solution I made is to parse only rust types when needed(exposed to Dart, even recursively), starting from raw APIs. That means, if a type B is in file of type A, A is needed, then A would but B would not be generated---IIRC, I used a regex way to do so, which might be unstable yet. So I guess that is why this issued struct was not taken into account at that time.

@fzyzcjy
Copy link
Owner

fzyzcjy commented Jun 12, 2024

so it was not generated at that time

I see, then it looks like v2 does not introduce anything special compared with v1. Anyway, I am working on it.

@dbsxdbsx
Copy link
Contributor Author

dbsxdbsx commented Jun 13, 2024

A side issue.
image
Here, I changed class to classes, which should be ok for generation. But it still failed due to the existing out-of-date generated files(the upper left corner in the image).

So I suggest introducing a mechanism, with which FRB could delete out-of-date generated files both for Rust and Dart first.

And the path to delete could be referred from rust_input and dart_output from the config file flutter_rust_bridge.yaml.

@fzyzcjy
Copy link
Owner

fzyzcjy commented Jun 13, 2024

Looks reasonable! However, there may be some complications: If a file does not change content, it is great to ensure it is not modified (this seems not to be implemented now but may impl in the future), otherwise Rust/Dart compilation cache will fail, and users may observe slower compilation time.

Copy link
Contributor

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new issue.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants