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

Fix #132 Failed to parse compiler message #133

Merged
merged 2 commits into from Oct 16, 2020
Merged

Fix #132 Failed to parse compiler message #133

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a41226c
Fix #132 Failed to parse compiler message
alopatindev Oct 14, 2020
5ccc247
Remove serde_stacker dependence
alopatindev Oct 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion Cargo.toml
View file
Open in desktop
Expand Up @@ -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


[dependencies.semver]
features = ["serde"]
Expand Down
7 changes: 6 additions & 1 deletion src/messages.rs
View file
Open in desktop
Expand Up @@ -144,7 +144,12 @@ impl<R: BufRead> Iterator for MessageIter<R> {
type Item = io::Result<Message>;
fn next(&mut self) -> Option<Self::Item> {
let line = self.lines.next()?;
let message = line.map(|it| serde_json::from_str(&it).unwrap_or(Message::TextLine(it)));
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))
});
Some(message)
}
}
Expand Down