dbsxdbsx commented 1 year ago

Changes

This pr is decided to introduce a new mechanism to handle flaws when using frb with multi-blocks.

idea

The idea is simple---moving all stuff(the shared user-defined structs, methods of the structs, or some other shared API like free_WireSyncReturn) shared(duplicated/conflicted before) to another auto-generated API block.

Checklist

[x] make sure this approach is workable(test if it is compilable)--- Yes, it is workable/runnable.
[x] fix bug #987 --- no redundant type would be produced in regular API-generated file(s) since these shared types are now moved to another shared file(both in Rust and Dart).
[x] fix bug #1111 --- this is solved apparently. For details, please refer here.
[x] fix bug #956 --- methods for a simple struct is solved, no matter where they are defined(API blocks or customized rust module files). But methods related to an Enum is not solved(rust-gen code is ok, dart-gen code is NOT ok, and the corresponding dart test code is added but commented out). Leave it to future pr since this pr is already too heavy.--- Still some issue left.
[x] add other type tests, types like list(vec), enum, option, and SyncReturn types, and their combination are tested,(other complex types like stream, and opaque type are not tested on purpose---leave them in future pr)...
[x] delete debug code
[x] refine test cases for multi-blocks --- the rust/dart test cases are totally rewritten from scratch with lots of useful comments for readability.
[x] CI is passing --- make sure everything is ok with(out) various flag, like wasm, dart_decl_output(MacOs with multi-blocks is not tested partly due to #1225)
[x] refine documents

fzyzcjy commented 1 year ago

Looks interesting!

(given the title has "draft" I converted it into draft. Feel free to convert it back and @ me when ready!)

dbsxdbsx commented 1 year ago

Looks interesting!

(given the title has "draft" I converted it into draft. Feel free to convert it back and @ me when ready!)

I guess you would not be called when committing to a draft pr, right? If so, there would be no worry to bother you.

fzyzcjy commented 1 year ago

No worries, I can just click "unsubscribe" for this. But I am not sure whether I will receive notification if you @ me later. So, if you @ me in the future but I do not reply, just contact me in other issues.

EDIT: I think I will receive notification when @ me given this:

dbsxdbsx commented 1 year ago

@fzyzcjy @dinbtechit It is glad to say that this shared mechanism is workable with multi-blocks---I tested it within my own project on win10. One thing to mention is that, to make the auto-generated shared instance runnable in dart, a logic need to change for frb_dart 7038ac2:

void _sanityCheckSingleton() {
    if (_instances.contains(runtimeType)) {
      // throw Exception(
      //   'Subclasses of `FlutterRustBridgeBase` should be singletons - '
      //   'there should not be two instances (runtimeType=$runtimeType)',
      // );
      print("already contains $runtimeType ");
      return; // no panic here，since the shared instance will parasitize in each regular API block instance.
    }
    _instances.add(runtimeType);
  }

My test code is like this:

  final obj1 = await api1.testRetCustomStruct2In1StBlock();
  print(obj1.defaultScore);
  print("===");
  final obj2 = await api2.testRetCustomStruct2In2EdBlock();
  print(obj2.defaultScore);
  print("==------------------=");

  final obj3 = await api2.testChangeCustomStruct2In2EdBlock(obj: obj1);
  print(obj3.defaultScore);
  print(obj1.defaultScore);

And at present, there are still some logical flaws to fix for generations.

dbsxdbsx commented 12 months ago

In c5957df and 67a5e25, while testing with flag dart-decl-output is added to example pure_dart_multi, I didn't find a way to both tests with/without flag dart-decl-output in the same project(pure_dart_multi) without syntax error (the import statement in main.dart is different for each case). So I set generation with dart-decl-output as default, just like that in pure_dart.

dbsxdbsx commented 11 months ago

In 57a90ac, a setting is changed: whenever a mutable method(not function) is going to be generated, it would be skipped(not panic as before) due to safety issue. This change is due to it being inconvenient for users to separate mutable/immutable methods from customized shared types(structs or enums).

But still, there are some other issues related to the method generations, though the tests are passed. They would be fixed in this pr.

dbsxdbsx commented 11 months ago

Note: Do NOT define shared types (especially those with methods) in the regular API block files, otherwise it will cause some inexplicable generation problems -- this behavior is also not encouraged, since the types are shared, they should not belong to((be defined) in) any sole regular API block.

dbsxdbsx commented 11 months ago

Now with current modifications related to methods, there are new issues occurring: Suppose there is a struct named PlayUrls:

impl PlayUrls {
    pub fn new(label: String, urls: &Vec<String>) -> Self {
    ...
    }
}

The method new is marked pub, so it will be generated in Dart as default, but there are issues:

the output type named Self is not syntactically supported in frb -- Changing it to PlayUrls would fix it;
the input type is of ref type(like &Vec<String>, Vec<&str> or &str), which is not supported in frb currently
the 2ed issue occurs also for method output.

To solve 2,3, since now ref type is not supported in frb, I think there are 2 ways:

since now any public method belonging to shared types(struct or enum) used in regular API blocks would be generated, what if adding a new frb macro like #[frb_gen] to tag the method, with which we can specify which method should be generated in dart code, and which should not -- Note: in this way, methods in shared types NOT defined in regular API blocks would not be generated in default.
Still generate all public methods belonging to shared types, but treat those ref input/output as corresponding non-ref types. -- For example,&Vec,Vec<&str> and &str would be treated as Vec,Vec and String respectively.

(These proposals are not aimed to add in this pr -- this pr is already too heavy =_=)

fzyzcjy commented 11 months ago

Firstly, to be more clear, I wonder what is regular API block? Do you mean impl SomeType { ... } in api.rs or something else?

Secondly, I wonder what frb behaves currently (without the PR) for that case?

(These proposals are not aimed to add in this pr -- this pr is already too heavy =_=)

Yes I totally agree! Adding it to this PR will make it non reviewable and future reader also cannot understand it.

Btw, feel free to quote your text into a new issue, and we can discuss there :)

dbsxdbsx commented 11 months ago

@fzyzcjy,

regular API block(sometimes called regular block in code comments, which is in contrast to shared block) means the rust file used to define all regular API. For example, in pure_dart, it only refers to the api.rs; while in pure_dart_multi, it refers to all rust files users defined to output API (there are currently two blocks, and in this pr, I increased it to 3 for complete tests). -- That is, since there is a struct called BlockIndex, I use the word block to make semantic consistent.
IIRC, I didn't modify the code related to it, so I guess they are still the same as I tested with current PR? I would look into it soon -- if the behavior is found to be introduced from this PR, I would try to fix it (If so, I hope it would not make too much change).

@fzyzcjy, updating with 2, I tested it. Currently, frb behaves the same as I said in this PR. I add this struct at the end of api.rs:
```
struct PlayUrls {}
impl PlayUrls {
pub fn new(label: String, urls: &Vec<String>) -> Self {
    PlayUrls {}
}
}
```
Within the currently published frb after https://github.com/fzyzcjy/flutter_rust_bridge/commit/7208ecd291b91e49b3bfc1c1b09a3ad8ff485d0d, they failed with panic:
```
//panicked at 'Failed to parse function argument type `&Vec<String>`', frb_codegen\src\parser\mod.rs:219:21
//panicked at 'Failed to parse function output type `Self`', frb_codegen\src\parser\mod.rs:255:25
```
@fzyzcjy, to clarify the concept of a block in frb, it refers not only to the original Rust API definition file, but also to the entire context of a specific API Rust file(even it is shared -- not defined obviously). This includes the generated Rust/Dart file and other data that may not be visible to users. Essentially, it is almost the same concept as the IrFile struct found in frb_codegen/src/ir/file.rs. By explaining the term block in this way, it should be easier to understand, even for a shared block.

dbsxdbsx commented 10 months ago

I found that -- if a Rust struct has a method with the same name as a field, the generated Dart code will throw an error. Anyway, this "issue" is ignored in this pr.

dbsxdbsx commented 10 months ago

Since commits 1503354, 905b763 and 110ad2f, the issue #1111 should be resolved with the latest release of frb (1.80.1). However, the current CLI can only assure that the generation is done successfully with flag c-output and extra-c-output-path, but it is not certain if those files can be used correctly with frb (like compiling the final App with the generated C files on MacOs).

To check completely, we would need to either modify the pure_dart_multi example to include a check for the MacOs/IOs platform, or add a new block to the with_flutter example (turn it from a single-block to multi-blocks case). However, it is not recommended to do this in this pull request, as the related CLI named CI / Flutter integration test on MacOS (pull_request) is not yet fixed, and this pull request is already too large.

fzyzcjy commented 10 months ago

or add a new block to the with_flutter example

Or maybe do something like creating a brand new with_flutter_multi example? Just like we have pure_dart + pure_dart_multi examples. (I agree it should be in the next PR)

dbsxdbsx commented 10 months ago

maybe do something like creating a brand new with_flutter_multi example?

Yes，I think ok, better be done after CI / Flutter integration test on MacOS (pull_request) is fixed.

dbsxdbsx commented 9 months ago

Since 0806efb, all basic features/bugs have been fixed/completed. But the doc is not updated yet. Without it, I think it is hard to say what is refined in this pr. So I think it is not time to review until the doc is refined.

dbsxdbsx commented 9 months ago

@fzyzcjy, after merging code within frb v1.81.0 bfd4351 and 9fa3f27, I deleted the build.rs script in example pure_dart_multi, since the corresponding build.rs from pure_dart was also deleted.

Also, since then, I notice the "Dart test with Valgrind" always failed, I don't know why.

Anyway, I think it is time to review. For a rough outline of what is in this pr, I've described in the refined doc of the article Generating multiple files.md and a little modification in the article ios_headers.md.

And since I wrote the pr almost half year ago, even I can't remember every detail, so to mitigate it, I added many code comments in frb_codegen and pure_dart_multi.

fzyzcjy commented 9 months ago

Wow finally this huge PR is ready for review! :tada: I am quite busy but will try to review within a few days (given it is so large). Great job!

fzyzcjy commented 9 months ago

As mentioned there, the Valgrind issue blocks this PR from merging, but I am really busy and have to squeeze some time to review this PR, let alone time for fixing that. Do you have some interest in digging a bit further about that error :)

dbsxdbsx commented 9 months ago

As mentioned there, the Valgrind issue blocks this PR from merging, but I am really busy and have to squeeze some time to review this PR, let alone time for fixing that. Do you have some interest in digging a bit further about that error :)

I have no idea about the issue https://github.com/fzyzcjy/flutter_rust_bridge/issues/1327. Besides, I think it is ok for you to take time to review. This pr is important, but not in a hurry. Maybe once this pr has been thoroughly reviewed/refined, this issue would be worked around.

dbsxdbsx commented 8 months ago

@fzyzcjy, I want to note that on Windows 10, using Flutter 3.10.6 and Rust 1.70.0 works fine. However, when upgrading to Flutter 3.13.x and Rust 1.72 or above, there are some unexplainable issues. Therefore, I recommend not upgrading both Flutter and Rust before merging this pull request.

fzyzcjy commented 8 months ago

OK, I will try to find a time to review this ASAP (still busy now...)

fzyzcjy commented 8 months ago

Let me review it now

fzyzcjy commented 8 months ago

Well let me pause checking details, but check the overall idea first

fzyzcjy commented 8 months ago

Summary

Firstly, great job! This PR is huge, which can be seen from the number of lines of code, but can also be seen from the fact that it fixes three bugs at once, and the logic change requires a nontrivial amount of work. The CI is also green (excluding unrelated failures), with old tests and new tests, so we are pretty sure that the code works.

Secondly, about the amount of code comments (87 up to now, but by tomorrow morning may be a bit more). I agree it is a bit scary, but indeed it is mostly (1) nits that can be resolved without thinking (2) pointing out things that happens repeatedly (e.g. suppose there is a problem occurring 5 times, then I may leave 5 comments at all those files to make it easier to find and fix). Thus, there are indeed no many issues. There does exist a bit of nontrivial issues though, to be honest, such as we should avoid the use of global variables to maintain code readability and maintainability.

I have tried to make as few suggestions as possible. In other words, if something only takes a minute to solve and can improve code quality, I post it; if something requires a non-trivial amount of time, I try my best to avoid posting it, unless we should really rely on it for code quality.

This PR takes me 3.25 hour (may take 0.5-1 hour more tomorrow) to review, which is really big :)

fzyzcjy commented 8 months ago

(copied from yesterday's post to make this the last post after all code review comments)

Summary

Firstly, great job! This PR is huge, which can be seen from the number of lines of code, but can also be seen from the fact that it fixes three bugs at once, and the logic change requires a nontrivial amount of work. The CI is also green (excluding unrelated failures), with old tests and new tests, so we are pretty sure that the code works.

Secondly, about the amount of code comments (87 up to now, but by tomorrow morning may be a bit more). I agree it is a bit scary, but indeed it is mostly (1) nits that can be resolved without thinking (2) pointing out things that happens repeatedly (e.g. suppose there is a problem occurring 5 times, then I may leave 5 comments at all those files to make it easier to find and fix). Thus, there are indeed no many issues. There does exist a bit of nontrivial issues though, to be honest, such as we should avoid the use of global variables to maintain code readability and maintainability.

I have tried to make as few suggestions as possible. In other words, if something only takes a minute to solve and can improve code quality, I post it; if something requires a non-trivial amount of time, I try my best to avoid posting it, unless we should really rely on it for code quality.

This PR takes me 3.25 hour (may take 0.5-1 hour more tomorrow) to review, which is really big :)

dbsxdbsx commented 8 months ago

@fzyzcjy , I gonna first solve the simple nit refinement. For this, I realized that you prefer map and if let instead of match, I understand that map would be more simple to read compared to match; but for if let, why would you prefer it over match? I think match is more readable than if let.

fzyzcjy commented 8 months ago

I gonna first solve the simple nit refinement.

Sure!

For this, I realized that you prefer map and if let instead of match, I understand that map would be more simple to read compared to match; but for if let, why would you prefer it over match? I think match is more readable than if let.

I guess there are several cases.

match bool_condition { true => ..., false => ...} vs if bool_condition - seems the latter is shorter.
say a: Option<Sth>. then about match a { Some(a) => ..., None => ...} vs if let Some(a) = a { ... } else { ... }, I agree both are equivalent, but from my personal perspective, the latter seems to be a bit more common. I googled a bit just now: https://stackoverflow.com/questions/69161173/whats-the-main-difference-between-match-and-if-let-in-rust, and it seems to be described as "a convenience feature"

By the way, in Dart, or Kotlin, or Swift (maybe?), things are a bit different:

int? a = ...;
if(a!=null) {
  // use `a` here, it AUTO become a non-null type
} else {
  // ...
}

which mimics the if let (indeed even shorter than if let).

dbsxdbsx commented 8 months ago

@fzyzcjy, I see there are several suggestions on "NOT using global variable". In fact, I did realize this would be an issue, and I did struggle with whether or not to use it. There are some tips I got from chatGpt on this topic:

Here are some suggestions for when and when not to use global variables in Rust:

Avoid overusing global variables. Global variables break modularity and increase coupling.

Only use global variables when you really need to share state across multiple modules.

Use const to define immutable global variables, and static mut for mutable ones. Make mutable globals private if possible.

Add detailed documentation explaining the purpose and usage of global variables.

Use lazy_static to create lazy global variables if they are only accessed from one thread.

Use Mutex or RwLock on global variables accessed from multiple threads to ensure thread safety.

Access global variables through function interfaces rather than directly when possible.

Consider dependency injection and other patterns to avoid globals where possible.

Use integration tests to ensure globals do not introduce hard-to-find bugs.

In summary, judicious use of global variables can simplify some code scenarios but should be approached with caution. Favor modularity and limit the scope/lifetime of globals where possible. Increase testing to avoid introducing bugs.

I decided to use global var since there would be quite overhead for getting some things unchanged but not known at first. Otherwise, besides the overhead, the signatures of functions/methods would be changed a lot. I think it is a dilemma. So when using global var, I try to avoid those pitfalls by following tips 4 and 7. Also, For some cases, I find it inevitable to use global var.

What do you think of it? Should I still erase all the global stuff?

(By the way, wrapping those global vars into Option<> is one way I came up with to avoid the pitfalls by using global var to a certain degree.)

fzyzcjy commented 8 months ago

@dbsxdbsx Sorry I did not see your comment... (github keeps giving me notifications whenever there is a code commit/push and when there is a comment, so this PR constantly popup because of code commit so I automatically ignored the unread dot... If next time I do not reply, please just ping me!)

I see there are several suggestions on "NOT using global variable". In fact, I did realize this would be an issue, and I did struggle with whether or not to use it. I decided to use global var since there would be quite overhead for getting some things unchanged but not known at first. Otherwise, besides the overhead, the signatures of functions/methods would be changed a lot. I think it is a dilemma. So when using global var, I try to avoid those pitfalls by following tips 4 and 7. Also, For some cases, I find it inevitable to use global var. What do you think of it? Should I still erase all the global stuff? (By the way, wrapping those global vars into Option<> is one way I came up with to avoid the pitfalls by using global var to a certain degree.)

In my humble opinion, I strongly hope the global variable is avoided. Mutability should also be avoided when possible (but that's not a strict requirement and we surely needs mut in many scenarios).

There are also many articles explaining why global variables should not be used, so I guess I do not need to persuade ;)

quite overhead for getting some things unchanged but not known at first

What about caching it? For example, a super simple naive cache is as follows (just a demo. the real case may or may not be this): (btw Rust even gives get_or_insert_with in std)

struct Lazy<F, T> where F: FnOnce() -> T {
  f: F,
  value: Option<T>,
}

impl Lazy {
  fn new(f: F) { Lazy {f, value: None} }
  fn get_or_compute(&mut self) {
    self.value. get_or_insert_with(self.f)
  }
}

then we do not pay extra cost.

the signatures of functions/methods would be changed a lot

Internal (i.e. non-public) signatures are meant to be changed, no worries!

btw, if "changed a lot" means "add 10 parameters to each function", then I guess we can make some abstraction, for example, put the 10 parameters into one struct and just pass around it.

dbsxdbsx commented 8 months ago

@fzyzcjy, with your suggestion. I made a template for those lazy-inited global const var:

use once_cell::sync::Lazy; // need once_cell crate
use std::cell::RefCell;
use std::sync::Mutex;

// the basic struct for lazy const object, replacing global variables
#[derive(Debug, Clone)]
pub struct LazyObject<F, T>
where
    F: FnOnce() -> T,
{
    f: Option<F>,
    value: Option<T>,
}

impl<F, T> LazyObject<F, T>
where
    F: FnOnce() -> T,
{
    pub fn new(f: F) -> Self {
        Self {
            f: Some(f),
            value: None,
        }
    }

    pub fn get(&mut self) -> &T {
        if self.value.is_none() {
            let f = self.f.take().expect("Closure has been called");
            self.value = Some(f());
        }
        self.value.as_ref().unwrap()
    }
}

pub fn expensive_computation_1() -> i32 {
    println!("Performing expensive computation 1...");
    42
}

pub fn expensive_computation_2(a: i32, b: i32) -> i32 {
    println!("Performing expensive computation 2...");
    a + b
}

// Custom struct
#[derive(Debug, Clone)]
pub struct CustomStruct {
    value: usize,
    text: String,
}

pub fn expensive_computation_3(a: usize, b: String) -> CustomStruct {
    println!("Performing expensive computation 3...");
    CustomStruct { value: a, text: b }
}

/// Function for users to call
pub fn get_global_obj1<F: FnOnce() -> i32 + Send + 'static>(f: F) -> i32 {
    static OBJ1: Lazy<Mutex<RefCell<Option<LazyObject<Box<dyn FnOnce() -> i32 + Send>, i32>>>>> =
        Lazy::new(|| Mutex::new(RefCell::new(None)));

    let obj1 = OBJ1.lock().unwrap();
    let mut obj1_ref = obj1.borrow_mut();
    if obj1_ref.is_none() {
        *obj1_ref = Some(LazyObject::new(Box::new(f)));
    }

    *obj1_ref.as_mut().unwrap().get()
}

/// Function for users to call
pub fn get_global_obj2<F: FnOnce(i32, i32) -> i32 + Send + 'static>(f: F, a: i32, b: i32) -> i32 {
    static OBJ2: Lazy<Mutex<RefCell<Option<LazyObject<Box<dyn FnOnce() -> i32 + Send>, i32>>>>> =
        Lazy::new(|| Mutex::new(RefCell::new(None)));

    let obj2 = OBJ2.lock().unwrap();
    let mut obj2_ref = obj2.borrow_mut();
    if obj2_ref.is_none() {
        *obj2_ref = Some(LazyObject::new(Box::new(move || f(a, b))));
    }
    *obj2_ref.as_mut().unwrap().get()
}

/// Function for users to call
pub fn get_global_obj3<F: FnOnce(usize, String) -> CustomStruct + Send + 'static>(
    f: F,
    a: usize,
    b: String,
) -> CustomStruct {
    static OBJ3: Lazy<
        Mutex<RefCell<Option<LazyObject<Box<dyn FnOnce() -> CustomStruct + Send>, CustomStruct>>>>,
    > = Lazy::new(|| Mutex::new(RefCell::new(None)));

    let obj3 = OBJ3.lock().unwrap();
    let mut obj3_ref = obj3.borrow_mut();
    if obj3_ref.is_none() {
        *obj3_ref = Some(LazyObject::new(Box::new(move || f(a, b))));
    }
    obj3_ref.as_mut().unwrap().get().clone()
}

/// Test case: the loop is used to mimic multi-times call on the same lazy object
pub fn test_multi_call() {
    for i in 0..3 {
        let value_1 = get_global_obj1(expensive_computation_1);
        let value_2 = get_global_obj2(expensive_computation_2, i, i + 1);
        let value_3 = get_global_obj3(
            expensive_computation_3,
            (i + 2) as usize,
            "example".to_string(),
        );
        // println!("{}: obj1={:?}", i, value_1);
        println!(
            "{}: obj1={:?},obj2={:?},obj3={:?}",
            i, value_1, value_2, value_3
        );
    }
}

Here the number of get_global_obj*(....) corresponds to the numbers of the lazy-inited global vars. And the output is like this:

Performing expensive computation 1...
Performing expensive computation 2...
Performing expensive computation 3...
0: obj1=42,obj2=1,obj3=CustomStruct { value: 2, text: "example" }
1: obj1=42,obj2=1,obj3=CustomStruct { value: 2, text: "example" }
2: obj1=42,obj2=1,obj3=CustomStruct { value: 2, text: "example" }

Though the code still looks ugly, at least all this dirty code is shrunk in the same file and all global vars are restricted to be used in the correspondingly sole function. Are you ok with it?

fzyzcjy commented 8 months ago

@dbsxdbsx Hi, nice code! However, IMHO the suggestion is to avoid global variables. Putting them inside a function (static variable) is indeed still global variable 😢

For example, we can have a look at Flutter framework https://github.com/flutter/flutter. As we can see, it uses almost zero global variables (except for special cases, for example, a flag that is enabled when in testing - so it never affects real code except for unit test). The frb is definitely much simpler than Flutter framework itself.

dbsxdbsx commented 8 months ago

@dbsxdbsx Hi, nice code! However, IMHO the suggestion is to avoid global variables. Putting them inside a function (static variable) is indeed still global variable 😢

For example, we can have a look at Flutter framework https://github.com/flutter/flutter. As we can see, it uses almost zero global variables (except for special cases, for example, a flag that is enabled when in testing - so it never affects real code except for unit test). The frb is definitely much simpler than Flutter framework itself.

Could you give a more detailed code example in Dart or Rust for the lazy-inited const objects? I have no idea to do so.

fzyzcjy commented 8 months ago

Could you give a more detailed code example in Dart or Rust for the lazy-inited const objects? I have no idea to do so.

IMHO frb does not need any lazy-inited const objects.

Btw, if not read before I guess this can be used as a reference to see the overall design: https://cjycode.com/flutter_rust_bridge/contributing/design.html

As for the global variable problem: For example, in a high-level thought, frb looks like

val ir = parser::parse();
dart_generator::generate(ir);
rust_generator::generate(ir);

This does not need any global variable, just normal data flow (of local variables).

Could you please provide an example why global variable is needed? I can see there are many in the code, but the context is so big (so many code) that it is not very easy to quickly get the main idea why it is needed. By providing a short sample, maybe I can suggest on the example how to refactor it.

dbsxdbsx commented 8 months ago

Could you please provide an example why global variable is needed?

Roughly, you know that the shared block needed to be dealt with first before the regular Api block(For example, the global var SHARED_METHODS_WIRE_NAMES), right? And it is easy to see it is because there is something shared that could only be fetched when dealing with the shared block but would be used when dealing with those regular blocks afterward.

Now if completely avoiding global vars, then it means there need to design the logic for the shared block completely differently--- make it another function that outputs essential shared stuff, then put the essential shared stuff as another param for the regular block. I am currently not sure if this change would lead to another big change.

fzyzcjy commented 8 months ago

@dbsxdbsx For this specific case, a quick brainstorm:

Create a new function, say generate_shared_wire_names, which generates what should be shared in advance. Then, use this data to generate the shared block and regular blocks. IMHO this may be easier to understand and maintain (at least compared with global variable)

i.e.

let ir = parser::parse();
let what_is_shared = generate_shared_wire_names(ir);
generator::generate(ir, what_is_shared, mode=shared_block);
generator::generate(ir, what_is_shared, mode=regular_block_i);

Similarly, we can handle other data like this. (Feel free to ask any questions if there are other problems!)

dbsxdbsx commented 8 months ago

@dbsxdbsx For this specific case, a quick brainstorm:

Create a new function, say generate_shared_wire_names, which generates what should be shared in advance. Then, use this data to generate the shared block and regular blocks. IMHO this may be easier to understand and maintain (at least compared with global variable)

i.e.
let ir = parser::parse();
let what_is_shared = generate_shared_wire_names(ir);
generator::generate(ir, what_is_shared, mode=shared_block);
generator::generate(ir, what_is_shared, mode=regular_block_i);
Similarly, we can handle other data like this. (Feel free to ask any questions if there are other problems!)

The core issue is... "shared block" is dealt in the same routine as what is dealt with the regular block before introducing multi-blocks---That is , they are isolated(otherwise the whole code would be too coupled, I want make as less effect as possible for developers responsible for the regular blocks), and each one is dealt with functionfrb_codegen_multi as the start one by one. Thus, following your idea, there need to be a way to return the current global vars produced after dealing with the whole shared block, so that it can be used (as params) when start to deal with the regular block afterward, avoiding using all global vars.

Let's still take SHARED_METHODS_WIRE_NAMES as an example. It is fetched when dealing with the shared block in func get_exclude_symbols, which only returns the excluded_symbols currently. So following your idea, it has to add another element to its current return type to fetch shared_methods. Then, since get_exclude_symbols is used in pub(crate) fn generate_dart_code, the return type of it needs also be changed correspondingly. Also the return type of frb_codegen_multi and frb_codegen which takes generate_dart_code needs to be changed.

Do you see what I mean? You think taking global/static vars paves the way for hard maintenance, while without using any global/static vars, it now would explicitly violate the whole design of frb.

May I propose my suggestion again.

This approach utilizes the once_cell library and a custom LazyObject structure to create lazy-initialized global variables restricted to specific functions. This ensures limited use of global variables without affecting the overall frb_codegen code readability and maintainability.

Advantages of this solution include:

Lazy-initialized global variables with LazyObject, avoiding unnecessary computations in aferward routines(with frb_codegen_multi or frb_codegen).
Restricted use of global variables within specific functions, maintaining code readability and maintainability--- the raw global/static vas are not pubed, to avoid code over-usage.
Thread-safe data structures like Mutex and RefCell to prevent data race issues(that is why they are invented)--make the whole project easy to debug if an issue is related to the global/static vars.

With the above 3 advantages, I believe this solution is the best approach to the problem and would not lead to hard maintainance of the whole code.

Please take a moment to review the provided code. If we don't adopt this solution, the entire project may require significant changes. I believe this approach minimizes risks associated with global variables while maintaining code quality.

(NOTE: As I've said, I've struggled in whether or not to use global/static var before. I don't want to use it unless I have to. So if avoiding use it would lead to making even further hard maintenance of the whole code, then I would decide to use it.)

Let me know if you have any questions or concerns. I'm happy to discuss further and explore other options if necessary.

fzyzcjy commented 8 months ago

Thanks for the information!

Firstly, I totally understand your hard work - this PR is quite nontrivial (when saying "trivial" I mean the keyword in math - roughly something that is quite easy)!

I can also understand that, if I did a lot of coding, and someone asks me to refactor it, I may feel sad because the code I have wrote may need to be changed. I do have such experience, indeed a lot (not sure whether I have told you before). Just look at my PRs in flutter/flutter and flutter/engine repositories (all are public, surely). Excluding the trivial ones, many of them are, I spent time working on it, submit PR, and gets rejected - not only "change it", but "reject it"; some of them requires refactor but eventually gets accepted (which are the lucky ones).

Why Flutter does so? I can totally understand them. They need to ensure the code is of super high quality (frb does not need that high quality, but still needs a clean and easy-to-maintain code), such that it can be maintained, can evolve in the future, etc.

As for frb, my philosophy is that, I try my best to never reject PRs, and even try to not require any nontrivial changes(refactors), since I know it is hard work. However, given the common goal of you and me - make the project more feature-rich, stable, highly maintainable, evolvable, ... - sometimes I have no choice but to require some refactors :( Luckily, the refactors are not super hard. The global-variable-removal mainly changes how data flows, and does not require something super fancy or hard.

Thus, following your idea, there need to be a way to return the current global vars produced after dealing with the whole shared block, so that it can be used (as params) when start to deal with the regular block afterward, avoiding using all global vars.

Well, I mean it maybe possible to create some information before generating the blocks. i.e.

let ir = parser::parse();

// e.g. maybe something like `struct { the_function_names_that_is_shared: Vec<string>, the_struct_names_that_is_shared, ... }`
let what_is_shared = generate_shared_wire_names(ir);

// here, the shared block is really generated
generator::generate(ir, what_is_shared, mode=shared_block);

// here, the regular blocks
// we may make use of what_is_shared, for example, by "if a wire function is mentioned in the_function_names_that_is_shared, we just call the shared api, instead of the block api"
generator::generate(ir, what_is_shared, mode=regular_block_i);

In other words:

We already pre-compute a lot of things prior to calling generator (e.g. the IR). Then let's just precompute a bit more - "which exact function/struct/... shall we put in the shared block vs regular block".

Let's still take SHARED_METHODS_WIRE_NAMES as an example. It is fetched when dealing with the shared block in func get_exclude_symbols, which only returns the excluded_symbols currently. So following your idea, it has to add another element to its current return type to fetch shared_methods. Then, since get_exclude_symbols is used in pub(crate) fn generate_dart_code, the return type of it needs also be changed correspondingly. Also the return type of frb_codegen_multi and frb_codegen which takes generate_dart_code needs to be changed.

If I understand correctly, maybe this paragraph means something like:

let ir = parser::parse();
// let what_is_shared = generate_shared_wire_names(ir); // no this line
let what_is_shared = generator::generate(ir, what_is_shared, mode=shared_block);
generator::generate(ir, what_is_shared, mode=regular_block_i);

i.e. the what_is_shared information (SHARED_METHODS_WIRE_NAMES) is generated when we are running the generate-shared-block code?

If that is the case, I hope my explanations above (the first few paragraphs) helps.

Do you see what I mean? You think taking global/static vars paves the way for hard maintenance, while without using any global/static vars, it now would explicitly violate the whole design of frb.

Hope my explanations above show that it does not violate the design of frb.

By the way, if global variable is a necessity, then similar projects - cbindgen, pyo3, whatever, will all use global variables.

Lazy-initialized global variables with LazyObject, avoiding unnecessary computations in aferward routines(with frb_codegen_multi or frb_codegen).

The proposal above (generate_shared_wire_names) also ensures only one computation - all data is computed and put in that what_is_shared variable. Even no need for lazy (or few need)

Restricted use of global variables within specific functions, maintaining code readability and maintainability--- the raw global/static vas are not pubed, to avoid code over-usage.

I am sorry, but this may not fully solve the problem. For example, one headache global variable causes is like: Suppose I have a function fn f(a: i32) -> i32. Then someone may think "f utlizes a and yields an output". But if f uses a global variable - either directly or hidden via another function (or even hidden via many deep function calls) - we are in trouble: The code is indeed "f utilizes a, but also an arbitrary number of global variables, which are introduced in arbitrary far away code, and you have no idea how f uses them by looking at the function signature". In other words, f now depends on some places far away in the whole world, without any notice. Then it is much harder to maintain, add new features, fix bugs, etc IMHO :(

Thread-safe data structures like Mutex and RefCell to prevent data race issues(that is why they are invented)--make the whole project easy to debug if an issue is related to the global/static vars.

Sure, Rust ensures this - otherwise we never compiles!

With the above 3 advantages, I believe this solution is the best approach to the problem and would not lead to hard maintainance of the whole code. Please take a moment to review the provided code. If we don't adopt this solution, the entire project may require significant changes. I believe this approach minimizes risks associated with global variables while maintaining code quality.

As is shown in the discussion above, it is not very easy for me to review the code using global variables. It is not as easy as "hey by looking at the input and output variables of the code, I can roughly know what this function should do, what is its side effects, and let me briefly glance at the implementation, and that's all". Instead, I have to be very very careful - "hey does this function use global variable? how does it use it? where is global variable introduced? we will run into trouble if in the future the code writting global variable - though far away - is somehow refactored?"

fzyzcjy commented 8 months ago

(btw, the code conflicts comes from the merging of the support of exception types, which started from a very ancient PR)

dbsxdbsx commented 8 months ago

@fzyzcjy , 3 things I want to clarify:

following your idea, for both avoiding using global variables and overhead computation, there are 2 key funcs: parse_configs_and_symbols and frb_codegen_multi, the former one outputs lists of raw_configs(including the shared one if necessary); and the latter would process each raw_config(including the shared one) one Opt at a time. So... I think you mean that trying best to gain all shared stuff inside parse_configs_and_symbols? For this, there may need a new field for struct Opt to store all shared stuff. If Ok, then I would say I would try my best to just shrink the current global vars into shared stuff only inside parse_configs_and_symbols, but I also think it is inevitable to also change the signature of frb_codegen_multi---Maybe a new param like Option<SharedStuff> is essential for both the input and output of frb_codegen_multi（Maybe it is not needed, not sure at present）. Is that OK?
The refactor job to avoid the global vars is indeed not easy, there would be many codes be changed. Would you think it is better for me to do the refactor first or solve other issues first? --- If refactoring first, lots of present conversatons would be out of dated. If refactoring aferwards, many changes in converstions would be meaningless then.
A side question, not related to this pr. For programming languages like Rust, Dart, Python， and C++... do you think it is always a better choice to not use global variables, even for a personal simple project?

dbsxdbsx commented 8 months ago

(btw, the code conflicts comes from the merging of the support of exception types, which started from a very ancient PR)

Do you refer to the CI / Flutter integration test on Windows (pull_request) and CI / Flutter integration test on MacOS (pull_request)? That is weird, since CI / Flutter integration test on Linux (pull_request) doesn't have issue.

fzyzcjy commented 8 months ago

Do you refer to the CI / Flutter integration test on Windows (pull_request) and CI / Flutter integration test on MacOS (pull_request)? That is weird, since CI / Flutter integration test on Linux (pull_request) doesn't have issue.

No, I mean https://github.com/fzyzcjy/flutter_rust_bridge/pull/1325 is merged

fzyzcjy commented 8 months ago

following your idea, for both avoiding using global variables and overhead computation, there are 2 key funcs: parse_configs_and_symbols and frb_codegen_multi, the former one outputs lists of raw_configs(including the shared one if necessary); and the latter would process each raw_config(including the shared one) one Opt at a time. So... I think you mean that trying best to gain all shared stuff inside parse_configs_and_symbols? For this, there may need a new field for struct Opt to store all shared stuff. If Ok, then I would say I would try my best to just shrink the current global vars into shared stuff only inside parse_configs_and_symbols, but I also think it is inevitable to also change the signature of frb_codegen_multi---Maybe a new param like Option is essential for both the input and output of frb_codegen_multi（Maybe it is not needed, not sure at present）. Is that OK?

For this, there may need a new field for struct Opt to store all shared stuff.

I am not very sure. "Opt" means "options", while the precomputed data like "which symbols are shared while which are not" does not look like an "option". So maybe we can change the signature and give proper names (e.g. just for brainstorming, a naive idea is struct What {opt: Opt, information_related_to_shared_blocks_etc: SomeOtherType}).

change the signature of frb_codegen_multi

As long as it is trivial to migrate, just write a few lines in the doc (maybe we should add a new page in doc, titled "migrating from old versions") mentioning how to do that, and it looks OK for me.

The refactor job to avoid the global vars is indeed not easy, there would be many codes be changed. Would you think it is better for me to do the refactor first or solve other issues first? --- If refactoring first, lots of present conversatons would be out of dated. If refactoring aferwards, many changes in converstions would be meaningless then.

For me both looks OK. I personally prefer resolving the easy conversations first, while leaving the conversations which are hard and likely to be invalidated by refactor for the future. Many conversations can be resolved within a minute (e.g. "flip the branches of if").

A side question, not related to this pr. For programming languages like Rust, Dart, Python， and C++... do you think it is always a better choice to not use global variables, even for a personal simple project?

From my personal perspective, it depends. For example, if you just want a quick script that will be used only once, write whatever code that violates all best practice, and it is totally fine. Or, for some fields, such as when you are writing deep learning or sci computing experiments that runs on Jupyter Notebook, global variables are used extensively - that's the nature. However, for a serious project that will affect many people, making the code clean is generally a good idea.

dbsxdbsx commented 8 months ago

One thing I want to mention here (though not related to this pr, it did bother me when commiting code):

I am always coding on win10 with dart 3.0 or above, since https://github.com/fzyzcjy/flutter_rust_bridge/commit/b20fda6a2131eae01168dbd3fcae0d8cb821e743 by @Desdaemon and before my new commit 2b994b4, every time running just precommit on the pure_dart_multicase inside frb_codegen\src\main.rs, the output of frb_example\pure_dart_multi\dart\lib\bridge_definitions.dart is randomly inconsistent --- The enum type would sometimes contain prefix sealed and sometimes not, while the online CI test would always output with the prefix sealed.

After a deep debugging, I realized it is related to the field `/// Disable language features introduced in Dart 3.

[arg(long = "no-dart3", action(ArgAction::SetFalse))]

#[serde(default = "r#true")]
pub dart3: bool,` inside `struct RawOpts`.  Well... though I worked around the issue by always setting `dart3: true` for  the `pure_dart_multi` example within 2b994b4, still this flag `no-dart3` seems to be somehow **unintuitive**, which let me do a little brainstorming to figure out what the situation is for field `dart3` if no setting the flag and what `dart3` would be if just set the flag with no true or false.  Hopefully, this could be refined in the future.

fzyzcjy commented 8 months ago

Hmm that looks like a problem. Feel free to create another PR to refine it!

dbsxdbsx commented 8 months ago

When considering the name for the concept of "share", it's important to distinguish between two situations:

For the Opts and IrFile structs, which represent the concept of the whole block of code, we can use a simple boolean flag shared: bool to indicate whether they are shared or not. This applies regardless of the programming language the block represented (e.g. Rust, Dart, or C) since the only options are for shared or not shared(regular) Api block.
For types and functions within a block, we can use a more nuanced approach with an enum ShareMode. This allows us to specify different ways in which a type or function can be shared, beyond just being unique or shared. For example, a shared type (currently represented by ShareMode::Shared) could be further specified as ShareMode::SharedBySafeIdent, ShareMode::SharedInAllBlocks, ShareMode::SharedInPartBlocks, and so on in the future.

To clarify the difference between shared and share_mode, I made a commit 237922e.

dbsxdbsx commented 7 months ago

A temporary report： The left conversation is mainly related to the global vars issue. Several days ago, after creating a new Struct AllConfigs like so( draft):

use std::collections::HashSet;
use std::ops::Index;
use std::path::Path;

use crate::ir::{IrFile, IrType};
use crate::parser::ParserResult;
use crate::Opts;

#[derive(Debug, Clone, PartialEq, Eq)]
pub struct AllConfigs {
    regular_configs: Vec<Opts>,
    shared_config: Option<Opts>,
    regular_ir_files: Vec<IrFile>,
    shared_ir_file: Option<IrFile>,
    all_shared_types: HashSet<IrType>,
    input_shared_types: HashSet<IrType>,
    output_shared_types: HashSet<IrType>,
}

I realized that it is inevitable to make the project compilable only after solving(removing) the global vars issue. And now I also found some logical flaws during refactoring--it needs to redesign the whole logic when generating Irfiles for both share or non-share blocks and compatible with the context of input, and output related to function distinct_types.

So I guess it still needs at least several days before the next commit. But after that, the left should be easy to solve.

fzyzcjy commented 7 months ago

Take your time - refactoring to remove global variables does require engineering work!

(Btw I am not sure whether we should call it AllConfigs, but that's another story we may discuss later, and that's pretty trivial - even if we are unhappy about the name, renaming it just takes a few second by an IDE)

fzyzcjy commented 6 months ago

I have been working on a tentative refactoring. However, during the refactor, I realize it seems possible to implement the multi-file by the way.

Surely, I appreciate your (a lot of) hard work on this PR (and previous PRs)! Therefore, even if I manage to implement the feature later, I will ensure your contribution is shown on GitHub. For example, in addition to mentioning the contribution on README as usual, maybe we can merge your PR and then revert it (no worries about the code conflict - it is not a problem since we revert it immediately). Then, your code will be attached to the master branch.

dbsxdbsx commented 6 months ago

I have been working on a tentative refactoring. However, during the refactor, I realize it seems possible to implement the multi-file by the way.

Surely, I appreciate your (a lot of) hard work on this PR (and previous PRs)! Therefore, even if I manage to implement the feature later, I will ensure your contribution is shown on GitHub. For example, in addition to mentioning the contribution on README as usual, maybe we can merge your PR and then revert it (no worries about the code conflict - it is not a problem since we revert it immediately). Then, your code will be attached to the master branch.

I see.

fzyzcjy / flutter_rust_bridge

Introduce auto-generated shared API block(file) for case multi-blocks #1145

Changes

idea

Checklist

IIRC, I didn't modify the code related to it, so I guess they are still the same as I tested with current PR? I would look into it soon -- if the behavior is found to be introduced from this PR, I would try to fix it (If so, I hope it would not make too much change).

Summary

Summary

[arg(long = "no-dart3", action(ArgAction::SetFalse))]