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

Fix #132 Failed to parse compiler message #133

Merged
merged 2 commits into from Oct 16, 2020

Conversation

alopatindev
Copy link
Contributor

No description provided.

@alopatindev
Copy link
Contributor Author

alopatindev commented Oct 14, 2020

Doesn't build with Rust 1.32.0

error[E0658]: use of unstable library feature 'maybe_uninit' (see issue #53491)
   --> /home/travis/.cargo/registry/src/github.com-1ecc6299db9ec823/psm-0.1.11/src/lib.rs:197:40
    |
197 |     let mut callback: MaybeUninit<F> = MaybeUninit::new(callback);
    |                                        ^^^^^^^^^^^^^^^^

@oli-obk do we really want support the old compiler version? Thanks.

Cargo.toml Outdated
@@ -10,7 +10,8 @@ edition = "2018"

[dependencies]
serde = { version = "1.0.107", features = ["derive"] }
serde_json = "1.0.1"
serde_json = { version = "1.0.59", features = ["unbounded_depth"] }
serde_stacker = "0.1"
Copy link
Owner

Choose a reason for hiding this comment

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

is this import really needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At least that's what serde documentation suggests:

You will want to provide some other way to protect against stack overflows, such as by wrapping your Deserializer in the dynamically growing stack adapter provided by the serde_stacker crate

I guess without it we're risking to run out of stack.

But in this particular case it works without serde_stacker anyway:

diff --git a/Cargo.toml b/Cargo.toml
index d028bec..642079e 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -11,7 +11,6 @@ edition = "2018"
 [dependencies]
 serde = { version = "1.0.107", features = ["derive"] }
 serde_json = { version = "1.0.59", features = ["unbounded_depth"] }
-serde_stacker = "0.1"

 [dependencies.semver]
 features = ["serde"]
diff --git a/src/messages.rs b/src/messages.rs
index 7884a00..9b4fceb 100644
--- a/src/messages.rs
+++ b/src/messages.rs
@@ -147,8 +147,7 @@ impl<R: BufRead> Iterator for MessageIter<R> {
         let message = line.map(|it| {
             let mut deserializer = serde_json::Deserializer::from_str(&it);
             deserializer.disable_recursion_limit();
-            let deserializer = serde_stacker::Deserializer::new(&mut deserializer);
-            Message::deserialize(deserializer).unwrap_or(Message::TextLine(it))
+            Message::deserialize(&mut deserializer).unwrap_or(Message::TextLine(it))
         });
         Some(message)
     }

What do you think: should we drop the dependence or not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't able to reproduce stack overflow, so I'll add the change. Feel free to revert it if you want.

Copy link
Owner

Choose a reason for hiding this comment

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

just adding a dependency will not help protect against stack overflows. I presume that you'd need to invoke something from that crate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

let deserializer = serde_stacker::Deserializer::new(&mut deserializer);

Yes, that was the usage from the first commit

Copy link
Owner

Choose a reason for hiding this comment

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

🤦 sorry, I totally overlooked that

@oli-obk
Copy link
Owner

oli-obk commented Oct 15, 2020

I think bumping the minimal supported Rust version to 1.36 is ok

@alopatindev
Copy link
Contributor Author

No need to update to 1.36 now 🎉

@oli-obk oli-obk merged commit 4f2488d into oli-obk:main Oct 16, 2020
@oli-obk oli-obk mentioned this pull request Oct 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants