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.

Sign up for GitHub

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

Jump to bottom

Rust full reflection #8102

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Rust full reflection #8102

wants to merge 12 commits into from

Conversation

candysonya
Copy link

@candysonya candysonya commented Sep 26, 2023 •
edited
Loading

(My teammate dextero has kindly helped to review the PR and left a lot of insightful comments. Unfortunately they are not a member of the community and they cannot approve it. Currently all the comments have been resolved and it would be highly appreciated if it could be reviewed by the community because the reflection feature in Rust is a blocker for our team to use Flatbuffers!)

Compared to the C++ implementation of the reflection feature,

It includes:

  • Helper functions to access schema
  • Full reflection - getters for scalar/string/struct/table/vector types
  • Full reflection - setters for scalar/string types
  • Verification of buffer
  • Tests for above

It doesn't include:

  • Getter for VectorOfAny (Vector without element type specified)
  • Setter from another buffer, and converting buffer to builder

TODOs on C++ side are retained.

@github-actions github-actions bot added the rust label Sep 26, 2023
@candysonya candysonya force-pushed the reflection branch 2 times, most recently from 3ea581d to 7e5b92b Compare September 27, 2023 11:43
dextero
dextero reviewed Sep 27, 2023
View reviewed changes
Copy link

@dextero dextero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bunch of small things. Feel free to ignore nits if they don't seem worthwhile.

Great work overall!

rust/flatbuffers/src/vector.rs Outdated Show resolved Hide resolved
rust/flatbuffers/src/vector.rs Outdated Show resolved Hide resolved
rust/reflection/src/lib.rs Outdated Show resolved Hide resolved
rust/reflection/src/lib.rs Outdated Show resolved Hide resolved
rust/reflection/src/lib.rs Outdated Show resolved Hide resolved
rust/reflection/src/lib.rs Outdated Show resolved Hide resolved
rust/reflection/src/lib.rs Outdated Show resolved Hide resolved
rust/reflection/src/lib.rs Outdated Show resolved Hide resolved
rust/reflection/src/lib.rs Show resolved Hide resolved
tests/rust_reflection_test/src/main.rs Outdated Show resolved Hide resolved
@candysonya candysonya changed the title [Draft] Rust full reflection Rust full reflection Dec 20, 2023
@candysonya candysonya marked this pull request as ready for review December 20, 2023 10:57
@candysonya candysonya mentioned this pull request Dec 20, 2023
Open
@candysonya candysonya force-pushed the reflection branch 5 times, most recently from 0ee84d1 to 6868cb3 Compare December 20, 2023 15:42
@adsnaider
Copy link
Contributor

@candysonya @dextero We are using flatbuffers at work and I have a small patch that allows us to have zero-allocation of flatbuffer messages. I noticed that this change makes all the &'static str into String. Is this strictly necessary? Could it be Cow<'static, str>?

@candysonya
Copy link
Author

@candysonya @dextero We are using flatbuffers at work and I have a small patch that allows us to have zero-allocation of flatbuffer messages. I noticed that this change makes all the &'static str into String. Is this strictly necessary? Could it be Cow<'static, str>?

Thanks for pointing it out! I've just updated the change with Cow<'static, str>.

adsnaider
adsnaider reviewed Jan 9, 2024
View reviewed changes
Copy link
Contributor

@adsnaider adsnaider left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for making the changes to Cow<'static, str>. This is an awesome patch set and I'm really excited to have reflection! My comments are a few cases where you used .to_string() but I believe that .into() should work instead to make the value into a Cow and not a String. I could be wrong though and in that case feel free to ignore my comments

rust/flatbuffers/src/verifier.rs Outdated Show resolved Hide resolved
rust/flatbuffers/src/verifier.rs Outdated Show resolved Hide resolved
rust/flatbuffers/src/verifier.rs Outdated Show resolved Hide resolved
dextero
dextero reviewed Jan 9, 2024
View reviewed changes
rust/flatbuffers/src/vector.rs Show resolved Hide resolved
rust/reflection/src/lib.rs Outdated Show resolved Hide resolved
rust/reflection/src/lib.rs Outdated Show resolved Hide resolved
rust/reflection/src/lib.rs Outdated Show resolved Hide resolved
rust/reflection/src/lib.rs Outdated Show resolved Hide resolved
rust/reflection/src/reflection_verifier.rs Outdated Show resolved Hide resolved
rust/reflection/Cargo.toml Show resolved Hide resolved
rust/reflection/src/reflection_verifier.rs Outdated Show resolved Hide resolved
tests/rust_reflection_test/src/lib.rs Show resolved Hide resolved
tests/rust_reflection_test/src/lib.rs Outdated Show resolved Hide resolved
@candysonya
Copy link
Author

Thank you for making the changes to Cow<'static, str>. This is an awesome patch set and I'm really excited to have reflection! My comments are a few cases where you used .to_string() but I believe that .into() should work instead to make the value into a Cow and not a String. I could be wrong though and in that case feel free to ignore my comments

Thanks so much for spotting that!

@github-actions github-actions bot added the swift label Jan 21, 2024
@github-actions github-actions bot removed the swift label Jan 21, 2024
@github-actions GitHub Actions
Copy link
Contributor

This pull request is stale because it has been open 6 months with no activity. Please comment or label not-stale, or this will be closed in 14 days.

@github-actions github-actions bot added the stale label Sep 24, 2024
@brt-adam-snaider
Copy link

I do care about this feature. I may pick it up if @candysonya is unavailable.

@brt-adam-snaider
Copy link

not-stale

@candysonya
Copy link
Author

Hi @brt-adam-snaider , yes please feel free to pick it up. Do you already have a plan?

@github-actions github-actions bot removed the stale label Sep 25, 2024
@candysonya
Copy link
Author

Hi @brt-adam-snaider , I'm considering picking it up. Before that, may I know if you're already working on this?

@brt-adam-snaider
Copy link

I haven't picked it up yet. Have at it :)

@candysonya candysonya force-pushed the reflection branch 3 times, most recently from 7d2ea1f to 6ed63ee Compare December 9, 2024 23:27
dextero
dextero reviewed Dec 10, 2024
View reviewed changes
Copy link

@dextero dextero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you for following through on this!

We'll need an owner approval, I can't grant one unfortunately.

@candysonya candysonya force-pushed the reflection branch 2 times, most recently from ad5edac to d3f36c9 Compare January 6, 2025 13:10
@dbaileychess
Copy link
Collaborator

Some CI issues with Rust. Can you take a look? It should auto run now next time you push to the PR.

dextero
dextero reviewed Jan 10, 2025
View reviewed changes
Copy link

@dextero dextero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a few nits, mostly about how some errors could be avoided by reorganizing the code a bit. LGTM overall, great work!

rust/reflection/src/safe_buffer.rs Show resolved Hide resolved
rust/reflection/src/safe_buffer.rs Show resolved Hide resolved
rust/reflection/src/safe_buffer.rs Show resolved Hide resolved
tests/rust_reflection_test/src/lib.rs Show resolved Hide resolved
rust/reflection/src/lib.rs Show resolved Hide resolved
@candysonya
#Rust Create a crate for reflection
ea19d65
@candysonya
#Rust Add a crate for reflection tests and helper to access schema
054544d
@candysonya
#Rust Get root table of a buffer and access field with schema
c727289
@candysonya
#Rust Add 'Struct' struct and corresponding getter
ee33b86
@candysonya
#Rust Add functions of getting any table/struct field value as integ…
7892dc6 
…er/float/string
@candysonya
#Rust Add setters for scalar fields
c951a93
@candysonya
#Rust Add setter for string fields
2f6bb1a
@candysonya
#Rust Add getter for Table/Vector fields
6c30257
@candysonya
#Rust Add buffer verification
6b160de
@candysonya
Add a 'SafeBuffer' struct which provides safe methods for reflection
f36c036 
It verifies buffer against schema during construction and provides all the unsafe getters in lib.rs in a safe way
@candysonya
Fix Rust CI failure on using 'std' module
e2b998f
@candysonya
Fix Rust CI failure on using 'std' module
2986c39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brt-adam-snaider brt-adam-snaider brt-adam-snaider left review comments

@adsnaider adsnaider adsnaider left review comments

@dextero dextero dextero left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
rust
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@candysonya @adsnaider @brt-adam-snaider @dbaileychess @dextero