-
Notifications
You must be signed in to change notification settings - Fork 319
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
How to adjust #[frb(external)]
with Trait bound?
#2102
Comments
You meant even if i just put crate name, it will automatically look for the workspace? # pkg/dart/flutter_rust_bridge.yaml
rust_input: crate::api,gluesql-core,gluesql_memory_storage
dart_output: lib/src/rust flutter_rust_bridge_codegen generate
cd rust
cargo check it gets error: // rust/src/frb_generated.rs
..
impl SseEncode for gluesql_memory_storage::MemoryStorage {
// failed to resolve: use of undeclared crate or module `gluesql_memory_storage`
.. It should be memory_storage::MemoryStorage. Cuz |
Oops I see. Such rename is not supported yet (since nobody said before and I did not know it), but feel free to PR! Alternatively, is it possible to rename your names, such that their name matches? (I guess this is not a breaking change since end users see gluesql_memory_storage, but please correct me if I am wrong) |
Btw, since you control the "third party" crate, this may be helpful: https://cjycode.com/flutter_rust_bridge/guides/miscellaneous/attributes#specifying-attributes (I will add a doc soon) EDIT: Doc preview (just screenshot of my local dev version) - indeed quite simple: It is on #2100 as well. |
As you told, I renamed our workspace names. But I'm also interested in contributing to your project as well. I will try to create PR to implement
To solve this, should i use both auto scan and manual mirror + non_opaque ?
|
Looking forward to your PR!
Firstly, do you really want those automatic getters/setters? If not, we can ignore it. If yes -
Yes!
|
// pkg/dart/rust/src/frb_generated.rs
use gluesql_core::ast_builder::expr::function::*; // module `expr` is private This line got error: module It looks like it happens when the third party module used https://github.com/gluesql/gluesql/blob/main/core/src/ast_builder/mod.rs#L79 // core/src/ast_builder/mod.rs
pub use expr::{
aggregate::{avg, count, max, min, stdev, sum, variance, AggregateNode},
function,
}; The correct generated code should be // pkg/dart/rust/src/frb_generated.rs
use gluesql_core::ast_builder::function::*; |
Yes, currently it parses pub use expr::aggregate::function;
pub use expr::aggregate::*; But may not parse that complex pub use. But pub use expr::aggregate::{avg, count, max, min, stdev, sum, variance, AggregateNode}; can be supported (i.e. |
What can be the workaround for now? just ignore the whole module? |
If you do not want it, try to ignore the module. If you want it, try But I may implement it today (not guaranteed; but if you see a PR then I do implement it), so if you are not in a hurry, just wait a bit :) |
I may also support the more complicated form now, i.e. pub use expr::{
aggregate::{avg, count, max, min, stdev, sum, variance, AggregateNode},
function,
}; since the parsing looks similar |
I found another issue. // GlueSQL
// core/src/translate/mod.rs
use {
crate::{ast::Statement, ..},
sqlparser::ast::{Statement as SqlStatement, ..},
};
pub fn translate(sql_statement: &SqlStatement) -> Result<Statement> Cuz GlueSQL has translate layer from sqlparser::ast::Statement to gluesql::ast::Statement, There are few aliasings like // FRB
// pkg/dart/rust/src/frb_generated.rs
fn wire__gluesql_core__translate__translate_impl(..) {
..
flutter_rust_bridge::for_generated::RustAutoOpaqueInner<SqlStatement> // cannot find type `SqlStatement` in this scope
.. To solve it, there should be import and aliasing like this. // FRB
// pkg/dart/rust/src/frb_generated.rs
use gluesql_core::sqlparser::ast::Statement as SqlStatement; I can maually add it by rust_preamble, But it would be nicer if it's done automatically |
Yes, aliasing is not supported yet. I may work on that later, but the priority may not be super high, since it may complicate things. Also feel free to PR! |
I know lifetime is not fully supported yet // gluesql/core/src/store/function.rs
#[derive(Clone, Debug)]
pub enum ExprNode<'a> {
Expr(Cow<'a, Expr>),
SqlExpr(Cow<'a, str>),
Identifier(Cow<'a, str>),
.. // pkg/dart/rust/src/frb_generated.rs
impl SseDecode for ExprNode { // 🦀 implicit elided lifetime not allowed here
// Codec=Sse (Serialization based), see doc to use other codecs
fn sse_decode(deserializer: &mut flutter_rust_bridge::for_generated::SseDeserializer) -> Self {
let mut inner = <RustOpaqueMoi<
flutter_rust_bridge::for_generated::RustAutoOpaqueInner<ExprNode>,
>>::sse_decode(deserializer);
return flutter_rust_bridge::for_generated::rust_auto_opaque_decode_owned(inner);
}
} Is it bug? or there is some option i can try? i already tried I manually added // pkg/dart/rust/src/frb_generated.rs
flutter_rust_bridge::frb_generated_moi_arc_impl_value!(
flutter_rust_bridge::for_generated::RustAutoOpaqueInner<ExprNode>
);
|
Hi, could you please provide a minimal reproducible sample? Then I can check what is happening. Without more info, I suspect it is because you have some function signature that confuses current frb and make it believe ExprNode should be treated without lifetime. |
Yes I just pushed to that PR. And the issue is at this line |
I will have a look at it soon! |
And btw, since that will be a package instead of app, this may be helpful: https://cjycode.com/flutter_rust_bridge/manual/integrate/cargokit; or https://cjycode.com/flutter_rust_bridge/guides/miscellaneous/pure-dart (but that requires Dart feature that is not on stable yet) |
Hmm, |
I tried and removed that line. cuz even after using that option, the issue was same and other part got worse with more errors.. |
Ok, I will try to enable it and see what is going on. Because if disable it, then lifetime is definitely not supported, then we can never compile with those struct-with-lifetimes! |
Btw, a side question: I am curious is there any doc about why/when should someone choose GlueSQL compared with other databases? (e.g. I have heard of MySQL, MongoDB, ... on server side; Hive, Isar, SQLite, Drift, Real, ObjectBox,... on Flutter side; etc etc...) GlueSQL looks interesting to me by glancing at the docs, but it would be great if someone from GlueSQL can explain a bit, since I definitely cannot say I am a database expert! My personal guess: Firstly, it is in Rust, thus (usually) fast and safe (especially https://github.com/search?q=repo%3Agluesql%2Fgluesql+%22unsafe%22&type=code seems no unsafe code); Secondly, maybe a lot of storages available, making it good for some scenarios. |
Btw, |
The latest frb has some confusing error (possibly because it now understands more advanced
Thus it seems these things needs to be solved (e.g. one error is "struct with unit fields are not supported yet) I will (temporarily) halt here, but feel free to ping me if there are any issues! |
Cuz those documents are quite outdated.. I will manually answer it here.
|
I see, now I understand the differences (it does seem to be quite different from things like SQLite etc), and that's pretty interesting! |
(Reopen since the task of discussing translating the whole package is not finished thus to make it appear on my tracker) |
Briefly checked the lifetime and other errors issue. I suggest to firstly |
After i upgraded frb to 2.0.0, my src/library/codegen/ir/hir/tree/module.rs:41:41: index out of bounds: the len is 0 but the index is 0 When i tried with master branch, it got [2024-07-06T06:51:37.412Z ERROR frb_codegen/src/library/utils/logs.rs:55] panicked at frb_codegen/src/library/codegen/ir/mir/ty/structure.rs:45:17:
no entry found for key=MirStructIdent(NamespacedName { namespace: Namespace { joined_path: "gluesql_core::ast_builder" }, name: "PrimaryKeyNode" })
thread 'main' panicked at frb_codegen/src/library/codegen/ir/mir/ty/structure.rs:45:17: All i did was just adding It did not happened with previous 2.0.0.-dev42 version. Can you guess why? |
Firstly, try to run frb on master branch which contain some fixes that may be related (I will publish 2.1.0 soon). If still error, feel free to create an issue! |
I tried with 2.1.0 and it says seems like it does not recognize some specific struct? #[derive(Clone, Debug)]
pub struct PrimaryKeyNode;
impl<'a> PrimaryKeyNode {
pub fn eq<T: Into<ExprNode<'a>>>(self, expr: T) -> IndexItemNode<'a> {
IndexItemNode::PrimaryKey(expr.into())
}
} And how can i ignore third party module? i couldn't find any information in docs |
IIRC
Since you control the third party module, just do this directly:
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Close since seems to be solved, but feel free to create new issues if you find any! @devgony Btw, how is the gluesql going on? |
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. |
Discussed in #2101
Originally posted by devgony June 18, 2024
First of all, Could you please check if i can use automatic third-party scanning for this crate
If not, I want to mirror these methods with Trait bounds
But it gets
Doesn't it support
#[frb(external)]
with Trait bound?Do you know how to solve it?
fzyzcjy
2 hours ago
Maintainer
영어로 된 원본 주석 -
I briefly checked https://github.com/gluesql/gluesql/blob/main/pkg/javascript/src/lib.rs, the JS api of that Rust crate to see what it looks like.
Looks like it has only several functions instead of many. Then in that case, I guess manually mirror some methods are quicker. (But if you need to mirror a ton of functions, maybe consider scanning the crate automatically.)
I want to mirror these methods with Trait bounds
Firstly,
fn plan<Sql: AsRef>(&mut self, sql: Sql)
may be
fn plan(&mut self, sql: String)
Secondly, generics are not supported yet (thus the T error). Thus I personally suggest checking how https://github.com/gluesql/gluesql/blob/main/pkg/javascript/src/lib.rs is done (it has no generics etc) and maybe follow that way.
This doc may be related as well: https://github.com/fzyzcjy/flutter_rust_bridge/blob/feat/12337/website/docs/guides/third-party/manual/wrappers.md
EDIT: Oh I realize you are 3rd contributor to that repo! Then the answer depends on your needs. For example, originally I thought you are an user, then I suggest creating small API that suffices your needs; but if you are package contributor and want to add official Dart/Flutter support, then that would be another story - e.g. whether you want to expose to users a very rich API.
EDIT: I see the PR gluesql/gluesql#1503 code. For things like https://github.com/devgony/gluesql/blob/6d059cb1314a97f87b43bb1246a093d6bcb49f8f/pkg/dart/rust/src/api/value.rs#L12, I think it can be automatically scanned. For complex things like the generic T you mentioned above, I guess there needs to be some manual work (to avoid code duplication, possibly also done with a Rust macro). One way is to firstly try to scan it automatically (and maybe put #[frb(ignore)] on some things), and then manually add other things that cannot be scanned.
I am modifying doc about this, thus please refer to https://github.com/fzyzcjy/flutter_rust_bridge/tree/feat/12337/website/docs/guides/third-party for latest before it is merged.
2 replies 1 new
@devgony
devgony
1 hour ago
Author
영어로 된 원본 주석 -
EDIT: Oh I realize you are 3rd contributor to that repo! Then the answer depends on your needs. For example, originally I thought you are an user, then I suggest creating small API that suffices your needs; but if you are package contributor and want to add official Dart/Flutter support, then that would be another story - e.g. whether you want to expose to users a very rich API.
Even though the JS (or Python) binding of GlueSQL used just simple wrapper, Dart is something different. Cuz it will be the first static typed language we are gonna support. I will make it as a standard of typed language binding.
So as you said, I wanted to expose very rich API. After Dart 3, with the power of switch expression and pattern matching, i thought it would be really cool to use all the types of GlueSQL in Flutter.
EDIT: I see the PR gluesql/gluesql#1503 code. For things like https://github.com/[devgony](https://github.com/devgony)/gluesql/blob/6d059cb1314a97f87b43bb1246a093d6bcb49f8f/pkg/dart/rust/src/api/value.rs#L12, I think it can be automatically scanned.
At first i thought it would be simple to implement with few converting wrapper like that.
Later, i tried #[frb(mirror(Error)] and it looks like i have to mirror all the types. Cuz the AST of SQL and enum Error are various in our project and i want flutter users to get rich types well eg)
Thanks to you, I just saw there is #[frb(ignore)]. So i will try automatic scan and ignore first!.
By the way.. When we automatically scan, Can we set workspace name instead of remote crate name?
Dart binding will be one of GlueSQL's workspace and I want to sync with the latest main branch even though it is not released yet.
pkg/dart/Cargo.toml
[dependencies]
gluesql-core.workspace = true
memory-storage.workspace = true
@fzyzcjy
fzyzcjy
1 hour ago
Maintainer
영어로 된 원본 주석 -
I see. Ping me if there are any questions!
Btw, what about converting this discussion to a github issue, since as we discuss further, this UI will quickly fold things except for several comments iirc and thus not very convenient.
When we automatically scan, Can we set workspace name instead of remote crate name?
Try to specify the crate name, and it uses cargo metadata to find that crate, thus it should find the one in you workspace, as long as your cargo.toml dependency points to your workspace.
The text was updated successfully, but these errors were encountered: