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 option to ignore "Automatically rename function names to avoid keyword conflict" check #2194

Closed
erdemyerebasmaz opened this issue Jul 8, 2024 · 4 comments · Fixed by #2227
Labels
awaiting Waiting for responses, PR, further discussions, upstream release, etc bug Something isn't working

Comments

@erdemyerebasmaz
Copy link

erdemyerebasmaz commented Jul 8, 2024

Describe the bug

Re: Automatically rename function names to avoid keyword conflict #2150

We have method named sync() on our project, which didn't cause any keyword conflict issues so far*, and with this new change, it's generated as sync_().

Force renaming it as sync via #[frb(name = "...")] macro isn't applied.

#[frb(name = "sync")]
pub fn sync(&self) {
    ...
}

Can we add a macro to ignore this check or apply the rename macro after this check?

Thanks!


*: After investigating, not all Dart keywords cause an issue when used as a function name so there has to be made a distinction between problematic & unproblematic ones when automatically renaming.


Steps to reproduce

Create an empty Dart project & class, then create functions using Dart keywords. Here are a couple of the cases I run into and there ought to be more.

  • default()
    • error: 'default' can't be used as an identifier because it's a keyword.
  • false()
    • error: The expression doesn't evaluate to a function, so it can't be invoked.
  • else()
    • info: Statements in an if should be enclosed in a block.
  • sync()
    • non-problematic

Expected behavior

Automatic rename should not apply to Dart keywords that are not problematic to be used as a function name.

Generated binding code

// Rust input
pub async fn sync() {
    ...
}

// Dart output
Future<void> sync_();

OS

macOS Sonoma Version 14.5

Version of flutter_rust_bridge_codegen

2.1.0

Flutter info

Flutter 3.22.2 • channel stable • https://github.com/flutter/flutter.git
Framework • revision 761747bfc5 (5 weeks ago) • 2024-06-05 22:15:13 +0200
Engine • revision edd8546116
Tools • Dart 3.4.3 • DevTools 2.34.3

Version of clang++

clang version 18.1.5

Additional context

No response

@erdemyerebasmaz erdemyerebasmaz added the bug Something isn't working label Jul 8, 2024
@fzyzcjy fzyzcjy added the awaiting Waiting for responses, PR, further discussions, upstream release, etc label Jul 8, 2024
@fzyzcjy
Copy link
Owner

fzyzcjy commented Jul 8, 2024

Looks reasonable! Then we should change the check when it is used as function name.

Feel free to PR for this, alternatively I will work on it in the next batch.

For completeness: The workaround - as you have already done - is to stick to 2.0.0 before it is fixed

@erdemyerebasmaz
Copy link
Author

Looks reasonable! Then we should change the check when it is used as function name.

Feel free to PR for this, alternatively I will work on it in the next batch.

For completeness: The workaround - as you have already done - is to stick to 2.0.0 before it is fixed

Thank you for the swift response!

Some notes: See footnote on Dart keywords, keywords that are flagged 2-3 should be good to use as a function name.

Keywords that are flagged 1 are not safe to use as a function name in some contexts.
For example; using yield() inside a generator function is unsafe
Which, imo, should be overwriteable using a macro if you are certain they won't be used in such contexts.

@fzyzcjy
Copy link
Owner

fzyzcjy commented Jul 8, 2024

I see.

Which, imo, should be overwriteable using a macro if you are certain they won't be used in such contexts.

Then a simpler method may be:

  • When user provide a dart name via #[frb(name = ...)], we should NOT run the auto checker and should directly follow what the user requires.

erdemyerebasmaz added a commit to breez/breez-sdk-liquid that referenced this issue Aug 12, 2024
sync API on bindings had caused a regression with 2.1.0 changes: fzyzcjy/flutter_rust_bridge#2194

which is now resolved with 2.2.0.
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 Aug 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
awaiting Waiting for responses, PR, further discussions, upstream release, etc bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants