Skip to content

feat(swc/common): wrap serialized struct with versioned (#5060: Part 3) #5128

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
Merged
merged 8 commits into from
Jul 7, 2022
Merged

feat(swc/common): wrap serialized struct with versioned (#5060: Part 3) #5128

merged 8 commits into from
Jul 7, 2022

Conversation

kwonoj
Copy link
Member

@kwonoj kwonoj commented Jul 6, 2022

Description:

This PR actually wraps up structs with VersionedSerializable for the plugins. PluginSerializedBytes's serialize / deserialize interface now explicitly accepts VersionedSerializable only. Most changes are straightforward, with implementing default for few structs & couple of copy around diagnostic / comment to workaround moves.

There is one item I need to revisit later (main...kwonoj:swc:feat-versioned-serialized#diff-b2b47f82dd3bafcc79a1e0e6ec2ccb75624c020e295e2a65c03d01058b56da92R178) since Result<T> is not possible to use VersionedSerializable::take() due to lacks of default.

BREAKING CHANGE:

Related issue (if exists):

#5060

@kwonoj kwonoj changed the title feat(swc/common): wrap serialized struct with versioned (#5060: Part 2) feat(swc/common): wrap serialized struct with versioned (#5060: Part 3) Jul 6, 2022
@kdy1 kdy1 added this to the Planned milestone Jul 7, 2022
kdy1
kdy1 requested changes Jul 7, 2022
View reviewed changes
Copy link
Member

@kdy1 kdy1 left a comment

Choose a reason for hiding this comment

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

Can't we add something like into_inner() instead of using .take()?

kdy1
kdy1 requested changes Jul 7, 2022
View reviewed changes
Copy link
Member

@kdy1 kdy1 left a comment

Choose a reason for hiding this comment

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

Wrong Default impl should not exist.
We should use Take::dummy() instead

crates/swc_common/src/comments.rs Outdated
@@ -548,6 +548,16 @@ pub struct Comment {
pub text: String,
}

impl Default for Comment {
Copy link
Member

Choose a reason for hiding this comment

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

Is this still required, even if we add into_inner()?

Copy link
Member Author

Choose a reason for hiding this comment

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

default was added to satisfy mem::take in VersionedSerializable::take. if there's other way default is not required mandatoriliy.

I'm bit unsure if I understood the suggestion correctly, mind elaborate bit more? The way I understood is let each struct implement Take::dummy() instead of default, and VersionedSerializable should utilize it for the take. Not sure where into_inner plays in here.

Copy link
Member

Choose a reason for hiding this comment

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

into_inner take ownership of VersionedSerializable so it does not require take(&mut self.0.1).

Copy link
Member

Choose a reason for hiding this comment

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

The Default constraint comes from it.

crates/swc_common/src/errors/diagnostic.rs Outdated
@@ -36,6 +36,19 @@ pub struct Diagnostic {
pub suggestions: Vec<CodeSuggestion>,
}

impl Default for Diagnostic {
Copy link
Member

Choose a reason for hiding this comment

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

Same here

crates/swc_common/src/plugin.rs Outdated
@@ -41,6 +41,12 @@ pub enum PluginError {
Serialize(String),
}

impl Default for PluginError {
Copy link
Member

Choose a reason for hiding this comment

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

Same here

crates/swc_common/src/syntax_pos.rs Outdated
@@ -130,6 +130,12 @@ pub enum FileName {
Custom(String),
}

impl Default for FileName {
Copy link
Member

Choose a reason for hiding this comment

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

Same here

crates/swc_common/src/syntax_pos.rs Outdated
@@ -805,7 +811,7 @@ impl Sub<BytePos> for NonNarrowChar {
derive(rkyv::Archive, rkyv::Serialize, rkyv::Deserialize)
)]
#[cfg_attr(feature = "rkyv", archive_attr(repr(C), derive(bytecheck::CheckBytes)))]
#[derive(Clone)]
#[derive(Clone, Default)]
Copy link
Member

Choose a reason for hiding this comment

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

Same here

crates/swc_plugin_proxy/src/memory_interop/read_returned_result_from_host.rs Outdated
@@ -60,7 +62,7 @@ where
pub fn read_returned_result_from_host<F, R>(f: F) -> Option<R>
where
F: FnOnce(i32) -> i32,
R: rkyv::Archive,
R: rkyv::Archive + std::default::Default,
Copy link
Member

Choose a reason for hiding this comment

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

Same here

crates/swc_common/src/syntax_pos.rs Outdated
@@ -1193,6 +1210,7 @@ pub struct LineCol {
pub col: u32,
}

#[derive(Default)]
Copy link
Member

Choose a reason for hiding this comment

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

Same here

crates/swc_plugin_proxy/src/memory_interop/read_returned_result_from_host.rs Outdated
@@ -85,11 +88,11 @@ where
pub fn read_returned_result_from_host_fallible<F, R>(f: F) -> Option<R>
where
F: FnOnce(i32) -> i32,
R: rkyv::Archive,
R: rkyv::Archive + std::default::Default,
Copy link
Member

Choose a reason for hiding this comment

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

Same here

@kwonoj kwonoj force-pushed the feat-versioned-serialized branch from 89966ab to e48bcbd Compare July 7, 2022 06:31
@kwonoj kwonoj force-pushed the feat-versioned-serialized branch from e48bcbd to 509e766 Compare July 7, 2022 06:37
kdy1
kdy1 approved these changes Jul 7, 2022
View reviewed changes
Copy link
Member

@kdy1 kdy1 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!

swc-bump:

  • swc_common --breaking

@kdy1 kdy1 enabled auto-merge (squash) July 7, 2022 06:50
swc-bot
swc-bot reviewed Jul 7, 2022
View reviewed changes
Copy link
Collaborator

@swc-bot swc-bot left a comment

Choose a reason for hiding this comment

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

Automated review comment generated by auto-rebase script

@kdy1 kdy1 merged commit 82fbe15 into swc-project:main Jul 7, 2022
@kwonoj kwonoj deleted the feat-versioned-serialized branch July 8, 2022 20:26
@kdy1 kdy1 modified the milestones: Planned, v1.2.211 Jul 9, 2022
@swc-project swc-project locked as resolved and limited conversation to collaborators Oct 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@swc-bot swc-bot swc-bot left review comments

@kdy1 kdy1 kdy1 approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
v1.2.211
Development

Successfully merging this pull request may close these issues.
3 participants
@kwonoj @kdy1 @swc-bot